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

Re: x86 setup code rewrite in C - revised

[Etienne Lorrain]

Andi wrote:
> The only thing questionable is that .code16gcc is arguably quite
> an abuse of gcc. I even checked with some gcc developers 
> and they weren't too happy about it. e.g. it's not regression
> tested at all so we would be basically on our own with it.

 On the other hand GCC just produces an assembly text file, it is
not the GCC developper problem what the user does with this text file.
Usually it goes to the assembler with standard options - .code16gcc
is a "special" option - and bugs (if there is) should be forwarded to binutils.
 GCC tries to go around CPU bugs, and those bug may be different in
protected or real mode - but I still do not have one example of such
a bug.
 GCC also has a base library support (multiplication & divisions of
64 bits...), and when you use .code16gcc you know you cannot touch it
(it is assembled with .code32); so that management may be what they
are refering to.

 Etienne.


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

Re: x86 setup code rewrite in C - revised

[Etienne Lorrain]

> This patch set replaces the x86 setup code, which is currently all in
> assembly, with a version written in C, using the ".code16gcc" feature
> of binutils (which has been present since at least 2001.)

 ".code16gcc" is useable since a bit earlier than that, in fact since:
http://sourceware.org/ml/binutils/2000-04/msg00021.html
 has been fixed, I am using it every other day.

> The new code is vastly easier to read, and, I hope, debug.

 Yes it is, and there is a lot of work in those 33 patch; in very few of
them I would have liked to read something like "inspired by ..." in one of
the C comments (no need to add any E-mail, I already receive enough spam).

 Etienne.

http://gujin.org


	

	
		
___________________________________________________________________________ 
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

x86 setup code rewrite in C - revised

[H. Peter Anvin]

This patch set replaces the x86 setup code, which is currently all in
assembly, with a version written in C, using the ".code16gcc" feature
of binutils (which has been present since at least 2001.)

The new code is vastly easier to read, and, I hope, debug.  It should
be noted that I found a fair number of minor bugs while going through
this code, and have attempted to correct them.

In the process of doing so, it introduces several cleanups, in
particular:

- Obsoletes the hd_info field in the boot_params structure; they are
  only ever used for ST-506 (pre-IDE) drives and are pretty much
  guaranteed to be wrong on current BIOSes;
- Unifies the CPU feature bits between i386 and x86-64.  In the
  future, it should be possible to use arch/i386/boot/cpucheck.c to do
  the post-invocation CPU check currently done in
  arch/x86_64/kernel/trampoline.S, although this patch set doesn't
  introduce that change.
- boot_params is now a proper structure.

