Re: x86 setup code rewrite in C - revised

Re: x86 setup code rewrite in C - revised

[Etienne Lorrain]

On Thu, 12 Jul 2007, Linus Torvalds wrote:
> On Thu, 12 Jul 2007, Andrew Morton wrote:
> > 
> > This code has been in -mm since 11 May, as git-newsetup.patch.  It has
> > caused (for what it is) astonishingly few problems.  Maybe a couple of
> > build glitches and one runtime failure, all quickly fixed.
> > 
> > I'd say it's ready.
> 
> Ok. That makes it easy. I'll just merge it.
> 
> 		Linus

 Have fun, this code:
 - do not open the fast A20 gate before checking if the slow A20 gate is open or closed.
 - uses in asm("") inputs which may or may not be set by the compiler in the stack,
   after modifying the stack pointer in the asm block: at least has_eflag()
 - The VGA recalc has the same bug as the assembly version where a VGA write protected
   register is written (Overflow register) without setting the enable bit (see VGA docs).
 - Does not save and restore %ds when printing a char on the screen (%ds is destroyed
   only when the content of the screen scroll - only for some video cards)
 - Has a "dn" for outl() which sliped in instead of "dN"
 and probably few other problems - just seen those by reading the patches (the asm("")
 are inlined in the C code - I find it more difficult to check).

 Also, I do not know if "m" is right in here:
static inline u8 rdfs8(addr_t addr)
{
	u8 v;
	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
	return v;
}

  I may repeat me, but to find these kind of problems, it is very nice to have an ELF
 file to do a readelf/objdump -D -m i8086 (even after final link).

  Etienne.
http://gujin.org

I like that message in this context...
   http://marc.info/?l=linux-kernel&m=117077712515509&w=4


      _____________________________________________________________________________ 
Ne gardez plus qu'une seule adresse mail ! Copiez vos mails vers Yahoo! Mail 

Re: x86 setup code rewrite in C - revised

[Chuck Ebbert]

On 07/13/2007 10:25 AM, Etienne Lorrain wrote:

