On 06.10.2010 14:51, Myles Watson wrote:
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_MSRI'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) */
Yeargh! You can't be serious! ASM macros for MTRR address lists? I have worked a lot with CAR code and I know the readability issues there, but this makes it worse.
This code is easy enough to break that we have to be very careful about changing it.
Not only that. ASM macros are really hard to get right, and some of the CAR code even has comments about this. For example, both space and comma are parameter separators in some versions of gas, so if you use ", " as parameter separator, you lose because this is treated as two separators with an empty parameter in between.
Regards, Carl-Daniel