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

["J. Mayer" <l_indien@magic.fr>]

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