[PATCH for-8.2 0/3] UI: fix default VC regressions

[PATCH for-8.2 0/3] UI: fix default VC regressions

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

There are a few annoying regressions with the default VCs introduced with the
pixman series. The "vl: revert behaviour for -display none" change solves most
of the issues. Another one is hit when using remote displays, and VCs are not
created as they used to, see: "ui/console: fix default VC when there are no
display". Finally, "ui: use "vc" chardev for dbus, gtk & spice-app" was meant to
be included in the pixman series and also brings back default VCs creation.

Marc-André Lureau (3):
  vl: revert behaviour for -display none
  ui: use "vc" chardev for dbus, gtk & spice-app
  ui/console: fix default VC when there are no display

 system/vl.c    |  2 +-
 ui/console.c   | 18 ++++++++----------
 ui/dbus.c      |  1 +
 ui/gtk.c       |  1 +
 ui/spice-app.c |  1 +
 5 files changed, 12 insertions(+), 11 deletions(-)

-- 
2.41.0

[PATCH for-8.2 1/3] vl: revert behaviour for -display none

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Commit 1bec1cc0d ("ui/console: allow to override the default VC") 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'

When qemu is compiled without PIXMAN, by default the serials aren't
muxed with the monitor anymore on stdio. The serials are redirected to
"null" instead, and the monitor isn't set up.

Fixes: commit 1bec1cc0d ("ui/console: allow to override the default VC")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 system/vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/system/vl.c b/system/vl.c
index 5af7ced2a1..14bf0cf0bf 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1391,7 +1391,7 @@ static void qemu_create_default_devices(void)
         }
     }
 
