On Wed, Oct 6, 2010 at 5:51 AM, Myles Watson mylesgw@gmail.com wrote:
On Tue, Oct 5, 2010 at 4:02 PM, Warren Turkal wt@penguintechs.org wrote:
*snip*
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.
They are used in at least 3 places, each of the vendor car implementations. They also should be used in the other intel car implementations, but those use a slightly different algorithm for clearing the registers, and I didn't want that change to be wrapped up in this one as well.
I don't like having to look up END_MTRR_MSRS_TABLE_ENTRY_ASM to find that it is just a .long 0
It just becomes a magic number if I don't give it a symbolic name.
I would like to change the way that the vendor CAR implementations work to work like the implementation in src/cpu/intel/model_106cx/cache_as_ram.inc. That file's implementation doesn't depend on a sentinel value at the end of the list. That would eliminate the need for the END_MTRR_MSRS_TABLE_ENTRY_ASM macro.
+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.
I definitely agree.
wt