diff mbox series

[PULL,v3,09/25] ui/console: allow to override the default VC

Message ID 20231107101524.2993389-10-marcandre.lureau@redhat.com
State New
Headers show
Series [PULL,v3,01/25] build-sys: add a "pixman" feature | expand

Commit Message

Marc-André Lureau Nov. 7, 2023, 10:15 a.m. UTC
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>
---
 include/ui/console.h |  2 ++
 system/vl.c          | 27 +++++++++++++++++----------
 ui/console.c         | 14 ++++++++++++++
 3 files changed, 33 insertions(+), 10 deletions(-)

Comments

David Woodhouse Nov. 9, 2023, 11:10 a.m. UTC | #1
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?
Stefan Hajnoczi Nov. 9, 2023, 11:34 a.m. UTC | #2
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
David Woodhouse Nov. 9, 2023, 11:45 a.m. UTC | #3
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'
David Woodhouse Nov. 16, 2023, 4:52 p.m. UTC | #4
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.
Peter Maydell Nov. 16, 2023, 5:52 p.m. UTC | #5
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
Richard Henderson Nov. 17, 2023, 12:51 a.m. UTC | #6
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 mbox series

Patch

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;