[Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[Andrzej Zaborowski]

CVSROOT:	/sources/qemu
Module name:	qemu
Changes by:	Andrzej Zaborowski <balrog>	07/10/31 01:54:05

Modified files:
	.              : vl.c vl.h 
	hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c 
	                 mips_malta.c mips_mipssim.c mips_pica61.c 
	                 mips_r4k.c palm.c pc.c ppc405_boards.c 
	                 ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c 
	                 realview.c shix.c spitz.c sun4m.c sun4u.c 
	                 versatilepb.c 

Log message:
	Set boot sequence from command line (Dan Kenigsberg).

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/qemu/vl.c?cvsroot=qemu&r1=1.352&r2=1.353
http://cvs.savannah.gnu.org/viewcvs/qemu/vl.h?cvsroot=qemu&r1=1.284&r2=1.285
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/an5206.c?cvsroot=qemu&r1=1.3&r2=1.4
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/etraxfs.c?cvsroot=qemu&r1=1.1&r2=1.2
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/integratorcp.c?cvsroot=qemu&r1=1.20&r2=1.21
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mcf5208.c?cvsroot=qemu&r1=1.4&r2=1.5
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mips_malta.c?cvsroot=qemu&r1=1.45&r2=1.46
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mips_mipssim.c?cvsroot=qemu&r1=1.2&r2=1.3
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mips_pica61.c?cvsroot=qemu&r1=1.9&r2=1.10
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mips_r4k.c?cvsroot=qemu&r1=1.49&r2=1.50
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/palm.c?cvsroot=qemu&r1=1.7&r2=1.8
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/pc.c?cvsroot=qemu&r1=1.87&r2=1.88
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc405_boards.c?cvsroot=qemu&r1=1.7&r2=1.8
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_chrp.c?cvsroot=qemu&r1=1.46&r2=1.47
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_oldworld.c?cvsroot=qemu&r1=1.2&r2=1.3
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_prep.c?cvsroot=qemu&r1=1.50&r2=1.51
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/r2d.c?cvsroot=qemu&r1=1.2&r2=1.3
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/realview.c?cvsroot=qemu&r1=1.11&r2=1.12
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/shix.c?cvsroot=qemu&r1=1.5&r2=1.6
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/spitz.c?cvsroot=qemu&r1=1.12&r2=1.13
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/sun4m.c?cvsroot=qemu&r1=1.56&r2=1.57
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/sun4u.c?cvsroot=qemu&r1=1.23&r2=1.24
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/versatilepb.c?cvsroot=qemu&r1=1.17&r2=1.18

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[J. Mayer]

On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> CVSROOT:	/sources/qemu
> Module name:	qemu
> Changes by:	Andrzej Zaborowski <balrog>	07/10/31 01:54:05
> 
> Modified files:
> 	.              : vl.c vl.h 
> 	hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c 
> 	                 mips_malta.c mips_mipssim.c mips_pica61.c 
> 	                 mips_r4k.c palm.c pc.c ppc405_boards.c 
> 	                 ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c 
> 	                 realview.c shix.c spitz.c sun4m.c sun4u.c 
> 	                 versatilepb.c 
> 
> Log message:
> 	Set boot sequence from command line (Dan Kenigsberg).

There have been remarks about this patch that have not been addressed
(not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
to 3 when more than 3 boot devices are possible to select (see the
BOOTCHARS definition), which clearly shows the patch is not consistent.
Furthermore, the patch breaks the coding style in some files (at least
the ones I checked), which is weird.
Seems _very_ strange to see it commited, then.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[andrzej zaborowski]

Hi,

On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
>
> On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > CVSROOT:      /sources/qemu
> > Module name:  qemu
> > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> >
> > Modified files:
> >       .              : vl.c vl.h
> >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> >                        mips_malta.c mips_mipssim.c mips_pica61.c
> >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> >                        versatilepb.c
> >
> > Log message:
> >       Set boot sequence from command line (Dan Kenigsberg).
>
> There have been remarks about this patch that have not been addressed
> (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> to 3 when more than 3 boot devices are possible to select (see the
> BOOTCHARS definition), which clearly shows the patch is not consistent.

I double-checked to make sure all remarks made on qemu-devel were
addressed, but I may have missed something. It was explained that the
default bios supports only three boot devices, on a second thought I
see how this may affect people using a non-default bios, but I guess 3
boot devices is better than only one that was possible without this
patch.

Feel free to revert if you see any issues.

> Furthermore, the patch breaks the coding style in some files (at least
> the ones I checked), which is weird.

I also tried to make sure that the original style in every file was
retained (i.e. I wrapped lines crossing 80 chars), but again I may
have missed something.

Regards

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[J. Mayer]

On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> Hi,
> 
> On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> >
> > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > CVSROOT:      /sources/qemu
> > > Module name:  qemu
> > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > >
> > > Modified files:
> > >       .              : vl.c vl.h
> > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > >                        versatilepb.c
> > >
> > > Log message:
> > >       Set boot sequence from command line (Dan Kenigsberg).
> >
> > There have been remarks about this patch that have not been addressed
> > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > to 3 when more than 3 boot devices are possible to select (see the
> > BOOTCHARS definition), which clearly shows the patch is not consistent.
> 
> I double-checked to make sure all remarks made on qemu-devel were
> addressed, but I may have missed something. It was explained that the
> default bios supports only three boot devices,

Then just take a look at the function boot_device2nibble in hw/pc.c. You
can see 4 possibilities implemented here. And I think I've never seen a
PC BIOS (on real machines, I mean) that don't allow more than 4 choices
in last 5 years (and maybe much more...)
The second point is that, as the legacy PC-BIOS is maybe the less
versatile architecture that can be, putting limitations to the emulation
model based on this spec seems to be a nonsense in Qemu, which is
supposed to emulate _a lot_ of different architectures. As a matter of
fact, a specific implementation (ie legacy PC target) should not lead to
have hardcoded limits that would affect all other emulated targets.

>  on a second thought I
> see how this may affect people using a non-default bios, but I guess 3
> boot devices is better than only one that was possible without this
> patch.

For most emulation targets, there still is a limit to 1. And the global
limit to 3 is not even related to the PC spec, according to the code
commited in pc.c. Then, imho, it cannot be better as it's inconsistent
for the PC case and provides nothing in most cases.
What is the explanation of a global define to 1 for most target when you
cannot globally know how the information will be exploited ? It would
seem really more logical to allow the user to give all defined possible
boot devices to the -boot parameter, then it's up to the target
initialisation code or the BIOS (some target may use different BIOS with
different ABIs for different usages...) to determine if the information
can be used totally, partially or not at all. Let me give an example:
what is the meaning of the -boot parameter for embedded board that can
only boot from a flash device (see the ppc405_boards.c, for
example...) ? My answer is that the user can always give the -boot
parameter but it will just be ignored by the target specific code. And
the number of boot devices that may be usefull for a target is target or
BIOS dependant. It's not in any way CPU architecture dependant, then the
MAX_BOOT_DEVICES as it is implemented is false for the legacy PC
architecture and has no meaning for all other cases.

> Feel free to revert if you see any issues.

I don't think it breaks anything, then now that it's commited, it seems
more urgent to see the patch reworked to make it consistent and really
usable in all cases (PC is not the only Qemu target !) than to revert
and generate CVS noise...

> > Furthermore, the patch breaks the coding style in some files (at least
> > the ones I checked), which is weird.
> 
> I also tried to make sure that the original style in every file was
> retained (i.e. I wrapped lines crossing 80 chars)

Apparently, not totally. (including 80 chars wrapping lines).

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[andrzej zaborowski]

On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
>
> On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> > Hi,
> >
> > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > >
> > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > > CVSROOT:      /sources/qemu
> > > > Module name:  qemu
> > > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > > >
> > > > Modified files:
> > > >       .              : vl.c vl.h
> > > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > > >                        versatilepb.c
> > > >
> > > > Log message:
> > > >       Set boot sequence from command line (Dan Kenigsberg).
> > >
> > > There have been remarks about this patch that have not been addressed
> > > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > > to 3 when more than 3 boot devices are possible to select (see the
> > > BOOTCHARS definition), which clearly shows the patch is not consistent.
> >
> > I double-checked to make sure all remarks made on qemu-devel were
> > addressed, but I may have missed something. It was explained that the
> > default bios supports only three boot devices,
>
> Then just take a look at the function boot_device2nibble in hw/pc.c. You
> can see 4 possibilities implemented here. And I think I've never seen a
> PC BIOS (on real machines, I mean) that don't allow more than 4 choices
> in last 5 years (and maybe much more...)

MAX_BOOT_DEVICES doesn't limit the number of possible boot devices, it
is only a limit for the length of the sequence given on command-line.

> The second point is that, as the legacy PC-BIOS is maybe the less
> versatile architecture that can be, putting limitations to the emulation
> model based on this spec seems to be a nonsense in Qemu, which is
> supposed to emulate _a lot_ of different architectures. As a matter of
> fact, a specific implementation (ie legacy PC target) should not lead to
> have hardcoded limits that would affect all other emulated targets.

I personally wouldn't hardcode any limit but this code was submitted
this way and doesn't limit any current functionality in any way, it
extends it. I prefer the GNU/Hurd style code where no software limits
are ever imposed and even the standard unix limits are undefined (e.g.
no MAXPATHLEN), sometimes at significant cost.

>
> >  on a second thought I
> > see how this may affect people using a non-default bios, but I guess 3
> > boot devices is better than only one that was possible without this
> > patch.
>
> For most emulation targets, there still is a limit to 1. And the global
> limit to 3 is not even related to the PC spec, according to the code
> commited in pc.c. Then, imho, it cannot be better as it's inconsistent
> for the PC case and provides nothing in most cases.

The limit of three is the max boot sequence length and the same (or
lower) limit is already hardcoded in the machine initialisations where
they deal with passing the sequence to the BIOS.

> What is the explanation of a global define to 1 for most target when you
> cannot globally know how the information will be exploited ?

Yes, you can, it all sits in one repository and is part of the same
project. vl.c doesn't have to deal with cases where, say, hw/pc.c is
modified. In such case the author is supposed to update vl.c too.

> It would
> seem really more logical to allow the user to give all defined possible
> boot devices to the -boot parameter, then it's up to the target
> initialisation code or the BIOS (some target may use different BIOS with
> different ABIs for different usages...) to determine if the information
> can be used totally, partially or not at all. Let me give an example:
> what is the meaning of the -boot parameter for embedded board that can
> only boot from a flash device (see the ppc405_boards.c, for
> example...) ? My answer is that the user can always give the -boot
> parameter but it will just be ignored by the target specific code.

This is exactly how it works in current CVS and also how it was before
the commit.

I will remove some of the check in vl.c though, so that machine code
is free to allow the combinations that it likes.

> And
> the number of boot devices that may be usefull for a target is target or
> BIOS dependant. It's not in any way CPU architecture dependant,

In qemu scope target arch is strictly connected with the target
machines and BIOSes available.

Regards

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[J. Mayer]

On Wed, 2007-10-31 at 11:01 +0100, andrzej zaborowski wrote:
> On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> >
> > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> > > Hi,
> > >
> > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > >
> > > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > > > CVSROOT:      /sources/qemu
> > > > > Module name:  qemu
> > > > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > > > >
> > > > > Modified files:
> > > > >       .              : vl.c vl.h
> > > > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > > > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > > > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > > > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > > > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > > > >                        versatilepb.c
> > > > >
> > > > > Log message:
> > > > >       Set boot sequence from command line (Dan Kenigsberg).
> > > >
> > > > There have been remarks about this patch that have not been addressed
> > > > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > > > to 3 when more than 3 boot devices are possible to select (see the
> > > > BOOTCHARS definition), which clearly shows the patch is not consistent.
> > >
> > > I double-checked to make sure all remarks made on qemu-devel were
> > > addressed, but I may have missed something. It was explained that the
> > > default bios supports only three boot devices,
> >
> > Then just take a look at the function boot_device2nibble in hw/pc.c. You
> > can see 4 possibilities implemented here. And I think I've never seen a
> > PC BIOS (on real machines, I mean) that don't allow more than 4 choices
> > in last 5 years (and maybe much more...)
> 
> MAX_BOOT_DEVICES doesn't limit the number of possible boot devices, it
> is only a limit for the length of the sequence given on command-line.
> 
> > The second point is that, as the legacy PC-BIOS is maybe the less
> > versatile architecture that can be, putting limitations to the emulation
> > model based on this spec seems to be a nonsense in Qemu, which is
> > supposed to emulate _a lot_ of different architectures. As a matter of
> > fact, a specific implementation (ie legacy PC target) should not lead to
> > have hardcoded limits that would affect all other emulated targets.
> 
> I personally wouldn't hardcode any limit but this code was submitted
> this way and doesn't limit any current functionality in any way, it
> extends it. I prefer the GNU/Hurd style code where no software limits
> are ever imposed and even the standard unix limits are undefined (e.g.
> no MAXPATHLEN), sometimes at significant cost.

Imho, in that case, the only thing that can be check is that the given
string contains only characters that can be valid devices in Qemu. Then,
making boot_device a pointer directly assigned to optarg then check that
all chars are >= 'a' and < 'c' + MAX_DISKS || chars == 'n' would greatly
simplify the code. And this kind of check is the only valid one you can
do in the generic code.

> > >  on a second thought I
> > > see how this may affect people using a non-default bios, but I guess 3
> > > boot devices is better than only one that was possible without this
> > > patch.
> >
> > For most emulation targets, there still is a limit to 1. And the global
> > limit to 3 is not even related to the PC spec, according to the code
> > commited in pc.c. Then, imho, it cannot be better as it's inconsistent
> > for the PC case and provides nothing in most cases.
> 
> The limit of three is the max boot sequence length and the same (or
> lower) limit is already hardcoded in the machine initialisations where
> they deal with passing the sequence to the BIOS.

Sure. And here is the only place you can hardcode / check anything like
this, because this is _the_ place where you know what physical devices
are actually emulated, and maybe (not always) what are the BIOS
features, ...

> > What is the explanation of a global define to 1 for most target when you
> > cannot globally know how the information will be exploited ?
> 
> Yes, you can, it all sits in one repository and is part of the same
> project. vl.c doesn't have to deal with cases where, say, hw/pc.c is
> modified. In such case the author is supposed to update vl.c too.

There is no information about the emulated target reaching vl.c. In
fact, in a ideal world, there should be not even a single #ifdef
TARGET_xxx in that code. All the hardware emulation part is in the
hardware library and the generic code has no idea about the actual
emulated hardware. It even does not know if a family of device is
supported or not by the target. In microcontrollers, you often have no
bloc device. But nothing prevents you to add '-hda <my_disk> -boot c' to
the command line...

> > It would
> > seem really more logical to allow the user to give all defined possible
> > boot devices to the -boot parameter, then it's up to the target
> > initialisation code or the BIOS (some target may use different BIOS with
> > different ABIs for different usages...) to determine if the information
> > can be used totally, partially or not at all. Let me give an example:
> > what is the meaning of the -boot parameter for embedded board that can
> > only boot from a flash device (see the ppc405_boards.c, for
> > example...) ? My answer is that the user can always give the -boot
> > parameter but it will just be ignored by the target specific code.
> 
> This is exactly how it works in current CVS and also how it was before
> the commit.
> 
> I will remove some of the check in vl.c though, so that machine code
> is free to allow the combinations that it likes.
> 
> > And
> > the number of boot devices that may be usefull for a target is target or
> > BIOS dependant. It's not in any way CPU architecture dependant,
> 
> In qemu scope target arch is strictly connected with the target
> machines and BIOSes available.

I guess you're wrong, here. i386 CPU is the special case for which you
have only 1 target available and only 1 BIOS ABI. For ARM, Mips, Sparc,
PowerPC, you've got several targets with different BIOS ABIs (this last
point, maybe not for Sparc). Using the -L and/or the -bios option, one
can boot on different BIOS, maybe proprietary ones (which often happens
on embedded targets) with specific ABIs. The vl.c code have no way to
know anything about this, the way Qemu is implemented.

Regards.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[J. Mayer]

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


On Wed, 2007-10-31 at 11:22 +0100, J. Mayer wrote:
> On Wed, 2007-10-31 at 11:01 +0100, andrzej zaborowski wrote:
> > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > >
> > > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> > > > Hi,
> > > >
> > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > >
> > > > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > > > > CVSROOT:      /sources/qemu
> > > > > > Module name:  qemu
> > > > > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > > > > >
> > > > > > Modified files:
> > > > > >       .              : vl.c vl.h
> > > > > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > > > > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > > > > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > > > > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > > > > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > > > > >                        versatilepb.c
> > > > > >
> > > > > > Log message:
> > > > > >       Set boot sequence from command line (Dan Kenigsberg).
> > > > >
> > > > > There have been remarks about this patch that have not been addressed
> > > > > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > > > > to 3 when more than 3 boot devices are possible to select (see the
> > > > > BOOTCHARS definition), which clearly shows the patch is not consistent.
> > > >
> > > > I double-checked to make sure all remarks made on qemu-devel were
> > > > addressed, but I may have missed something. It was explained that the
> > > > default bios supports only three boot devices,
> > >
> > > Then just take a look at the function boot_device2nibble in hw/pc.c. You
> > > can see 4 possibilities implemented here. And I think I've never seen a
> > > PC BIOS (on real machines, I mean) that don't allow more than 4 choices
> > > in last 5 years (and maybe much more...)
> > 
> > MAX_BOOT_DEVICES doesn't limit the number of possible boot devices, it
> > is only a limit for the length of the sequence given on command-line.
> > 
> > > The second point is that, as the legacy PC-BIOS is maybe the less
> > > versatile architecture that can be, putting limitations to the emulation
> > > model based on this spec seems to be a nonsense in Qemu, which is
> > > supposed to emulate _a lot_ of different architectures. As a matter of
> > > fact, a specific implementation (ie legacy PC target) should not lead to
> > > have hardcoded limits that would affect all other emulated targets.
> > 
> > I personally wouldn't hardcode any limit but this code was submitted
> > this way and doesn't limit any current functionality in any way, it
> > extends it. I prefer the GNU/Hurd style code where no software limits
> > are ever imposed and even the standard unix limits are undefined (e.g.
> > no MAXPATHLEN), sometimes at significant cost.
> 
> Imho, in that case, the only thing that can be check is that the given
> string contains only characters that can be valid devices in Qemu. Then,
> making boot_device a pointer directly assigned to optarg then check that
> all chars are >= 'a' and < 'c' + MAX_DISKS || chars == 'n' would greatly
> simplify the code. And this kind of check is the only valid one you can
> do in the generic code.

Here's a generic implementation that checks only the boot devices known
to be supported, ie 'a', 'c', 'd' and 'n', thus need no change in the
machine emulation code to work. When the machines will be able to check
properly if the boot devices match the emulated hardware and the BIOS
ABI, then it can be easily extended, changing one line, to allow boot
from more devices. I think that this code should allow choosing to (try
to...) boot from at least the 2 floppies and the 4 possible IDE devices.
The consistency test could also be changed to add more drives if it
seems to be needed.
For consistency, I also made the boot_devices variable local to the main
routine, as it's never used anywhere else.

This patch does not make the code simpler (in fact it's even more
complicated as it does more generic consistency checks) but is generic
and extensible, not breaking the previous patch and being consistent
with the i386 machine BIOS features, as implemented now.

The machine specific checks can be added later, for each target that
need some. Another solution could be that every machine implements a
callback that return a features bitmap, then the generic code could
check if the given command line arguments (including the -boot option,
but not only) are consistent with the emulated hardware platform.

[...]

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: boot_devices.diff --]
[-- Type: text/x-patch, Size: 4496 bytes --]

Index: vl.c
===================================================================
RCS file: /sources/qemu/qemu/vl.c,v
retrieving revision 1.353
diff -u -d -d -p -r1.353 vl.c
--- vl.c	31 Oct 2007 01:54:03 -0000	1.353
+++ vl.c	31 Oct 2007 22:39:27 -0000
@@ -162,12 +162,6 @@ static DisplayState display_state;
 int nographic;
 const char* keyboard_layout = NULL;
 int64_t ticks_per_sec;
-#if defined(TARGET_I386)
-#define MAX_BOOT_DEVICES 3
-#else
-#define MAX_BOOT_DEVICES 1
-#endif
-static char boot_device[MAX_BOOT_DEVICES + 1];
 int ram_size;
 int pit_min_timer_count = 0;
 int nb_nics;
@@ -7564,6 +7560,7 @@ int main(int argc, char **argv)
     const char *sd_filename;
     const char *mtd_filename;
     const char *kernel_filename, *kernel_cmdline;
+    const char *boot_devices = "";
     DisplayState *ds = &display_state;
     int cyls, heads, secs, translation;
     char net_clients[MAX_NET_CLIENTS][256];
@@ -7815,20 +7812,31 @@ int main(int argc, char **argv)
                 }
                 break;
             case QEMU_OPTION_boot:
