Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33504 )
Change subject: MSR: Move common intel's MSRs to common place ......................................................................
Patch Set 4:
Patch Set 4: Code-Review-1
Patch Set 4:
(1 comment)
FYI: I see little value here and wont be doing any proper review.
You should not push MSR register definitions that do not apply to all intel CPU models into a common file cpu/intel/msr.h.
I agree. MSR stands for Model-Specific Register. I would rather have the same register definition in a dozen different files, so that one knows what MSRs exist for a certain CPU model (the code also acts as documentation).
As ~95% of all definitions are copy pasta, you can't rely on these anyway. If we had the resources to review / cleanup the mess of silicon vendors, I would agree. But as long as we don't, I would prefer a practice that avoids copy pasta.
Having a single definition on a common file is very likely to cause confusion: "Does this MSR exist for *my* chip?"
Does it start with IA32_? then yes. Does it start with MSR_? then check documentation.