1. Patchew
  2. QEMU
  3. View series

[PATCH for-8.2] Revert "ui/console: allow to override the default VC"

Peter Maydell posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20231116182228.3062796-1-peter.maydell@linaro.org
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
include/ui/console.h |  2 --
system/vl.c          | 27 ++++++++++-----------------
ui/console.c         | 17 -----------------
3 files changed, 10 insertions(+), 36 deletions(-)
[PATCH for-8.2] Revert "ui/console: allow to override the default VC"
Posted by Peter Maydell 1 year ago 
This reverts commit 1bec1cc0da497e55c16e2a7b50f94cdb2a02197f.  This
commit changed the behaviour of the "-display none" option, so that
it now creates a QEMU monitor on the terminal.  "-display none"
should not be tangled up with whether we create a monitor or a serial
terminal; it should purely and only disable the graphical window.
Changing its behaviour like this breaks command lines which, for
example, use semihosting for their output and don't want a graphical
window, as they now get a monitor they never asked for.

It also breaks the command line we document for Xen in
docs/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'

Revert the commit to restore the previous handling of "-display
none".

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1974
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/ui/console.h |  2 --
 system/vl.c          | 27 ++++++++++-----------------
 ui/console.c         | 17 -----------------
 3 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index a4a49ffc640..acb61a7f152 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -462,14 +462,12 @@ 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 5af7ced2a16..3d64a90f253 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1372,7 +1372,6 @@ 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
@@ -1391,30 +1390,24 @@ static void qemu_create_default_devices(void)
         }
     }
 
-    if (nographic || (!vc && !is_daemonized() && isatty(STDOUT_FILENO))) {
-        if (default_parallel) {
+    if (nographic) {
+        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 ?: "null");
-        }
-        if (default_parallel) {
-            add_device_config(DEV_PARALLEL, vc ?: "null");
-        }
-        if (default_monitor && vc) {
-            monitor_parse(vc, "readline", false);
-        }
+        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_net) {
diff --git a/ui/console.c b/ui/console.c
index 8e688d35695..676d0cbaba2 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1677,23 +1677,6 @@ 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 {
-#ifdef CONFIG_PIXMAN
-        return "vc:80Cx24C";
-#endif
-    }
-    return NULL;
-}
-
 void qemu_display_help(void)
 {
     int idx;
-- 
2.34.1
Re: [PATCH for-8.2] Revert "ui/console: allow to override the default VC"
Posted by Marc-André Lureau 1 year ago 
Hi

On Thu, Nov 16, 2023 at 10:25 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This reverts commit 1bec1cc0da497e55c16e2a7b50f94cdb2a02197f.  This
> commit changed the behaviour of the "-display none" option, so that
> it now creates a QEMU monitor on the terminal.  "-display none"
> should not be tangled up with whether we create a monitor or a serial
> terminal; it should purely and only disable the graphical window.
> Changing its behaviour like this breaks command lines which, for
> example, use semihosting for their output and don't want a graphical
> window, as they now get a monitor they never asked for.
>
> It also breaks the command line we document for Xen in
> docs/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'
>
> Revert the commit to restore the previous handling of "-display
> none".
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1974
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

By simply reverting, we break qemu with --disable-pixman:
qemu-system-x86_64: 'vc' is not a valid char driver name

The change of behaviour was a consequence of Paolo suggestion from v2:
https://patchew.org/QEMU/20230918135206.2739222-1-marcandre.lureau@redhat.com/20230918135206.2739222-6-marcandre.lureau@redhat.com/#4b890258-3426-0e0f-dd65-6114b9bee5e3@redhat.com

Let's get back to the current behaviour and not add muxed console in
any case (disable pixman or not). I'll try to make a patch asap.

> ---
>  include/ui/console.h |  2 --
>  system/vl.c          | 27 ++++++++++-----------------
>  ui/console.c         | 17 -----------------
>  3 files changed, 10 insertions(+), 36 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index a4a49ffc640..acb61a7f152 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -462,14 +462,12 @@ 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 5af7ced2a16..3d64a90f253 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1372,7 +1372,6 @@ 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
> @@ -1391,30 +1390,24 @@ static void qemu_create_default_devices(void)
>          }
>      }
>
> -    if (nographic || (!vc && !is_daemonized() && isatty(STDOUT_FILENO))) {
> -        if (default_parallel) {
> +    if (nographic) {
> +        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 ?: "null");
> -        }
> -        if (default_parallel) {
> -            add_device_config(DEV_PARALLEL, vc ?: "null");
> -        }
> -        if (default_monitor && vc) {
> -            monitor_parse(vc, "readline", false);
> -        }
> +        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_net) {
> diff --git a/ui/console.c b/ui/console.c
> index 8e688d35695..676d0cbaba2 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1677,23 +1677,6 @@ 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 {
> -#ifdef CONFIG_PIXMAN
> -        return "vc:80Cx24C";
> -#endif
> -    }
> -    return NULL;
> -}
> -
>  void qemu_display_help(void)
>  {
>      int idx;
> --
> 2.34.1
>
>


-- 
Marc-André Lureau
Re: [PATCH for-8.2] Revert "ui/console: allow to override the default VC"
Posted by David Woodhouse 1 year ago 
On 16 November 2023 13:22:28 GMT-05:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>This reverts commit 1bec1cc0da497e55c16e2a7b50f94cdb2a02197f.  This
>commit changed the behaviour of the "-display none" option, so that
>it now creates a QEMU monitor on the terminal.  "-display none"
>should not be tangled up with whether we create a monitor or a serial
>terminal; it should purely and only disable the graphical window.
>Changing its behaviour like this breaks command lines which, for
>example, use semihosting for their output and don't want a graphical
>window, as they now get a monitor they never asked for.
>
>It also breaks the command line we document for Xen in
>docs/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'
>
>Revert the commit to restore the previous handling of "-display
>none".
>
>Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1974
>Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: <dwmw@amazon.co.uk>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.