Patchwork fix 'AMD Fam10 code breaks with gcc 4.5.0'

login
register
about
Submitter Scott
Date 2010-09-02 21:03:35
Message ID <1446358636964B60A97BC93F7ACA5FDC@m3a78>
Download mbox | patch
Permalink /patch/1854/
State New
Headers show

Comments

Scott - 2010-09-02 21:03:35
Signed-off-by: Scott Duplichan <scott@notabs.org>

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 - 2010-09-02 21:42:50
> 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
Peter Stuge - 2010-09-02 21:51:58
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
Flemming Richter Mikkelsen - 2010-09-03 13:47:13
On 2010-09-02, Myles Watson <mylesgw@gmail.com> 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 - 2010-09-03 14:46:39
On Fri, Sep 3, 2010 at 7:47 AM, Flemming Richter Mikkelsen
<quatrox@member.fsf.org> wrote:
> On 2010-09-02, Myles Watson <mylesgw@gmail.com> 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 - 2010-09-03 14:48:43
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

Patch

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