[PATCH] Make bootsector stub 16-bit-only (i386)

[PATCH] Make bootsector stub 16-bit-only (i386)

[Alexander van Heukelum]

Hi!

The x86 bzImage contains a stub to inform people that it is not possible
any more to run a Linux kernel by catting it to a floppy and then
booting from it. This was meant to be all 16-bit code. The first
instruction, however, ended up as being coded as a 16:32-bit far jump. I
assume the intention was a 16:16-bit far jump.

This patch changes only i386.

Greetings,
	Alexander

Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>

---

diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
index 011b7a4..ae9df0d 100644
--- a/arch/i386/boot/bootsect.S
+++ b/arch/i386/boot/bootsect.S
@@ -44,7 +44,7 @@ #endif
 _start:
 
 	# Normalize the start address
-	jmpl	$BOOTSEG, $start2
+	jmpw	$BOOTSEG, $start2
 
 start2:
 	movw	%cs, %ax

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Acked-by: H. Peter Anvin <hpa@zytor.com>

Alexander van Heukelum wrote:
> Hi!
> 
> The x86 bzImage contains a stub to inform people that it is not possible
> any more to run a Linux kernel by catting it to a floppy and then
> booting from it. This was meant to be all 16-bit code. The first
> instruction, however, ended up as being coded as a 16:32-bit far jump. I
> assume the intention was a 16:16-bit far jump.
> 
> This patch changes only i386.
> 
> Greetings,
> 	Alexander
> 
> Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>
> 
> ---
> 
> diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
> index 011b7a4..ae9df0d 100644
> --- a/arch/i386/boot/bootsect.S
> +++ b/arch/i386/boot/bootsect.S
> @@ -44,7 +44,7 @@ #endif
>  _start:
>  
>  	# Normalize the start address
> -	jmpl	$BOOTSEG, $start2
> +	jmpw	$BOOTSEG, $start2
>  
>  start2:
>  	movw	%cs, %ax

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Andrew Morton]

On Sat, 5 May 2007 12:44:52 +0200 Alexander van Heukelum <heukelum@mailshack.com> wrote:

> The x86 bzImage contains a stub to inform people that it is not possible
> any more to run a Linux kernel by catting it to a floppy and then
> booting from it. This was meant to be all 16-bit code. The first
> instruction, however, ended up as being coded as a 16:32-bit far jump. I
> assume the intention was a 16:16-bit far jump.
> 
> This patch changes only i386.
> 
> Greetings,
> 	Alexander
> 
> Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>
> 
> ---
> 
> diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
> index 011b7a4..ae9df0d 100644
> --- a/arch/i386/boot/bootsect.S
> +++ b/arch/i386/boot/bootsect.S
> @@ -44,7 +44,7 @@ #endif
>  _start:
>  
>  	# Normalize the start address
> -	jmpl	$BOOTSEG, $start2
> +	jmpw	$BOOTSEG, $start2

Sigh, another blow struck in the ongoing struggle between my Vaio and the
rest of the world.

Stone-cold black-screen lockup immediately upon boot.

Stock FC5 install, config at
http://userweb.kernel.org/~akpm/config-sony.txt

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Andrew Morton wrote:

>>  
>>  	# Normalize the start address
>> -	jmpl	$BOOTSEG, $start2
>> +	jmpw	$BOOTSEG, $start2
> 
> Sigh, another blow struck in the ongoing struggle between my Vaio and the
> rest of the world.
> 
> Stone-cold black-screen lockup immediately upon boot.
> 
> Stock FC5 install, config at
> http://userweb.kernel.org/~akpm/config-sony.txt

Andrew, I'm seriously starting to think there is something fundamentally
wrong with that test setup.

The bootsect code in question is never executed.  AT ALL.  The only
raison d'être for it at all is to print an error message if someone
writes the kernel to a raw floppy disk.  Nor does it change the
alignment of the header or anything else to that effect -- the assembly
code downstream has an explicit ".org" directive.  For what it's worth,
just to make sure I'm not crazy, I just re-tested both booting the
kernel and booting the raw disk image, in simulation and on real
hardware, and it doesn't change anything.  I used your configuration
file (yes '' | make oldconfig) minus Bluetooth (which is broken in
current top of Linus) against top of tree Linus + the jmpw patch.  I
obviously don't have your Vaio, but I do have my own share of quirky
hardware.  FWIW, I netbooted the hardware using pxelinux 3.50-pre7 as
the bootloader.[*]

