| Message ID | 1260189773-20728-6-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07.12.2009, at 13:42, Gerd Hoffmann wrote: > Qemu creates a default serial line for you in case you didn't specify > one on the command line. Right now this is tied to the '-serial > <chardev>' command line switch, which in turn causes trouble if you are > creating your serial line via '-device isa-serial,<props>'. > > This patch adds a variable default_serial which says whenever a default > serial line should be added. It is enabled by default. It is cleared > when qemu finds '-serial' or '-device isa-serial' on the command line. > > Part of the patch is some infrastructure for the '-device $driver' > checking (default_driver_check function) which will also be used by the > other patches of this series. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > vl.c | 122 +++++++++++++++++++++++++++++++++++++++++++---------------------- > 1 files changed, 81 insertions(+), 41 deletions(-) > > diff --git a/vl.c b/vl.c > index 24d5d92..eca4eee 100644 > --- a/vl.c > +++ b/vl.c > @@ -271,6 +271,30 @@ uint8_t qemu_uuid[16]; > static QEMUBootSetHandler *boot_set_handler; > static void *boot_set_opaque; > > +static int default_serial = 1; Default variables belong in the machine description IMHO. On S390, we can't create serial, parallel or vga (or any MMIO/PIO requiring) devices. Alex
On 07.12.2009, at 13:42, Gerd Hoffmann wrote: > Qemu creates a default serial line for you in case you didn't specify > one on the command line. Right now this is tied to the '-serial > <chardev>' command line switch, which in turn causes trouble if you are > creating your serial line via '-device isa-serial,<props>'. > > This patch adds a variable default_serial which says whenever a default > serial line should be added. It is enabled by default. It is cleared > when qemu finds '-serial' or '-device isa-serial' on the command line. > > Part of the patch is some infrastructure for the '-device $driver' > checking (default_driver_check function) which will also be used by the > other patches of this series. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > vl.c | 122 +++++++++++++++++++++++++++++++++++++++++++---------------------- > 1 files changed, 81 insertions(+), 41 deletions(-) > > diff --git a/vl.c b/vl.c > index 24d5d92..eca4eee 100644 > --- a/vl.c > +++ b/vl.c > @@ -271,6 +271,30 @@ uint8_t qemu_uuid[16]; > static QEMUBootSetHandler *boot_set_handler; > static void *boot_set_opaque; > > +static int default_serial = 1; > + > +static struct { > + const char *driver; > + int *flag; > +} default_list[] = { > + { .driver = "isa-serial", .flag = &default_serial }, What is the "isa-serial" driver supposed to say here? Is that the qdev driver name? If so, that's just plain wrong. PPC for example uses the ESCC chip to export its serial port. Alex
On 12/07/09 13:52, Alexander Graf wrote: >> diff --git a/vl.c b/vl.c index 24d5d92..eca4eee 100644 --- a/vl.c >> +++ b/vl.c @@ -271,6 +271,30 @@ uint8_t qemu_uuid[16]; static >> QEMUBootSetHandler *boot_set_handler; static void >> *boot_set_opaque; >> >> +static int default_serial = 1; > > Default variables belong in the machine description IMHO. On S390, we > can't create serial, parallel or vga (or any MMIO/PIO requiring) > devices. See last patch of this series. Moving that into the machine description is certainly worth looking at. For now I'm happy that the #ifdef S390 code for the default virtio console is down to just two lines. cheers Gerd
On 12/07/09 13:54, Alexander Graf wrote: > > On 07.12.2009, at 13:42, Gerd Hoffmann wrote: > >> Qemu creates a default serial line for you in case you didn't specify >> one on the command line. Right now this is tied to the '-serial >> <chardev>' command line switch, which in turn causes trouble if you are >> creating your serial line via '-device isa-serial,<props>'. >> +static int default_serial = 1; >> + >> +static struct { >> + const char *driver; >> + int *flag; >> +} default_list[] = { >> + { .driver = "isa-serial", .flag =&default_serial }, > > What is the "isa-serial" driver supposed to say here? Yes. > Is that the qdev driver name? If so, that's just plain wrong. PPC for example uses the ESCC chip to export its serial port. Just add the driver name to the list then, so '-device ESCC,...' (or however it is named in qdev) will stop conflicting with the automagically created -serial port too. cheers, Gerd
On 07.12.2009, at 14:27, Gerd Hoffmann wrote: > On 12/07/09 13:52, Alexander Graf wrote: >>> diff --git a/vl.c b/vl.c index 24d5d92..eca4eee 100644 --- a/vl.c >>> +++ b/vl.c @@ -271,6 +271,30 @@ uint8_t qemu_uuid[16]; static >>> QEMUBootSetHandler *boot_set_handler; static void >>> *boot_set_opaque; >>> >>> +static int default_serial = 1; >> >> Default variables belong in the machine description IMHO. On S390, we >> can't create serial, parallel or vga (or any MMIO/PIO requiring) >> devices. > > See last patch of this series. Moving that into the machine description is certainly worth looking at. For now I'm happy that the #ifdef S390 code for the default virtio console is down to just two lines. Would it be that hard to move the list of default devices to the machine description? You could probably still keep the default_xxx variables in vl.c, but access them from e.g. pc.c. Alex
On 12/07/09 15:07, Alexander Graf wrote: > > On 07.12.2009, at 14:27, Gerd Hoffmann wrote: > >>> Default variables belong in the machine description IMHO. On >>> S390, we can't create serial, parallel or vga (or any MMIO/PIO >>> requiring) devices. >> >> See last patch of this series. Moving that into the machine >> description is certainly worth looking at. For now I'm happy that >> the #ifdef S390 code for the default virtio console is down to just >> two lines. > > Would it be that hard to move the list of default devices to the > machine description? You could probably still keep the default_xxx > variables in vl.c, but access them from e.g. pc.c. Hmm? access from pc.c? What exactly do you have in mind? I think we could add flags to QEMUMachine to disable default devices not supported anyway, i.e. add no_parallel variable, then in vl.c do if (machine->no_parallel) default_parallel = 0; So qemu would stop creating a useless parallel0 chardev which isn't used because the machine in question can't support parallel ports anyway. cheers, Gerd
On 07.12.2009, at 15:39, Gerd Hoffmann wrote: > On 12/07/09 15:07, Alexander Graf wrote: >> >> On 07.12.2009, at 14:27, Gerd Hoffmann wrote: >> >>>> Default variables belong in the machine description IMHO. On >>>> S390, we can't create serial, parallel or vga (or any MMIO/PIO >>>> requiring) devices. >>> >>> See last patch of this series. Moving that into the machine >>> description is certainly worth looking at. For now I'm happy that >>> the #ifdef S390 code for the default virtio console is down to just >>> two lines. >> >> Would it be that hard to move the list of default devices to the >> machine description? You could probably still keep the default_xxx >> variables in vl.c, but access them from e.g. pc.c. > > > Hmm? access from pc.c? What exactly do you have in mind? > > > I think we could add flags to QEMUMachine to disable default devices not supported anyway, i.e. add no_parallel variable, then in vl.c do > > if (machine->no_parallel) > default_parallel = 0; > > So qemu would stop creating a useless parallel0 chardev which isn't used because the machine in question can't support parallel ports anyway. Well I was basically thinking of moving default_list inside the machine description. That way you can easily choose what default devices there are available. Then make default_serial and friends visible to pc.c and just put the same code you have now in there. For other machines the default device list surely looks different. Alex
On 12/07/09 16:12, Alexander Graf wrote: > Well I was basically thinking of moving default_list inside the > machine description. That way you can easily choose what default > devices there are available. Doesn't make sense. The list is not about *creating* default devices, but about *not* creating them. > Then make default_serial and friends visible to pc.c and just put the > same code you have now in there. For other machines the default > device list surely looks different. ppc, sparc and x86 serial devices can happily live together in this list without causing any harm. cheers, Gerd
On 07.12.2009, at 17:05, Gerd Hoffmann wrote: > On 12/07/09 16:12, Alexander Graf wrote: >> Well I was basically thinking of moving default_list inside the >> machine description. That way you can easily choose what default >> devices there are available. > > Doesn't make sense. The list is not about *creating* default devices, but about *not* creating them. Why do we need a negative list? Wouldn't it be a lot more useful to have positive lists? Maybe I'm just missing the whole point of your patchset though. Alex
Hi, >> Doesn't make sense. The list is not about *creating* default devices, but about *not* creating them. > > Why do we need a negative list? Wouldn't it be a lot more useful to have positive lists? Maybe I'm just missing the whole point of your patchset though. Probably the latter ;) The situation we have right is (with the serial lines as example, the same applies to some other devices as well): (1) If you start qemu without any arguments, it will automagically create a chardev for the serial line (serial_hds[]), which then will be used by machine->init() to create the (board-specific) serial lines. (2) If you start qemu with -serial <something> the specified chardev will be created and assigned to serial_hds[], which again will be used by machine->init() ... Worked fine before qdev appeared. Now we also can create serial lines like this: (3) Start qemu with '-device isa-serial,...'. Problem is this clashes with (1) and qemu will attempt to create the serial port twice. So this patchset introduces default_serial (+friends) to fix this issue. The new workflow is: * default_serial defaults to enabled, when qemu finds it still enabled after processing all command line the automagic default serial line is actually created. * If qemu finds '-serial <something>' it clears default_serial. * If qemu finds '-device isa-serial,...' it clears default_serial. - this fixes the conflict mentioned above. - other serial drivers can be added to the list if needed to solve the same conflict for them. * If qemu finds '-nodefaults' it clears default_serial (and the others). - new feature, figured this could be useful. What we could do on top of the above is: * If the board has the machine->no_serial flag set, clear default_serial. s390 could set that flag then and qemu would stop creating a chardev for a serial line nobody will ever use because s390 has no serial ports. /me hopes the whole picture is more clear now. cheers, Gerd
diff --git a/vl.c b/vl.c index 24d5d92..eca4eee 100644 --- a/vl.c +++ b/vl.c @@ -271,6 +271,30 @@ uint8_t qemu_uuid[16]; static QEMUBootSetHandler *boot_set_handler; static void *boot_set_opaque; +static int default_serial = 1; + +static struct { + const char *driver; + int *flag; +} default_list[] = { + { .driver = "isa-serial", .flag = &default_serial }, +}; + +static int default_driver_check(QemuOpts *opts, void *opaque) +{ + const char *driver = qemu_opt_get(opts, "driver"); + int i; + + if (!driver) + return 0; + for (i = 0; i < ARRAY_SIZE(default_list); i++) { + if (strcmp(default_list[i].driver, driver) != 0) + continue; + *(default_list[i].flag) = 0; + } + return 0; +} + /***********************************************************/ /* x86 ISA bus support */ @@ -4590,6 +4614,7 @@ struct device_config { enum { DEV_USB, /* -usbdevice */ DEV_BT, /* -bt */ + DEV_SERIAL, /* -serial */ } type; const char *cmdline; QTAILQ_ENTRY(device_config) next; @@ -4621,6 +4646,50 @@ static int foreach_device_config(int type, int (*func)(const char *cmdline)) return 0; } +static void serial_monitor_mux(const char *monitor_devices[]) +{ + struct device_config *serial; + const char *devname; + + if (strcmp(monitor_devices[0],"stdio") != 0) + return; + QTAILQ_FOREACH(serial, &device_configs, next) { + if (serial->type != DEV_SERIAL) + continue; + devname = serial->cmdline; + if (devname && !strcmp(devname,"mon:stdio")) { + monitor_devices[0] = NULL; + break; + } else if (devname && !strcmp(devname,"stdio")) { + monitor_devices[0] = NULL; + serial->cmdline = "mon:stdio"; + break; + } + } +} + +static int serial_parse(const char *devname) +{ + static int index = 0; + char label[32]; + + if (strcmp(devname, "none") == 0) + return 0; + if (index == MAX_SERIAL_PORTS) { + fprintf(stderr, "qemu: too many serial ports\n"); + exit(1); + } + snprintf(label, sizeof(label), "serial%d", index); + serial_hds[index] = qemu_chr_open(label, devname, NULL); + if (!serial_hds[index]) { + fprintf(stderr, "qemu: could not open serial device '%s': %s\n", + devname, strerror(errno)); + return -1; + } + index++; + return 0; +} + int main(int argc, char **argv, char **envp) { const char *gdbstub_dev = NULL; @@ -4639,8 +4708,6 @@ int main(int argc, char **argv, char **envp) CharDriverState *monitor_hds[MAX_MONITOR_DEVICES]; const char *monitor_devices[MAX_MONITOR_DEVICES]; int monitor_device_index; - const char *serial_devices[MAX_SERIAL_PORTS]; - int serial_device_index; const char *parallel_devices[MAX_PARALLEL_PORTS]; int parallel_device_index; const char *virtio_consoles[MAX_VIRTIO_CONSOLES]; @@ -4710,11 +4777,6 @@ int main(int argc, char **argv, char **envp) cyls = heads = secs = 0; translation = BIOS_ATA_TRANSLATION_AUTO; - serial_devices[0] = "vc:80Cx24C"; - for(i = 1; i < MAX_SERIAL_PORTS; i++) - serial_devices[i] = NULL; - serial_device_index = 0; - parallel_devices[0] = "vc:80Cx24C"; for(i = 1; i < MAX_PARALLEL_PORTS; i++) parallel_devices[i] = NULL; @@ -5161,12 +5223,8 @@ int main(int argc, char **argv, char **envp) } break; case QEMU_OPTION_serial: - if (serial_device_index >= MAX_SERIAL_PORTS) { - fprintf(stderr, "qemu: too many serial ports\n"); - exit(1); - } - serial_devices[serial_device_index] = optarg; - serial_device_index++; + add_device_config(DEV_SERIAL, optarg); + default_serial = 0; break; case QEMU_OPTION_watchdog: if (watchdog) { @@ -5467,14 +5525,19 @@ int main(int argc, char **argv, char **envp) exit(1); } + qemu_opts_foreach(&qemu_device_opts, default_driver_check, NULL, 0); + if (display_type == DT_NOGRAPHIC) { - if (serial_device_index == 0) - serial_devices[0] = "stdio"; + if (default_serial) + add_device_config(DEV_SERIAL, "stdio"); if (parallel_device_index == 0) parallel_devices[0] = "null"; if (strncmp(monitor_devices[0], "vc", 2) == 0) { monitor_devices[0] = "stdio"; } + } else { + if (default_serial) + add_device_config(DEV_SERIAL, "vc:80Cx24C"); } #ifndef _WIN32 @@ -5623,19 +5686,7 @@ int main(int argc, char **argv, char **envp) ram_load, NULL); /* Maintain compatibility with multiple stdio monitors */ - if (!strcmp(monitor_devices[0],"stdio")) { - for (i = 0; i < MAX_SERIAL_PORTS; i++) { - const char *devname = serial_devices[i]; - if (devname && !strcmp(devname,"mon:stdio")) { - monitor_devices[0] = NULL; - break; - } else if (devname && !strcmp(devname,"stdio")) { - monitor_devices[0] = NULL; - serial_devices[i] = "mon:stdio"; - break; - } - } - } + serial_monitor_mux(monitor_devices); if (nb_numa_nodes > 0) { int i; @@ -5697,19 +5748,8 @@ int main(int argc, char **argv, char **envp) } } - for(i = 0; i < MAX_SERIAL_PORTS; i++) { - const char *devname = serial_devices[i]; - if (devname && strcmp(devname, "none")) { - char label[32]; - snprintf(label, sizeof(label), "serial%d", i); - serial_hds[i] = qemu_chr_open(label, devname, NULL); - if (!serial_hds[i]) { - fprintf(stderr, "qemu: could not open serial device '%s': %s\n", - devname, strerror(errno)); - exit(1); - } - } - } + if (foreach_device_config(DEV_SERIAL, serial_parse) < 0) + exit(1); for(i = 0; i < MAX_PARALLEL_PORTS; i++) { const char *devname = parallel_devices[i];
Qemu creates a default serial line for you in case you didn't specify one on the command line. Right now this is tied to the '-serial <chardev>' command line switch, which in turn causes trouble if you are creating your serial line via '-device isa-serial,<props>'. This patch adds a variable default_serial which says whenever a default serial line should be added. It is enabled by default. It is cleared when qemu finds '-serial' or '-device isa-serial' on the command line. Part of the patch is some infrastructure for the '-device $driver' checking (default_driver_check function) which will also be used by the other patches of this series. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- vl.c | 122 +++++++++++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 81 insertions(+), 41 deletions(-)