On Tue, Oct 5, 2010 at 4:02 PM, Warren Turkal wt@penguintechs.org wrote:
Here's another cut at this patch that is more comprehensive. I have included all major vendor car implementations. What do you all think about this approach?
Thanks, wt 8<------------ Macros for the register addresses for the MTRR MSRs are already defined in include/cpu/x86/car.h. This patch uses those macros instead of creating a second instance of that same data.
I also combined common MTRR definitions into a macro.
-fixed_mtrr_msr:
- .long 0x250, 0x258, 0x259
...
- /* fixed mtrr MSRs */
- .long MTRRfix64K_00000_MSR
- .long MTRRfix16K_80000_MSR
- .long MTRRfix16K_A0000_MSR
I'm fine with replacing the magic numbers, but moving the lists to ASM macros in C header files, when they're only used in this one place, doesn't make sense to me.
I don't like having to look up END_MTRR_MSRS_TABLE_ENTRY_ASM to find that it is just a .long 0
+X86_MTRR_MSRS_TABLE_ENTRIES_ASM +AMD_MTRR_MSRS_TABLE_ENTRIES_ASM +END_MTRR_MSRS_TABLE_ENTRY_ASM
I don't find this easier to read than the original:
+#if defined(ASSEMBLY) +.macro AMD_MTRR_MSRS_TABLE_ENTRIES_ASM
/* Variable IORR MTRR MSRs */.long 0xC0010016, 0xC0010017, 0xC0010018, 0xC0010019/* Top of memory MTRR MSRs */.long 0xC001001A, 0xC001001D+.endm /* AMD_MTRR_MSRS_TABLE_ENTRIES_ASM */ +#endif /* defined(ASSEMBLY) */
This code is easy enough to break that we have to be very careful about changing it.
Thanks, Myles