[PATCH] mark %esi as clobbered in E820 BIOS call

[PATCH] mark %esi as clobbered in E820 BIOS call

[Michael K. Johnson]

Justin and Peter, please review this for stable .y trees.  Peter,
please review this for trunk unless you decide to do a more 
extensive workaround for BIOS register clobbering.

Thanks!

$ cat e820-esi-clobber-workaround.patch
Jordan Hargrave diagnosed a BIOS clobbering %esi in the E820 call.
That particular BIOS has been fixed, but there is a possibility that
this is responsible for other occasional reports of early boot
failure, and it does not hurt to add %esi to the clobbers.

Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Justin Forbes <jmforbes@linuxtx.org>

Signed-off-by: Michael K Johnson <johnsonm@rpath.com>

diff --git a/arch/x86/boot/memory.c b/arch/x86/boot/memory.c
index 8c3c25f..fa85af7 100644
--- a/arch/x86/boot/memory.c
+++ b/arch/x86/boot/memory.c
@@ -27,13 +27,14 @@ static int detect_memory_e820(void)
 	do {
 		size = sizeof(struct e820entry);
 
-		/* Important: %edx is clobbered by some BIOSes,
-		   so it must be either used for the error output
+		/* Important: %edx and %esi are clobbered by some BIOSes,
+		   so they must be either used for the error output
 		   or explicitly marked clobbered. */
 		asm("int $0x15; setc %0"
 		    : "=d" (err), "+b" (next), "=a" (id), "+c" (size),
 		      "=m" (*desc)
-		    : "D" (desc), "d" (SMAP), "a" (0xe820));
+		    : "D" (desc), "d" (SMAP), "a" (0xe820)
+		    : "%esi");
 
 		/* BIOSes which terminate the chain with CF = 1 as opposed
 		   to %ebx = 0 don't always report the SMAP signature on

Re: [PATCH] mark %esi as clobbered in E820 BIOS call

[Ingo Molnar]

* Michael K. Johnson <johnsonm@rpath.com> wrote:

> Justin and Peter, please review this for stable .y trees.  Peter,
> please review this for trunk unless you decide to do a more 
> extensive workaround for BIOS register clobbering.
> 
> Thanks!
> 
> $ cat e820-esi-clobber-workaround.patch
> Jordan Hargrave diagnosed a BIOS clobbering %esi in the E820 call.
> That particular BIOS has been fixed, but there is a possibility that
> this is responsible for other occasional reports of early boot
> failure, and it does not hurt to add %esi to the clobbers.

nice fix! Would you mind to update the clobber list to include _all_ 
registers? I dont see why any of the other registers couldnt be 
clobbered by a BIOS, and this is boot-only code so micro-performance 
is not an issue.

	Ingo

Re: [PATCH] mark %esi as clobbered in E820 BIOS call

[H. Peter Anvin]

Ingo Molnar wrote:
> * Michael K. Johnson <johnsonm@rpath.com> wrote:
> 
>> Justin and Peter, please review this for stable .y trees.  Peter,
>> please review this for trunk unless you decide to do a more 
>> extensive workaround for BIOS register clobbering.
>>
>> Thanks!
>>
>> $ cat e820-esi-clobber-workaround.patch
>> Jordan Hargrave diagnosed a BIOS clobbering %esi in the E820 call.
>> That particular BIOS has been fixed, but there is a possibility that
>> this is responsible for other occasional reports of early boot
>> failure, and it does not hurt to add %esi to the clobbers.
> 
> nice fix! Would you mind to update the clobber list to include _all_ 
> registers? I dont see why any of the other registers couldnt be 
> clobbered by a BIOS, and this is boot-only code so micro-performance 
> is not an issue.

%esi and %ebp are the only registers that aren't already clobbered.  gcc
doesn't like %ebp clobbers, so it has to be done via push..pop.  I have
been thinking about doing a generic register-save wrapper for *all* BIOS
calls; this plus the recent patch to do a bunch of similar hacks for
VESA calls kind of tells me it's time to do this.  However, I'm going to
clean up this for the trunk and Cc: stable for now.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[GIT PULL] x86 setup BIOS workarounds

[H. Peter Anvin]

Hi Linus,

These three patches work around one identified and two hypothetical
BIOS problems with the E820 memory probe.

The identified problem is one specific box which clobbers %esi; the
other two patches protect %edi and %ebp, and implements handling for
the backwards-incompatible(!) E820 handling in ACPI 3.


The following changes since commit 15f7176eb1cccec0a332541285ee752b935c1c85:
  Linus Torvalds (1):
        Merge git://git.kernel.org/.../davem/net-2.6

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git x86/setup

H. Peter Anvin (2):
      x86, setup: preemptively save/restore edi and ebp around INT 15 E820
      x86, setup: ACPI 3, BIOS workaround for E820-probing code

Michael K. Johnson (1):
      x86, setup: mark %esi as clobbered in E820 BIOS call

 arch/x86/boot/memory.c |   33 ++++++++++++++++++++++++---------
 1 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/memory.c b/arch/x86/boot/memory.c
index 8c3c25f..d5d2360 100644
--- a/arch/x86/boot/memory.c
+++ b/arch/x86/boot/memory.c
@@ -2,6 +2,7 @@
  *
  *   Copyright (C) 1991, 1992 Linus Torvalds
  *   Copyright 2007 rPath, Inc. - All Rights Reserved
+ *   Copyright 2009 Intel Corporation; author H. Peter Anvin
  *
  *   This file is part of the Linux kernel, and is made available under
  *   the terms of the GNU General Public License version 2.
@@ -16,24 +17,32 @@
 
 #define SMAP	0x534d4150	/* ASCII "SMAP" */
 
