[coreboot] [PATCH try 2] Reduce duplicate definition in CAR code.
c-d.hailfinger.devel.2006 at gmx.net
Wed Oct 6 15:42:15 CEST 2010
On 06.10.2010 14:51, Myles Watson wrote:
> On Tue, Oct 5, 2010 at 4:02 PM, Warren Turkal <wt at 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?
>> 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.
>> - .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
> 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.
More information about the coreboot