I'm not writing this to give you a hard time, far from it.  I'm
suggesting that there might be something wrong with that rig that's
giving you false testing failures.  I don't particularly care about the
patch itself -- all it does is save 3 bytes which are currently unused
anyway, (although it might help Vivek's work.)  However, I'm very
concerned that you might be getting false failures, for obvious reasons.

	-hpa


[*] On the other hand, as I discovered in the process,
arch/i386/kernel/cpu/transmeta.c apparently gets miscompiled on my
development system for top-of-Linus.  This is a Linux bug and not gcc's
fault.  The following code:

	rdmsr(0x80860004, cap_mask, uk);
	wrmsr(0x80860004, ~0, uk);
	c->x86_capability[0] = cpuid_edx(0x00000001);
	wrmsr(0x80860004, cap_mask, uk);

... gets turned into ...
c0430f2d:       b9 04 00 86 80          mov    $0x80860004,%ecx
c0430f32:       0f 32                   rdmsr
c0430f34:       89 c6                   mov    %eax,%esi
c0430f36:       83 c8 ff                or     $0xffffffff,%eax
c0430f39:       89 d7                   mov    %edx,%edi
c0430f3b:       89 c2                   mov    %eax,%edx	## WTF!!
c0430f3d:       0f 30                   wrmsr
c0430f3f:       31 c9                   xor    %ecx,%ecx
c0430f41:       b8 01 00 00 00          mov    $0x1,%eax
c0430f46:       0f a2                   cpuid
c0430f48:       8b 45 8c                mov    0xffffff8c(%ebp),%eax
c0430f4b:       b9 04 00 86 80          mov    $0x80860004,%ecx
c0430f50:       89 50 0c                mov    %edx,0xc(%eax)
c0430f53:       b8 00 00 00 00          mov    $0x0,%eax
c0430f58:       89 fa                   mov    %edi,%edx
c0430f5a:       09 f0                   or     %esi,%eax
c0430f5c:       0f 30                   wrmsr

... with an immediate crash as a result, since -1 is not legal to write
into the high part (%edx) of MSR 0x80860004.

This is due to native_write_msr() being incorrectly implemented as a
macro.  The preprocessed code expands to:
 do { unsigned long long __val = native_read_msr(0x80860004); cap_mask =
__val; uk = __val >> 32; } while(0);
 native_write_msr(0x80860004, ((unsigned long long)uk << 32) | ~0);
 c->x86_capability[0] = cpuid_edx(0x00000001);
 native_write_msr(0x80860004, ((unsigned long long)uk << 32) | cap_mask);

Spot the looney?  "((unsigned long long)uk << 32) | ~0)" is *exactly*
the same thing as "(unsigned long long)~0" which is exactly
0xffffffffffffffffULL.  Without casting the lower argument to u32 before
ORing, it is not guarded against sign extension as a result of promotion.

	-hpa

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Andi Kleen]

On Tuesday 08 May 2007 13:25, H. Peter Anvin wrote:
> Andrew Morton wrote:
> >>  	# Normalize the start address
> >> -	jmpl	$BOOTSEG, $start2
> >> +	jmpw	$BOOTSEG, $start2
> >
> > Sigh, another blow struck in the ongoing struggle between my Vaio and the
> > rest of the world.
> >
> > Stone-cold black-screen lockup immediately upon boot.
> >
> > Stock FC5 install, config at
> > http://userweb.kernel.org/~akpm/config-sony.txt
>
> Andrew, I'm seriously starting to think there is something fundamentally
> wrong with that test setup.

I agree. The patch above does not change any code in the running
kernel and is not even used in a normal kernel boot. Andrew,
can you please double check that?

> [*] On the other hand, as I discovered in the process,
> arch/i386/kernel/cpu/transmeta.c apparently gets miscompiled on my
> development system for top-of-Linus.  This is a Linux bug and not gcc's
> fault.  The following code:

You should have put that into a different mail (is there a electron shortage
somewhere now? @)  Ok, thanks for the well researched bug report.  Will fix in 
my tree.

-Andi

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Andi Kleen wrote:
> 
> You should have put that into a different mail (is there a electron shortage
> somewhere now? @)  Ok, thanks for the well researched bug report.  Will fix in 
> my tree.
> 

Nah.  Just a brain shortage at 5 in the morning.

	-hpa

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Andrew Morton]

On Tue, 08 May 2007 04:25:00 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:

> Andrew Morton wrote:
> 
> >>  
> >>  	# Normalize the start address
> >> -	jmpl	$BOOTSEG, $start2
> >> +	jmpw	$BOOTSEG, $start2
> > 
> > Sigh, another blow struck in the ongoing struggle between my Vaio and the
> > rest of the world.
> > 
> > Stone-cold black-screen lockup immediately upon boot.
> > 
> > Stock FC5 install, config at
> > http://userweb.kernel.org/~akpm/config-sony.txt
> 
> Andrew, I'm seriously starting to think there is something fundamentally
> wrong with that test setup.

heh.  All the other bugs have been real oh-yeah-youre-right bugs.  This is
the only mystery bug which I recall.