[ Added back cc:'s]

> On Thu, 12 Jul 2007, Linus Torvalds wrote:
>> On Thu, 12 Jul 2007, Andrew Morton wrote:
>>> This code has been in -mm since 11 May, as git-newsetup.patch.  It has
>>> caused (for what it is) astonishingly few problems.  Maybe a couple of
>>> build glitches and one runtime failure, all quickly fixed.
>>>
>>> I'd say it's ready.
>> Ok. That makes it easy. I'll just merge it.
>>
>> 		Linus
> 
>  Have fun, this code:
>  - do not open the fast A20 gate before checking if the slow A20 gate is open or closed.
>  - uses in asm("") inputs which may or may not be set by the compiler in the stack,
>    after modifying the stack pointer in the asm block: at least has_eflag()
>  - The VGA recalc has the same bug as the assembly version where a VGA write protected
>    register is written (Overflow register) without setting the enable bit (see VGA docs).
>  - Does not save and restore %ds when printing a char on the screen (%ds is destroyed
>    only when the content of the screen scroll - only for some video cards)
>  - Has a "dn" for outl() which sliped in instead of "dN"
>  and probably few other problems - just seen those by reading the patches (the asm("")
>  are inlined in the C code - I find it more difficult to check).
> 
>  Also, I do not know if "m" is right in here:
> static inline u8 rdfs8(addr_t addr)
> {
> 	u8 v;
> 	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
> 	return v;
> }
> 
>   I may repeat me, but to find these kind of problems, it is very nice to have an ELF
>  file to do a readelf/objdump -D -m i8086 (even after final link).

Re: x86 setup code rewrite in C - revised

[H. Peter Anvin]

Chuck Ebbert wrote:
>>  Have fun, this code:
>>  - do not open the fast A20 gate before checking if the slow A20 gate is open or closed.

As does the current code; this is highly intentional behaviour since
there are machines (in particular a whole series of machines made by
Olivetti) which lock up if you do it differently.

>>  - uses in asm("") inputs which may or may not be set by the compiler in the stack,
>>    after modifying the stack pointer in the asm block: at least has_eflag()

Point.  "g" should be "ri".  I will send a patch.

>>  - The VGA recalc has the same bug as the assembly version where a VGA write protected
>>    register is written (Overflow register) without setting the enable bit (see VGA docs).

OK, that would be a bug ported directly from the assembly version.  The
fact that the bug can be seen now is part of why I did this work.
Please feel free to submit a patch.

>>  - Does not save and restore %ds when printing a char on the screen (%ds is destroyed
>>    only when the content of the screen scroll - only for some video cards)

%ds?  Aren't you confusing it with the old bug which would destroy %bp?
 If you have any references to %ds being destroyed I would be very
surprised.  I can guarantee that very little if any assembly code I've
ever seen that deals with INT 10h -- and I've seen a lot of it -- guards
against %ds being randomly trashed.

However, the trashing of %bp is a well-known bug (although only for
machines older than the ones that can run Linux) -- the Interrupt List has:

BUGS:   some implementations (including the original IBM PC) have a bug
 	which destroys BP

>>  - Has a "dn" for outl() which sliped in instead of "dN"

That's a bug, although currently nonmanifest -- there are no users of
outl() at the present.  I will send a patch.

>>  and probably few other problems - just seen those by reading the patches (the asm("")
>>  are inlined in the C code - I find it more difficult to check).
>>
>>  Also, I do not know if "m" is right in here:
>> static inline u8 rdfs8(addr_t addr)
>> {
>> 	u8 v;
>> 	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
>> 	return v;
>> }

The "m" is correct right there.

>>   I may repeat me, but to find these kind of problems, it is very nice to have an ELF
>>  file to do a readelf/objdump -D -m i8086 (even after final link).

There is such a file (arch/i386/boot/setup.elf) which is retained, for
exactly this reason.

	-hpa

RE : Re: x86 setup code rewrite in C - revised

[Etienne Lorrain]

--- "H. Peter Anvin" <hpa@zytor.com> wrote:
> Chuck Ebbert wrote:

  Wrong name.

> >>  Have fun, this code:
> >>  - do not open the fast A20 gate before checking if the slow A20 gate is open or
> closed.
> 
> As does the current code; this is highly intentional behaviour since
> there are machines (in particular a whole series of machines made by
> Olivetti) which lock up if you do it differently.

 There was some discussion on this list about some machine which would not wake-up
correctly if slow A20 was not closed, long time ago. I did not really follow
the code after that discussion.
 I wonder if you should do a "outb(0xFF, 0x64);" after "outb(0xdf, 0x60);" like
the HIMEM.SYS driver, to force an immediate update of the I/O ports - I think
I also read that in this initial chip docs, long time ago also.

> >>  - Does not save and restore %ds when printing a char on the screen (%ds is
> >>  destroyed only when the content of the screen scroll - only for some video cards)
> 
> %ds?  Aren't you confusing it with the old bug which would destroy %bp?
>  If you have any references to %ds being destroyed I would be very
> surprised.  I can guarantee that very little if any assembly code I've
> ever seen that deals with INT 10h -- and I've seen a lot of it -- guards
> against %ds being randomly trashed.
> 
> However, the trashing of %bp is a well-known bug (although only for
> machines older than the ones that can run Linux) -- the Interrupt List has:
> 
> BUGS:   some implementations (including the original IBM PC) have a bug
>  	which destroys BP

 That is on Trident cards, old card but may still be used, and BIOS may have
been copied to other cards.
 Detected and documented on Gujin (boot.c and vgabios.h scroll)

> >>  and probably few other problems - just seen those by reading the patches (the
> >>  asm("") are inlined in the C code - I find it more difficult to check).
> >>
> >>  Also, I do not know if "m" is right in here:
> >> static inline u8 rdfs8(addr_t addr)
> >> {
> >> 	u8 v;
> >> 	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
> >> 	return v;
> >> }
> 
> The "m" is correct right there.

 strange, "g" would mean anything can go there - and this assembly instruction
should accept every access modes.
 
> >>   I may repeat me, but to find these kind of problems, it is very nice to have
> >> an ELF file to do a readelf/objdump -D -m i8086 (even after final link).
> 
> There is such a file (arch/i386/boot/setup.elf) which is retained, for
> exactly this reason.
> 
> 	-hpa
> 

  Etienne.




	
		
___________________________________________________________________________ 
Découvrez une nouvelle façon d'obtenir des réponses à toutes vos questions ! 
Profitez des connaissances, des opinions et des expériences des internautes sur Yahoo! Questions/Réponses 
http://fr.answers.yahoo.com

Re: RE : Re: x86 setup code rewrite in C - revised

[H. Peter Anvin]

Etienne Lorrain wrote:
> --- "H. Peter Anvin" <hpa@zytor.com> wrote:
>> Chuck Ebbert wrote:
> 
>   Wrong name.
> 
>>>>  Have fun, this code:
>>>>  - do not open the fast A20 gate before checking if the slow A20 gate is open or
>> closed.
>>
>> As does the current code; this is highly intentional behaviour since
>> there are machines (in particular a whole series of machines made by
>> Olivetti) which lock up if you do it differently.
> 
>  There was some discussion on this list about some machine which would not wake-up
> correctly if slow A20 was not closed, long time ago. I did not really follow
> the code after that discussion.
>  I wonder if you should do a "outb(0xFF, 0x64);" after "outb(0xdf, 0x60);" like
> the HIMEM.SYS driver, to force an immediate update of the I/O ports - I think
> I also read that in this initial chip docs, long time ago also.

Well, the code we have now has worked for quite some years as-is.  This
code is not an algorithmic change.  I haven't seen any machines which
need outb(0xff, 0x64) -- I wonder if that has the same effect as our
io_delay() or if it is actually potent.  I shall look into it.

>>>>  - Does not save and restore %ds when printing a char on the screen (%ds is
>>>>  destroyed only when the content of the screen scroll - only for some video cards)
>> %ds?  Aren't you confusing it with the old bug which would destroy %bp?
>>  If you have any references to %ds being destroyed I would be very
>> surprised.  I can guarantee that very little if any assembly code I've
>> ever seen that deals with INT 10h -- and I've seen a lot of it -- guards
>> against %ds being randomly trashed.
>>
>> However, the trashing of %bp is a well-known bug (although only for
>> machines older than the ones that can run Linux) -- the Interrupt List has:
>>
>> BUGS:   some implementations (including the original IBM PC) have a bug
>>  	which destroys BP
> 
>  That is on Trident cards, old card but may still be used, and BIOS may have
> been copied to other cards.
>  Detected and documented on Gujin (boot.c and vgabios.h scroll)

Are you talking about BP or DS?  As I said, the BP is well-known, and
the code accounts for it in the form of the INT10 macro.

>>>>  and probably few other problems - just seen those by reading the patches (the
>>>>  asm("") are inlined in the C code - I find it more difficult to check).
>>>>
>>>>  Also, I do not know if "m" is right in here:
>>>> static inline u8 rdfs8(addr_t addr)
>>>> {
>>>> 	u8 v;
>>>> 	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
>>>> 	return v;
>>>> }
>> The "m" is correct right there.
> 
>  strange, "g" would mean anything can go there - and this assembly instruction
> should accept every access modes.

Not with an %fs: prefix.  It would also allow the compiler to do a move
into a register "on its own", which would be disastrous, since it would
lack the prefix.  So "m" is correct.

	-hpa

RE : Re: RE : Re: x86 setup code rewrite in C - revised

[Etienne Lorrain]

--- "H. Peter Anvin" <hpa@zytor.com> wrote:
> >>>>  - Does not save and restore %ds when printing a char on the screen (%ds is
> >>>>  destroyed only when the content of the screen scroll - only for some video cards)
> >> %ds?  Aren't you confusing it with the old bug which would destroy %bp?
> >>  If you have any references to %ds being destroyed I would be very
> >> surprised.  I can guarantee that very little if any assembly code I've
> >> ever seen that deals with INT 10h -- and I've seen a lot of it -- guards
> >> against %ds being randomly trashed.
> >>
> >> However, the trashing of %bp is a well-known bug (although only for
> >> machines older than the ones that can run Linux) -- the Interrupt List has:
> >>
> >> BUGS:   some implementations (including the original IBM PC) have a bug
> >>  	which destroys BP
> > 
> >  That is on Trident cards, old card but may still be used, and BIOS may have
> > been copied to other cards.
> >  Detected and documented on Gujin (boot.c and vgabios.h scroll)
> 
> Are you talking about BP or DS?  As I said, the BP is well-known, and
> the code accounts for it in the form of the INT10 macro.

 Extract of RBIL61:
INT 10 - VIDEO - SCROLL UP WINDOW
	AH = 06h
	AL = number of lines by which to scroll up (00h = clear entire window)
	BH = attribute used to write blank lines at bottom of window
	CH,CL = row,column of window's upper left corner
	DH,DL = row,column of window's lower right corner
Return: nothing
Note:	affects only the currently active page (see AH=05h)
BUGS:	some implementations (including the original IBM PC) have a bug which
	  destroys BP
	the Trident TVGA8900CL (BIOS dated 1992/9/8) clears DS to 0000h when
	  scrolling in an SVGA mode (800x600 or higher)

 Scrolling is only (and automatically) done if the cursor is at bottom right.

> >>>>  Also, I do not know if "m" is right in here:
> >>>> static inline u8 rdfs8(addr_t addr)
> >>>> {
> >>>> 	u8 v;
> >>>> 	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
> >>>> 	return v;
> >>>> }
> >> The "m" is correct right there.
> > 
> >  strange, "g" would mean anything can go there - and this assembly instruction
> > should accept every access modes.
> 
> Not with an %fs: prefix.  It would also allow the compiler to do a move
> into a register "on its own", which would be disastrous, since it would
> lack the prefix.  So "m" is correct.

  "mov %fs:(%ebx,%eax,4),%ecx" works for me.

 Etienne.


      _____________________________________________________________________________ 
Ne gardez plus qu'une seule adresse mail ! Copiez vos mails vers Yahoo! Mail 

Re: RE : Re: RE : Re: x86 setup code rewrite in C - revised

[H. Peter Anvin]

Etienne Lorrain wrote:

> BUGS:	some implementations (including the original IBM PC) have a bug which
> 	  destroys BP
> 	the Trident TVGA8900CL (BIOS dated 1992/9/8) clears DS to 0000h when
> 	  scrolling in an SVGA mode (800x600 or higher)

"When scrolling in an SVGA mode", sounds to me like a bug when using
BIOS for text output in graphics mode.  We don't do that.

>>>>>>  Also, I do not know if "m" is right in here:
>>>>>> static inline u8 rdfs8(addr_t addr)
>>>>>> {
>>>>>> 	u8 v;
>>>>>> 	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
>>>>>> 	return v;
>>>>>> }
>>>> The "m" is correct right there.
>>>  strange, "g" would mean anything can go there - and this assembly instruction
>>> should accept every access modes.
>> Not with an %fs: prefix.  It would also allow the compiler to do a move
>> into a register "on its own", which would be disastrous, since it would
>> lack the prefix.  So "m" is correct.
> 
>   "mov %fs:(%ebx,%eax,4),%ecx" works for me.

That's an example on what "m" can generate.

"g" could produce stuff like:

	mov %fs:$1234,%ecx
	mov %fs:%eax,%ecx

	-hpa

Re: x86 setup code rewrite in C - revised

[Etienne Lorrain]

--- "H. Peter Anvin" <hpa@zytor.com> wrote:
> Etienne Lorrain wrote:
> >   "mov %fs:(%ebx,%eax,4),%ecx" works for me.
> 
> That's an example on what "m" can generate.
> 
> "g" could produce stuff like:
> 
> 	mov %fs:$1234,%ecx
> 	mov %fs:%eax,%ecx

 My solution was to use %fs:%a1 instead of %fs:%1 , but after checking your
solution produces better assembly.

 Etienne.


      _____________________________________________________________________________ 
Ne gardez plus qu'une seule adresse mail ! Copiez vos mails vers Yahoo! Mail 

Re: RE : Re: x86 setup code rewrite in C - revised

[Linus Torvalds]

On Fri, 13 Jul 2007, Etienne Lorrain wrote:
>
> > >>  Also, I do not know if "m" is right in here:
> > >> static inline u8 rdfs8(addr_t addr)
> > >> {
> > >> 	u8 v;
> > >> 	asm("movb %%fs:%1,%0" : "=r" (v) : "m" (*(u8 *)addr));
> > >> 	return v;
> > >> }
> > 
> > The "m" is correct right there.
> 
>  strange, "g" would mean anything can go there - and this assembly instruction
> should accept every access modes.

No it damn well should NOT.

the %fs: override works only with memory ops. End of story.

		Linus

Re: x86 setup code rewrite in C - revised

[H. Peter Anvin]

Chuck Ebbert wrote:
>>  - The VGA recalc has the same bug as the assembly version where a VGA write protected
>>    register is written (Overflow register) without setting the enable bit (see VGA docs).

I dug into this, and it turns out you're incorrect.  Both the assembly
code and the C code are, in fact, 100% correct:

The only instance of writing the vertical overflow register is this code
in vga_set_480_scanlines():

	out_idx(0x0c, crtc, 0x11); /* Vertical sync end, unlock CR0-7 */
	out_idx(0x0b, crtc, 0x06); /* Vertical total */
	out_idx(0x3e, crtc, 0x07); /* Vertical overflow */
	out_idx(0xea, crtc, 0x10); /* Vertical sync start */
	out_idx(end,  crtc, 0x12); /* Vertical display end */
	out_idx(0xe7, crtc, 0x15); /* Vertical blank start */
	out_idx(0x04, crtc, 0x16); /* Vertical blank end */

Register 0x11 has the Protect (not enable!) bit in it, it is bit 7.  As
you can see, it is cleared (meaning writable) at the beginning of this
sequence, and the fact that it's being done is even documented.

	-hpa

Re: x86 setup code rewrite in C - revised

[Etienne Lorrain]

--- "H. Peter Anvin" <hpa@zytor.com> wrote:
> >>  - The VGA recalc has the same bug as the assembly version where a VGA
> >> write protected register is written (Overflow register) without setting
> >> the enable bit (see VGA docs).
> 
> I dug into this, and it turns out you're incorrect.  Both the assembly
> code and the C code are, in fact, 100% correct:
> 
> The only instance of writing the vertical overflow register is this code
> in vga_set_480_scanlines():
> 
> 	out_idx(0x0c, crtc, 0x11); /* Vertical sync end, unlock CR0-7 */
> 	out_idx(0x0b, crtc, 0x06); /* Vertical total */
> 	out_idx(0x3e, crtc, 0x07); /* Vertical overflow */
> 	out_idx(0xea, crtc, 0x10); /* Vertical sync start */
> 	out_idx(end,  crtc, 0x12); /* Vertical display end */
> 	out_idx(0xe7, crtc, 0x15); /* Vertical blank start */
> 	out_idx(0x04, crtc, 0x16); /* Vertical blank end */
> 
> Register 0x11 has the Protect (not enable!) bit in it, it is bit 7.  As
> you can see, it is cleared (meaning writable) at the beginning of this
> sequence, and the fact that it's being done is even documented.

  The only time I ever needed this "end line recalculation" was when the heigh in
 graphic lines was not a multiple of the character heigh - i.e. 640x350 with 8x16
 or 8x8 chars - some VGA adapters do not hide the bottom graphic lines.
  The function vga_set_480_scanlines() is not called, and the protect bit is never
 cleared - the video BIOS leaving those low index register protected.
  The function vga_recalc_vertical() (or its assembler equivalent) is probably
 perfectly called but because the protect bit is never cleared, the few graphic
 line are displayed during the whole Linux text session...
  I have myself never seen any other problems when the graphic heigh is a multiple
 of the character heigh - tested on ~40 video boards.

  Etienne.




	
		
___________________________________________________________________________ 
Découvrez une nouvelle façon d'obtenir des réponses à toutes vos questions ! 
Profitez des connaissances, des opinions et des expériences des internautes sur Yahoo! Questions/Réponses 
http://fr.answers.yahoo.com

Re: x86 setup code rewrite in C - revised

[H. Peter Anvin]

Etienne Lorrain wrote:
> 
>   The only time I ever needed this "end line recalculation" was when the heigh in
>  graphic lines was not a multiple of the character heigh - i.e. 640x350 with 8x16
>  or 8x8 chars - some VGA adapters do not hide the bottom graphic lines.
>   The function vga_set_480_scanlines() is not called, and the protect bit is never
>  cleared - the video BIOS leaving those low index register protected.
>   The function vga_recalc_vertical() (or its assembler equivalent) is probably
>  perfectly called but because the protect bit is never cleared, the few graphic
>  line are displayed during the whole Linux text session...
>   I have myself never seen any other problems when the graphic heigh is a multiple
>  of the character heigh - tested on ~40 video boards.
> 

OK, I see what you mean.  This would be a problem if:

- the VGA BIOS leaves the protected bit set
- the user enables vertical recalculation
- the size crosses a multiple of 256

It is a bug (ported from the assembly) and fortunately quite easy to
fix.  I don't know why I missed this when I looked before.

	-hpa