Ghost EDD devices in /sys again

Ghost EDD devices in /sys again

[Andrey Borzenkov]

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

This is regression but old enough. Apparently I had for whatever reasons
EDD turned off till recently. This is 2.6.27-rc5 just in case.

In 2006 I fixed ghost devices due to buggy BIOS:

http://marc.info/?l=linux-kernel&m=114087765422490&w=2

Later edd.S has been rewritten in C, and apparently this patch has been
lost:

{pts/1}% ls /sys/firmware/edd
int13_dev80/  int13_dev84/  int13_dev88/  int13_dev8c/
int13_dev81/  int13_dev85/  int13_dev89/  int13_dev8d/
int13_dev82/  int13_dev86/  int13_dev8a/  int13_dev8e/
int13_dev83/  int13_dev87/  int13_dev8b/  int13_dev8f/

But I have just a single disk. This is the same system BTW.

Unfortunately I do not speak asm language of C; if somebody could provide
equivalent fix for current edd.c, I am more than ready to test it.

Thank you

-andrey

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] Ghost EDD devices in /sys again

[Andrey Borzenkov]

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

On Friday 05 September 2008, Andrey Borzenkov wrote:
> This is regression but old enough. Apparently I had for whatever reasons
> EDD turned off till recently. This is 2.6.27-rc5 just in case.
> 
> In 2006 I fixed ghost devices due to buggy BIOS:
> 
> http://marc.info/?l=linux-kernel&m=114087765422490&w=2
> 
> Later edd.S has been rewritten in C, and apparently this patch has been
> lost:
> 
> {pts/1}% ls /sys/firmware/edd
> int13_dev80/  int13_dev84/  int13_dev88/  int13_dev8c/
> int13_dev81/  int13_dev85/  int13_dev89/  int13_dev8d/
> int13_dev82/  int13_dev86/  int13_dev8a/  int13_dev8e/
> int13_dev83/  int13_dev87/  int13_dev8b/  int13_dev8f/
> 
> But I have just a single disk. This is the same system BTW.
> 
> Unfortunately I do not speak asm language of C; if somebody could provide
> equivalent fix for current edd.c, I am more than ready to test it.

Was it really that simple?

Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
From: Andrey Borzenkov <arvidjaar@mail.ru>

Some BIOSes do not always set CF on error before return from int13.
The patch adds additional check for status being zero (AH == 0).

This was fixed for edd.S in
http://marc.info/?l=linux-kernel&m=114087765422490&w=2, but lost
again when edd.S was rewritten in C.

Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>

---

 arch/x86/boot/edd.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)


diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
index d93cbc6..bf4ae6f 100644
--- a/arch/x86/boot/edd.c
+++ b/arch/x86/boot/edd.c
@@ -32,7 +32,9 @@ static int read_mbr(u8 devno, void *buf)
 		     : "+a" (ax), "+c" (cx), "+d" (dx), "+b" (bx)
 		     : : "esi", "edi", "memory");
 
-	return -(u8)ax;		/* 0 or -1 */
+	/* Some BIOSes do not set carry flag on error but still return
+	 * error in AH. The condition below is expected to catch both */
+	return -!!ax;		/* 0 or -1 */
 }
 
 static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Ghost EDD devices in /sys again

[Ingo Molnar]

* Andrey Borzenkov <arvidjaar@newmail.ru> wrote:

> On Friday 05 September 2008, Andrey Borzenkov wrote:
> > This is regression but old enough. Apparently I had for whatever reasons
> > EDD turned off till recently. This is 2.6.27-rc5 just in case.
> > 
> > In 2006 I fixed ghost devices due to buggy BIOS:
> > 
> > http://marc.info/?l=linux-kernel&m=114087765422490&w=2
> > 
> > Later edd.S has been rewritten in C, and apparently this patch has been
> > lost:
> > 
> > {pts/1}% ls /sys/firmware/edd
> > int13_dev80/  int13_dev84/  int13_dev88/  int13_dev8c/
> > int13_dev81/  int13_dev85/  int13_dev89/  int13_dev8d/
> > int13_dev82/  int13_dev86/  int13_dev8a/  int13_dev8e/
> > int13_dev83/  int13_dev87/  int13_dev8b/  int13_dev8f/
> > 
> > But I have just a single disk. This is the same system BTW.
> > 
> > Unfortunately I do not speak asm language of C; if somebody could provide
> > equivalent fix for current edd.c, I am more than ready to test it.
> 
> Was it really that simple?
> 
> Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
> From: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> Some BIOSes do not always set CF on error before return from int13.
> The patch adds additional check for status being zero (AH == 0).
> 
> This was fixed for edd.S in
> http://marc.info/?l=linux-kernel&m=114087765422490&w=2, but lost
> again when edd.S was rewritten in C.
> 
> Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> ---
> 
>  arch/x86/boot/edd.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
> index d93cbc6..bf4ae6f 100644
> --- a/arch/x86/boot/edd.c
> +++ b/arch/x86/boot/edd.c
> @@ -32,7 +32,9 @@ static int read_mbr(u8 devno, void *buf)
>  		     : "+a" (ax), "+c" (cx), "+d" (dx), "+b" (bx)
>  		     : : "esi", "edi", "memory");
>  
> -	return -(u8)ax;		/* 0 or -1 */
> +	/* Some BIOSes do not set carry flag on error but still return
> +	 * error in AH. The condition below is expected to catch both */
> +	return -!!ax;		/* 0 or -1 */
>  }
>  
>  static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)

looks good to me - applied it to tip/x86/urgent. Thanks Andrey!

	Ingo

Re: [PATCH] Ghost EDD devices in /sys again

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> looks good to me - applied it to tip/x86/urgent. Thanks Andrey!

ok - Peter says he'd rather like to investigate this some more and have 
the fix in v2.6.28 instead - so i moved it from tip/x86/urgent to 
tip/x86/setup.

	Ingo

Re: [PATCH] Ghost EDD devices in /sys again

[H. Peter Anvin]

Andrey Borzenkov wrote:
> 
> Was it really that simple?
> 
> Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
> From: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> Some BIOSes do not always set CF on error before return from int13.
> The patch adds additional check for status being zero (AH == 0).
> 

Do you have any evidence that there aren't any BIOSes which leave AH != 
0 on legitimate success?  If so, you have turned a nuisance bug into 
something that could cause install failures.

	-hpa

Re: [PATCH] Ghost EDD devices in /sys again

[Andrey Borzenkov]

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

On Saturday 06 September 2008, H. Peter Anvin wrote:
> 
> Andrey Borzenkov wrote:
> > 
> > Was it really that simple?
> > 
> > Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
> > From: Andrey Borzenkov <arvidjaar@mail.ru>
> > 
> > Some BIOSes do not always set CF on error before return from int13.
> > The patch adds additional check for status being zero (AH == 0).
> > 
> 
> Do you have any evidence that there aren't any BIOSes which leave AH != 
> 0 on legitimate success?  If so, you have turned a nuisance bug into 
> something that could cause install failures.
> 

Hmm ... if install depends on having valid MBR, it will break right
now as well, because it will have duplicated signatures. So at least
something like following is needed.

diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
index d93cbc6..4c20d31 100644
--- a/arch/x86/boot/edd.c
+++ b/arch/x86/boot/edd.c
@@ -58,6 +58,7 @@ static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)
        if (mbrbuf_end > (char *)(size_t)boot_params.hdr.heap_end_ptr)
                return -1;

+       memset(mbrbuf_ptr, 0, sector_size);
        if (read_mbr(devno, mbrbuf_ptr))
                return -1;


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]