> The bootsect code in question is never executed.  AT ALL.  The only
> raison d'être for it at all is to print an error message if someone
> writes the kernel to a raw floppy disk.  Nor does it change the
> alignment of the header or anything else to that effect -- the assembly
> code downstream has an explicit ".org" directive.  For what it's worth,
> just to make sure I'm not crazy, I just re-tested both booting the
> kernel and booting the raw disk image, in simulation and on real
> hardware, and it doesn't change anything.  I used your configuration
> file (yes '' | make oldconfig) minus Bluetooth (which is broken in
> current top of Linus) against top of tree Linus + the jmpw patch.  I
> obviously don't have your Vaio, but I do have my own share of quirky
> hardware.  FWIW, I netbooted the hardware using pxelinux 3.50-pre7 as
> the bootloader.[*]
> 
> I'm not writing this to give you a hard time, far from it.  I'm
> suggesting that there might be something wrong with that rig that's
> giving you false testing failures.  I don't particularly care about the
> patch itself -- all it does is save 3 bytes which are currently unused
> anyway, (although it might help Vivek's work.)  However, I'm very
> concerned that you might be getting false failures, for obvious reasons.
> 

I just retested bare 2.6.21 with that patch.  Same hang.

Maybe the assembler or linker screwed something up.

Without patch:

(gdb)  x/20i _start
0x0 <_start>:   ljmpw  $0x0,$0x8
0x6 <_start+6>: rolb   $0x8c,(%edi)
0x9 <start2+1>: enter  $0xd88e,$0x8e
0xd <start2+5>: rorb   $0xfb,0x7c00bcd0(%esi)
0x14 <start2+12>:       cld    
0x15 <start2+13>:       mov    $0x20ac0031,%esi
0x1a <msg_loop+2>:      (bad)  
0x1b <msg_loop+3>:      je     0x26 <die>
0x1d <msg_loop+5>:      mov    $0xe,%ah
0x1f <msg_loop+7>:      mov    $0x10cd0007,%ebx
0x24 <msg_loop+12>:     jmp    0x18 <msg_loop>
0x26 <die>:     xor    %eax,%eax
0x28 <die+2>:   int    $0x16

With patch:

(gdb) x/20i _start
0x0 <_start>:   ljmp   $0xc88c,$0x7c00005
0x7 <start2+2>: mov    %eax,%ds
0x9 <start2+4>: mov    %eax,%es
0xb <start2+6>: mov    %eax,%ss
0xd <start2+8>: mov    $0xfcfb7c00,%esp
0x12 <start2+13>:       mov    $0x20ac002e,%esi
0x17 <msg_loop+2>:      (bad)  
0x18 <msg_loop+3>:      je     0x23 <die>
0x1a <msg_loop+5>:      mov    $0xe,%ah
0x1c <msg_loop+7>:      mov    $0x10cd0007,%ebx
0x21 <msg_loop+12>:     jmp    0x15 <msg_loop>

not sure what's going on there.  We seem to have confused gdb.

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Andrew Morton wrote:
> 
> Without patch:
> 
> (gdb)  x/20i _start
> 0x0 <_start>:   ljmpw  $0x0,$0x8
> 0x6 <_start+6>: rolb   $0x8c,(%edi)
> 0x9 <start2+1>: enter  $0xd88e,$0x8e
> 0xd <start2+5>: rorb   $0xfb,0x7c00bcd0(%esi)
> 0x14 <start2+12>:       cld    
> 0x15 <start2+13>:       mov    $0x20ac0031,%esi
> 0x1a <msg_loop+2>:      (bad)  
> 0x1b <msg_loop+3>:      je     0x26 <die>
> 0x1d <msg_loop+5>:      mov    $0xe,%ah
> 0x1f <msg_loop+7>:      mov    $0x10cd0007,%ebx
> 0x24 <msg_loop+12>:     jmp    0x18 <msg_loop>
> 0x26 <die>:     xor    %eax,%eax
> 0x28 <die+2>:   int    $0x16
> 
> With patch:
> 
> (gdb) x/20i _start
> 0x0 <_start>:   ljmp   $0xc88c,$0x7c00005
> 0x7 <start2+2>: mov    %eax,%ds
> 0x9 <start2+4>: mov    %eax,%es
> 0xb <start2+6>: mov    %eax,%ss
> 0xd <start2+8>: mov    $0xfcfb7c00,%esp
> 0x12 <start2+13>:       mov    $0x20ac002e,%esi
> 0x17 <msg_loop+2>:      (bad)  
> 0x18 <msg_loop+3>:      je     0x23 <die>
> 0x1a <msg_loop+5>:      mov    $0xe,%ah
> 0x1c <msg_loop+7>:      mov    $0x10cd0007,%ebx
> 0x21 <msg_loop+12>:     jmp    0x15 <msg_loop>
> 
> not sure what's going on there.  We seem to have confused gdb.
> 

You're trying to disassemble 16-bit code as if it was 32-bit code.  The
easiest way to disassemble is to have NASM installed on your machine and
just do:

	ndisasm bzImage

My setup code rewrite creates an intermediate .elf file to retain
debugging information and make 16-bit disassembly
("objdump -d -m i8086") easier.

	-hpa

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Alexander van Heukelum]

On Tue, May 08, 2007 at 03:28:17AM -0700, Andrew Morton wrote:
> On Sat, 5 May 2007 12:44:52 +0200 Alexander van Heukelum <heukelum@mailshack.com> wrote:
> > --- a/arch/i386/boot/bootsect.S
> > +++ b/arch/i386/boot/bootsect.S
> > @@ -44,7 +44,7 @@ #endif
> >  _start:
> >  
> >  	# Normalize the start address
> > -	jmpl	$BOOTSEG, $start2
> > +	jmpw	$BOOTSEG, $start2
> 
> Sigh, another blow struck in the ongoing struggle between my Vaio and the
> rest of the world.
> 
> Stone-cold black-screen lockup immediately upon boot.
> 
> Stock FC5 install, config at
> http://userweb.kernel.org/~akpm/config-sony.txt

Sigh... sounds impossible.

