| Submitter | Scott |
|---|---|
| Date | 2010-09-06 03:32:17 |
| Message ID | <2F6225DCDF084C9D8CFD78A8C6B10AA3@m3a78> |
| Download | mbox | patch |
| Permalink | /patch/1868/ |
| State | New |
| Headers | show |
Comments
On 9/6/10 5:32 AM, Scott Duplichan wrote: > Resend: one more attempt to get this patch right. The previous > submission included the patch as an attachement. The attachment > contained Windows-style line endings. The attachment is missing > from the mailing list archive: "A non-text attachment was scrubbed". > http://www.coreboot.org/pipermail/coreboot/2010-September/060090.html The patch is there, just click on the link below the message. (Unfortunately with a wrong name) > Root cause: After function STOP_CAR_AND_CPU disables cache as > ram, the cache as ram stack can no longer be used. Called > functions must be inlined to avoid stack usage. Also, the > compiler must keep local variables register based and not > allocated them from the stack. With gcc 4.5.0, some functions > declared as inline are not being inlined. This patch forces > these functions to always be inlined by adding the qualifier > __attribute__((always_inline)) to their declaration. I think this explanation should go into the two files that get the always_inline attributes so people looking at this code 3ys (or months) from now know why it was done this way. > Update: Still no test reports for real hardware are available. > If we cannot get this change tested on real hardware, I suggest > we conditionally compile in only if gcc 4.5.0 or later is used. I think it's safe to use it as is, though I don't have a Tilapia to test it on. > Signed-off-by: Scott Duplichan <scott@notabs.org> With the explanation above added to each of the two files, this is Acked-by: Stefan Reinauer <stepan@coresystems.de> However, I think we should additionally look at "fixing" AMD's CAR code to not call C functions with neither CAR or RAM backing them. I reworked all CAR code to behave like that a while ago (ie. , but the AMD code was considerably more complex . The complexity of the code also brings along some bugs that currently need workarounds[1]. A proper fix for this would be nice, but it's hard to determine what's wrong with the current code. Should you have some insights on this, please share! Stefan [1] http://article.gmane.org/gmane.linux.bios/57707
On Mon, Sep 6, 2010 at 4:03 AM, Stefan Reinauer <stefan.reinauer@coresystems.de> wrote: > On 9/6/10 5:32 AM, Scott Duplichan wrote: >> Resend: one more attempt to get this patch right. The previous >> submission included the patch as an attachement. The attachment >> contained Windows-style line endings. The attachment is missing >> from the mailing list archive: "A non-text attachment was scrubbed". >> http://www.coreboot.org/pipermail/coreboot/2010-September/060090.html > The patch is there, just click on the link below the message. > (Unfortunately with a wrong name) >> Root cause: After function STOP_CAR_AND_CPU disables cache as >> ram, the cache as ram stack can no longer be used. Called >> functions must be inlined to avoid stack usage. Also, the >> compiler must keep local variables register based and not >> allocated them from the stack. With gcc 4.5.0, some functions >> declared as inline are not being inlined. This patch forces >> these functions to always be inlined by adding the qualifier >> __attribute__((always_inline)) to their declaration. > > I think this explanation should go into the two files that get the > always_inline attributes so people looking at this code 3ys (or months) > from now know why it was done this way. >> Update: Still no test reports for real hardware are available. >> If we cannot get this change tested on real hardware, I suggest >> we conditionally compile in only if gcc 4.5.0 or later is used. > I think it's safe to use it as is, though I don't have a Tilapia to test > it on. > >> Signed-off-by: Scott Duplichan <scott@notabs.org> > > With the explanation above added to each of the two files, this is > > Acked-by: Stefan Reinauer <stepan@coresystems.de> > > However, I think we should additionally look at "fixing" AMD's CAR code > to not call C functions with neither CAR or RAM backing them. I reworked > all CAR code to behave like that a while ago (ie. , but the AMD code was > considerably more complex . The complexity of the code also brings along > some bugs that currently need workarounds[1]. A proper fix for this > would be nice, but it's hard to determine what's wrong with the current > code. Should you have some insights on this, please share! > > Stefan Yes, this is "wrong" since memory is working at this point, I don't see why we don't do a stack switch and then disable CAR. I added comments to the code to explain why they require the new attribute. r5818 Marc
]-----Original Message----- ]From: Marc Jones [mailto:marcj303@gmail.com] ]Sent: Friday, September 17, 2010 04:40 PM ]To: Stefan Reinauer; Frank Vibrans; Scott ]Cc: coreboot@coreboot.org ]Subject: Re: [coreboot] [PATCH] fix 'AMD Fam10 code breaks with gcc 4.5.0' ] ]On Mon, Sep 6, 2010 at 4:03 AM, Stefan Reinauer ]<stefan.reinauer@coresystems.de> wrote: ]> On 9/6/10 5:32 AM, Scott Duplichan wrote: ]>> Resend: one more attempt to get this patch right. The previous ]>> submission included the patch as an attachement. The attachment ]>> contained Windows-style line endings. The attachment is missing ]>> from the mailing list archive: "A non-text attachment was scrubbed". ]>> http://www.coreboot.org/pipermail/coreboot/2010-September/060090.html ]> The patch is there, just click on the link below the message. ]> (Unfortunately with a wrong name) ]>> Root cause: After function STOP_CAR_AND_CPU disables cache as ]>> ram, the cache as ram stack can no longer be used. Called ]>> functions must be inlined to avoid stack usage. Also, the ]>> compiler must keep local variables register based and not ]>> allocated them from the stack. With gcc 4.5.0, some functions ]>> declared as inline are not being inlined. This patch forces ]>> these functions to always be inlined by adding the qualifier ]>> __attribute__((always_inline)) to their declaration. ]> ]> I think this explanation should go into the two files that get the ]> always_inline attributes so people looking at this code 3ys (or months) ]> from now know why it was done this way. ]>> Update: Still no test reports for real hardware are available. ]>> If we cannot get this change tested on real hardware, I suggest ]>> we conditionally compile in only if gcc 4.5.0 or later is used. ]> I think it's safe to use it as is, though I don't have a Tilapia to test ]> it on. ]> ]>> Signed-off-by: Scott Duplichan <scott@notabs.org> ]> ]> With the explanation above added to each of the two files, this is ]> ]> Acked-by: Stefan Reinauer <stepan@coresystems.de> ]> ]> However, I think we should additionally look at "fixing" AMD's CAR code ]> to not call C functions with neither CAR or RAM backing them. I reworked ]> all CAR code to behave like that a while ago (ie. , but the AMD code was ]> considerably more complex . The complexity of the code also brings along ]> some bugs that currently need workarounds[1]. A proper fix for this ]> would be nice, but it's hard to determine what's wrong with the current ]> code. Should you have some insights on this, please share! ]> ]> Stefan ] ]Yes, this is "wrong" since memory is working at this point, I don't ]see why we don't do a stack switch and then disable CAR. For me, the gcc 4.5.0 crash happens before memory is initialized. It is at the conclusion of the first AP execution, where the APs do fid-vid, microcode patch, etc. The APs must leave their fixed MTRRs setup to map the lower 1MB to DRAM before HLTing, so that startup IPIs can be used to start them later. But DRAM has not been initialized, so a DRAM stack cannot be used. The crash occurs when an AP is making this transition from cache-as-ram MTRRs to startup IPI compatible MTRRs. Thanks, Scott ]I added comments to the code to explain why they require the new attribute. ] ]r5818 ] ]Marc ]-- ]http://se-eng.com
Patch
Index: src/include/cpu/x86/msr.h =================================================================== --- src/include/cpu/x86/msr.h (revision 5777) +++ src/include/cpu/x86/msr.h (working copy) @@ -29,7 +29,7 @@ msr_t msr; } msrinit_t; -static inline msr_t rdmsr(unsigned index) +static inline __attribute__((always_inline)) msr_t rdmsr(unsigned index) { msr_t result; __asm__ __volatile__ ( @@ -40,7 +40,7 @@ return result; } -static inline void wrmsr(unsigned index, msr_t msr) +static inline __attribute__((always_inline)) void wrmsr(unsigned index, msr_t msr) { __asm__ __volatile__ ( "wrmsr" Index: src/include/cpu/x86/cache.h =================================================================== --- src/include/cpu/x86/cache.h (revision 5777) +++ src/include/cpu/x86/cache.h (working copy) @@ -74,7 +74,7 @@ asm volatile("invd" ::: "memory"); } -static inline void enable_cache(void) +static inline __attribute__((always_inline)) void enable_cache(void) { unsigned long cr0; cr0 = read_cr0(); @@ -82,7 +82,7 @@ write_cr0(cr0); } -static inline void disable_cache(void) +static inline __attribute__((always_inline)) void disable_cache(void) { /* Disable and write back the cache */ unsigned long cr0;
Resend: one more attempt to get this patch right. The previous submission included the patch as an attachement. The attachment contained Windows-style line endings. The attachment is missing from the mailing list archive: "A non-text attachment was scrubbed". http://www.coreboot.org/pipermail/coreboot/2010-September/060090.html This time the patch is inline, which hopefully will solve the eol-style problem. The attached patch allows the use of gcc 4.5.0 for AMD builds. The AMD Tilapia BIOS built with gcc 4.5.0 and this patch has passed testing on the simnow target only. Can someone confirm that the attached patch allows an AMD family 10h BIOS such as Tilapia to work on real hardware? At the same time I will try to get the change tested on real hardware. Root cause: After function STOP_CAR_AND_CPU disables cache as ram, the cache as ram stack can no longer be used. Called functions must be inlined to avoid stack usage. Also, the compiler must keep local variables register based and not allocated them from the stack. With gcc 4.5.0, some functions declared as inline are not being inlined. This patch forces these functions to always be inlined by adding the qualifier __attribute__((always_inline)) to their declaration. Update: Still no test reports for real hardware are available. If we cannot get this change tested on real hardware, I suggest we conditionally compile in only if gcc 4.5.0 or later is used. Thanks, Scott Signed-off-by: Scott Duplichan <scott@notabs.org>