| Message ID | 20231107101524.2993389-10-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,v3,01/25] build-sys: add a "pixman" feature | expand |
On Tue, 2023-11-07 at 14:15 +0400, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > If a display is backed by a specialized VC, allow to override the > default "vc:80Cx24C". > > As suggested by Paolo, if the display doesn't implement a VC (get_vc() > returns NULL), use a fallback that will use a muxed console on stdio. > > This changes the behaviour of "qemu -display none", to create a muxed > serial/monitor by default (on TTY & not daemonized). > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> Hrm. This breaks the command line documented at https://qemu-project.gitlab.io/qemu/system/i386/xen.html $ ./qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \ -display none -chardev stdio,mux=on,id=char0,signal=off -mon char0 \ -device xen-console,chardev=char0 -drive file=${GUEST_IMAGE},if=xen qemu-system-x86_64: cannot use stdio by multiple character devices qemu-system-x86_64: could not connect serial device to character backend 'stdio' Can we make it create a Xen console by default, instead of a serial port? And/or make it *not* use stdio if something else on the command line already does?
On Thu, 9 Nov 2023 at 19:10, David Woodhouse <dwmw2@infradead.org> wrote: > > On Tue, 2023-11-07 at 14:15 +0400, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > If a display is backed by a specialized VC, allow to override the > > default "vc:80Cx24C". > > > > As suggested by Paolo, if the display doesn't implement a VC (get_vc() > > returns NULL), use a fallback that will use a muxed console on stdio. > > > > This changes the behaviour of "qemu -display none", to create a muxed > > serial/monitor by default (on TTY & not daemonized). > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > Hrm. This breaks the command line documented at > https://qemu-project.gitlab.io/qemu/system/i386/xen.html > > $ ./qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \ > -display none -chardev stdio,mux=on,id=char0,signal=off -mon char0 \ > -device xen-console,chardev=char0 -drive file=${GUEST_IMAGE},if=xen > > qemu-system-x86_64: cannot use stdio by multiple character devices > qemu-system-x86_64: could not connect serial device to character backend 'stdio' > > Can we make it create a Xen console by default, instead of a serial > port? And/or make it *not* use stdio if something else on the command > line already does? I have filed this in QEMU's bug tracker so it's not forgotten: https://gitlab.com/qemu-project/qemu/-/issues/1974 Here is the list of open 8.2 bugs: https://gitlab.com/qemu-project/qemu/-/milestones/10 Stefan
On Thu, 2023-11-09 at 19:34 +0800, Stefan Hajnoczi wrote: > On Thu, 9 Nov 2023 at 19:10, David Woodhouse <dwmw2@infradead.org> wrote: > > > > On Tue, 2023-11-07 at 14:15 +0400, marcandre.lureau@redhat.com wrote: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > If a display is backed by a specialized VC, allow to override the > > > default "vc:80Cx24C". > > > > > > As suggested by Paolo, if the display doesn't implement a VC (get_vc() > > > returns NULL), use a fallback that will use a muxed console on stdio. > > > > > > This changes the behaviour of "qemu -display none", to create a muxed > > > serial/monitor by default (on TTY & not daemonized). > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > > Hrm. This breaks the command line documented at > > https://qemu-project.gitlab.io/qemu/system/i386/xen.html > > > > $ ./qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \ > > -display none -chardev stdio,mux=on,id=char0,signal=off -mon char0 \ > > -device xen-console,chardev=char0 -drive file=${GUEST_IMAGE},if=xen > > > > qemu-system-x86_64: cannot use stdio by multiple character devices > > qemu-system-x86_64: could not connect serial device to character backend 'stdio' > > > > Can we make it create a Xen console by default, instead of a serial > > port? And/or make it *not* use stdio if something else on the command > > line already does? > > I have filed this in QEMU's bug tracker so it's not forgotten: > https://gitlab.com/qemu-project/qemu/-/issues/1974 > > Here is the list of open 8.2 bugs: > https://gitlab.com/qemu-project/qemu/-/milestones/10 > > Stefan Thanks. Added a link there to the patch I just sent, along with a note suggesting that perhaps we should go a bit further. We're changing the QEMU behaviour in 8.2 to add these new devices using stdio, and *failing* if something else on the command line already used stdio. My patch avoids adding a serial port if there's already a xen-console, which is all well and good. But surely we shouldn't *fail* if something else is already using stdio; we should just *not* add the new default serial port? This might break other command lines which use stdio but *not* for a serial port? This breaks too: $ ./qemu-system-x86_64 -display none -chardev stdio,id=char0 qemu-system-x86_64: cannot use stdio by multiple character devices qemu-system-x86_64: could not connect serial device to character backend 'mon:stdio'
On Thu, 2023-11-09 at 11:45 +0000, David Woodhouse wrote: > On Thu, 2023-11-09 at 19:34 +0800, Stefan Hajnoczi wrote: > > On Thu, 9 Nov 2023 at 19:10, David Woodhouse <dwmw2@infradead.org> wrote: > > > > > > On Tue, 2023-11-07 at 14:15 +0400, marcandre.lureau@redhat.com wrote: > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > > > If a display is backed by a specialized VC, allow to override the > > > > default "vc:80Cx24C". > > > > > > > > As suggested by Paolo, if the display doesn't implement a VC (get_vc() > > > > returns NULL), use a fallback that will use a muxed console on stdio. > > > > > > > > This changes the behaviour of "qemu -display none", to create a muxed > > > > serial/monitor by default (on TTY & not daemonized). > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > > > > Hrm. This breaks the command line documented at > > > https://qemu-project.gitlab.io/qemu/system/i386/xen.html > > > > > > $ ./qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \ > > > -display none -chardev stdio,mux=on,id=char0,signal=off -mon char0 \ > > > -device xen-console,chardev=char0 -drive file=${GUEST_IMAGE},if=xen > > > > > > qemu-system-x86_64: cannot use stdio by multiple character devices > > > qemu-system-x86_64: could not connect serial device to character backend 'stdio' > > > > > > Can we make it create a Xen console by default, instead of a serial > > > port? And/or make it *not* use stdio if something else on the command > > > line already does? > > > > I have filed this in QEMU's bug tracker so it's not forgotten: > > https://gitlab.com/qemu-project/qemu/-/issues/1974 > > > > Here is the list of open 8.2 bugs: > > https://gitlab.com/qemu-project/qemu/-/milestones/10 > > > > Stefan > > Thanks. Added a link there to the patch I just sent, along with a note > suggesting that perhaps we should go a bit further. > > We're changing the QEMU behaviour in 8.2 to add these new devices using > stdio, and *failing* if something else on the command line already used > stdio. > > My patch avoids adding a serial port if there's already a xen-console, > which is all well and good. But surely we shouldn't *fail* if something > else is already using stdio; we should just *not* add the new default > serial port? This might break other command lines which use stdio but > *not* for a serial port? This breaks too: > > $ ./qemu-system-x86_64 -display none -chardev stdio,id=char0 > qemu-system-x86_64: cannot use stdio by multiple character devices > qemu-system-x86_64: could not connect serial device to character backend 'mon:stdio' Regardless of possible further improvements, please could I have a review for the patch at https://lore.kernel.org/qemu-devel/20231115172723.1161679-3-dwmw2@infradead.org/ which at least makes the documented command line work again. Thanks.
On Tue, 7 Nov 2023 at 10:24, <marcandre.lureau@redhat.com> wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > If a display is backed by a specialized VC, allow to override the > default "vc:80Cx24C". > > As suggested by Paolo, if the display doesn't implement a VC (get_vc() > returns NULL), use a fallback that will use a muxed console on stdio. > > This changes the behaviour of "qemu -display none", to create a muxed > serial/monitor by default (on TTY & not daemonized). This breaks existing command line setups -- if I say "-display none" I just mean "don't do a display", not "please also give me a monitor". We already have a "do what I mean" option for "no graphics", which is "-nographic". The advantage of -display none is that it does only and exactly what it says it does. Setups using semihosting for output now get a spurious load of output from the monitor on their terminal. I think we should revert this; I'll send a patch. thanks -- PMM
On 11/16/23 09:52, Peter Maydell wrote: > On Tue, 7 Nov 2023 at 10:24, <marcandre.lureau@redhat.com> wrote: >> >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> If a display is backed by a specialized VC, allow to override the >> default "vc:80Cx24C". >> >> As suggested by Paolo, if the display doesn't implement a VC (get_vc() >> returns NULL), use a fallback that will use a muxed console on stdio. >> >> This changes the behaviour of "qemu -display none", to create a muxed >> serial/monitor by default (on TTY & not daemonized). > > This breaks existing command line setups -- if I say > "-display none" I just mean "don't do a display", not > "please also give me a monitor". We already have a > "do what I mean" option for "no graphics", which is > "-nographic". The advantage of -display none is that > it does only and exactly what it says it does. > > Setups using semihosting for output now get a spurious > load of output from the monitor on their terminal. > > I think we should revert this; I'll send a patch. Yes indeed. I think this may be why I've seen my xterm go into strange i/o modes during "make check-tcg" of the semihosting tests. r~
diff --git a/include/ui/console.h b/include/ui/console.h index acb61a7f15..a4a49ffc64 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -462,12 +462,14 @@ struct QemuDisplay { DisplayType type; void (*early_init)(DisplayOptions *opts); void (*init)(DisplayState *ds, DisplayOptions *opts); + const char *vc; }; void qemu_display_register(QemuDisplay *ui); bool qemu_display_find_default(DisplayOptions *opts); void qemu_display_early_init(DisplayOptions *opts); void qemu_display_init(DisplayState *ds, DisplayOptions *opts); +const char *qemu_display_get_vc(DisplayOptions *opts); void qemu_display_help(void); /* vnc.c */ diff --git a/system/vl.c b/system/vl.c index cf46e438cc..bd7fad770b 100644 --- a/system/vl.c +++ b/system/vl.c @@ -1372,6 +1372,7 @@ static void qemu_setup_display(void) static void qemu_create_default_devices(void) { MachineClass *machine_class = MACHINE_GET_CLASS(current_machine); + const char *vc = qemu_display_get_vc(&dpy); if (is_daemonized()) { /* According to documentation and historically, -nographic redirects @@ -1390,24 +1391,30 @@ static void qemu_create_default_devices(void) } } - if (nographic) { - if (default_parallel) + if (nographic || (!vc && !is_daemonized() && isatty(STDOUT_FILENO))) { + if (default_parallel) { add_device_config(DEV_PARALLEL, "null"); + } if (default_serial && default_monitor) { add_device_config(DEV_SERIAL, "mon:stdio"); } else { - if (default_serial) + if (default_serial) { add_device_config(DEV_SERIAL, "stdio"); - if (default_monitor) + } + if (default_monitor) { monitor_parse("stdio", "readline", false); + } } } else { - if (default_serial) - add_device_config(DEV_SERIAL, "vc:80Cx24C"); - if (default_parallel) - add_device_config(DEV_PARALLEL, "vc:80Cx24C"); - if (default_monitor) - monitor_parse("vc:80Cx24C", "readline", false); + if (default_serial) { + add_device_config(DEV_SERIAL, vc ?: "null"); + } + if (default_parallel) { + add_device_config(DEV_PARALLEL, vc ?: "null"); + } + if (default_monitor && vc) { + monitor_parse(vc, "readline", false); + } } if (default_net) { diff --git a/ui/console.c b/ui/console.c index 8ee66d10c5..a758ed62ad 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1675,6 +1675,20 @@ void qemu_display_init(DisplayState *ds, DisplayOptions *opts) dpys[opts->type]->init(ds, opts); } +const char *qemu_display_get_vc(DisplayOptions *opts) +{ + assert(opts->type < DISPLAY_TYPE__MAX); + if (opts->type == DISPLAY_TYPE_NONE) { + return NULL; + } + assert(dpys[opts->type] != NULL); + if (dpys[opts->type]->vc) { + return dpys[opts->type]->vc; + } else { + return "vc:80Cx24C"; + } +} + void qemu_display_help(void) { int idx;