I assume this is repeatable? And this patch changes behaviour? What do
you mean by immediate? Before or after decompression?

Fishy... As Andi and Peter have mentioned, this code is not executed.
There is only one user of the same memory area this early... video.S
uses it to store some collected data, but as far as I can see only the
'CONFIG_VIDEO_RETAIN' code also reads it back. If it happens before
decompression, could you see if changing
	#define CONFIG_VIDEO_RETAIN -> #undef CONFIG_VIDEO_RETAIN
in arch/i386/boot/video.S changes anything?

Do you select a non-standard textmode? Does vga=ask show up?

*looks around some*

Oh! A padding hole in a struct! That could be a problem. If the freeze
is after decompression, could you test if this makes it work again?

Greetings,
	Alexander

---

Commit 89ec4c238e7a3d7e660291f3f1a8181381baad77 introduced a discrepancy
between the struct screen_info which is used by the C code, and the
PARAM_* offsets which are used in the real-mode kernel. This is an
attempt to rectify the situation, but I have no way to test it.

Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>

---

diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
index 8143c95..8e404cb 100644
--- a/arch/i386/boot/video.S
+++ b/arch/i386/boot/video.S
@@ -95,7 +95,8 @@ #define PARAM_VESAPM_SEG	0x2e
 #define PARAM_VESAPM_OFF	0x30
 #define PARAM_LFB_PAGES		0x32
 #define PARAM_VESA_ATTRIB	0x34
-#define PARAM_CAPABILITIES	0x36
+#define PARAM_VESA_PAD		0x36
+#define PARAM_CAPABILITIES	0x38
 
 /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
 #ifdef CONFIG_VIDEO_RETAIN
diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
index b02308e..0a2e892 100644
--- a/include/linux/screen_info.h
+++ b/include/linux/screen_info.h
@@ -41,8 +41,8 @@ struct screen_info {
 	u16 vesapm_off;		/* 0x30 */
 	u16 pages;		/* 0x32 */
 	u16 vesa_attributes;	/* 0x34 */
-	u32 capabilities;       /* 0x36 */
-				/* 0x3a -- 0x3b reserved for future expansion */
+	u16 vesa_pad;		/* 0x36 */
+	u32 capabilities;       /* 0x38 */
 				/* 0x3c -- 0x3f micro stack for relocatable kernels */
 };
 

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Alexander van Heukelum wrote:
> 
> Oh! A padding hole in a struct! That could be a problem. If the freeze
> is after decompression, could you test if this makes it work again?
> 

The correct fix is to apply __attribute__((packed)) to this structure.
Of course, changing the boot sector will cause different random values
to be poked into this field...

I actually caught this during my setup code rewrite, but I forgot that
in the old setup code the parameter structure overlays the boot sector
(it doesn't in my code -- it starts out with nice clean zero-filled memory.)

	-hpa

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Alexander van Heukelum]

On Tue, May 08, 2007 at 11:45:47AM -0700, H. Peter Anvin wrote:
> Alexander van Heukelum wrote:
> > 
> > Oh! A padding hole in a struct! That could be a problem. If the freeze
> > is after decompression, could you test if this makes it work again?
> > 
> 
> The correct fix is to apply __attribute__((packed)) to this structure.

Yeah, I thought about that possibility too, but the struct didn't need
that (in my opinion ugly) annotation before. Oh well, here you go.

Greetings,
	Alexander

---

Commit 89ec4c238e7a3d7e660291f3f1a8181381baad77 introduced a discrepancy
between the struct screen_info which is used by the C code, and the
PARAM_* offsets which are used in the real-mode kernel. As hpa suggests,
adding __attribute__((packed)) to the struct fixes it.

Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>

---

diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
index b02308e..4a7c24b 100644
--- a/include/linux/screen_info.h
+++ b/include/linux/screen_info.h
@@ -44,7 +44,7 @@ struct screen_info {
 	u32 capabilities;       /* 0x36 */
 				/* 0x3a -- 0x3b reserved for future expansion */
 				/* 0x3c -- 0x3f micro stack for relocatable kernels */
-};
+} __attribute__((packed));
 
 extern struct screen_info screen_info;
 

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Alexander van Heukelum wrote:
> On Tue, May 08, 2007 at 11:45:47AM -0700, H. Peter Anvin wrote:
>> Alexander van Heukelum wrote:
>>> Oh! A padding hole in a struct! That could be a problem. If the freeze
>>> is after decompression, could you test if this makes it work again?
>>>
>> The correct fix is to apply __attribute__((packed)) to this structure.
> 
> Yeah, I thought about that possibility too, but the struct didn't need
> that (in my opinion ugly) annotation before. Oh well, here you go.
> 

It did need it, however, it just "happened to work" anyway.

Yes, it's ugly, but the damage is already done.

	-hpa

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Andrew Morton]

On Tue, 8 May 2007 20:32:32 +0200
Alexander van Heukelum <heukelum@mailshack.com> wrote:

