On Sat, 2007-11-03 at 01:01 +0100, andrzej zaborowski wrote: > Hi, > > On 01/11/2007, J. Mayer <[EMAIL PROTECTED]> wrote: > > On Thu, 2007-11-01 at 01:01 +0100, andrzej zaborowski wrote: > > > On 31/10/2007, J. Mayer <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> wrote: > > > > > > > > > > > > > > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On 31/10/2007, J. Mayer <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> Never organized
