| Submitter | Warren Turkal |
|---|---|
| Date | 2010-10-05 22:02:11 |
| Message ID | <1286316131-6936-1-git-send-email-wt@penguintechs.org> |
| Download | mbox | patch |
| Permalink | /patch/2049/ |
| State | Superseded |
| Headers | show |
Comments
It's proven dangerous in the past to cross mtrr settings across vendors. Which is what you are doing. Then somebody patches, e.g., cpu/x86/mtrr.h for some fix to via, and we find out a year later it is not right for some flavor of AMD. MTRRs have been a rolling headache for 10 years now. Sure they should all be the same. Sometimes there are weird issues. So what I'd prefer, personally: leave the settings in each vendor file: amd, via, whatever, don't make the common settings in cpu/x86/mtrr.h. But use your nice macros to set those up. ron
The common mtrr registers are separated into their own macros. For instance, check out the AMD car code. AMD has additional mtrr registers that are used. There is an AMD_MTRR_MSRS_TABLE_ENTRIES_ASM as a result. Is this not enough to separate the vendors? Or is your assertion that the values in X86_MTRR_MSRS_TABLE_ENTRIES_ASM can vary? Thanks, wt On Tue, Oct 5, 2010 at 3:18 PM, ron minnich <rminnich@gmail.com> wrote: > It's proven dangerous in the past to cross mtrr settings across vendors. > > Which is what you are doing. Then somebody patches, e.g., > cpu/x86/mtrr.h for some fix to via, and we find out a year later it is > not right for some flavor of AMD. MTRRs have been a rolling headache > for 10 years now. > > Sure they should all be the same. Sometimes there are weird issues. > > So what I'd prefer, personally: leave the settings in each vendor > file: amd, via, whatever, don't make the common settings in > cpu/x86/mtrr.h. But use your nice macros to set those up. > > ron >
On Tue, Oct 5, 2010 at 3:28 PM, Warren Turkal <wt@penguintechs.org> wrote: > The common mtrr registers are separated into their own macros. For > instance, check out the AMD car code. AMD has additional mtrr > registers that are used. There is an AMD_MTRR_MSRS_TABLE_ENTRIES_ASM > as a result. Is this not enough to separate the vendors? > > Or is your assertion that the values in > X86_MTRR_MSRS_TABLE_ENTRIES_ASM can vary? oops, I misread it. OK, got it, makes sense. I think you can leave the _ASM off since you are using as .macro anyway -- you can't use it anywhere but assembly. If we're all certain that those registers are all safely common across all systems, I guess it works for me. Possibly we should get some testing to make sure it's good. ron
I have run "util/abuild/abuild -a -c 8", and nothing showed up except some lib/gcc.c stuff that I have no idea how to fix. I put the _ASM on to note that it's adding asm code. I figured that'd make it more obvious. Is it obvious enough without the _ASM? wt On Tue, Oct 5, 2010 at 3:46 PM, ron minnich <rminnich@gmail.com> wrote: > On Tue, Oct 5, 2010 at 3:28 PM, Warren Turkal <wt@penguintechs.org> wrote: >> The common mtrr registers are separated into their own macros. For >> instance, check out the AMD car code. AMD has additional mtrr >> registers that are used. There is an AMD_MTRR_MSRS_TABLE_ENTRIES_ASM >> as a result. Is this not enough to separate the vendors? >> >> Or is your assertion that the values in >> X86_MTRR_MSRS_TABLE_ENTRIES_ASM can vary? > > > oops, I misread it. OK, got it, makes sense. > > I think you can leave the _ASM off since you are using as .macro > anyway -- you can't use it anywhere but assembly. If we're all certain > that those registers are all safely common across all systems, I guess > it works for me. > > Possibly we should get some testing to make sure it's good. > > ron >
On Tue, Oct 5, 2010 at 3:54 PM, Warren Turkal <wt@penguintechs.org> wrote: > I have run "util/abuild/abuild -a -c 8", and nothing showed up except > some lib/gcc.c stuff that I have no idea how to fix. > > I put the _ASM on to note that it's adding asm code. I figured that'd > make it more obvious. Is it obvious enough without the _ASM? It is to me ... ron
Warren Turkal wrote: > I have run "util/abuild/abuild -a -c 8", and nothing showed up except > some lib/gcc.c stuff that I have no idea how to fix. No good to break things. What were the errors? //Peter
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
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_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) */ >> 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
Well, Warren, this one clearly needs another iteration. :-) ron
On Wed, Oct 06, 2010 at 06:51:13AM -0600, Myles Watson wrote: > > + /* 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 Yep. > macros in C header files, when they're only used in this one place, > doesn't make sense to me. They're only used once per cache_as_ram.inc file, but every such file has that list, so putting the list in an mtrr_defs.inc or the like and using ".include mtrr_defs.inc" in every cache_as_ram.inc would be nice IMHO. Uwe.
On 10/6/10 10:23 AM, Uwe Hermann wrote: > They're only used once per cache_as_ram.inc file, but every such file > has that list, so putting the list in an mtrr_defs.inc or the like > and using ".include mtrr_defs.inc" in every cache_as_ram.inc would > be nice IMHO. They're not necessarily the same on all platforms though I think. Stefan
On Wed, Oct 6, 2010 at 12:34 PM, Stefan Reinauer <stefan.reinauer@coresystems.de> wrote: > On 10/6/10 10:23 AM, Uwe Hermann wrote: >> They're only used once per cache_as_ram.inc file, but every such file >> has that list, so putting the list in an mtrr_defs.inc or the like >> and using ".include mtrr_defs.inc" in every cache_as_ram.inc would >> be nice IMHO. > They're not necessarily the same on all platforms though I think. The common ones appear to be the same. In fact, they appear to have been copy/pasted. AMD had some that were AMD specific, so those got their own macro. Thanks, wt
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
With regard to mtrr_def.inc instead of the header, I would still need to create an asm macro so that they could be put in the file where desired. Would it be better to have a separate include mtrr_def.inc instead of just including it in the x86/mtrr.h header? Thanks, wt On Wed, Oct 6, 2010 at 10:23 AM, Uwe Hermann <uwe@hermann-uwe.de> wrote: > On Wed, Oct 06, 2010 at 06:51:13AM -0600, Myles Watson wrote: >> > + /* 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 > > Yep. > > >> macros in C header files, when they're only used in this one place, >> doesn't make sense to me. > > They're only used once per cache_as_ram.inc file, but every such file > has that list, so putting the list in an mtrr_defs.inc or the like > and using ".include mtrr_defs.inc" in every cache_as_ram.inc would > be nice IMHO. > > > Uwe. > -- > http://hermann-uwe.de | http://sigrok.org > http://randomprojects.org | http://unmaintained-free-software.org >
On Wed, Oct 6, 2010 at 6:42 AM, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: *snip* > 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 asm macro allows the list to be place where it is needed in the file. Since the file seems to be executed from the top, I can't just define a label with extra data in it in the header. AFAICT, that data would be executed as code if I did such a thing since the car code doesn't seem to have a starting label. >> 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. Are you saying that the following: .long 0x0,<space>0x1 could be treated as if it has two separators? Are you saying that this can only happen in asm macros? If that is not what you are saying, then the code is already broken as all the vendor main car implementations current use <comma><space> to separate the register long values. Thanks, wt
On Tue, Oct 5, 2010 at 4:15 PM, Peter Stuge <peter@stuge.se> wrote:
> No good to break things. What were the errors?
Building from HEAD, I see the following errors for a number of different boards:
Building amd/pistachio; i386: ok, amd64 using gcc
Creating config file... ok; Compiling image on 8 cpus in parallel
.. FAILED after 3s!
Log excerpt:
/home/wt/projects/coreboot/coreboot/src/lib/gcc.c:31: undefined
reference to `__udivdi3'
coreboot-builds/amd_pistachio/coreboot_ram.o: In function `__wrap___divdi3':
/home/wt/projects/coreboot/coreboot/src/lib/gcc.c:30: undefined
reference to `__divdi3'
collect2: ld returned 1 exit status
make: *** [coreboot-builds/amd_pistachio/coreboot_ram] Error 1
make: *** Waiting for unfinished jobs....
wt
Working on another cut. wt On Wed, Oct 6, 2010 at 7:58 AM, ron minnich <rminnich@gmail.com> wrote: > Well, Warren, this one clearly needs another iteration. > > :-) > > ron >
Patch
diff --git a/src/cpu/amd/car/cache_as_ram.inc b/src/cpu/amd/car/cache_as_ram.inc index 5318272..a8c6972 100644 --- a/src/cpu/amd/car/cache_as_ram.inc +++ b/src/cpu/amd/car/cache_as_ram.inc @@ -155,7 +155,7 @@ enable_fixed_mtrr_dram_modify: /* Clear all MTRRs. */ xorl %edx, %edx - movl $fixed_mtrr_msr, %esi + movl $all_mtrr_msrs, %esi clear_fixed_var_mtrr: lodsl (%esi), %eax @@ -396,23 +396,9 @@ CAR_FAM10_ap_out: post_code(0xaf) /* Should never see this POST code. */ -fixed_mtrr_msr: - .long 0x250, 0x258, 0x259 - .long 0x268, 0x269, 0x26A - .long 0x26B, 0x26C, 0x26D - .long 0x26E, 0x26F - -var_mtrr_msr: - .long 0x200, 0x201, 0x202, 0x203 - .long 0x204, 0x205, 0x206, 0x207 - .long 0x208, 0x209, 0x20A, 0x20B - .long 0x20C, 0x20D, 0x20E, 0x20F - -var_iorr_msr: - .long 0xC0010016, 0xC0010017, 0xC0010018, 0xC0010019 - -mem_top: - .long 0xC001001A, 0xC001001D - .long 0x000 /* NULL, end of table */ +all_mtrr_msrs: +X86_MTRR_MSRS_TABLE_ENTRIES_ASM +AMD_MTRR_MSRS_TABLE_ENTRIES_ASM +END_MTRR_MSRS_TABLE_ENTRY_ASM cache_as_ram_setup_out: diff --git a/src/cpu/intel/car/cache_as_ram.inc b/src/cpu/intel/car/cache_as_ram.inc index d8465f4..84e2f2f 100644 --- a/src/cpu/intel/car/cache_as_ram.inc +++ b/src/cpu/intel/car/cache_as_ram.inc @@ -115,7 +115,7 @@ NotHtProcessor: /* Clear all MTRRs. */ xorl %edx, %edx - movl $fixed_mtrr_msr, %esi + movl $all_mtrr_msrs, %esi clear_fixed_var_mtrr: lodsl (%esi), %eax @@ -128,18 +128,9 @@ clear_fixed_var_mtrr: jmp clear_fixed_var_mtrr -fixed_mtrr_msr: - .long 0x250, 0x258, 0x259 - .long 0x268, 0x269, 0x26A - .long 0x26B, 0x26C, 0x26D - .long 0x26E, 0x26F - -var_mtrr_msr: - .long 0x200, 0x201, 0x202, 0x203 - .long 0x204, 0x205, 0x206, 0x207 - .long 0x208, 0x209, 0x20A, 0x20B - .long 0x20C, 0x20D, 0x20E, 0x20F - .long 0x000 /* NULL, end of table */ +all_mtrr_msrs: +X86_MTRR_MSRS_TABLE_ENTRIES_ASM +END_MTRR_MSRS_TABLE_ENTRY_ASM clear_fixed_var_mtrr_out: diff --git a/src/cpu/via/car/cache_as_ram.inc b/src/cpu/via/car/cache_as_ram.inc index d6df4a9..80f8c13 100644 --- a/src/cpu/via/car/cache_as_ram.inc +++ b/src/cpu/via/car/cache_as_ram.inc @@ -47,7 +47,7 @@ CacheAsRam: /* Clear all MTRRs. */ xorl %edx, %edx - movl $fixed_mtrr_msr, %esi + movl $all_mtrr_msrs, %esi clear_fixed_var_mtrr: lodsl (%esi), %eax @@ -60,18 +60,9 @@ clear_fixed_var_mtrr: jmp clear_fixed_var_mtrr -fixed_mtrr_msr: - .long 0x250, 0x258, 0x259 - .long 0x268, 0x269, 0x26A - .long 0x26B, 0x26C, 0x26D - .long 0x26E, 0x26F - -var_mtrr_msr: - .long 0x200, 0x201, 0x202, 0x203 - .long 0x204, 0x205, 0x206, 0x207 - .long 0x208, 0x209, 0x20A, 0x20B - .long 0x20C, 0x20D, 0x20E, 0x20F - .long 0x000 /* NULL, end of table */ +all_mtrr_msrs: +X86_MTRR_MSRS_TABLE_ENTRIES_ASM +END_MTRR_MSRS_TABLE_ENTRY_ASM clear_fixed_var_mtrr_out: movl $MTRRphysBase_MSR(0), %ecx diff --git a/src/include/cpu/amd/mtrr.h b/src/include/cpu/amd/mtrr.h index c7b3fca..1649632 100644 --- a/src/include/cpu/amd/mtrr.h +++ b/src/include/cpu/amd/mtrr.h @@ -31,6 +31,15 @@ #define TOP_MEM_MASK 0x007fffff #define TOP_MEM_MASK_KB (TOP_MEM_MASK >> 10) +#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) */ + #if !defined(__PRE_RAM__) && !defined(ASSEMBLY) void amd_setup_mtrrs(void); #endif diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index e79c90e..ba3ab08 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -34,6 +34,37 @@ #define MTRRfix4K_F0000_MSR 0x26e #define MTRRfix4K_F8000_MSR 0x26f +#if defined (ASSEMBLY) +.macro X86_MTRR_MSRS_TABLE_ENTRIES_ASM + /* fixed mtrr MSRs */ + .long MTRRfix64K_00000_MSR + .long MTRRfix16K_80000_MSR + .long MTRRfix16K_A0000_MSR + .long MTRRfix4K_C0000_MSR + .long MTRRfix4K_C8000_MSR + .long MTRRfix4K_D0000_MSR + .long MTRRfix4K_D8000_MSR + .long MTRRfix4K_E0000_MSR + .long MTRRfix4K_E8000_MSR + .long MTRRfix4K_F0000_MSR + .long MTRRfix4K_F8000_MSR + + /* var mtrr MSRs */ + .long MTRRphysBase_MSR(0), MTRRphysBase_MSR(1) + .long MTRRphysBase_MSR(2), MTRRphysBase_MSR(3) + .long MTRRphysBase_MSR(4), MTRRphysBase_MSR(5) + .long MTRRphysBase_MSR(6), MTRRphysBase_MSR(7) + .long MTRRphysBase_MSR(8), MTRRphysBase_MSR(9) + .long MTRRphysBase_MSR(10), MTRRphysBase_MSR(11) + .long MTRRphysBase_MSR(12), MTRRphysBase_MSR(13) + .long MTRRphysBase_MSR(14), MTRRphysBase_MSR(15) +.endm /* X86_MTRR_MSRS_TABLE_ENTRIES_ASM */ + +.macro END_MTRR_MSRS_TABLE_ENTRY_ASM + .long 0x000 /* NULL, end of table */ +.endm /* END_MTRR_MSRS_TABLE_ASM */ +#endif /* defined (ASSEMBLY) */ + #if !defined (ASSEMBLY) && !defined(__PRE_RAM__) #include <device/device.h> void enable_fixed_mtrr(void);
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. Signed-off-by: Warren Turkal <wt@penguintechs.org> --- src/cpu/amd/car/cache_as_ram.inc | 24 +++++------------------- src/cpu/intel/car/cache_as_ram.inc | 17 ++++------------- src/cpu/via/car/cache_as_ram.inc | 17 ++++------------- src/include/cpu/amd/mtrr.h | 9 +++++++++ src/include/cpu/x86/mtrr.h | 31 +++++++++++++++++++++++++++++++ 5 files changed, 53 insertions(+), 45 deletions(-)