> On Tue, May 08, 2007 at 03:28:17AM -0700, Andrew Morton wrote:
> > On Sat, 5 May 2007 12:44:52 +0200 Alexander van Heukelum <heukelum@mailshack.com> wrote:
> > > --- a/arch/i386/boot/bootsect.S
> > > +++ b/arch/i386/boot/bootsect.S
> > > @@ -44,7 +44,7 @@ #endif
> > >  _start:
> > >  
> > >  	# Normalize the start address
> > > -	jmpl	$BOOTSEG, $start2
> > > +	jmpw	$BOOTSEG, $start2
> > 
> > Sigh, another blow struck in the ongoing struggle between my Vaio and the
> > rest of the world.
> > 
> > Stone-cold black-screen lockup immediately upon boot.
> > 
> > Stock FC5 install, config at
> > http://userweb.kernel.org/~akpm/config-sony.txt
> 
> Sigh... sounds impossible.
> 
> I assume this is repeatable?

100%

> And this patch changes behaviour?

Yup.

> What do you mean by immediate?

Basically the last thing I see is grub.

> Before or after decompression?

Not sure - things happen pretty quickly and I suspect info is getting lost
when the display is changing modes.

> Fishy... As Andi and Peter have mentioned, this code is not executed.
> There is only one user of the same memory area this early... video.S
> uses it to store some collected data, but as far as I can see only the
> 'CONFIG_VIDEO_RETAIN' code also reads it back. If it happens before
> decompression, could you see if changing
> 	#define CONFIG_VIDEO_RETAIN -> #undef CONFIG_VIDEO_RETAIN
> in arch/i386/boot/video.S changes anything?
> 
> Do you select a non-standard textmode?

yep.

> Does vga=ask show up?

no.

> *looks around some*
> 
> Oh! A padding hole in a struct! That could be a problem. If the freeze
> is after decompression, could you test if this makes it work again?

I shall try that later in the day.

> 
> ---
> 
> Commit 89ec4c238e7a3d7e660291f3f1a8181381baad77 introduced a discrepancy
> between the struct screen_info which is used by the C code, and the
> PARAM_* offsets which are used in the real-mode kernel. This is an
> attempt to rectify the situation, but I have no way to test it.
> 
> Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>
> 
> ---
> 
> diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
> diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> index 8143c95..8e404cb 100644
> --- a/arch/i386/boot/video.S
> +++ b/arch/i386/boot/video.S
> @@ -95,7 +95,8 @@ #define PARAM_VESAPM_SEG	0x2e
>  #define PARAM_VESAPM_OFF	0x30
>  #define PARAM_LFB_PAGES		0x32
>  #define PARAM_VESA_ATTRIB	0x34
> -#define PARAM_CAPABILITIES	0x36
> +#define PARAM_VESA_PAD		0x36
> +#define PARAM_CAPABILITIES	0x38
>  
>  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
>  #ifdef CONFIG_VIDEO_RETAIN
> diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
> index b02308e..0a2e892 100644
> --- a/include/linux/screen_info.h
> +++ b/include/linux/screen_info.h
> @@ -41,8 +41,8 @@ struct screen_info {
>  	u16 vesapm_off;		/* 0x30 */
>  	u16 pages;		/* 0x32 */
>  	u16 vesa_attributes;	/* 0x34 */
> -	u32 capabilities;       /* 0x36 */
> -				/* 0x3a -- 0x3b reserved for future expansion */
> +	u16 vesa_pad;		/* 0x36 */
> +	u32 capabilities;       /* 0x38 */
>  				/* 0x3c -- 0x3f micro stack for relocatable kernels */
>  };
>  

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Antonino A. Daplas]

On Tue, 2007-05-08 at 20:32 +0200, Alexander van Heukelum wrote:
> On Tue, May 08, 2007 at 03:28:17AM -0700, Andrew Morton wrote:
> > On Sat, 5 May 2007 12:44:52 +0200 Alexander van Heukelum <heukelum@mailshack.com> wrote:
> > > --- a/arch/i386/boot/bootsect.S
> > > +++ b/arch/i386/boot/bootsect.S
> > > @@ -44,7 +44,7 @@ #endif
> > >  _start:
> > >  
> > >  	# Normalize the start address
> > > -	jmpl	$BOOTSEG, $start2
> > > +	jmpw	$BOOTSEG, $start2
> > 
> > Sigh, another blow struck in the ongoing struggle between my Vaio and the
> > rest of the world.
> > 
> > Stone-cold black-screen lockup immediately upon boot.
> > 
> > Stock FC5 install, config at
> > http://userweb.kernel.org/~akpm/config-sony.txt
> 
> Sigh... sounds impossible.
> 
> I assume this is repeatable? And this patch changes behaviour? What do
> you mean by immediate? Before or after decompression?
> 
> Fishy... As Andi and Peter have mentioned, this code is not executed.
> There is only one user of the same memory area this early... video.S
> uses it to store some collected data, but as far as I can see only the
> 'CONFIG_VIDEO_RETAIN' code also reads it back. If it happens before
> decompression, could you see if changing
> 	#define CONFIG_VIDEO_RETAIN -> #undef CONFIG_VIDEO_RETAIN
> in arch/i386/boot/video.S changes anything?
> 
> Do you select a non-standard textmode? Does vga=ask show up?
> 
> *looks around some*
> 
> Oh! A padding hole in a struct! That could be a problem. If the freeze
> is after decompression, could you test if this makes it work again?
> 
> Greetings,
> 	Alexander
> 
> ---
> 
> Commit 89ec4c238e7a3d7e660291f3f1a8181381baad77 introduced a discrepancy
> between the struct screen_info which is used by the C code, and the
> PARAM_* offsets which are used in the real-mode kernel. This is an
> attempt to rectify the situation, but I have no way to test it.
> 
> Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>
> 
> ---
> 
> diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
> diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> index 8143c95..8e404cb 100644
> --- a/arch/i386/boot/video.S
> +++ b/arch/i386/boot/video.S
> @@ -95,7 +95,8 @@ #define PARAM_VESAPM_SEG	0x2e
>  #define PARAM_VESAPM_OFF	0x30
>  #define PARAM_LFB_PAGES		0x32
>  #define PARAM_VESA_ATTRIB	0x34
> -#define PARAM_CAPABILITIES	0x36
> +#define PARAM_VESA_PAD		0x36
> +#define PARAM_CAPABILITIES	0x38
>  
>  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
>  #ifdef CONFIG_VIDEO_RETAIN
> diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
> index b02308e..0a2e892 100644
> --- a/include/linux/screen_info.h
> +++ b/include/linux/screen_info.h
> @@ -41,8 +41,8 @@ struct screen_info {
>  	u16 vesapm_off;		/* 0x30 */
>  	u16 pages;		/* 0x32 */
>  	u16 vesa_attributes;	/* 0x34 */
> -	u32 capabilities;       /* 0x36 */
> -				/* 0x3a -- 0x3b reserved for future expansion */
> +	u16 vesa_pad;		/* 0x36 */
> +	u32 capabilities;       /* 0x38 */
>  				/* 0x3c -- 0x3f micro stack for relocatable kernels */
>  };
>  

