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

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

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