[PATCH] fix 'AMD Fam10 code breaks with gcc 4.5.0'

[Scott]

Hello,

The subversion comment for -r 5571 states:

The AMD Fam10 code breaks with coreboot 4.5.0.
   Potentially caused by reordering. Going back to 4.4.4
   which is known working on Fam10 until gcc or the Fam10 code is fixed.

Signed-off-by: Stefan Reinauer 
   Acked-by: Stefan Reinauer

I encountered the same problem and debugged it. The AP code that disables
cache as ram before the final halt has to be all inline. Function calls
require a valid stack, and the stack is kept in the very cache as ram that
the code is disabling. I found with gcc 450, the code for rdmsr, disable_cache,
and enable_cache and not getting inlined as intended. Function calls are
generated, and the first one after the AP clears msr 268 fails. The solution
is to force these functions to generate inline code by adding
__attribute__((always_inline)) to their declarations:

Index: src/include/cpu/x86/msr.h
===================================================================
--- src/include/cpu/x86/msr.h	(revision 5763)
+++ 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 5763)
+++ 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;

Thanks,
Scott

[Paul Menzel]

Dear Scott,

Am Donnerstag, den 02.09.2010, 00:40 -0500 schrieb Scott:
> The subversion comment for -r 5571 states:
>
> The AMD Fam10 code breaks with coreboot 4.5.0.
>    Potentially caused by reordering. Going back to 4.4.4
>    which is known working on Fam10 until gcc or the Fam10 code is fixed.
>
> Signed-off-by: Stefan Reinauer 
>    Acked-by: Stefan Reinauer
>
> I encountered the same problem and debugged it. The AP code that disables
> cache as ram before the final halt has to be all inline. Function calls
> require a valid stack, and the stack is kept in the very cache as ram that
> the code is disabling. I found with gcc 450, the code for rdmsr, disable_cache,
> and enable_cache and not getting inlined as intended. Function calls are

s/and/are/
> generated, and the first one after the AP clears msr 268 fails. The solution
> is to force these functions to generate inline code by adding
> __attribute__((always_inline)) to their declarations:

great find!!! Could you please send a patch according to the development
guidelines. Especially do not forget to add your Signed-off-by line.

Just to leave no doubt, could you please add that you tested your fix
using GCC 4.5.0 with the used hardware.

[…]

Thanks,

Paul

[Paul Menzel]

Dear Scott,

Am Donnerstag, den 02.09.2010, 10:59 +0200 schrieb Paul Menzel:
> Am Donnerstag, den 02.09.2010, 00:40 -0500 schrieb Scott:
> > The subversion comment for -r 5571 states:
> >
> > The AMD Fam10 code breaks with coreboot 4.5.0.
> >    Potentially caused by reordering. Going back to 4.4.4
> >    which is known working on Fam10 until gcc or the Fam10 code is fixed.
> >
> > Signed-off-by: Stefan Reinauer 
> >    Acked-by: Stefan Reinauer
> >
> > I encountered the same problem and debugged it. The AP code that disables
> > cache as ram before the final halt has to be all inline. Function calls
> > require a valid stack, and the stack is kept in the very cache as ram that
> > the code is disabling. I found with gcc 450, the code for rdmsr, disable_cache,
> > and enable_cache and not getting inlined as intended. Function calls are
>
> s/and/are/
> > generated, and the first one after the AP clears msr 268 fails. The solution
> > is to force these functions to generate inline code by adding
> > __attribute__((always_inline)) to their declarations:
>
> great find!!! Could you please send a patch according to the development
> guidelines. Especially do not forget to add your Signed-off-by line.

They are documented in the Wiki [1].
> Just to leave no doubt, could you please add that you tested your fix
> using GCC 4.5.0 with the used hardware.
>
> […]

Thanks,

Paul

[1] http://www.coreboot.org/Development_Guidelines

[Scott]

Signed-off-by: Scott Duplichan

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.

Thanks,
Scott

[Myles Watson]

> 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.

Should we do something like

#define INLINE __attribute__((always_inline))

and replace all (or most) inline directives with INLINE?  There are
probably other places where the code depends on inline working.

Thanks,
Myles

[Flemming Richter Mikkelsen]

On 2010-09-02, Myles Watson  wrote:
> > 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.
>
> Should we do something like
>
> #define INLINE __attribute__((always_inline))
>
> and replace all (or most) inline directives with INLINE?  There are
> probably other places where the code depends on inline working.

gcc supports -Winline and this warning should, if not already, be
enabled by default (in the Makefiles).

[Myles Watson]

On Fri, Sep 3, 2010 at 7:47 AM, Flemming Richter Mikkelsen
 wrote:
> On 2010-09-02, Myles Watson  wrote:
> > > 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.
> >
> > Should we do something like
> >
> > #define INLINE __attribute__((always_inline))
> >
> > and replace all (or most) inline directives with INLINE?  There are
> > probably other places where the code depends on inline working.
>
> gcc supports -Winline and this warning should, if not already, be
> enabled by default (in the Makefiles).

It isn't.  Tyan s2895 has 27 warnings just in romstage.c when -Winline
is specified.

Thanks,
Myles

[Patrick Georgi]

Am 03.09.2010 15:47, schrieb Flemming Richter Mikkelsen:
> gcc supports -Winline and this warning should, if not already, be
> enabled by default (in the Makefiles).

I doubt this option is really very useful. We often inline at places
where we advise it, but where there's no harm if it isn't inlined.
The code that requires that attribute _requires_ inlining.

Given that we use -Werror, -Winline (which is triggered by heuristics)
is a really bad idea in my opinion.

Regards,
Patrick

[Peter Stuge]

Scott wrote:
> The AMD Tilapia BIOS built with gcc 4.5.0

Please keep in mind that coreboot is not a BIOS.

Some/many use the term BIOS to mean any firmware, but since coreboot
only does one part of what a BIOS would do I think it's important to
keep the two terms separate.

There are e.g. no interrupt services at all in coreboot, and the
generic payload interface in coreboot are IMO distinguishing features
for coreboot, making it difficult to equate coreboot with a BIOS.

If coreboot is used together with a SeaBIOS payload in a system then
there is certainly a BIOS, but that would be SeaBIOS then.

//Peter

[Scott Duplichan]

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

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;

[Stefan Reinauer]

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

With the explanation above added to each of the two files, this is

Acked-by: Stefan Reinauer

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

[Marc Jones]

On Mon, Sep 6, 2010 at 4:03 AM, Stefan Reinauer
 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
>
> With the explanation above added to each of the two files, this is
>
> Acked-by: Stefan Reinauer
>
> 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

[Scott Duplichan]

]-----Original Message-----
]From: Marc Jones [mailto:[e-mail]] 
]Sent: Friday, September 17, 2010 04:40 PM
]To: Stefan Reinauer; Frank Vibrans; Scott
]Cc: [e-mail]
]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
] 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 
]>
]> With the explanation above added to each of the two files, this is
]>
]> Acked-by: Stefan Reinauer 
]>
]> 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