This patchset incorporates all feedback received by 2007-07-11 12:00 PDT.

 arch/i386/boot/bootsect.S                     |   98 -
 arch/i386/boot/edd.S                          |  231 --
 arch/i386/boot/setup.S                        | 1075 -------------
 arch/i386/boot/video.S                        | 2043 --------------------------
 arch/i386/kernel/verify_cpu.S                 |   94 -
 arch/x86_64/boot/bootsect.S                   |   98 -
 arch/x86_64/boot/install.sh                   |    2 
 arch/x86_64/boot/mtools.conf.in               |   17 
 arch/x86_64/boot/setup.S                      |  826 ----------
 arch/x86_64/boot/tools/build.c                |  185 --
 b/Documentation/i386/zero-page.txt            |    1 
 b/MAINTAINERS                                 |    4 
 b/arch/i386/Kconfig.cpu                       |    6 
 b/arch/i386/boot/Makefile                     |   48 
 b/arch/i386/boot/a20.c                        |  161 ++
 b/arch/i386/boot/apm.c                        |   97 +
 b/arch/i386/boot/bitops.h                     |   45 
 b/arch/i386/boot/boot.h                       |  296 +++
 b/arch/i386/boot/cmdline.c                    |   97 +
 b/arch/i386/boot/code16gcc.h                  |   15 
 b/arch/i386/boot/compressed/Makefile          |    7 
 b/arch/i386/boot/compressed/head.S            |    6 
 b/arch/i386/boot/copy.S                       |  101 +
 b/arch/i386/boot/cpu.c                        |   69 
 b/arch/i386/boot/cpucheck.c                   |  267 +++
 b/arch/i386/boot/edd.c                        |  196 ++
 b/arch/i386/boot/header.S                     |  283 +++
 b/arch/i386/boot/main.c                       |  161 ++
 b/arch/i386/boot/mca.c                        |   43 
 b/arch/i386/boot/memory.c                     |   99 +
 b/arch/i386/boot/pm.c                         |  170 ++
 b/arch/i386/boot/pmjump.S                     |   54 
 b/arch/i386/boot/printf.c                     |  307 +++
 b/arch/i386/boot/setup.ld                     |   54 
 b/arch/i386/boot/string.c                     |   52 
 b/arch/i386/boot/tools/build.c                |  160 --
 b/arch/i386/boot/tty.c                        |  112 +
 b/arch/i386/boot/version.c                    |   23 
 b/arch/i386/boot/vesa.h                       |   79 +
 b/arch/i386/boot/video-bios.c                 |  125 +
 b/arch/i386/boot/video-vesa.c                 |  284 +++
 b/arch/i386/boot/video-vga.c                  |  260 +++
 b/arch/i386/boot/video.c                      |  456 +++++
 b/arch/i386/boot/video.h                      |  145 +
 b/arch/i386/boot/voyager.c                    |   46 
 b/arch/i386/kernel/cpu/Makefile               |    2 
 b/arch/i386/kernel/cpu/addon_cpuid_features.c |   50 
 b/arch/i386/kernel/cpu/common.c               |    2 
 b/arch/i386/kernel/cpu/proc.c                 |   21 
 b/arch/i386/kernel/e820.c                     |    2 
 b/arch/i386/kernel/setup.c                    |   12 
 b/arch/x86_64/Kconfig                         |    4 
 b/arch/x86_64/boot/Makefile                   |  136 -
 b/arch/x86_64/boot/compressed/Makefile        |    9 
 b/arch/x86_64/boot/compressed/head.S          |    6 
 b/arch/x86_64/kernel/Makefile                 |    2 
 b/arch/x86_64/kernel/setup.c                  |   21 
 b/arch/x86_64/kernel/verify_cpu.S             |   22 
 b/drivers/ide/legacy/hd.c                     |   73 
 b/include/asm-i386/boot.h                     |    6 
 b/include/asm-i386/bootparam.h                |   85 +
 b/include/asm-i386/cpufeature.h               |   26 
 b/include/asm-i386/e820.h                     |   14 
 b/include/asm-i386/processor.h                |    1 
 b/include/asm-i386/required-features.h        |   39 
 b/include/asm-i386/setup.h                    |   10 
 b/include/asm-x86_64/alternative.h            |   68 
 b/include/asm-x86_64/boot.h                   |   16 
 b/include/asm-x86_64/bootparam.h              |    1 
 b/include/asm-x86_64/cpufeature.h             |  115 -
 b/include/asm-x86_64/e820.h                   |    4 
 b/include/asm-x86_64/processor.h              |    3 
 b/include/asm-x86_64/required-features.h      |   46 
 b/include/asm-x86_64/segment.h                |    8 
 b/include/linux/edd.h                         |    4 
 b/include/linux/screen_info.h                 |    9 
 76 files changed, 4606 insertions(+), 5209 deletions(-)

Re: x86 setup code rewrite in C - revised

[Jeff Garzik]

H. Peter Anvin wrote:
> This patch set replaces the x86 setup code, which is currently all in
> assembly, with a version written in C, using the ".code16gcc" feature
> of binutils (which has been present since at least 2001.)
> 
> The new code is vastly easier to read, and, I hope, debug.  It should
> be noted that I found a fair number of minor bugs while going through
> this code, and have attempted to correct them.
> 
> In the process of doing so, it introduces several cleanups, in
> particular:
> 
> - Obsoletes the hd_info field in the boot_params structure; they are
>   only ever used for ST-506 (pre-IDE) drives and are pretty much
>   guaranteed to be wrong on current BIOSes;
> - Unifies the CPU feature bits between i386 and x86-64.  In the
>   future, it should be possible to use arch/i386/boot/cpucheck.c to do
>   the post-invocation CPU check currently done in
>   arch/x86_64/kernel/trampoline.S, although this patch set doesn't
>   introduce that change.
> - boot_params is now a proper structure.
> 
> This patchset incorporates all feedback received by 2007-07-11 12:00 PDT.

