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

marcandre.lureau@redhat.com posted 25 patches 1 year ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Dr. David Alan Gilbert" <dave@treblig.org>, Peter Maydell <peter.maydell@linaro.org>, BALATON Zoltan <balaton@eik.bme.hu>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Daniel P. Berrangé" <berrange@redhat.com>, Thomas Huth <thuth@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
[PULL v3 09/25] ui/console: allow to override the default VC
Posted by marcandre.lureau@redhat.com 1 year ago
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(-)

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;
-- 
2.41.0


Re: [PULL v3 09/25] ui/console: allow to override the default VC
Posted by Peter Maydell 1 year ago
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
Re: [PULL v3 09/25] ui/console: allow to override the default VC
Posted by Richard Henderson 1 year ago
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~


Re: [PULL v3 09/25] ui/console: allow to override the default VC
Posted by David Woodhouse 1 year ago
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?
Re: [PULL v3 09/25] ui/console: allow to override the default VC
Posted by Stefan Hajnoczi 1 year ago
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
Re: [PULL v3 09/25] ui/console: allow to override the default VC
Posted by David Woodhouse 1 year ago
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'

Re: [PULL v3 09/25] ui/console: allow to override the default VC
Posted by David Woodhouse 1 year ago
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.