You would also need this minor change.

Tony

diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
index 5e2280c..2d637d1 100644
--- a/arch/i386/boot/video.S
+++ b/arch/i386/boot/video.S
@@ -95,7 +95,7 @@ #define PARAM_VESAPM_SEG      0x2e
 #define PARAM_VESAPM_OFF       0x30
 #define PARAM_LFB_PAGES                0x32
 #define PARAM_VESA_ATTRIB      0x34
-#define PARAM_CAPABILITIES     0x36
+#define PARAM_CAPABILITIES     0x38
 
 /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
 #ifdef CONFIG_VIDEO_RETAIN

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Alexander van Heukelum]

On Wed, May 09, 2007 at 08:04:07AM +0800, Antonino A. Daplas wrote:
> On Tue, 2007-05-08 at 20:32 +0200, Alexander van Heukelum wrote:
> > diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
> > diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> > index 8143c95..8e404cb 100644
> > --- a/arch/i386/boot/video.S
> > +++ b/arch/i386/boot/video.S
> > @@ -95,7 +95,8 @@ #define PARAM_VESAPM_SEG	0x2e
> >  #define PARAM_VESAPM_OFF	0x30
> >  #define PARAM_LFB_PAGES		0x32
> >  #define PARAM_VESA_ATTRIB	0x34
> > -#define PARAM_CAPABILITIES	0x36
> > +#define PARAM_VESA_PAD		0x36
> > +#define PARAM_CAPABILITIES	0x38
> >  
> >  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
> >  #ifdef CONFIG_VIDEO_RETAIN
> > diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
> > index b02308e..0a2e892 100644
> > --- a/include/linux/screen_info.h
> > +++ b/include/linux/screen_info.h
> > @@ -41,8 +41,8 @@ struct screen_info {
> >  	u16 vesapm_off;		/* 0x30 */
> >  	u16 pages;		/* 0x32 */
> >  	u16 vesa_attributes;	/* 0x34 */
> > -	u32 capabilities;       /* 0x36 */
> > -				/* 0x3a -- 0x3b reserved for future expansion */
> > +	u16 vesa_pad;		/* 0x36 */
> > +	u32 capabilities;       /* 0x38 */
> >  				/* 0x3c -- 0x3f micro stack for relocatable kernels */
> >  };
> >  
> 
> You would also need this minor change.
> 
> Tony
> 
> diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> index 5e2280c..2d637d1 100644
> --- a/arch/i386/boot/video.S
> +++ b/arch/i386/boot/video.S
> @@ -95,7 +95,7 @@ #define PARAM_VESAPM_SEG      0x2e
>  #define PARAM_VESAPM_OFF       0x30
>  #define PARAM_LFB_PAGES                0x32
>  #define PARAM_VESA_ATTRIB      0x34
> -#define PARAM_CAPABILITIES     0x36
> +#define PARAM_CAPABILITIES     0x38
>  
>  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
>  #ifdef CONFIG_VIDEO_RETAIN

Hi!

I'm making coffee now. I just don't see what I missed? Maybe you were
led astray by the new PARAM_VESA_PAD I added?

Greetings,
	Alexander

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Antonino A. Daplas]