-    if (nographic || (!vc && !is_daemonized() && isatty(STDOUT_FILENO))) {
+    if (nographic) {
         if (default_parallel) {
             add_device_config(DEV_PARALLEL, "null");
         }
-- 
2.41.0

Re: [PATCH for-8.2 1/3] vl: revert behaviour for -display none

[Peter Maydell]

On Fri, 17 Nov 2023 at 14:35, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Commit 1bec1cc0d ("ui/console: allow to override the default VC") 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'
>
> When qemu is compiled without PIXMAN, by default the serials aren't
> muxed with the monitor anymore on stdio. The serials are redirected to
> "null" instead, and the monitor isn't set up.
>
> Fixes: commit 1bec1cc0d ("ui/console: allow to override the default VC")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  system/vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index 5af7ced2a1..14bf0cf0bf 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1391,7 +1391,7 @@ static void qemu_create_default_devices(void)
>          }
>      }
>
> -    if (nographic || (!vc && !is_daemonized() && isatty(STDOUT_FILENO))) {
> +    if (nographic) {
>          if (default_parallel) {
>              add_device_config(DEV_PARALLEL, "null");
>          }

This fixes the regression I was seeing with the semihosting
use case. I haven't checked the Xen setup.

Tested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [EXTERNAL] [PATCH for-8.2 1/3] vl: revert behaviour for -display none

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

On Mon, 2023-11-20 at 12:42 +0000, Peter Maydell wrote:
> On Fri, 17 Nov 2023 at 14:35, <marcandre.lureau@redhat.com> wrote:
> > 
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Commit 1bec1cc0d ("ui/console: allow to override the default VC")
> > 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'
> > 
> > When qemu is compiled without PIXMAN, by default the serials aren't
> > muxed with the monitor anymore on stdio. The serials are redirected
> > to
> > "null" instead, and the monitor isn't set up.
> > 
> > Fixes: commit 1bec1cc0d ("ui/console: allow to override the default
> > VC")
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  system/vl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/system/vl.c b/system/vl.c
> > index 5af7ced2a1..14bf0cf0bf 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -1391,7 +1391,7 @@ static void qemu_create_default_devices(void)
> >          }
> >      }
> > 
> > -    if (nographic || (!vc && !is_daemonized() &&
> > isatty(STDOUT_FILENO))) {
> > +    if (nographic) {
> >          if (default_parallel) {
> >              add_device_config(DEV_PARALLEL, "null");
> >          }
> 
> This fixes the regression I was seeing with the semihosting
> use case. I haven't checked the Xen setup.

Should probably work, but we want the better fix for Xen anyway:
https://lore.kernel.org/qemu-devel/20231115172723.1161679-3-dwmw2@infradead.org/




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH for-8.2 1/3] vl: revert behaviour for -display none

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Mon, 2023-11-20 at 12:42 +0000, Peter Maydell wrote:
> 
> This fixes the regression I was seeing with the semihosting
> use case. I haven't checked the Xen setup.
> 
> Tested-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

It does also work for the Xen command line (with my other fix
reverted).

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


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[PATCH for-8.2 2/3] ui: use "vc" chardev for dbus, gtk & spice-app

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Those display have their own implementation of "vc" chardev, which
doesn't use pixman. They also don't implement the width/height/cols/rows
options, so qemu_display_get_vc() should return a compatible argument.

This patch was meant to be with the pixman series, when the "vc" field
was introduced. It fixes a regression where VC are created on the
tty (or null) instead of the display own "vc" implementation.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus.c      | 1 +
 ui/gtk.c       | 1 +
 ui/spice-app.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/ui/dbus.c b/ui/dbus.c
index 866467ad2e..e08b5de064 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -518,6 +518,7 @@ static QemuDisplay qemu_display_dbus = {
     .type       = DISPLAY_TYPE_DBUS,
     .early_init = early_dbus_init,
     .init       = dbus_init,
+    .vc         = "vc",
 };
 
 static void register_dbus(void)
diff --git a/ui/gtk.c b/ui/gtk.c
index be047a41ad..810d7fc796 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2534,6 +2534,7 @@ static QemuDisplay qemu_display_gtk = {
     .type       = DISPLAY_TYPE_GTK,
     .early_init = early_gtk_display_init,
     .init       = gtk_display_init,
+    .vc         = "vc",
 };
 
 static void register_gtk(void)
diff --git a/ui/spice-app.c b/ui/spice-app.c
index 405fb7f9f5..a10b4a58fe 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -220,6 +220,7 @@ static QemuDisplay qemu_display_spice_app = {
     .type       = DISPLAY_TYPE_SPICE_APP,
     .early_init = spice_app_display_early_init,
     .init       = spice_app_display_init,
+    .vc         = "vc",
 };
 
 static void register_spice_app(void)
-- 
2.41.0

Re: [PATCH for-8.2 2/3] ui: use "vc" chardev for dbus, gtk & spice-app

[Thomas Huth]

On 17/11/2023 15.35, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Those display have their own implementation of "vc" chardev, which
> doesn't use pixman. They also don't implement the width/height/cols/rows
> options, so qemu_display_get_vc() should return a compatible argument.
> 
> This patch was meant to be with the pixman series, when the "vc" field
> was introduced. It fixes a regression where VC are created on the
> tty (or null) instead of the display own "vc" implementation.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/dbus.c      | 1 +
>   ui/gtk.c       | 1 +
>   ui/spice-app.c | 1 +
>   3 files changed, 3 insertions(+)

FWIW,
Acked-by: Thomas Huth <thuth@redhat.com>

[PATCH for-8.2 3/3] ui/console: fix default VC when there are no display

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

When display is "none", we may still have remote displays (I think it
would be simpler if VNC/Spice were regular display btw). Return the
default VC then, and set them up to fix a regression when using remote
display and it used the TTY instead.

Fixes: commit 1bec1cc0d ("ui/console: allow to override the default VC")
Reported-by: German Maglione <gmaglione@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 8e688d3569..7db921e3b7 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1679,19 +1679,17 @@ void qemu_display_init(DisplayState *ds, DisplayOptions *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";
+    const char *vc = "vc:80Cx24C";
+#else
+    const char *vc = NULL;
 #endif
+
+    assert(opts->type < DISPLAY_TYPE__MAX);
+    if (dpys[opts->type] && dpys[opts->type]->vc) {
+        vc = dpys[opts->type]->vc;
     }
-    return NULL;
+    return vc;
 }
 
 void qemu_display_help(void)
-- 
2.41.0

Re: [PATCH for-8.2 3/3] ui/console: fix default VC when there are no display

[Thomas Huth]

On 17/11/2023 15.35, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> When display is "none", we may still have remote displays (I think it
> would be simpler if VNC/Spice were regular display btw). Return the
> default VC then, and set them up to fix a regression when using remote
> display and it used the TTY instead.
> 
> Fixes: commit 1bec1cc0d ("ui/console: allow to override the default VC")
> Reported-by: German Maglione <gmaglione@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/console.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-8.2 0/3] UI: fix default VC regressions

[Marc-André Lureau]

Hi

On Fri, Nov 17, 2023 at 6:36 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> There are a few annoying regressions with the default VCs introduced with the
> pixman series. The "vl: revert behaviour for -display none" change solves most
> of the issues. Another one is hit when using remote displays, and VCs are not
> created as they used to, see: "ui/console: fix default VC when there are no
> display". Finally, "ui: use "vc" chardev for dbus, gtk & spice-app" was meant to
> be included in the pixman series and also brings back default VCs creation.
>
> Marc-André Lureau (3):
>   vl: revert behaviour for -display none
>   ui: use "vc" chardev for dbus, gtk & spice-app
>   ui/console: fix default VC when there are no display

I wish to send a PR (rc1 today), together with "[PATCH] vl: add
missing display_remote++".

Some R-B/A-B appreciated! thanks


-- 
Marc-André Lureau

Re: [PATCH for-8.2 0/3] UI: fix default VC regressions

[Woodhouse, David via]

[-- Attachment #1.1: Type: text/plain, Size: 1347 bytes --]

On Tue, 2023-11-21 at 11:37 +0400, Marc-André Lureau wrote:
> On Fri, Nov 17, 2023 at 6:36 PM <marcandre.lureau@redhat.com> wrote:
> > 
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Hi,
> > 
> > There are a few annoying regressions with the default VCs introduced with the
> > pixman series. The "vl: revert behaviour for -display none" change solves most
> > of the issues. Another one is hit when using remote displays, and VCs are not
> > created as they used to, see: "ui/console: fix default VC when there are no
> > display". Finally, "ui: use "vc" chardev for dbus, gtk & spice-app" was meant to
> > be included in the pixman series and also brings back default VCs creation.
> > 
> > Marc-André Lureau (3):
> >    vl: revert behaviour for -display none
> >    ui: use "vc" chardev for dbus, gtk & spice-app
> >    ui/console: fix default VC when there are no display
> 
> I wish to send a PR (rc1 today), together with "[PATCH] vl: add
> missing display_remote++".
> 
> Some R-B/A-B appreciated! thanks

Not sure I can give coherent review on the other two, but the first
patch does fix the Xen command line and looks sane.

Please could I ask you to also include
https://lore.kernel.org/qemu-devel/20231115172723.1161679-3-dwmw2@infradead.org/
in the series as you push it?


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5957 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

Re: [PATCH for-8.2 0/3] UI: fix default VC regressions

[Marc-André Lureau]

Hi David

On Tue, Nov 21, 2023 at 2:15 PM Woodhouse, David <dwmw@amazon.co.uk> wrote:
>
> On Tue, 2023-11-21 at 11:37 +0400, Marc-André Lureau wrote:
> > On Fri, Nov 17, 2023 at 6:36 PM <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Hi,
> > >
> > > There are a few annoying regressions with the default VCs introduced with the
> > > pixman series. The "vl: revert behaviour for -display none" change solves most
> > > of the issues. Another one is hit when using remote displays, and VCs are not
> > > created as they used to, see: "ui/console: fix default VC when there are no
> > > display". Finally, "ui: use "vc" chardev for dbus, gtk & spice-app" was meant to
> > > be included in the pixman series and also brings back default VCs creation.
> > >
> > > Marc-André Lureau (3):
> > >    vl: revert behaviour for -display none
> > >    ui: use "vc" chardev for dbus, gtk & spice-app
> > >    ui/console: fix default VC when there are no display
> >
> > I wish to send a PR (rc1 today), together with "[PATCH] vl: add
> > missing display_remote++".
> >
> > Some R-B/A-B appreciated! thanks
>
> Not sure I can give coherent review on the other two, but the first
> patch does fix the Xen command line and looks sane.
>
> Please could I ask you to also include
> https://lore.kernel.org/qemu-devel/20231115172723.1161679-3-dwmw2@infradead.org/
> in the series as you push it?
>
>

Thanks for the quick test. I am bit reluctant to push your change in
8.2 too. It's a change in behaviour at this point, not simply a fix.
But as the maintainer of Xen stuff, you have perhaps the final call.?

-- 
Marc-André Lureau

Re: [PATCH for-8.2 0/3] UI: fix default VC regressions

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

On Tue, 2023-11-21 at 14:37 +0400, Marc-André Lureau wrote:
> 
> Thanks for the quick test. I am bit reluctant to push your change in
> 8.2 too. It's a change in behaviour at this point, not simply a fix.
> But as the maintainer of Xen stuff, you have perhaps the final call.?

it's not a change in behaviour yet. Being able to add xen-console
devices on the command line at all is new in 8.2, so it only ends up
being a "change" if we do it after the 8.2 release, which is why I'm
keen to do it now.

Thanks.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH for-8.2 0/3] UI: fix default VC regressions

[Marc-André Lureau]

Hi

On Tue, Nov 21, 2023 at 2:42 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Tue, 2023-11-21 at 14:37 +0400, Marc-André Lureau wrote:
> >
> > Thanks for the quick test. I am bit reluctant to push your change in
> > 8.2 too. It's a change in behaviour at this point, not simply a fix.
> > But as the maintainer of Xen stuff, you have perhaps the final call.?
>
> it's not a change in behaviour yet. Being able to add xen-console
> devices on the command line at all is new in 8.2, so it only ends up
> being a "change" if we do it after the 8.2 release, which is why I'm
> keen to do it now.
>

I didn't realize that. Perhaps it's best to go through the Xen queue.
I already sent a PR for UI regressions, as we are close to rc1.

thanks!


-- 
Marc-André Lureau

Re: [PATCH for-8.2 0/3] UI: fix default VC regressions

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Tue, 2023-11-21 at 14:45 +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Nov 21, 2023 at 2:42 PM David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > On Tue, 2023-11-21 at 14:37 +0400, Marc-André Lureau wrote:
> > > 
> > > Thanks for the quick test. I am bit reluctant to push your change in
> > > 8.2 too. It's a change in behaviour at this point, not simply a fix.
> > > But as the maintainer of Xen stuff, you have perhaps the final call.?
> > 
> > it's not a change in behaviour yet. Being able to add xen-console
> > devices on the command line at all is new in 8.2, so it only ends up
> > being a "change" if we do it after the 8.2 release, which is why I'm
> > keen to do it now.
> > 
> 
> I didn't realize that. Perhaps it's best to go through the Xen queue.
> I already sent a PR for UI regressions, as we are close to rc1.

Makes sense. Could I trouble you for a R-b for it and then I'll do so?

Thanks.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]