I'm sure you know what's changed since the last revision, but nobody 
everybody does :)  It would be nice to know what's different from the 
last posting.

	Jeff

Re: x86 setup code rewrite in C - revised

[H. Peter Anvin]

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

Jeff Garzik wrote:
> 
> I'm sure you know what's changed since the last revision, but nobody
> everybody does :)  It would be nice to know what's different from the
> last posting.
> 

- Added -fno-toplevel-reorder/-fno-unit-at-a-time to the Makefile; this
  is necessary for .code16gcc to be safe on all current versions of gcc.
  Added comment to that effect to code16gcc.h.
- Moved strnlen, atou and isdigit out of printf.c.
- Corrected title of cpu.c and cpucheck.c.
- Fixed "paragraps" typo in pm.c.
- Removed bogus <linux/edd.h> in string.c.
- Removed #include <linux/mmzone.h> from asm-x86/e820.h; according to
  Andi it isn't needed even for non-_SETUP, and my compile testing seems
  to confirm that.

Full diff attached.

	-hpa



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 5256 bytes --]

diff --git a/arch/i386/boot/Makefile b/arch/i386/boot/Makefile
index e9f46fd..08678a0 100644
--- a/arch/i386/boot/Makefile
+++ b/arch/i386/boot/Makefile
@@ -57,6 +57,8 @@ CFLAGS		:= $(LINUXINCLUDE) -g -Os -D_SETUP -D__KERNEL__ \
 		   -include $(srctree)/$(src)/code16gcc.h \
 		   -fno-strict-aliasing -fomit-frame-pointer \
 		   $(call cc-option, -ffreestanding) \
+		   $(call cc-option, -fno-toplevel-reorder,\
+			$(call cc-option, -fno-unit-at-a-time)) \
 		   $(call cc-option, -fno-stack-protector) \
 		   $(call cc-option, -mpreferred-stack-boundary=2)
 AFLAGS		:= $(CFLAGS) -D__ASSEMBLY__
diff --git a/arch/i386/boot/boot.h b/arch/i386/boot/boot.h
index 7251ef6..0329c4f 100644
--- a/arch/i386/boot/boot.h
+++ b/arch/i386/boot/boot.h
@@ -192,6 +192,11 @@ static inline int memcmp_gs(const void *s1, addr_t s2, size_t len)
 	return diff;
 }
 
+static inline int isdigit(int ch)
+{
+	return (ch >= '0') && (ch <= '9');
+}
+
 /* Heap -- available for dynamic lists. */
 #define STACK_SIZE	512	/* Minimum number of bytes for stack */
 
@@ -261,13 +266,14 @@ void __attribute__((noreturn))
 	protected_mode_jump(u32 entrypoint, u32 bootparams);
 
 /* printf.c */
-unsigned int atou(const char *s);
 int sprintf(char *buf, const char *fmt, ...);
 int vsprintf(char *buf, const char *fmt, va_list args);
 int printf(const char *fmt, ...);
 
 /* string.c */
 int strcmp(const char *str1, const char *str2);
+size_t strnlen(const char *s, size_t maxlen);
+unsigned int atou(const char *s);
 
 /* tty.c */
 void puts(const char *);
diff --git a/arch/i386/boot/code16gcc.h b/arch/i386/boot/code16gcc.h
index dbc6414..3bd8480 100644
--- a/arch/i386/boot/code16gcc.h
+++ b/arch/i386/boot/code16gcc.h
@@ -2,6 +2,12 @@
  * code16gcc.h
  *
  * This file is -include'd when compiling 16-bit C code.
+ * Note: this asm() needs to be emitted before gcc omits any code.
+ * Depending on gcc version, this requires -fno-unit-at-a-time or
+ * -fno-toplevel-reorder.
+ *
+ * Hopefully gcc will eventually have a real -m16 option so we can
+ * drop this hack long term.
  */
 
 #ifndef __ASSEMBLY__