On Wed, 2007-05-09 at 10:54 +0200, Alexander van Heukelum wrote:
> On Wed, May 09, 2007 at 08:04:07AM +0800, Antonino A. Daplas wrote:
> > On Tue, 2007-05-08 at 20:32 +0200, Alexander van Heukelum wrote:
> > > diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
> > > diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> > > index 8143c95..8e404cb 100644
> > > --- a/arch/i386/boot/video.S
> > > +++ b/arch/i386/boot/video.S
> > > @@ -95,7 +95,8 @@ #define PARAM_VESAPM_SEG	0x2e
> > >  #define PARAM_VESAPM_OFF	0x30
> > >  #define PARAM_LFB_PAGES		0x32
> > >  #define PARAM_VESA_ATTRIB	0x34
> > > -#define PARAM_CAPABILITIES	0x36
> > > +#define PARAM_VESA_PAD		0x36
> > > +#define PARAM_CAPABILITIES	0x38
> > >  
> > >  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
> > >  #ifdef CONFIG_VIDEO_RETAIN
> > > diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
> > > index b02308e..0a2e892 100644
> > > --- a/include/linux/screen_info.h
> > > +++ b/include/linux/screen_info.h
> > > @@ -41,8 +41,8 @@ struct screen_info {
> > >  	u16 vesapm_off;		/* 0x30 */
> > >  	u16 pages;		/* 0x32 */
> > >  	u16 vesa_attributes;	/* 0x34 */
> > > -	u32 capabilities;       /* 0x36 */
> > > -				/* 0x3a -- 0x3b reserved for future expansion */
> > > +	u16 vesa_pad;		/* 0x36 */
> > > +	u32 capabilities;       /* 0x38 */
> > >  				/* 0x3c -- 0x3f micro stack for relocatable kernels */
> > >  };
> > >  
> > 
> > You would also need this minor change.
> > 
> > Tony
> > 
> > diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> > index 5e2280c..2d637d1 100644
> > --- a/arch/i386/boot/video.S
> > +++ b/arch/i386/boot/video.S
> > @@ -95,7 +95,7 @@ #define PARAM_VESAPM_SEG      0x2e
> >  #define PARAM_VESAPM_OFF       0x30
> >  #define PARAM_LFB_PAGES                0x32
> >  #define PARAM_VESA_ATTRIB      0x34
> > -#define PARAM_CAPABILITIES     0x36
> > +#define PARAM_CAPABILITIES     0x38
> >  
> >  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
> >  #ifdef CONFIG_VIDEO_RETAIN
> 
> Hi!
> 
> I'm making coffee now. I just don't see what I missed? Maybe you were
> led astray by the new PARAM_VESA_PAD I added?

Since you moved screen_info.capabilities forward by 2 bytes, you must
also tell video.S where to poke for the new location, otherwise, vesafb
(which uses the capabilities field) will get confused.

These are the values for the unadjusted and adjusted PARAM_CAPABILITIES:
(The latter one is correct).

capabilities: 746f0000
capabilities: 1

Tony

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Alexander van Heukelum wrote:
> 
> Hi!
> 
> I'm making coffee now. I just don't see what I missed? Maybe you were
> led astray by the new PARAM_VESA_PAD I added?
> 

Again, I object to changing a documented interface when it's not
necessary to do so.  Just declare the structure packed.

	-hpa

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Alexander van Heukelum]

On Wed, May 09, 2007 at 09:51:36AM -0700, H. Peter Anvin wrote:
> Alexander van Heukelum wrote:
> > 
> > Hi!
> > 
> > I'm making coffee now. I just don't see what I missed? Maybe you were
> > led astray by the new PARAM_VESA_PAD I added?
> > 
> 
> Again, I object to changing a documented interface when it's not
> necessary to do so.  Just declare the structure packed.

That would be like this, right?

http://www.ussg.iu.edu/hypermail/linux/kernel/0705.1/0383.html

Greetings,
	Alexander

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[H. Peter Anvin]

Alexander van Heukelum wrote:
> On Wed, May 09, 2007 at 09:51:36AM -0700, H. Peter Anvin wrote:
>> Alexander van Heukelum wrote:
>>> Hi!
>>>
>>> I'm making coffee now. I just don't see what I missed? Maybe you were
>>> led astray by the new PARAM_VESA_PAD I added?
>>>
>> Again, I object to changing a documented interface when it's not
>> necessary to do so.  Just declare the structure packed.
> 
> That would be like this, right?
> 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0705.1/0383.html
> 

Absolutely.

	-hpa

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Lennart Sorensen]

