Re: [GIT PULL] Correct the SMAP check in the e820 probe

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, 28 Sep 2007, H. Peter Anvin wrote:
>
>     [x86 setup] Correct the SMAP check for INT 0x15, AX=0xe820
>     
>     The e820 probe code was checking %edx, not %eax, for the SMAP
>     signature on return.  This worked on *almost* all systems, since %edx
>     still contained SMAP from the call on entry, but on a handful of
>     systems it failed -- plus, we would have missed real mismatches.
>     
>     Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> 
> diff --git a/arch/i386/boot/memory.c b/arch/i386/boot/memory.c
> index bccaa1c..2f37568 100644
> --- a/arch/i386/boot/memory.c
> +++ b/arch/i386/boot/memory.c
> @@ -28,11 +28,10 @@ static int detect_memory_e820(void)
>  
>  	do {
>  		size = sizeof(struct e820entry);
> -		id = SMAP;
>  		asm("int $0x15; setc %0"
> -		    : "=am" (err), "+b" (next), "+d" (id), "+c" (size),
> +		    : "=dm" (err), "+b" (next), "=a" (id), "+c" (size),
>  		      "=m" (*desc)
> -		    : "D" (desc), "a" (0xe820));
> +		    : "D" (desc), "d" (SMAP), "a" (0xe820));

Hmm. If I read this correctly, I don't think this can be right.

Why? You don't mark %edx as possibly corrupted by the asm any more.

The "=dm" means that quite often (probably effectively always), gcc will 
allocate %edx to be the output register for %0, but at least in theory, it 
could easily decide that it's going to put %0 in memory, and in that case, 
it may well decide that %edx is not modified by the asm statement. Which 
may or may not be true - I'd bet that there are BIOSes out there that *do* 
modify it.

So what happens then? If gcc decides that %edx isn't modified by the asm, 
it will assume that it still contains the value it had on entry, which is 
the "SMAP" value, and then it might decide to do the

	if (id != SMAP) {

check as a 

	cmpl %edx,%eax

since the "id" return is in %eax, and the compiler decides that it may be 
cheaper to re-use the register that already contains the constant, than to 
use a (longer) compare instruction with an explicit constant.

IOW, I think you need to either (a) _force_ gcc to use %edx for the "err" 
return, avoiding this issue, or (b) mark edx clobbered (which in turn 
means that you need to remove it from the output constraint for "err"). I 
suspect (a) is simpler/more straightforward.

I haven't pulled this, so maybe you can just amend the commit in-place, 
and we can avoid?

			Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/