mark %esi as clobbered in E820 BIOS call

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.

index 8c3c25f..fa85af7 100644
@@ -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

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

%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.

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(-)

index 8c3c25f..d5d2360 100644
@@ -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;

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

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 <[email protected]>
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.

index d5d2360..5054c2d 100644
@@ -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;
 

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.

Good idea.

Len Brown, Intel Open Source Technology Center

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?

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

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