diff mbox series

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

Message ID 20231117143506.1521718-2-marcandre.lureau@redhat.com (mailing list archive)
State New, archived
Headers show
Series UI: fix default VC regressions | expand

Commit Message

Marc-André Lureau Nov. 17, 2023, 2:35 p.m. UTC
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(-)

Comments

Peter Maydell Nov. 20, 2023, 12:42 p.m. UTC | #1
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
David Woodhouse Nov. 20, 2023, 1:17 p.m. UTC | #2
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/
David Woodhouse Nov. 21, 2023, 10:11 a.m. UTC | #3
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>
diff mbox series

Patch

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");
         }