diff --git a/arch/i386/boot/cpu.c b/arch/i386/boot/cpu.c
index 042d894..2a5c32d 100644
--- a/arch/i386/boot/cpu.c
+++ b/arch/i386/boot/cpu.c
@@ -9,7 +9,7 @@
  * ----------------------------------------------------------------------- */
 
 /*
- * arch/i386/boot/cpucheck.c
+ * arch/i386/boot/cpu.c
  *
  * Check for obligatory CPU features and abort if the features are not
  * present.
diff --git a/arch/i386/boot/cpucheck.c b/arch/i386/boot/cpucheck.c
index 431bef8..8b0f447 100644
--- a/arch/i386/boot/cpucheck.c
+++ b/arch/i386/boot/cpucheck.c
@@ -9,7 +9,7 @@
  * ----------------------------------------------------------------------- */
 
 /*
- * arch/i386/boot/cpu.c
+ * arch/i386/boot/cpucheck.c
  *
  * Check for obligatory CPU features and abort if the features are not
  * present.  This code should be compilable as 16-, 32- or 64-bit
diff --git a/arch/i386/boot/pm.c b/arch/i386/boot/pm.c
index 4deb298..3fa53e1 100644
--- a/arch/i386/boot/pm.c
+++ b/arch/i386/boot/pm.c
@@ -49,7 +49,7 @@ static void move_kernel_around(void)
 
 	dst_seg =  0x1000 >> 4;
 	src_seg = 0x10000 >> 4;
-	syssize = boot_params.hdr.syssize; /* Size in 16-byte paragraps */
+	syssize = boot_params.hdr.syssize; /* Size in 16-byte paragraphs */
 
 	while (syssize) {
 		int paras  = (syssize >= 0x1000) ? 0x1000 : syssize;
diff --git a/arch/i386/boot/printf.c b/arch/i386/boot/printf.c
index ee3d9fc..1a09f93 100644
--- a/arch/i386/boot/printf.c
+++ b/arch/i386/boot/printf.c
@@ -19,11 +19,6 @@
 
 #include "boot.h"
 
-static inline int isdigit(int ch)
-{
-	return (ch >= '0') && (ch <= '9');
-}
-
 static int skip_atoi(const char **s)
 {
 	int i = 0;
@@ -33,25 +28,6 @@ static int skip_atoi(const char **s)
 	return i;
 }
 
-unsigned int atou(const char *s)
-{
-	unsigned int i = 0;
-	while (isdigit(*s))
-		i = i * 10 + (*s++ - '0');
-	return i;
-}
-
-static int strnlen(const char *s, int maxlen)
-{
-	const char *es = s;
-	while (*es && maxlen) {
-		es++;
-		maxlen--;
-	}
-
-	return (es - s);
-}
-
 #define ZEROPAD	1		/* pad with zero */
 #define SIGN	2		/* unsigned/signed long */
 #define PLUS	4		/* show plus */
diff --git a/arch/i386/boot/string.c b/arch/i386/boot/string.c
index 0808197..481a220 100644
--- a/arch/i386/boot/string.c
+++ b/arch/i386/boot/string.c
@@ -15,7 +15,6 @@
  */
 
 #include "boot.h"
-#include <linux/edd.h>
 
 int strcmp(const char *str1, const char *str2)
 {
@@ -32,3 +31,22 @@ int strcmp(const char *str1, const char *str2)
 	}
 	return 0;
 }
+
+size_t strnlen(const char *s, size_t maxlen)
+{
+	const char *es = s;
+	while (*es && maxlen) {
+		es++;
+		maxlen--;
+	}
+
+	return (es - s);
+}
+
+unsigned int atou(const char *s)
+{
+	unsigned int i = 0;
+	while (isdigit(*s))
+		i = i * 10 + (*s++ - '0');
+	return i;
+}
diff --git a/include/asm-x86_64/e820.h b/include/asm-x86_64/e820.h
index 15eb8e2..3486e70 100644
--- a/include/asm-x86_64/e820.h
+++ b/include/asm-x86_64/e820.h
@@ -11,10 +11,6 @@
 #ifndef __E820_HEADER
 #define __E820_HEADER
 
