On Thu, 2007-11-01 at 01:01 +0100, andrzej zaborowski wrote:
> Hi,
>
> 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.
> > 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 <[EMAIL PROTECTED]>
Never organized
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;