-                if (strlen(optarg) > MAX_BOOT_DEVICES) {
-                    fprintf(stderr, "qemu: too many boot devices\n");
-                    exit(1);
-                }
-                strncpy(boot_device, optarg, MAX_BOOT_DEVICES);
-#if defined(TARGET_SPARC) || defined(TARGET_I386)
-#define BOOTCHARS "acdn"
-#else
-#define BOOTCHARS "acd"
-#endif
-                if (strlen(boot_device) != strspn(boot_device, BOOTCHARS)) {
-                    fprintf(stderr, "qemu: invalid boot device "
-                                    "sequence '%s'\n", boot_device);
-                    exit(1);
+                boot_devices = optarg;
+                /* We just do some generic consistency checks */
+                {
+                    /* Could easily be extended to 64 devices if needed */
+                    const unsigned char *p;
+                    uint32_t boot_devices_bitmap = 0;
+                    
+                    for (p = boot_devices; *p != '\0'; p++) {
+                        /* Note this could easily be extended in a way like:
+                         * if (*p < 'a' || (*p > 'e' && *p != 'n'))
+                         * then provide at least the ability to boot from
+                         * any floppy or IDE drive for targets that can
+                         * handle it.
+                         */
+                        if (*p != 'a' && *p != 'c' && *p != 'd' && *p != 'n') {
+                            fprintf(stderr, "Invalid boot device '%c'\n", *p);
+                            exit(1);
+                        }
+                        if (boot_devices_bitmap & (1 << (*p - 'a'))) {
+                            fprintf(stderr,
+                                    "Boot device '%c' was given twice\n",*p);
+                            exit(1);
+                        }
+                        boot_devices_bitmap |= 1 << (p - 'a');
+                    }
                 }
                 break;
             case QEMU_OPTION_fda:
@@ -8178,21 +8186,20 @@ int main(int argc, char **argv)
     linux_boot = (kernel_filename != NULL);
 
     if (!linux_boot &&
-        (!strchr(boot_device, 'n')) &&
+        (!strchr(boot_devices, 'n')) &&
         hd_filename[0] == '\0' &&
         (cdrom_index >= 0 && hd_filename[cdrom_index] == '\0') &&
         fd_filename[0] == '\0')
         help(1);
 
     /* boot to floppy or the default cd if no hard disk defined yet */
-    if (!boot_device[0]) {
+    if (!boot_devices[0]) {
         if (hd_filename[0] != '\0')
-            boot_device[0] = 'c';
+            boot_devices = "c";
         else if (fd_filename[0] != '\0')
-            boot_device[0] = 'a';
+            boot_devices = "a";
         else
-            boot_device[0] = 'd';
-        boot_device[1] = 0;
+            boot_devices = "d";
     }
     setvbuf(stdout, NULL, _IOLBF, 0);
 
@@ -8232,7 +8239,7 @@ int main(int argc, char **argv)
     }
 
 #ifdef TARGET_I386
-    if (strchr(boot_device, 'n')) {
+    if (strchr(boot_devices, 'n')) {
 	for (i = 0; i < nb_nics; i++) {
 	    const char *model = nd_table[i].model;
 	    char buf[1024];
@@ -8425,7 +8432,7 @@ int main(int argc, char **argv)
         }
     }
 
-    machine->init(ram_size, vga_ram_size, boot_device,
+    machine->init(ram_size, vga_ram_size, boot_devices,
                   ds, fd_filename, snapshot,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[andrzej zaborowski]

Hi,

On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
>
> On Wed, 2007-10-31 at 11:22 +0100, J. Mayer wrote:
> > On Wed, 2007-10-31 at 11:01 +0100, andrzej zaborowski wrote:
> > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > >
> > > > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> > > > > Hi,
> > > > >
> > > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > > >
> > > > > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > > > > > CVSROOT:      /sources/qemu
> > > > > > > Module name:  qemu
> > > > > > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > > > > > >
> > > > > > > Modified files:
> > > > > > >       .              : vl.c vl.h
> > > > > > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > > > > > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > > > > > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > > > > > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > > > > > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > > > > > >                        versatilepb.c
> > > > > > >
> > > > > > > Log message:
> > > > > > >       Set boot sequence from command line (Dan Kenigsberg).
> > > > > >
> > > > > > There have been remarks about this patch that have not been addressed
> > > > > > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > > > > > to 3 when more than 3 boot devices are possible to select (see the
> > > > > > BOOTCHARS definition), which clearly shows the patch is not consistent.
> > > > >
> > > > > I double-checked to make sure all remarks made on qemu-devel were
> > > > > addressed, but I may have missed something. It was explained that the
> > > > > default bios supports only three boot devices,
> > > >
> > > > Then just take a look at the function boot_device2nibble in hw/pc.c. You
> > > > can see 4 possibilities implemented here. And I think I've never seen a
> > > > PC BIOS (on real machines, I mean) that don't allow more than 4 choices
> > > > in last 5 years (and maybe much more...)
> > >
> > > MAX_BOOT_DEVICES doesn't limit the number of possible boot devices, it
> > > is only a limit for the length of the sequence given on command-line.
> > >
> > > > The second point is that, as the legacy PC-BIOS is maybe the less
> > > > versatile architecture that can be, putting limitations to the emulation
> > > > model based on this spec seems to be a nonsense in Qemu, which is
> > > > supposed to emulate _a lot_ of different architectures. As a matter of
> > > > fact, a specific implementation (ie legacy PC target) should not lead to
> > > > have hardcoded limits that would affect all other emulated targets.
> > >
> > > I personally wouldn't hardcode any limit but this code was submitted
> > > this way and doesn't limit any current functionality in any way, it
> > > extends it. I prefer the GNU/Hurd style code where no software limits
> > > are ever imposed and even the standard unix limits are undefined (e.g.
> > > no MAXPATHLEN), sometimes at significant cost.
> >
> > Imho, in that case, the only thing that can be check is that the given
> > string contains only characters that can be valid devices in Qemu. Then,
> > making boot_device a pointer directly assigned to optarg then check that
> > all chars are >= 'a' and < 'c' + MAX_DISKS || chars == 'n' would greatly
> > simplify the code. And this kind of check is the only valid one you can
> > do in the generic code.
>
> Here's a generic implementation that checks only the boot devices known
> to be supported, ie 'a', 'c', 'd' and 'n', thus need no change in the
> machine emulation code to work. When the machines will be able to check
> properly if the boot devices match the emulated hardware and the BIOS
> ABI, then it can be easily extended, changing one line, to allow boot
> from more devices. I think that this code should allow choosing to (try
> to...) boot from at least the 2 floppies and the 4 possible IDE devices.
> The consistency test could also be changed to add more drives if it
> seems to be needed.
> For consistency, I also made the boot_devices variable local to the main
> routine, as it's never used anywhere else.

Thanks for the rework, I'm in favour of this patch. However, similar
to the previous approach it still restricts the "driver letters" set
and assumes that vl.c will be extended when some per-machine code
needs more letters (which is okay with me, but I had understood that
this was your concern). The letter check is equivalent to
!strchr(BOOTCHARS, *p).

>
> This patch does not make the code simpler (in fact it's even more
> complicated as it does more generic consistency checks) but is generic
> and extensible, not breaking the previous patch and being consistent
> with the i386 machine BIOS features, as implemented now.
>
> The machine specific checks can be added later, for each target that
> need some. Another solution could be that every machine implements a
> callback that return a features bitmap, then the generic code could
> check if the given command line arguments (including the -boot option,
> but not only) are consistent with the emulated hardware platform.

This sounds like the correct approach, although considering the way
error checking is (or isn't) done across qemu, it is perhaps enough
for machine init code to print a message and exit(1) on an
inconsistency.

Regards

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[J. Mayer]

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


On Thu, 2007-11-01 at 01:01 +0100, andrzej zaborowski wrote:
> Hi,
> 
> On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> >
> > On Wed, 2007-10-31 at 11:22 +0100, J. Mayer wrote:
> > > On Wed, 2007-10-31 at 11:01 +0100, andrzej zaborowski wrote:
> > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > >
> > > > > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > > > >
> > > > > > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > > > > > > CVSROOT:      /sources/qemu
> > > > > > > > Module name:  qemu
> > > > > > > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > > > > > > >
> > > > > > > > Modified files:
> > > > > > > >       .              : vl.c vl.h
> > > > > > > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > > > > > > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > > > > > > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > > > > > > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > > > > > > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > > > > > > >                        versatilepb.c
> > > > > > > >
> > > > > > > > Log message:
> > > > > > > >       Set boot sequence from command line (Dan Kenigsberg).
> > > > > > >
> > > > > > > There have been remarks about this patch that have not been addressed
> > > > > > > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > > > > > > to 3 when more than 3 boot devices are possible to select (see the
> > > > > > > BOOTCHARS definition), which clearly shows the patch is not consistent.
> > > > > >
> > > > > > I double-checked to make sure all remarks made on qemu-devel were
> > > > > > addressed, but I may have missed something. It was explained that the
> > > > > > default bios supports only three boot devices,
> > > > >
> > > > > Then just take a look at the function boot_device2nibble in hw/pc.c. You
> > > > > can see 4 possibilities implemented here. And I think I've never seen a
> > > > > PC BIOS (on real machines, I mean) that don't allow more than 4 choices
> > > > > in last 5 years (and maybe much more...)
> > > >
> > > > MAX_BOOT_DEVICES doesn't limit the number of possible boot devices, it
> > > > is only a limit for the length of the sequence given on command-line.
> > > >
> > > > > The second point is that, as the legacy PC-BIOS is maybe the less
> > > > > versatile architecture that can be, putting limitations to the emulation
> > > > > model based on this spec seems to be a nonsense in Qemu, which is
> > > > > supposed to emulate _a lot_ of different architectures. As a matter of
> > > > > fact, a specific implementation (ie legacy PC target) should not lead to
> > > > > have hardcoded limits that would affect all other emulated targets.
> > > >
> > > > I personally wouldn't hardcode any limit but this code was submitted
> > > > this way and doesn't limit any current functionality in any way, it
> > > > extends it. I prefer the GNU/Hurd style code where no software limits
> > > > are ever imposed and even the standard unix limits are undefined (e.g.
> > > > no MAXPATHLEN), sometimes at significant cost.
> > >
> > > Imho, in that case, the only thing that can be check is that the given
> > > string contains only characters that can be valid devices in Qemu. Then,
> > > making boot_device a pointer directly assigned to optarg then check that
> > > all chars are >= 'a' and < 'c' + MAX_DISKS || chars == 'n' would greatly
> > > simplify the code. And this kind of check is the only valid one you can
> > > do in the generic code.
> >
> > Here's a generic implementation that checks only the boot devices known
> > to be supported, ie 'a', 'c', 'd' and 'n', thus need no change in the
> > machine emulation code to work. When the machines will be able to check
> > properly if the boot devices match the emulated hardware and the BIOS
> > ABI, then it can be easily extended, changing one line, to allow boot
> > from more devices. I think that this code should allow choosing to (try
> > to...) boot from at least the 2 floppies and the 4 possible IDE devices.
> > The consistency test could also be changed to add more drives if it
> > seems to be needed.
> > For consistency, I also made the boot_devices variable local to the main
> > routine, as it's never used anywhere else.
> 
> Thanks for the rework, I'm in favour of this patch. However, similar
> to the previous approach it still restricts the "driver letters" set
> and assumes that vl.c will be extended when some per-machine code
> needs more letters (which is okay with me, but I had understood that
> this was your concern). The letter check is equivalent to
> !strchr(BOOTCHARS, *p).

It restricts the letter to the ones historically allowed by Qemu, not to
anything specific to any architecture or hw platform. What I like in my
implementation, compared to the strchr..., is that it exactly tells the
user which given device is incorrect.

> > This patch does not make the code simpler (in fact it's even more
> > complicated as it does more generic consistency checks) but is generic
> > and extensible, not breaking the previous patch and being consistent
> > with the i386 machine BIOS features, as implemented now.
> >
> > The machine specific checks can be added later, for each target that
> > need some. Another solution could be that every machine implements a
> > callback that return a features bitmap, then the generic code could
> > check if the given command line arguments (including the -boot option,
> > but not only) are consistent with the emulated hardware platform.
> 
> This sounds like the correct approach, although considering the way
> error checking is (or isn't) done across qemu, it is perhaps enough
> for machine init code to print a message and exit(1) on an
> inconsistency.

I also think this is the best way to do but it's a greater work to do
and it needs to define well the way it has to be implemented.

Here's a second pass cleanup, adding the machine dependant checks for
the PC machine and the PowerPC ones. As one can see, the OpenHack'Ware
firmware is able to boot from devices 'e' and 'f'. For the PowerPC
machines, I choosed to try to boot from the first given usable device,
some may not agree with this choice. It can be noticed that the
available boot devices are not the same for PowerPC PreP, g3bw and mac99
machines.
As I don't know the features and requirements for the other
architectures, I prefered not to add any check for those ones.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: boot_devices.diff --]
[-- Type: text/x-patch, Size: 10656 bytes --]

Index: vl.c
===================================================================
RCS file: /sources/qemu/qemu/vl.c,v
retrieving revision 1.353
diff -u -d -d -p -r1.353 vl.c
--- vl.c	31 Oct 2007 01:54:03 -0000	1.353
+++ vl.c	1 Nov 2007 19:07:06 -0000
@@ -162,12 +162,6 @@ static DisplayState display_state;
 int nographic;
 const char* keyboard_layout = NULL;
 int64_t ticks_per_sec;
-#if defined(TARGET_I386)
-#define MAX_BOOT_DEVICES 3
-#else
-#define MAX_BOOT_DEVICES 1
-#endif
-static char boot_device[MAX_BOOT_DEVICES + 1];
 int ram_size;
 int pit_min_timer_count = 0;
 int nb_nics;
@@ -7564,6 +7560,7 @@ int main(int argc, char **argv)
     const char *sd_filename;
     const char *mtd_filename;
     const char *kernel_filename, *kernel_cmdline;
+    const char *boot_devices = "";
     DisplayState *ds = &display_state;
     int cyls, heads, secs, translation;
     char net_clients[MAX_NET_CLIENTS][256];
@@ -7815,20 +7812,33 @@ int main(int argc, char **argv)
                 }
                 break;
             case QEMU_OPTION_boot:
-                if (strlen(optarg) > MAX_BOOT_DEVICES) {
-                    fprintf(stderr, "qemu: too many boot devices\n");
-                    exit(1);
-                }
-                strncpy(boot_device, optarg, MAX_BOOT_DEVICES);
-#if defined(TARGET_SPARC) || defined(TARGET_I386)
-#define BOOTCHARS "acdn"
-#else
-#define BOOTCHARS "acd"
-#endif
-                if (strlen(boot_device) != strspn(boot_device, BOOTCHARS)) {
-                    fprintf(stderr, "qemu: invalid boot device "
-                                    "sequence '%s'\n", boot_device);
-                    exit(1);
+                boot_devices = optarg;
+                /* We just do some generic consistency checks */
+                {
+                    /* Could easily be extended to 64 devices if needed */
+                    const unsigned char *p;
+                    uint32_t boot_devices_bitmap = 0;
+                    
+                    for (p = boot_devices; *p != '\0'; p++) {
+                        /* Allowed boot devices are:
+                         * a b     : floppies
+                         * c d e f : IDE disk drives
+                         * n       : network
+                         * It's up to each machine implementation to check
+                         * if the given boot devices match the actual hardware
+                         * implementation and firmware features.
+                         */
+                        if (*p < 'a' || (*p > 'f' && *p != 'n')) {
+                            fprintf(stderr, "Invalid boot device '%c'\n", *p);
+                            exit(1);
+                        }
+                        if (boot_devices_bitmap & (1 << (*p - 'a'))) {
+                            fprintf(stderr,
+                                    "Boot device '%c' was given twice\n",*p);
+                            exit(1);
+                        }
+                        boot_devices_bitmap |= 1 << (*p - 'a');
+                    }
                 }
                 break;
             case QEMU_OPTION_fda:
@@ -8178,21 +8188,20 @@ int main(int argc, char **argv)
     linux_boot = (kernel_filename != NULL);
 
     if (!linux_boot &&
-        (!strchr(boot_device, 'n')) &&
+        (!strchr(boot_devices, 'n')) &&
         hd_filename[0] == '\0' &&
         (cdrom_index >= 0 && hd_filename[cdrom_index] == '\0') &&
         fd_filename[0] == '\0')
         help(1);
 
     /* boot to floppy or the default cd if no hard disk defined yet */
-    if (!boot_device[0]) {
+    if (!boot_devices[0]) {
         if (hd_filename[0] != '\0')
-            boot_device[0] = 'c';
+            boot_devices = "c";
         else if (fd_filename[0] != '\0')
-            boot_device[0] = 'a';
+            boot_devices = "a";
         else
-            boot_device[0] = 'd';
-        boot_device[1] = 0;
+            boot_devices = "d";
     }
     setvbuf(stdout, NULL, _IOLBF, 0);
 
@@ -8232,7 +8241,7 @@ int main(int argc, char **argv)
     }
 
 #ifdef TARGET_I386
-    if (strchr(boot_device, 'n')) {
+    if (strchr(boot_devices, 'n')) {
 	for (i = 0; i < nb_nics; i++) {
 	    const char *model = nd_table[i].model;
 	    char buf[1024];
@@ -8425,7 +8434,7 @@ int main(int argc, char **argv)
         }
     }
 
-    machine->init(ram_size, vga_ram_size, boot_device,
+    machine->init(ram_size, vga_ram_size, boot_devices,
                   ds, fd_filename, snapshot,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
Index: hw/pc.c
===================================================================
RCS file: /sources/qemu/qemu/hw/pc.c,v
retrieving revision 1.88
diff -u -d -d -p -r1.88 pc.c
--- hw/pc.c	31 Oct 2007 01:54:04 -0000	1.88
+++ hw/pc.c	1 Nov 2007 19:07:06 -0000
@@ -173,6 +173,7 @@ static int boot_device2nibble(char boot_
 static void cmos_init(int ram_size, const char *boot_device, BlockDriverState **hd_table)
 {
     RTCState *s = rtc_state;
+    int bds[3], nbds;
     int val;
     int fd0, fd1, nb;
     int i;
@@ -202,11 +203,22 @@ static void cmos_init(int ram_size, cons
     rtc_set_memory(s, 0x35, val >> 8);
 
     /* set boot devices, and disable floppy signature check if requested */
-    rtc_set_memory(s, 0x3d,
-            boot_device2nibble(boot_device[1]) << 4 |
-            boot_device2nibble(boot_device[0]) );
-    rtc_set_memory(s, 0x38,
-            boot_device2nibble(boot_device[2]) << 4 | (fd_bootchk ?  0x0 : 0x1));
+#define PC_MAX_BOOT_DEVICES 3
+    nbds = strlen(boot_device);
+    if (nbds > PC_MAX_BOOT_DEVICES) {
+        fprintf(stderr, "Too many boot devices for PC\n");
+        exit(1);
+    }
+    for (i = 0; i < nbds; i++) {
+        bds[i] = boot_device2nibble(boot_device[i]);
+        if (bds[i] == 0) {
+            fprintf(stderr, "Invalid boot device for PC: '%c'\n",
+                    boot_device[i]);
+            exit(1);
+        }
+    }
+    rtc_set_memory(s, 0x3d, (bds[1] << 4) | bds[0]);
+    rtc_set_memory(s, 0x38, (bds[2] << 4) | (fd_bootchk ?  0x0 : 0x1));
 
     /* floppy type */
 
Index: hw/ppc_chrp.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ppc_chrp.c,v
retrieving revision 1.47
diff -u -d -d -p -r1.47 ppc_chrp.c
--- hw/ppc_chrp.c	31 Oct 2007 01:54:04 -0000	1.47
+++ hw/ppc_chrp.c	1 Nov 2007 19:07:06 -0000
@@ -66,7 +66,7 @@ static void ppc_core99_init (int ram_siz
     qemu_irq *dummy_irq;
     int pic_mem_index, dbdma_mem_index, cuda_mem_index;
     int ide_mem_index[2];
-    int ppc_boot_device = boot_device[0];
+    int ppc_boot_device;
 
     linux_boot = (kernel_filename != NULL);
 
@@ -178,6 +175,19 @@ static void ppc_core99_init (int ram_siz
         kernel_size = 0;
         initrd_base = 0;
         initrd_size = 0;
+        ppc_boot_device = '\0';
+        /* We consider that NewWorld PowerMac never have any floppy drives
+         * For now, OHW cannot boot from the network.
+         */
+        for (i = 0; i < boot_device[i] != '\0'; i++) {
+            ppc_boot_device = boot_device[i];
+            if (ppc_boot_device >= 'c' && ppc_boot_device <= 'f')
+                break;
+        }
+        if (ppc_boot_device == '\0') {
+            fprintf(stderr, "No valid boot device for Mac99 machine\n");
+            exit(1);
+        }
     }
 
     isa_mem_base = 0x80000000;
Index: hw/ppc_oldworld.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ppc_oldworld.c,v
retrieving revision 1.3
diff -u -d -d -p -r1.3 ppc_oldworld.c
--- hw/ppc_oldworld.c	31 Oct 2007 01:54:04 -0000	1.3
+++ hw/ppc_oldworld.c	1 Nov 2007 19:07:06 -0000
@@ -114,7 +113,7 @@ static void ppc_heathrow_init (int ram_s
     int vga_bios_size, bios_size;
     qemu_irq *dummy_irq;
     int pic_mem_index, nvram_mem_index, dbdma_mem_index, cuda_mem_index;
-    int ppc_boot_device = boot_device[0];
+    int ppc_boot_device;
 
     linux_boot = (kernel_filename != NULL);
 
@@ -215,6 +214,25 @@ static void ppc_heathrow_init (int ram_s
         kernel_size = 0;
         initrd_base = 0;
         initrd_size = 0;
+        ppc_boot_device = '\0';
+        for (i = 0; i < boot_device[i] != '\0'; i++) {
+            ppc_boot_device = boot_device[i];
+            /* TOFIX: for now, the second IDE channel is not properly
+             *        emulated. The Mac floppy disks are not emulated at all.
+             *        For now, OHW cannot boot from the network.
+             */
+#if 0
+            if (ppc_boot_device >= 'a' && ppc_boot_device <= 'f')
+                break;
+#else
+            if (ppc_boot_device >= 'c' && ppc_boot_device <= 'd')
+                break;
+#endif
+        }
+        if (ppc_boot_device == '\0') {
+            fprintf(stderr, "No valid boot device for Mac99 machine\n");
+            exit(1);
+        }
     }
 
     isa_mem_base = 0x80000000;
Index: hw/ppc_prep.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ppc_prep.c,v
retrieving revision 1.51
diff -u -d -d -p -r1.51 ppc_prep.c
--- hw/ppc_prep.c	31 Oct 2007 01:54:04 -0000	1.51
+++ hw/ppc_prep.c	1 Nov 2007 19:07:06 -0000
@@ -521,7 +521,8 @@ CPUReadMemoryFunc *PPC_prep_io_read[] = 
 #define NVRAM_SIZE        0x2000
 
 /* PowerPC PREP hardware initialisation */
-static void ppc_prep_init (int ram_size, int vga_ram_size, const char *boot_device,
+static void ppc_prep_init (int ram_size, int vga_ram_size,
+                           const char *boot_device,
                            DisplayState *ds, const char **fd_filename,
                            int snapshot, const char *kernel_filename,
                            const char *kernel_cmdline,
@@ -539,7 +540,7 @@ static void ppc_prep_init (int ram_size,
     ppc_def_t *def;
     PCIBus *pci_bus;
     qemu_irq *i8259;
-    int ppc_boot_device = boot_device[0];
+    int ppc_boot_device;
 
     sysctrl = qemu_mallocz(sizeof(sysctrl_t));
     if (sysctrl == NULL)
@@ -614,6 +615,17 @@ static void ppc_prep_init (int ram_size,
         kernel_size = 0;
         initrd_base = 0;
         initrd_size = 0;
+        ppc_boot_device = '\0';
+        /* For now, OHW cannot boot from the network. */
+        for (i = 0; i < boot_device[i] != '\0'; i++) {
+            ppc_boot_device = boot_device[i];
+            if (ppc_boot_device >= 'a' && ppc_boot_device <= 'f')
+                break;
+        }
+        if (ppc_boot_device == '\0') {
+            fprintf(stderr, "No valid boot device for Mac99 machine\n");
+            exit(1);
+        }
     }
 
     isa_mem_base = 0xc0000000;

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[andrzej zaborowski]

Hi,

On 01/11/2007, J. Mayer <l_indien@magic.fr> wrote:
> On Thu, 2007-11-01 at 01:01 +0100, andrzej zaborowski wrote:
> > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > >
> > > On Wed, 2007-10-31 at 11:22 +0100, J. Mayer wrote:
> > > > On Wed, 2007-10-31 at 11:01 +0100, andrzej zaborowski wrote:
> > > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > > >
> > > > > > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > > > > >
> > > > > > > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > > > > > > > CVSROOT:      /sources/qemu
> > > > > > > > > Module name:  qemu
> > > > > > > > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > > > > > > > >
> > > > > > > > > Modified files:
> > > > > > > > >       .              : vl.c vl.h
> > > > > > > > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > > > > > > > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > > > > > > > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > > > > > > > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > > > > > > > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > > > > > > > >                        versatilepb.c
> > > > > > > > >
> > > > > > > > > Log message:
> > > > > > > > >       Set boot sequence from command line (Dan Kenigsberg).
> > > > > > > >
> > > > > > > > There have been remarks about this patch that have not been addressed
> > > > > > > > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > > > > > > > to 3 when more than 3 boot devices are possible to select (see the
> > > > > > > > BOOTCHARS definition), which clearly shows the patch is not consistent.
> > > > > > >
> > > > > > > I double-checked to make sure all remarks made on qemu-devel were
> > > > > > > addressed, but I may have missed something. It was explained that the
> > > > > > > default bios supports only three boot devices,
> > > > > >
> > > > > > Then just take a look at the function boot_device2nibble in hw/pc.c. You
> > > > > > can see 4 possibilities implemented here. And I think I've never seen a
> > > > > > PC BIOS (on real machines, I mean) that don't allow more than 4 choices
> > > > > > in last 5 years (and maybe much more...)
> > > > >
> > > > > MAX_BOOT_DEVICES doesn't limit the number of possible boot devices, it
> > > > > is only a limit for the length of the sequence given on command-line.
> > > > >
> > > > > > The second point is that, as the legacy PC-BIOS is maybe the less
> > > > > > versatile architecture that can be, putting limitations to the emulation
> > > > > > model based on this spec seems to be a nonsense in Qemu, which is
> > > > > > supposed to emulate _a lot_ of different architectures. As a matter of
> > > > > > fact, a specific implementation (ie legacy PC target) should not lead to
> > > > > > have hardcoded limits that would affect all other emulated targets.
> > > > >
> > > > > I personally wouldn't hardcode any limit but this code was submitted
> > > > > this way and doesn't limit any current functionality in any way, it
> > > > > extends it. I prefer the GNU/Hurd style code where no software limits
> > > > > are ever imposed and even the standard unix limits are undefined (e.g.
> > > > > no MAXPATHLEN), sometimes at significant cost.
> > > >
> > > > Imho, in that case, the only thing that can be check is that the given
> > > > string contains only characters that can be valid devices in Qemu. Then,
> > > > making boot_device a pointer directly assigned to optarg then check that
> > > > all chars are >= 'a' and < 'c' + MAX_DISKS || chars == 'n' would greatly
> > > > simplify the code. And this kind of check is the only valid one you can
> > > > do in the generic code.
> > >
> > > Here's a generic implementation that checks only the boot devices known
> > > to be supported, ie 'a', 'c', 'd' and 'n', thus need no change in the
> > > machine emulation code to work. When the machines will be able to check
> > > properly if the boot devices match the emulated hardware and the BIOS
> > > ABI, then it can be easily extended, changing one line, to allow boot
> > > from more devices. I think that this code should allow choosing to (try
> > > to...) boot from at least the 2 floppies and the 4 possible IDE devices.
> > > The consistency test could also be changed to add more drives if it
> > > seems to be needed.
> > > For consistency, I also made the boot_devices variable local to the main
> > > routine, as it's never used anywhere else.
> >
> > Thanks for the rework, I'm in favour of this patch. However, similar
> > to the previous approach it still restricts the "driver letters" set
> > and assumes that vl.c will be extended when some per-machine code
> > needs more letters (which is okay with me, but I had understood that
> > this was your concern). The letter check is equivalent to
> > !strchr(BOOTCHARS, *p).
>
> It restricts the letter to the ones historically allowed by Qemu, not to
> anything specific to any architecture or hw platform. What I like in my
> implementation, compared to the strchr..., is that it exactly tells the
> user which given device is incorrect.

Well, here it makes no difference, strchr tells you exactly same as much.

Instead of the check, the code could also allow everything from 'a' to
'z' and then just AND the produced 32bit bitmap with a machine defined
bitmap that would be part of QEMUMachine.

>
> > > This patch does not make the code simpler (in fact it's even more
> > > complicated as it does more generic consistency checks) but is generic
> > > and extensible, not breaking the previous patch and being consistent
> > > with the i386 machine BIOS features, as implemented now.
> > >
> > > The machine specific checks can be added later, for each target that
> > > need some. Another solution could be that every machine implements a
> > > callback that return a features bitmap, then the generic code could
> > > check if the given command line arguments (including the -boot option,
> > > but not only) are consistent with the emulated hardware platform.
> >
> > This sounds like the correct approach, although considering the way
> > error checking is (or isn't) done across qemu, it is perhaps enough
> > for machine init code to print a message and exit(1) on an
> > inconsistency.
>
> I also think this is the best way to do but it's a greater work to do
> and it needs to define well the way it has to be implemented.
>
> Here's a second pass cleanup, adding the machine dependant checks for
> the PC machine and the PowerPC ones. As one can see, the OpenHack'Ware
> firmware is able to boot from devices 'e' and 'f'. For the PowerPC
> machines, I choosed to try to boot from the first given usable device,
> some may not agree with this choice. It can be noticed that the
> available boot devices are not the same for PowerPC PreP, g3bw and mac99
> machines.
> As I don't know the features and requirements for the other
> architectures, I prefered not to add any check for those ones.

Most other machines ignore -boot and those that don't, shouldn't break
from the introduced change, so please commit it when you feel ok with
it.

Regards

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[J. Mayer]

On Sat, 2007-11-03 at 01:01 +0100, andrzej zaborowski wrote:
> Hi,
> 
> On 01/11/2007, J. Mayer <l_indien@magic.fr> wrote:
> > On Thu, 2007-11-01 at 01:01 +0100, andrzej zaborowski wrote:
> > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > >
> > > > On Wed, 2007-10-31 at 11:22 +0100, J. Mayer wrote:
> > > > > On Wed, 2007-10-31 at 11:01 +0100, andrzej zaborowski wrote:
> > > > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > > > >
> > > > > > > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On 31/10/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote:
> > > > > > > > > > CVSROOT:      /sources/qemu
> > > > > > > > > > Module name:  qemu
> > > > > > > > > > Changes by:   Andrzej Zaborowski <balrog>     07/10/31 01:54:05
> > > > > > > > > >
> > > > > > > > > > Modified files:
> > > > > > > > > >       .              : vl.c vl.h
> > > > > > > > > >       hw             : an5206.c etraxfs.c integratorcp.c mcf5208.c
> > > > > > > > > >                        mips_malta.c mips_mipssim.c mips_pica61.c
> > > > > > > > > >                        mips_r4k.c palm.c pc.c ppc405_boards.c
> > > > > > > > > >                        ppc_chrp.c ppc_oldworld.c ppc_prep.c r2d.c
> > > > > > > > > >                        realview.c shix.c spitz.c sun4m.c sun4u.c
> > > > > > > > > >                        versatilepb.c
> > > > > > > > > >
> > > > > > > > > > Log message:
> > > > > > > > > >       Set boot sequence from command line (Dan Kenigsberg).
> > > > > > > > >
> > > > > > > > > There have been remarks about this patch that have not been addressed
> > > > > > > > > (not even answered, in fact).  For example, the MAX_BOOT_DEVICES is set
> > > > > > > > > to 3 when more than 3 boot devices are possible to select (see the
> > > > > > > > > BOOTCHARS definition), which clearly shows the patch is not consistent.
> > > > > > > >
> > > > > > > > I double-checked to make sure all remarks made on qemu-devel were
> > > > > > > > addressed, but I may have missed something. It was explained that the
> > > > > > > > default bios supports only three boot devices,
> > > > > > >
> > > > > > > Then just take a look at the function boot_device2nibble in hw/pc.c. You
> > > > > > > can see 4 possibilities implemented here. And I think I've never seen a
> > > > > > > PC BIOS (on real machines, I mean) that don't allow more than 4 choices
> > > > > > > in last 5 years (and maybe much more...)
> > > > > >
> > > > > > MAX_BOOT_DEVICES doesn't limit the number of possible boot devices, it
> > > > > > is only a limit for the length of the sequence given on command-line.
> > > > > >
> > > > > > > The second point is that, as the legacy PC-BIOS is maybe the less
> > > > > > > versatile architecture that can be, putting limitations to the emulation
> > > > > > > model based on this spec seems to be a nonsense in Qemu, which is
> > > > > > > supposed to emulate _a lot_ of different architectures. As a matter of
> > > > > > > fact, a specific implementation (ie legacy PC target) should not lead to
> > > > > > > have hardcoded limits that would affect all other emulated targets.
> > > > > >
> > > > > > I personally wouldn't hardcode any limit but this code was submitted
> > > > > > this way and doesn't limit any current functionality in any way, it
> > > > > > extends it. I prefer the GNU/Hurd style code where no software limits
> > > > > > are ever imposed and even the standard unix limits are undefined (e.g.
> > > > > > no MAXPATHLEN), sometimes at significant cost.
> > > > >
> > > > > Imho, in that case, the only thing that can be check is that the given
> > > > > string contains only characters that can be valid devices in Qemu. Then,
> > > > > making boot_device a pointer directly assigned to optarg then check that
> > > > > all chars are >= 'a' and < 'c' + MAX_DISKS || chars == 'n' would greatly
> > > > > simplify the code. And this kind of check is the only valid one you can
> > > > > do in the generic code.
> > > >
> > > > Here's a generic implementation that checks only the boot devices known
> > > > to be supported, ie 'a', 'c', 'd' and 'n', thus need no change in the
> > > > machine emulation code to work. When the machines will be able to check
> > > > properly if the boot devices match the emulated hardware and the BIOS
> > > > ABI, then it can be easily extended, changing one line, to allow boot
> > > > from more devices. I think that this code should allow choosing to (try
> > > > to...) boot from at least the 2 floppies and the 4 possible IDE devices.
> > > > The consistency test could also be changed to add more drives if it
> > > > seems to be needed.
> > > > For consistency, I also made the boot_devices variable local to the main
> > > > routine, as it's never used anywhere else.
> > >
> > > Thanks for the rework, I'm in favour of this patch. However, similar
> > > to the previous approach it still restricts the "driver letters" set
> > > and assumes that vl.c will be extended when some per-machine code
> > > needs more letters (which is okay with me, but I had understood that
> > > this was your concern). The letter check is equivalent to
> > > !strchr(BOOTCHARS, *p).
> >
> > It restricts the letter to the ones historically allowed by Qemu, not to
> > anything specific to any architecture or hw platform. What I like in my
> > implementation, compared to the strchr..., is that it exactly tells the
> > user which given device is incorrect.
> 
> Well, here it makes no difference, strchr tells you exactly same as much.

Yes, you're right. Was thinking about the original strspn.

> Instead of the check, the code could also allow everything from 'a' to
> 'z' and then just AND the produced 32bit bitmap with a machine defined
> bitmap that would be part of QEMUMachine.

I guess we would better stop at 'n', because we can easily define a
semantic for devices 'c' to 'm' (ie hard disk drives in a hardware
platform specific order) but we have to define what means 'o' to 'z'.
But I agree we would better extend it now, instead of having to rework
it later...

> > > > This patch does not make the code simpler (in fact it's even more
> > > > complicated as it does more generic consistency checks) but is generic
> > > > and extensible, not breaking the previous patch and being consistent
> > > > with the i386 machine BIOS features, as implemented now.
> > > >
> > > > The machine specific checks can be added later, for each target that
> > > > need some. Another solution could be that every machine implements a
> > > > callback that return a features bitmap, then the generic code could
> > > > check if the given command line arguments (including the -boot option,
> > > > but not only) are consistent with the emulated hardware platform.
> > >
> > > This sounds like the correct approach, although considering the way
> > > error checking is (or isn't) done across qemu, it is perhaps enough
> > > for machine init code to print a message and exit(1) on an
> > > inconsistency.
> >
> > I also think this is the best way to do but it's a greater work to do
> > and it needs to define well the way it has to be implemented.
> >
> > Here's a second pass cleanup, adding the machine dependant checks for
> > the PC machine and the PowerPC ones. As one can see, the OpenHack'Ware
> > firmware is able to boot from devices 'e' and 'f'. For the PowerPC
> > machines, I choosed to try to boot from the first given usable device,
> > some may not agree with this choice. It can be noticed that the
> > available boot devices are not the same for PowerPC PreP, g3bw and mac99
> > machines.
> > As I don't know the features and requirements for the other
> > architectures, I prefered not to add any check for those ones.
> 
> Most other machines ignore -boot and those that don't, shouldn't break
> from the introduced change, so please commit it when you feel ok with
> it.

I'd like to know what are the feelings around about this patch and if
there are specific requirements and/or problems for some platforms to be
addressed before...

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[Thiemo Seufer]

J. Mayer wrote:
[snip]
> > > It restricts the letter to the ones historically allowed by Qemu, not to
> > > anything specific to any architecture or hw platform. What I like in my
> > > implementation, compared to the strchr..., is that it exactly tells the
> > > user which given device is incorrect.
> > 
> > Well, here it makes no difference, strchr tells you exactly same as much.
> 
> Yes, you're right. Was thinking about the original strspn.
> 
> > Instead of the check, the code could also allow everything from 'a' to
> > 'z' and then just AND the produced 32bit bitmap with a machine defined
> > bitmap that would be part of QEMUMachine.
> 
> I guess we would better stop at 'n', because we can easily define a
> semantic for devices 'c' to 'm' (ie hard disk drives in a hardware
> platform specific order) but we have to define what means 'o' to 'z'.
> But I agree we would better extend it now, instead of having to rework
> it later...

To select the network device to boot from would probably become a
'n' 'o' 'p' 'q' series.

[snip]
> > > Here's a second pass cleanup, adding the machine dependant checks for
> > > the PC machine and the PowerPC ones. As one can see, the OpenHack'Ware
> > > firmware is able to boot from devices 'e' and 'f'. For the PowerPC
> > > machines, I choosed to try to boot from the first given usable device,
> > > some may not agree with this choice. It can be noticed that the
> > > available boot devices are not the same for PowerPC PreP, g3bw and mac99
> > > machines.
> > > As I don't know the features and requirements for the other
> > > architectures, I prefered not to add any check for those ones.
> > 
> > Most other machines ignore -boot and those that don't, shouldn't break
> > from the introduced change, so please commit it when you feel ok with
> > it.
> 
> I'd like to know what are the feelings around about this patch and if
> there are specific requirements and/or problems for some platforms to be
> addressed before...

I think the proposed scheme (and the implementation) is flexible enough
to accomodate all relevant platforms.


Thiemo

Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte...

[J. Mayer]

On Sat, 2007-11-03 at 01:18 +0000, Thiemo Seufer wrote:
> J. Mayer wrote:
> [snip]
> > > > It restricts the letter to the ones historically allowed by Qemu, not to
> > > > anything specific to any architecture or hw platform. What I like in my
> > > > implementation, compared to the strchr..., is that it exactly tells the
> > > > user which given device is incorrect.
> > > 
> > > Well, here it makes no difference, strchr tells you exactly same as much.
> > 
> > Yes, you're right. Was thinking about the original strspn.
> > 
> > > Instead of the check, the code could also allow everything from 'a' to
> > > 'z' and then just AND the produced 32bit bitmap with a machine defined
> > > bitmap that would be part of QEMUMachine.
> > 
> > I guess we would better stop at 'n', because we can easily define a
> > semantic for devices 'c' to 'm' (ie hard disk drives in a hardware
> > platform specific order) but we have to define what means 'o' to 'z'.
> > But I agree we would better extend it now, instead of having to rework
> > it later...
> 
> To select the network device to boot from would probably become a
> 'n' 'o' 'p' 'q' series.

Seems OK. Can we say 'c' to 'm' is sufficient to address all disk drive
cases or some more possibilities are needed for SCSI devices, or MTD
devices boot ? Maybe 'u'... for USB, as most available machines know how
to boot on USB, in the real world ? Or may we just consider 'c' to 'm'
are sufficient and it's up to the machine to determine the real meaning
of the letters, according to its implementation ? In this case, I think
we would better provide a per-machine callback to handle the '?' case,
printing an help on available boot devices letters and their meaning...

> [snip]
> > > > Here's a second pass cleanup, adding the machine dependant checks for
> > > > the PC machine and the PowerPC ones. As one can see, the OpenHack'Ware
> > > > firmware is able to boot from devices 'e' and 'f'. For the PowerPC
> > > > machines, I choosed to try to boot from the first given usable device,
> > > > some may not agree with this choice. It can be noticed that the
> > > > available boot devices are not the same for PowerPC PreP, g3bw and mac99
> > > > machines.
> > > > As I don't know the features and requirements for the other
> > > > architectures, I prefered not to add any check for those ones.
> > > 
> > > Most other machines ignore -boot and those that don't, shouldn't break
> > > from the introduced change, so please commit it when you feel ok with
> > > it.
> > 
> > I'd like to know what are the feelings around about this patch and if
> > there are specific requirements and/or problems for some platforms to be
> > addressed before...
> 
> I think the proposed scheme (and the implementation) is flexible enough
> to accomodate all relevant platforms.

I think there are still a few problems here:
1/ it would not be easy to add a way to use the disk syntax as proposed
here, but this could be useful. Another option could be added for this;
or it could be part of the '-disk' syntax.
2/ doing a generic check in vl.c using the machine features would need a
great rework. We then would have to first parse all options, then
retrieve the machine features, then check all options according to those
features. But this can be designed and done later
3/ it would be a great idea to provide a way to boot without any bloc
device available. Embedded devices often just have a flash device or a
ROM, then the checks done in vl.c to be sure there is a least one device
present should be machine specific, imho. Then having an empty
boot_devices string may not be a mistake.

The two last point can sure be addressed separatly. For the first one,
it seems to me that defining the way it has to be would be great: if it
needs the -boot option to be extended or redesigned, I think the best
would be to do it in the same patch...

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] multiple boot devices

[J. Mayer]

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


On Sat, 2007-11-03 at 01:18 +0000, Thiemo Seufer wrote:
> J. Mayer wrote:
> [snip]
> > > > It restricts the letter to the ones historically allowed by Qemu, not to
> > > > anything specific to any architecture or hw platform. What I like in my
> > > > implementation, compared to the strchr..., is that it exactly tells the
> > > > user which given device is incorrect.
> > > 
> > > Well, here it makes no difference, strchr tells you exactly same as much.
> > 
> > Yes, you're right. Was thinking about the original strspn.
> > 
> > > Instead of the check, the code could also allow everything from 'a' to
> > > 'z' and then just AND the produced 32bit bitmap with a machine defined
> > > bitmap that would be part of QEMUMachine.
> > 
> > I guess we would better stop at 'n', because we can easily define a
> > semantic for devices 'c' to 'm' (ie hard disk drives in a hardware
> > platform specific order) but we have to define what means 'o' to 'z'.
> > But I agree we would better extend it now, instead of having to rework
> > it later...
> 
> To select the network device to boot from would probably become a
> 'n' 'o' 'p' 'q' series.
> 
> [snip]
> > > > Here's a second pass cleanup, adding the machine dependant checks for
> > > > the PC machine and the PowerPC ones. As one can see, the OpenHack'Ware
> > > > firmware is able to boot from devices 'e' and 'f'. For the PowerPC
> > > > machines, I choosed to try to boot from the first given usable device,
> > > > some may not agree with this choice. It can be noticed that the
> > > > available boot devices are not the same for PowerPC PreP, g3bw and mac99
> > > > machines.
> > > > As I don't know the features and requirements for the other
> > > > architectures, I prefered not to add any check for those ones.
> > > 
> > > Most other machines ignore -boot and those that don't, shouldn't break
> > > from the introduced change, so please commit it when you feel ok with
> > > it.
> > 
> > I'd like to know what are the feelings around about this patch and if
> > there are specific requirements and/or problems for some platforms to be
> > addressed before...
> 
> I think the proposed scheme (and the implementation) is flexible enough
> to accomodate all relevant platforms.

Here's an updated patch that address the remark about network boot
devices.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: boot_devices.diff --]
[-- Type: text/x-patch, Size: 12365 bytes --]

Index: vl.c
===================================================================
RCS file: /sources/qemu/qemu/vl.c,v
retrieving revision 1.353
diff -u -d -d -p -r1.353 vl.c
--- vl.c	31 Oct 2007 01:54:03 -0000	1.353
+++ vl.c	5 Nov 2007 12:07:05 -0000
@@ -162,12 +162,6 @@ static DisplayState display_state;
 int nographic;
 const char* keyboard_layout = NULL;
 int64_t ticks_per_sec;
-#if defined(TARGET_I386)
-#define MAX_BOOT_DEVICES 3
-#else
-#define MAX_BOOT_DEVICES 1
-#endif
-static char boot_device[MAX_BOOT_DEVICES + 1];
 int ram_size;
 int pit_min_timer_count = 0;
 int nb_nics;
@@ -7556,14 +7552,16 @@ int main(int argc, char **argv)
     int use_gdbstub;
     const char *gdbstub_port;
 #endif
+    uint32_t boot_devices_bitmap = 0;
     int i, cdrom_index, pflash_index;
-    int snapshot, linux_boot;
+    int snapshot, linux_boot, net_boot;
     const char *initrd_filename;
     const char *hd_filename[MAX_DISKS], *fd_filename[MAX_FD];
     const char *pflash_filename[MAX_PFLASH];
     const char *sd_filename;
     const char *mtd_filename;
     const char *kernel_filename, *kernel_cmdline;
+    const char *boot_devices = "";
     DisplayState *ds = &display_state;
     int cyls, heads, secs, translation;
     char net_clients[MAX_NET_CLIENTS][256];
@@ -7815,20 +7813,34 @@ int main(int argc, char **argv)
                 }
                 break;
             case QEMU_OPTION_boot:
-                if (strlen(optarg) > MAX_BOOT_DEVICES) {
-                    fprintf(stderr, "qemu: too many boot devices\n");
-                    exit(1);
-                }
-                strncpy(boot_device, optarg, MAX_BOOT_DEVICES);
-#if defined(TARGET_SPARC) || defined(TARGET_I386)
-#define BOOTCHARS "acdn"
-#else
-#define BOOTCHARS "acd"
-#endif
-                if (strlen(boot_device) != strspn(boot_device, BOOTCHARS)) {
-                    fprintf(stderr, "qemu: invalid boot device "
-                                    "sequence '%s'\n", boot_device);
-                    exit(1);
+                boot_devices = optarg;
+                /* We just do some generic consistency checks */
+                {
+                    /* Could easily be extended to 64 devices if needed */
+                    const unsigned char *p;
+                    
+                    boot_devices_bitmap = 0;
+                    for (p = boot_devices; *p != '\0'; p++) {
+                        /* Allowed boot devices are:
+                         * a b     : floppy disk drives
+                         * c ... f : IDE disk drives
+                         * g ... m : machine implementation dependant drives
+                         * n ... p : network devices
+                         * It's up to each machine implementation to check
+                         * if the given boot devices match the actual hardware
+                         * implementation and firmware features.
+                         */
+                        if (*p < 'a' || *p > 'q') {
+                            fprintf(stderr, "Invalid boot device '%c'\n", *p);
+                            exit(1);
+                        }
+                        if (boot_devices_bitmap & (1 << (*p - 'a'))) {
+                            fprintf(stderr,
+                                    "Boot device '%c' was given twice\n",*p);
+                            exit(1);
+                        }
+                        boot_devices_bitmap |= 1 << (*p - 'a');
+                    }
                 }
                 break;
             case QEMU_OPTION_fda:
@@ -8176,23 +8188,23 @@ int main(int argc, char **argv)
         kqemu_allowed = 0;
 #endif
     linux_boot = (kernel_filename != NULL);
-
-    if (!linux_boot &&
-        (!strchr(boot_device, 'n')) &&
+    net_boot = (boot_devices_bitmap >> ('n' - 'a')) && 0xF;
+    
+    /* XXX: this should not be: some embedded targets just have flash */
+    if (!linux_boot && net_boot == 0 &&
         hd_filename[0] == '\0' &&
         (cdrom_index >= 0 && hd_filename[cdrom_index] == '\0') &&
         fd_filename[0] == '\0')
         help(1);
 
     /* boot to floppy or the default cd if no hard disk defined yet */
-    if (!boot_device[0]) {
+    if (!boot_devices[0]) {
         if (hd_filename[0] != '\0')
-            boot_device[0] = 'c';
+            boot_devices = "c";
         else if (fd_filename[0] != '\0')
-            boot_device[0] = 'a';
+            boot_devices = "a";
         else
-            boot_device[0] = 'd';
-        boot_device[1] = 0;
+            boot_devices = "d";
     }
     setvbuf(stdout, NULL, _IOLBF, 0);
 
@@ -8232,20 +8244,28 @@ int main(int argc, char **argv)
     }
 
 #ifdef TARGET_I386
-    if (strchr(boot_device, 'n')) {
-	for (i = 0; i < nb_nics; i++) {
+    /* XXX: this should be moved in the PC machine instanciation code */
+    if (net_boot != 0) {
+        int netroms = 0;
+	for (i = 0; i < nb_nics && i < 4; i++) {
 	    const char *model = nd_table[i].model;
 	    char buf[1024];
-	    if (model == NULL)
-		model = "ne2k_pci";
-	    snprintf(buf, sizeof(buf), "%s/pxe-%s.bin", bios_dir, model);
-	    if (get_image_size(buf) > 0) {
-		option_rom[nb_option_roms] = strdup(buf);
-		nb_option_roms++;
-		break;
-	    }
+            if (net_boot & (1 << i)) {
+                if (model == NULL)
+                    model = "ne2k_pci";
+                snprintf(buf, sizeof(buf), "%s/pxe-%s.bin", bios_dir, model);
+                if (get_image_size(buf) > 0) {
+                    if (nb_option_roms >= MAX_OPTION_ROMS) {
+                        fprintf(stderr, "Too many option ROMs\n");
+                        exit(1);
+                    }
+                    option_rom[nb_option_roms] = strdup(buf);
+                    nb_option_roms++;
+                    netroms++;
+                }
+            }
 	}
-	if (i == nb_nics) {
+	if (netroms == 0) {
 	    fprintf(stderr, "No valid PXE rom found for network device\n");
 	    exit(1);
 	}
@@ -8425,7 +8445,7 @@ int main(int argc, char **argv)
         }
     }
 
-    machine->init(ram_size, vga_ram_size, boot_device,
+    machine->init(ram_size, vga_ram_size, boot_devices,
                   ds, fd_filename, snapshot,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
Index: hw/pc.c
===================================================================
RCS file: /sources/qemu/qemu/hw/pc.c,v
retrieving revision 1.88
diff -u -d -d -p -r1.88 pc.c
--- hw/pc.c	31 Oct 2007 01:54:04 -0000	1.88
+++ hw/pc.c	5 Nov 2007 12:07:06 -0000
@@ -173,6 +173,7 @@ static int boot_device2nibble(char boot_
 static void cmos_init(int ram_size, const char *boot_device, BlockDriverState **hd_table)
 {
     RTCState *s = rtc_state;
+    int nbds, bds[3] = { 0, };
     int val;
     int fd0, fd1, nb;
     int i;
@@ -202,11 +203,22 @@ static void cmos_init(int ram_size, cons
     rtc_set_memory(s, 0x35, val >> 8);
 
     /* set boot devices, and disable floppy signature check if requested */
-    rtc_set_memory(s, 0x3d,
-            boot_device2nibble(boot_device[1]) << 4 |
-            boot_device2nibble(boot_device[0]) );
-    rtc_set_memory(s, 0x38,
-            boot_device2nibble(boot_device[2]) << 4 | (fd_bootchk ?  0x0 : 0x1));
+#define PC_MAX_BOOT_DEVICES 3
+    nbds = strlen(boot_device);
+    if (nbds > PC_MAX_BOOT_DEVICES) {
+        fprintf(stderr, "Too many boot devices for PC\n");
+        exit(1);
+    }
+    for (i = 0; i < nbds; i++) {
+        bds[i] = boot_device2nibble(boot_device[i]);
+        if (bds[i] == 0) {
+            fprintf(stderr, "Invalid boot device for PC: '%c'\n",
+                    boot_device[i]);
+            exit(1);
+        }
+    }
+    rtc_set_memory(s, 0x3d, (bds[1] << 4) | bds[0]);
+    rtc_set_memory(s, 0x38, (bds[2] << 4) | (fd_bootchk ?  0x0 : 0x1));
 
     /* floppy type */
 
Index: hw/ppc_chrp.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ppc_chrp.c,v
retrieving revision 1.48
diff -u -d -d -p -r1.48 ppc_chrp.c
--- hw/ppc_chrp.c	4 Nov 2007 01:16:04 -0000	1.48
+++ hw/ppc_chrp.c	5 Nov 2007 12:07:06 -0000
@@ -66,7 +66,7 @@ static void ppc_core99_init (int ram_siz
     qemu_irq *dummy_irq;
     int pic_mem_index, dbdma_mem_index, cuda_mem_index;
     int ide_mem_index[2];
-    int ppc_boot_device = boot_device[0];
+    int ppc_boot_device;
 
     linux_boot = (kernel_filename != NULL);
 
@@ -178,6 +175,19 @@ static void ppc_core99_init (int ram_siz
         kernel_size = 0;
         initrd_base = 0;
         initrd_size = 0;
+        ppc_boot_device = '\0';
+        /* We consider that NewWorld PowerMac never have any floppy drive
+         * For now, OHW cannot boot from the network.
+         */
+        for (i = 0; i < boot_device[i] != '\0'; i++) {
+            ppc_boot_device = boot_device[i];
+            if (ppc_boot_device >= 'c' && ppc_boot_device <= 'f')
+                break;
+        }
+        if (ppc_boot_device == '\0') {
+            fprintf(stderr, "No valid boot device for Mac99 machine\n");
+            exit(1);
+        }
     }
 
     isa_mem_base = 0x80000000;
Index: hw/ppc_oldworld.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ppc_oldworld.c,v
retrieving revision 1.4
diff -u -d -d -p -r1.4 ppc_oldworld.c
--- hw/ppc_oldworld.c	4 Nov 2007 01:16:04 -0000	1.4
+++ hw/ppc_oldworld.c	5 Nov 2007 12:07:06 -0000
@@ -114,7 +113,7 @@ static void ppc_heathrow_init (int ram_s
     int vga_bios_size, bios_size;
     qemu_irq *dummy_irq;
     int pic_mem_index, nvram_mem_index, dbdma_mem_index, cuda_mem_index;
-    int ppc_boot_device = boot_device[0];
+    int ppc_boot_device;
 
     linux_boot = (kernel_filename != NULL);
 
@@ -215,6 +214,25 @@ static void ppc_heathrow_init (int ram_s
         kernel_size = 0;
         initrd_base = 0;
         initrd_size = 0;
+        ppc_boot_device = '\0';
+        for (i = 0; i < boot_device[i] != '\0'; i++) {
+            ppc_boot_device = boot_device[i];
+            /* TOFIX: for now, the second IDE channel is not properly
+             *        emulated. The Mac floppy disk are not emulated.
+             *        For now, OHW cannot boot from the network.
+             */
+#if 0
+            if (ppc_boot_device >= 'a' && ppc_boot_device <= 'f')
+                break;
+#else
+            if (ppc_boot_device >= 'c' && ppc_boot_device <= 'd')
+                break;
+#endif
+        }
+        if (ppc_boot_device == '\0') {
+            fprintf(stderr, "No valid boot device for Mac99 machine\n");
+            exit(1);
+        }
     }
 
     isa_mem_base = 0x80000000;
Index: hw/ppc_prep.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ppc_prep.c,v
retrieving revision 1.51
diff -u -d -d -p -r1.51 ppc_prep.c
--- hw/ppc_prep.c	31 Oct 2007 01:54:04 -0000	1.51
+++ hw/ppc_prep.c	5 Nov 2007 12:07:06 -0000
@@ -521,7 +521,8 @@ CPUReadMemoryFunc *PPC_prep_io_read[] = 
 #define NVRAM_SIZE        0x2000
 
 /* PowerPC PREP hardware initialisation */
-static void ppc_prep_init (int ram_size, int vga_ram_size, const char *boot_device,
+static void ppc_prep_init (int ram_size, int vga_ram_size,
+                           const char *boot_device,
                            DisplayState *ds, const char **fd_filename,
                            int snapshot, const char *kernel_filename,
                            const char *kernel_cmdline,
@@ -539,7 +540,7 @@ static void ppc_prep_init (int ram_size,
     ppc_def_t *def;
     PCIBus *pci_bus;
     qemu_irq *i8259;
-    int ppc_boot_device = boot_device[0];
+    int ppc_boot_device;
 
     sysctrl = qemu_mallocz(sizeof(sysctrl_t));
     if (sysctrl == NULL)
@@ -614,6 +615,17 @@ static void ppc_prep_init (int ram_size,
         kernel_size = 0;
         initrd_base = 0;
         initrd_size = 0;
+        ppc_boot_device = '\0';
+        /* For now, OHW cannot boot from the network. */
+        for (i = 0; i < boot_device[i] != '\0'; i++) {
+            ppc_boot_device = boot_device[i];
+            if (ppc_boot_device >= 'a' && ppc_boot_device <= 'f')
+                break;
+        }
+        if (ppc_boot_device == '\0') {
+            fprintf(stderr, "No valid boot device for Mac99 machine\n");
+            exit(1);
+        }
     }
 
     isa_mem_base = 0xc0000000;