-#ifndef _SETUP
-# include <linux/mmzone.h>
-#endif
-
 #define E820MAP	0x2d0		/* our map */
 #define E820MAX	128		/* number of entries in E820MAP */
 #define E820NR	0x1e8		/* # entries in E820MAP */

Re: x86 setup code rewrite in C - revised

[Linus Torvalds]

On Wed, 11 Jul 2007, H. Peter Anvin wrote:
>
> This patch set replaces the x86 setup code, which is currently all in
> assembly, with a version written in C, using the ".code16gcc" feature
> of binutils (which has been present since at least 2001.)
>
>  76 files changed, 4606 insertions(+), 5209 deletions(-)

I can't really argue against this on any sane grounds - not only is it 
removing more lines than it adds, but moving from mostly unreadable 
assembly to C seems a good idea.

How does this impact the size of that code? Do we even care?

But as to how to integrate it, I'm not sure I really want to just merge 
it. I suspect we would want to have it in some public tree that people 
actually test at least to some degree first, and the -mm tree seems to 
make most sense.

I didn't see anything objectionable in the series, although I do think the 
explanations need to be re-done for a number of them. You seem to have 
violated the "a single line to explain the patch at the top" rule, and as 
a result they make no sense for some of them (the explanation for patch 
05/33 doesn't parse for me and 07/33 seems to have the single-line 
problem)

So let's just get this merged. But the question is, do we put it in 
2.6.23-rc1, or do we put it in -mm for a few weeks, which would imply 
waiting for the next merge window? Andrew?

			Linus

Re: x86 setup code rewrite in C - revised

[Andrew Morton]

On Thu, 12 Jul 2007 10:24:48 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Wed, 11 Jul 2007, H. Peter Anvin wrote:
> >
> > This patch set replaces the x86 setup code, which is currently all in
> > assembly, with a version written in C, using the ".code16gcc" feature
> > of binutils (which has been present since at least 2001.)
> >
> >  76 files changed, 4606 insertions(+), 5209 deletions(-)
> 
> I can't really argue against this on any sane grounds - not only is it 
> removing more lines than it adds, but moving from mostly unreadable 
> assembly to C seems a good idea.
> 
> How does this impact the size of that code? Do we even care?
> 
> But as to how to integrate it, I'm not sure I really want to just merge 
> it. I suspect we would want to have it in some public tree that people 
> actually test at least to some degree first, and the -mm tree seems to 
> make most sense.
> 
> I didn't see anything objectionable in the series, although I do think the 
> explanations need to be re-done for a number of them. You seem to have 
> violated the "a single line to explain the patch at the top" rule, and as 
> a result they make no sense for some of them (the explanation for patch 
> 05/33 doesn't parse for me and 07/33 seems to have the single-line 
> problem)
> 
> So let's just get this merged. But the question is, do we put it in 
> 2.6.23-rc1, or do we put it in -mm for a few weeks, which would imply 
> waiting for the next merge window? Andrew?
> 

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.

Re: x86 setup code rewrite in C - revised

[Linus Torvalds]

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

Re: x86 setup code rewrite in C - revised

[Andi Kleen]

On Thu, Jul 12, 2007 at 10:24:48AM -0700, Linus Torvalds wrote:
> 
> 
> On Wed, 11 Jul 2007, H. Peter Anvin wrote:
> >
> > This patch set replaces the x86 setup code, which is currently all in
> > assembly, with a version written in C, using the ".code16gcc" feature
> > of binutils (which has been present since at least 2001.)
> >
> >  76 files changed, 4606 insertions(+), 5209 deletions(-)
> 
> I can't really argue against this on any sane grounds - not only is it 
> removing more lines than it adds, but moving from mostly unreadable 
> assembly to C seems a good idea.

The only thing questionable is that .code16gcc is arguably quite
an abuse of gcc. I even checked with some gcc developers 
and they weren't too happy about it. e.g. it's not regression
tested at all so we would be basically on our own with it.

But yes the code looks good.

-Andi