On Wed, May 09, 2007 at 08:04:07AM +0800, Antonino A. Daplas wrote:
> On Tue, 2007-05-08 at 20:32 +0200, Alexander van Heukelum wrote:
> > On Tue, May 08, 2007 at 03:28:17AM -0700, Andrew Morton wrote:
> > > On Sat, 5 May 2007 12:44:52 +0200 Alexander van Heukelum <heukelum@mailshack.com> wrote:
> > > > --- a/arch/i386/boot/bootsect.S
> > > > +++ b/arch/i386/boot/bootsect.S
> > > > @@ -44,7 +44,7 @@ #endif
> > > >  _start:
> > > >  
> > > >  	# Normalize the start address
> > > > -	jmpl	$BOOTSEG, $start2
> > > > +	jmpw	$BOOTSEG, $start2
> > > 
> > > Sigh, another blow struck in the ongoing struggle between my Vaio and the
> > > rest of the world.
> > > 
> > > Stone-cold black-screen lockup immediately upon boot.
> > > 
> > > Stock FC5 install, config at
> > > http://userweb.kernel.org/~akpm/config-sony.txt
> > 
> > Sigh... sounds impossible.
> > 
> > I assume this is repeatable? And this patch changes behaviour? What do
> > you mean by immediate? Before or after decompression?
> > 
> > Fishy... As Andi and Peter have mentioned, this code is not executed.
> > There is only one user of the same memory area this early... video.S
> > uses it to store some collected data, but as far as I can see only the
> > 'CONFIG_VIDEO_RETAIN' code also reads it back. If it happens before
> > decompression, could you see if changing
> > 	#define CONFIG_VIDEO_RETAIN -> #undef CONFIG_VIDEO_RETAIN
> > in arch/i386/boot/video.S changes anything?
> > 
> > Do you select a non-standard textmode? Does vga=ask show up?
> > 
> > *looks around some*
> > 
> > Oh! A padding hole in a struct! That could be a problem. If the freeze
> > is after decompression, could you test if this makes it work again?
> > 
> > Greetings,
> > 	Alexander
> > 
> > ---
> > 
> > Commit 89ec4c238e7a3d7e660291f3f1a8181381baad77 introduced a discrepancy
> > between the struct screen_info which is used by the C code, and the
> > PARAM_* offsets which are used in the real-mode kernel. This is an
> > attempt to rectify the situation, but I have no way to test it.
> > 
> > Signed-off-by: Alexander van Heukelum <heukelum@mailshack.com>
> > 
> > ---
> > 
> > diff --git a/arch/i386/boot/bootsect.S b/arch/i386/boot/bootsect.S
> > diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> > index 8143c95..8e404cb 100644
> > --- a/arch/i386/boot/video.S
> > +++ b/arch/i386/boot/video.S
> > @@ -95,7 +95,8 @@ #define PARAM_VESAPM_SEG	0x2e
> >  #define PARAM_VESAPM_OFF	0x30
> >  #define PARAM_LFB_PAGES		0x32
> >  #define PARAM_VESA_ATTRIB	0x34
> > -#define PARAM_CAPABILITIES	0x36
> > +#define PARAM_VESA_PAD		0x36
> > +#define PARAM_CAPABILITIES	0x38

Isn't this ^^^ what you are suggesting below?

> >  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
> >  #ifdef CONFIG_VIDEO_RETAIN
> > diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
> > index b02308e..0a2e892 100644
> > --- a/include/linux/screen_info.h
> > +++ b/include/linux/screen_info.h
> > @@ -41,8 +41,8 @@ struct screen_info {
> >  	u16 vesapm_off;		/* 0x30 */
> >  	u16 pages;		/* 0x32 */
> >  	u16 vesa_attributes;	/* 0x34 */
> > -	u32 capabilities;       /* 0x36 */
> > -				/* 0x3a -- 0x3b reserved for future expansion */
> > +	u16 vesa_pad;		/* 0x36 */
> > +	u32 capabilities;       /* 0x38 */
> >  				/* 0x3c -- 0x3f micro stack for relocatable kernels */
> >  };
> >  
> 
> You would also need this minor change.
> 
> Tony
> 
> diff --git a/arch/i386/boot/video.S b/arch/i386/boot/video.S
> index 5e2280c..2d637d1 100644
> --- a/arch/i386/boot/video.S
> +++ b/arch/i386/boot/video.S
> @@ -95,7 +95,7 @@ #define PARAM_VESAPM_SEG      0x2e
>  #define PARAM_VESAPM_OFF       0x30
>  #define PARAM_LFB_PAGES                0x32
>  #define PARAM_VESA_ATTRIB      0x34
> -#define PARAM_CAPABILITIES     0x36
> +#define PARAM_CAPABILITIES     0x38
>  
>  /* Define DO_STORE according to CONFIG_VIDEO_RETAIN */
>  #ifdef CONFIG_VIDEO_RETAIN

--
Len Sorensen

Re: [PATCH] Make bootsector stub 16-bit-only (i386)

[Antonino A. Daplas]

On Wed, 2007-05-09 at 10:30 -0400, Lennart Sorensen wrote:
> On Wed, May 09, 2007 at 08:04:07AM +0800, Antonino A. Daplas wrote:
> > On Tue, 2007-05-08 at 20:32 +0200, Alexander van Heukelum wrote:
> > > On Tue, May 08, 2007 at 03:28:17AM -0700, Andrew Morton wrote:
> > > > On Sat, 5 May 2007 12:44:52 +0200 Alexander van Heukelum <heukelum@mailshack.com> wrote:

> > > @@ -95,7 +95,8 @@ #define PARAM_VESAPM_SEG	0x2e
> > >  #define PARAM_VESAPM_OFF	0x30
> > >  #define PARAM_LFB_PAGES		0x32
> > >  #define PARAM_VESA_ATTRIB	0x34
> > > -#define PARAM_CAPABILITIES	0x36
> > > +#define PARAM_VESA_PAD		0x36
> > > +#define PARAM_CAPABILITIES	0x38
> 
> Isn't this ^^^ what you are suggesting below?
> 

Yes, sorry, why did I miss that one :-)

Tony