+struct e820_ext_entry {
+	struct e820entry std;
+	u32 ext_flags;
+} __attribute__((packed));
+
 static int detect_memory_e820(void)
 {
 	int count = 0;
 	u32 next = 0;
-	u32 size, id;
+	u32 size, id, edi;
 	u8 err;
 	struct e820entry *desc = boot_params.e820_map;
+	static struct e820_ext_entry buf; /* static so it is zeroed */
 
 	do {
-		size = sizeof(struct e820entry);
+		size = sizeof buf;
 
-		/* Important: %edx is clobbered by some BIOSes,
-		   so it must be either used for the error output
-		   or explicitly marked clobbered. */
-		asm("int $0x15; setc %0"
+		/* Important: %edx and %esi are clobbered by some BIOSes,
+		   so they must be either used for the error output
+		   or explicitly marked clobbered.  Given that, assume there
+		   is something out there clobbering %ebp and %edi, too. */
+		asm("pushl %%ebp; int $0x15; popl %%ebp; setc %0"
 		    : "=d" (err), "+b" (next), "=a" (id), "+c" (size),
-		      "=m" (*desc)
-		    : "D" (desc), "d" (SMAP), "a" (0xe820));
+		      "=D" (edi), "+m" (buf)
+		    : "D" (&buf), "d" (SMAP), "a" (0xe820)
+		    : "esi");
 
 		/* BIOSes which terminate the chain with CF = 1 as opposed
 		   to %ebx = 0 don't always report the SMAP signature on
@@ -51,8 +60,14 @@ static int detect_memory_e820(void)
 			break;
 		}
 
+		/* ACPI 3.0 added the extended flags support.  If bit 0
+		   in the extended flags is zero, we're supposed to simply
+		   ignore the entry -- a backwards incompatible change! */
+		if (size > 20 && !(buf.ext_flags & 1))
+			continue;
+
+		*desc++ = buf.std;
 		count++;
-		desc++;
 	} while (next && count < ARRAY_SIZE(boot_params.e820_map));
 
 	return boot_params.e820_entries = count;

Re: [GIT PULL] x86 setup BIOS workarounds

[Linus Torvalds]

On Wed, 1 Apr 2009, H. Peter Anvin wrote:
> 
> implements handling for the backwards-incompatible(!) E820 handling in 
> ACPI 3.

I am _extremely_ nervous about this one.

You do

	size = sizeof buf;	/* ACPI-3 size */
	asm(.. "+c" (size)	/* size might change */
	..
	if (size > 20 && !(buf.ext_flags & 1))
		continue;

ie you are expecting that _all_ old pre-ACPI-3 BIOSES will always set size 
to 20, or always write a low-bit-set value to that extended flag field 
that doesn't even exist previously.

I don't think that's likely true. Quite frankly, I'd expect a number of 
BIOSen to entirely ignore %ecx, since it's irrelevant (it _has_ to be 
bigger than 20 anyway on entry, and I doubt anybody really ever bothered 
to test that it's 20 on exit).

So at a _minimum_, I'd suggest that we set bug.ext_flags to 1 before the 
call - so that if some random BIOS just leaves %ecx unchanged, it won't 
mean that the area just gets ignored as a ACPI-3 entry.

And if it really _is_ a new ACPI-3 BIOS, it will overwrite ext_flags with 
its own value.

I've pulled you changes, but I really don't like how they seem to be 
designed to be very fragile, and designed to expect BIOSes to actually 
implement documented interfaces without bugs. So please look at it.

		Linus

[GIT PULL] x86 setup BIOS workarounds

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> So at a _minimum_, I'd suggest that we set bug.ext_flags to 1 before the 
> call - so that if some random BIOS just leaves %ecx unchanged, it won't 
> mean that the area just gets ignored as a ACPI-3 entry.
> 

Argh, I had that in there originally, and then I shuffled stuff around
and it got left out.  Total thinko on my part.  I'm sorry, and thanks
for catching that.

I have had the ACPI 3 stuff in Syslinux for quite a while now (*with*
the flag initialization) and it hasn't caused problems there, so I
feel it should be safe to push into the kernel.

The following changes since commit c549e71d073a6e9a4847497344db28a784061455:
  H. Peter Anvin (1):
        x86, setup: ACPI 3, BIOS workaround for E820-probing code

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git x86-setup-for-linus

H. Peter Anvin (1):
      x86, setup: guard against pre-ACPI 3 e820 code not updating %ecx

 arch/x86/boot/memory.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

commit cd670599b7b00d9263f6f11a05c0edeb9cbedaf3
Author: H. Peter Anvin <hpa@zytor.com>
Date:   Wed Apr 1 11:35:00 2009 -0700

    x86, setup: guard against pre-ACPI 3 e820 code not updating %ecx
    
    Impact: BIOS bug safety
    
    For pre-ACPI 3 BIOSes, pre-initialize the end of the e820 buffer just
    in case the BIOS returns an unchanged %ecx but without actually
    touching the ACPI 3 extended flags field.
    
    Signed-off-by: H. Peter Anvin <hpa@zytor.com>

diff --git a/arch/x86/boot/memory.c b/arch/x86/boot/memory.c
index d5d2360..5054c2d 100644
--- a/arch/x86/boot/memory.c
+++ b/arch/x86/boot/memory.c
@@ -31,6 +31,12 @@ static int detect_memory_e820(void)
 	struct e820entry *desc = boot_params.e820_map;
 	static struct e820_ext_entry buf; /* static so it is zeroed */
 
+	/*
+	 * Set this here so that if the BIOS doesn't change this field
+	 * but still doesn't change %ecx, we're still okay...
+	 */
+	buf.ext_flags = 1;
+
 	do {
 		size = sizeof buf;
 

Re: [GIT PULL] x86 setup BIOS workarounds

[Len Brown]

> +		/* ACPI 3.0 added the extended flags support.  If bit 0
> +		   in the extended flags is zero, we're supposed to simply
> +		   ignore the entry -- a backwards incompatible change! */
> +		if (size > 20 && !(buf.ext_flags & 1))
> +			continue;


At the risk of rushing to the defense of the ACPI spec...

This does not look like a backwards incompatible change to me.

In ACPI 2.0, size of 20 is always returned, and it would
be a Linux bug if we examined the undefined values after byte 19.

In ACPI 3.0, byte 20 is now defined.  So if the BIOS returns
a size >= 21, we are permitted to examine byte 20.

So I agree with the test above, but I do not agree with the comment.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [GIT PULL] x86 setup BIOS workarounds

[Len Brown]

On Wed, 1 Apr 2009, Linus Torvalds wrote:

> 
> 
> On Wed, 1 Apr 2009, H. Peter Anvin wrote:
> > 
> > implements handling for the backwards-incompatible(!) E820 handling in 
> > ACPI 3.
> 
> I am _extremely_ nervous about this one.
> 
> You do
> 
> 	size = sizeof buf;	/* ACPI-3 size */
> 	asm(.. "+c" (size)	/* size might change */
> 	..
> 	if (size > 20 && !(buf.ext_flags & 1))
> 		continue;
> 
> ie you are expecting that _all_ old pre-ACPI-3 BIOSES will always set size 
> to 20, or always write a low-bit-set value to that extended flag field 
> that doesn't even exist previously.

Yes, this expects old BIOS to always return 20.
No, it does not expect old BIOS to have any particular value
in buf.ext_flags -- since that is examined only for size > 20.

> I don't think that's likely true. Quite frankly, I'd expect a number of 
> BIOSen to entirely ignore %ecx, since it's irrelevant (it _has_ to be 
> bigger than 20 anyway on entry, and I doubt anybody really ever bothered 
> to test that it's 20 on exit).
> 
> So at a _minimum_, I'd suggest that we set bug.ext_flags to 1 before the 
> call - so that if some random BIOS just leaves %ecx unchanged, it won't 
> mean that the area just gets ignored as a ACPI-3 entry.

Good idea.

Len Brown, Intel Open Source Technology Center

Re: [GIT PULL] x86 setup BIOS workarounds

[Linus Torvalds]

On Thu, 2 Apr 2009, Len Brown wrote:
> 
> Yes, this expects old BIOS to always return 20.

Do you have any reason to expect that all BIOS'es are bug-free in this 
area? 

That would be a first.

We already check for other error cases where the BIOS didn't do the right 
thing in other ways in its e820 routine, or clobbered the wrogn registers 
or whatever. Why would you expect that the return value would always be 
ok?

> No, it does not expect old BIOS to have any particular value
> in buf.ext_flags -- since that is examined only for size > 20.

The point is, that expectation that the BIOS returns 20 seems very 
unreasonable. BIOS writers tend to have been on pain medication for so 
long that they can hardly remember their own name, much less actually make 
sure they follow all the documentation.

Now, if Windows has actually _depended_ on the right return value since 
Win95, that would be a good, strong argument.

Because that's the only case where we can pretty much depend on BIOS 
writers get things right - if Windows doesn't boot when they get it wrong. 
As far as I can tell, that has always been the only real quality assurance 
for most BIOS'es.

			Linus

Re: [GIT PULL] x86 setup BIOS workarounds

[H. Peter Anvin]

Len Brown wrote:
> 
> At the risk of rushing to the defense of the ACPI spec...
> 
> This does not look like a backwards incompatible change to me.
> 
> In ACPI 2.0, size of 20 is always returned, and it would
> be a Linux bug if we examined the undefined values after byte 19.
> 
> In ACPI 3.0, byte 20 is now defined.  So if the BIOS returns
> a size >= 21, we are permitted to examine byte 20.
> 
> So I agree with the test above, but I do not agree with the comment.
> 

If you look at the details of the ACPI spec, this flag is explicitly 
specified so that the BIOS can always return a fixed number of entries. 
  Now, a clever BIOS could of course skip these entries if the OS only 
requests 20 bytes -- but if it can do that, it could just equally well 
have *always* skipped those entries!  So the flag is useless.

However, the ACPI 3 spec is written so to actively lead people into 
implement things this way, and given BIOS developer track records, they 
would simply truncate the result to 20 bytes if the size passed in is 
20.  This, of course, means that it is now reporting an invalid entry, 
without retaining the information that it is invalid...

	-hpa