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

Scott Duplichan scott at notabs.org
Sat Sep 18 00:40:22 CEST 2010


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





More information about the coreboot mailing list