Patchwork fix 'AMD Fam10 code breaks with gcc 4.5.0'

login
register
about
Submitter Scott
Date 2010-09-06 03:32:17
Message ID <2F6225DCDF084C9D8CFD78A8C6B10AA3@m3a78>
Download mbox | patch
Permalink /patch/1868/
State New
Headers show

Comments

Scott - 2010-09-06 03:32:17
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>
Stefan Reinauer - 2010-09-06 10:03:27
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
Marc Jones - 2010-09-17 21:39:36
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
Scott - 2010-09-17 22:40:22
]-----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;