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