From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Io6x9-0002kr-E6 for qemu-devel@nongnu.org; Fri, 02 Nov 2007 20:33:31 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Io6x8-0002ju-NZ for qemu-devel@nongnu.org; Fri, 02 Nov 2007 20:33:31 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Io6x8-0002jl-IK for qemu-devel@nongnu.org; Fri, 02 Nov 2007 20:33:30 -0400 Received: from bangui.magic.fr ([195.154.194.245]) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Io6ly-0008Lt-Ak for qemu-devel@nongnu.org; Fri, 02 Nov 2007 20:21:58 -0400 Subject: Re: [Qemu-devel] qemu vl.c vl.h hw/an5206.c hw/etraxfs.c hw/inte... From: "J. Mayer" In-Reply-To: References: <1193797260.16781.380.camel@rapid> <1193805724.16781.406.camel@rapid> <1193826145.16781.424.camel@rapid> <1193870969.16781.461.camel@rapid> <1193944366.16781.473.camel@rapid> Content-Type: text/plain Date: Sat, 03 Nov 2007 01:21:55 +0100 Message-Id: <1194049315.16781.537.camel@rapid> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: andrzej zaborowski Cc: qemu-devel@nongnu.org On Sat, 2007-11-03 at 01:01 +0100, andrzej zaborowski wrote: > Hi, > > On 01/11/2007, J. Mayer wrote: > > On Thu, 2007-11-01 at 01:01 +0100, andrzej zaborowski wrote: > > > On 31/10/2007, J. Mayer 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 wrote: > > > > > > > > > > > > > > On Wed, 2007-10-31 at 03:35 +0100, andrzej zaborowski wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On 31/10/2007, J. Mayer wrote: > > > > > > > > > > > > > > > > > > On Wed, 2007-10-31 at 01:54 +0000, Andrzej Zaborowski wrote: > > > > > > > > > > CVSROOT: /sources/qemu > > > > > > > > > > Module name: qemu > > > > > > > > > > Changes by: Andrzej Zaborowski 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 Never organized