Re: Regression in 2.6.23-pre Was: Problems with 2.6.23-rc6 on AMD Geode LX800

["Jordan Crouse" <jordan.crouse@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 2102 bytes --]

On 26/09/07 12:14 -0700, H. Peter Anvin wrote:
> Please try the following debug patch to let us know what is going on.
> 
> 	-hpa

> diff --git a/arch/i386/boot/memory.c b/arch/i386/boot/memory.c
> index 1a2e62d..a0ccf29 100644
> --- a/arch/i386/boot/memory.c
> +++ b/arch/i386/boot/memory.c
> @@ -33,6 +33,12 @@ static int detect_memory_e820(void)
>  		      "=m" (*desc)
>  		    : "D" (desc), "a" (0xe820));
>  
> +		printf("e820: err %d id 0x%08x next %u %08x:%08x %u\n",
> +		       err, id, next,
> +		       (unsigned int)desc->addr,
> +		       (unsigned int)desc->size,
> +		       desc->type);
> +
>  		if (err || id != SMAP)
>  			break;

Okay, we have clarity.   Here is the output

e820: err 0 id 0x534d4150 next 15476 00000000:0009fc00 1
e820: err 0 id 0x534d4150 next 15496 0009fc00:00000400 2
e820: err 0 id 0x534d4150 next 15516 000e0000:00020000 2
e820: err 0 id 0x0e7b0000 next 11536 00100000:0e6b0000 1

In the last entry,  id is obviously wrong (it should be 'SMAP' or
0x534d4150).  This is the BIOS bug.

Here's the reason why this bothers us now.  In the old assembly code,
if the returned ID wasn't equal to 'SMAP', we jumped straight to the e801
code.  In the new code in memory.c, if id != SMAP, we break out of the
int15 loop, and return boot_params.e820_entries, which in our case is
3.  detect_memory() considers this to be successful, and no attempt to
parse e801 is made.

So thats where the problem is - in the old code with the buggy BIOS, we
punted to reading the e801 information, and that was enough to keep us 
going.   In the new code, we allow a partial table to be used, and we
blow up.

Attached is a patch to fix this - it returns -1 on error, and only sets
boot_params.e820_entries to be non-zero if we have something useful
in it.  This punts the detection to the e801 code, which then is
then successful.

This fixes the problem with the DB800, and so it probably should
with the other Geode platforms affected by this.

Many thanks to hpa for the guiding hand.

Jordan

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

[-- Attachment #2: memory-bail-on-error.patch --]
[-- Type: text/plain, Size: 1168 bytes --]

[i386]: Return an error if the e820 detection goes bad

From: Jordan Crouse <jordan.crouse@amd.com>

Change the e820 code to always return an error if something
bad happens while reading the e820 map.  This matches the
old code behavior, and allows brain-dead e820 implementations
to still work.

Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
---

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

diff --git a/arch/i386/boot/memory.c b/arch/i386/boot/memory.c
index 1a2e62d..4c7f0f6 100644
--- a/arch/i386/boot/memory.c
+++ b/arch/i386/boot/memory.c
@@ -22,7 +22,7 @@ static int detect_memory_e820(void)
 {
 	u32 next = 0;
 	u32 size, id;
-	u8 err;
+	u8 err, count = 0;
 	struct e820entry *desc = boot_params.e820_map;
 
 	do {
@@ -34,13 +34,14 @@ static int detect_memory_e820(void)
 		    : "D" (desc), "a" (0xe820));
 
 		if (err || id != SMAP)
-			break;
+			return -1;
 
-		boot_params.e820_entries++;
+		count++;
 		desc++;
 	} while (next && boot_params.e820_entries < E820MAX);
 
-	return boot_params.e820_entries;
+	boot_params.e820_entries = count;
+	return count;
 }
 
 static int detect_memory_e801(void)