Hello,
My co-worker sent me a patch many months ago to forward upstream as appropriate, but apparently I had forgotten to do so and now hit the bug with my compiler version as well while testing something from SVN.
So attached is that patch now, so that flash chip detection would work again on my Geode/CS5536 system.
PS: I'm not subscribed to flashrom list right now, so please CC me on any replies
Regards, Mart Raudsepp
Am Dienstag, den 18.05.2010, 01:30 +0300 schrieb Mart Raudsepp:
So attached is that patch now, so that flash chip detection would work again on my Geode/CS5536 system.
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Regards, Michael Karcher
Hi, I'd like to know a bit more.
This basic use of the msr device has worked for a very long time. I'm not saying there is not a problem, but if there is, it might be best to change the struct than to add this code.
Do you have some assembly code showing how it failed?
For example ... certainly, in user mode, why isn't the msr_t just a 64-bit number?
ron
On 18.05.2010 01:20, ron minnich wrote:
Hi, I'd like to know a bit more.
This basic use of the msr device has worked for a very long time. I'm not saying there is not a problem, but if there is, it might be best to change the struct than to add this code.
Do you have some assembly code showing how it failed?
For example ... certainly, in user mode, why isn't the msr_t just a 64-bit number?
Legacy? MSR access in flashrom is one of the areas (besides cbtable support and layout support) which still shows a coreboot heritage. coreboot has a struct msr_struct (typedef'ed to msr_t) and flashrom somehow inherited this. We could make it a 64bit number, yes. The question is why it isn't a 64bit number in coreboot, and unless the reason there is that nobody bothered to change it, I think we should avoid any traps in flashrom. OTOH, I don't want to employ cargo cult programming just because the MSR situation feels a bit odd.
I'd happily use a solution which reduces potential breakage, so fire away.
Regards, Carl-Daniel
Hello,
On E, 2010-05-17 at 16:20 -0700, ron minnich wrote:
Hi, I'd like to know a bit more.
This basic use of the msr device has worked for a very long time. I'm not saying there is not a problem, but if there is, it might be best to change the struct than to add this code.
Right. This got broken on our toolchains ever since this struct was introduced to flashrom fater 0.9 release. I'm not even sure the same thing hit me now and him back in October, but the same patch fixes it. It would be weird if this is a new regression in newer GCC version though, as that would be an ABI break I believe..
Do however note that rdmsr goes through a temporary uint32_t array as well, just like this patch makes wrmsr. So with this patch it's sort of uniform that way too.
Do you have some assembly code showing how it failed?
No I didn't. But now I got some :) Attached is the physmap.c file from r998 compiled to assembly with all the same optimization and other flags as the object file is made, just saved to assembly with -S option. So if anyone can read from that what's going on, then feel free to; I don't speak assembler well :(
For example ... certainly, in user mode, why isn't the msr_t just a 64-bit number?
Don't really know. All I know is that C language does not really give a guarantee of structs being tightly packed (are there other rules though, like for default alignment instead of compiler specific?), unless using compiler extensions like GCC packed attribute. I have it hard to imagine why it would be adding alignment padding in there either, if that's what it is doing per the attached assembler. However sizeof(msr_t) seem to be returning 8 as desired...
Mart
On 18.05.2010 14:19, Mart Raudsepp wrote:
Hello,
On E, 2010-05-17 at 16:20 -0700, ron minnich wrote:
Hi, I'd like to know a bit more.
This basic use of the msr device has worked for a very long time. I'm not saying there is not a problem, but if there is, it might be best to change the struct than to add this code.
Right. This got broken on our toolchains ever since this struct was introduced to flashrom fater 0.9 release. I'm not even sure the same thing hit me now and him back in October, but the same patch fixes it. It would be weird if this is a new regression in newer GCC version though, as that would be an ABI break I believe..
Do however note that rdmsr goes through a temporary uint32_t array as well, just like this patch makes wrmsr. So with this patch it's sort of uniform that way too.
Do you have some assembly code showing how it failed?
No I didn't. But now I got some :) Attached is the physmap.c file from r998 compiled to assembly with all the same optimization and other flags as the object file is made, just saved to assembly with -S option. So if anyone can read from that what's going on, then feel free to; I don't speak assembler well :(
For example ... certainly, in user mode, why isn't the msr_t just a 64-bit number?
Don't really know. All I know is that C language does not really give a guarantee of structs being tightly packed (are there other rules though, like for default alignment instead of compiler specific?), unless using compiler extensions like GCC packed attribute. I have it hard to imagine why it would be adding alignment padding in there either, if that's what it is doing per the attached assembler. However sizeof(msr_t) seem to be returning 8 as desired...
Thanks, r999.
We will need to revisit this in the future once we want to run flashrom on top of libpayload. Until then, this fix is good enough unless someone sends a patch to convert the MSR stuff to a 64bit variable.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Mart Raudsepp -
Michael Karcher -
ron minnich