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

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

[H. Peter Anvin]

Hi Linus,

Please pull:

  git://git.kernel.org/pub/scm/linux/kernel/git/hpa/linux-2.6-x86setup.git for-linus

H. Peter Anvin (1):
      [x86 setup] Correct the SMAP check for INT 0x15, AX=0xe820

 arch/i386/boot/memory.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

[Log messages and full diffs follow]

commit 24b4ac15fe9ea5772630e0c58d9b6243a6c68385
Author: H. Peter Anvin <hpa@zytor.com>
Date:   Thu Sep 27 17:17:12 2007 -0700

    [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));
 
 		/* Some BIOSes stop returning SMAP in the middle of
 		   the search loop.  We don't know exactly how the BIOS

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

[Linus Torvalds]

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

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

[Chuck Ebbert]

On 09/28/2007 10:27 AM, Linus Torvalds wrote:
> 
> 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.
> 

Patch with option (a) applied [output 0 changed to: "=d" (err)] tested and
works on the Dell XPS M1330 that was broken by the previous e820 change.

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

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> 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.
> 

Yes indeed.

> 
> 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?
> 

Will do.

	-hpa