[PATCH 0/2] system: Fix handling of '-serial none -serial something'

[PATCH 0/2] system: Fix handling of '-serial none -serial something'

[Peter Maydell]

(This patchset fixes a bug reported by Bohdan Kostiv at
https://lore.kernel.org/qemu-devel/CAA3Sv1LQ8yDUNLCB5WqLVZjsHffrU0uSbL_YYJW_m+Db2PhEeQ@mail.gmail.com/
 -- my patch 1 avoids a bug in his suggested change, and
patch 2 is new, improving the documentation.)

Currently if the user passes multiple -serial options on the command
line, we mostly treat those as applying to the different serial
devices in order, so that for example
 -serial stdio -serial file:filename
will connect the first serial port to stdio and the second to the
named file.

The exception to this is the '-serial none' serial device type.  This
means "don't allocate this serial device", but a bug means that
following -serial options are not correctly handled, so that
 -serial none -serial stdio
has the unexpected effect that stdio is connected to the first serial
port, not the second.

This is a very long-standing bug that dates back at least as far as
commit 998bbd74b9d81 from 2009.
        
It's possible that some users have commandlines that mistakenly
include a previously-redundant '-serial none'; those users can
simply delete that option in order to produce a command line that
has the same effect on both old and new QEMU. We can mention this
in the release notes.

Our documentation for -serial none and -serial null was also a
bit lacking; I've provided a patch here which tries to improve it.

thanks
-- PMM

Peter Maydell (2):
  system/vl.c: Fix handling of '-serial none -serial something'
  qemu-options.hx: Improve -serial option documentation

 system/vl.c     | 22 +++++++++++++---------
 qemu-options.hx | 14 +++++++++++---
 2 files changed, 24 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 1/2] system/vl.c: Fix handling of '-serial none -serial something'

[Peter Maydell]

Currently if the user passes multiple -serial options on the command
line, we mostly treat those as applying to the different serial
devices in order, so that for example
 -serial stdio -serial file:filename
will connect the first serial port to stdio and the second to the
named file.

The exception to this is the '-serial none' serial device type.  This
means "don't allocate this serial device", but a bug means that
following -serial options are not correctly handled, so that
 -serial none -serial stdio
has the unexpected effect that stdio is connected to the first serial
port, not the second.

This is a very long-standing bug that dates back at least as far as
commit 998bbd74b9d81 from 2009.

Make the 'none' serial type move forward in the indexing of serial
devices like all the other serial types, so that any subsequent
-serial options are correctly handled.

Note that if your commandline mistakenly had a '-serial none' that
was being overridden by a following '-serial something' option, you
should delete the unnecessary '-serial none'.  This will give you the
same behaviour as before, on QEMU versions both with and without this
bug fix.

Cc: qemu-stable@nongnu.org
Reported-by: Bohdan Kostiv <bohdan.kostiv@tii.ae>
Fixes: 998bbd74b9d81 ("default devices: core code & serial lines")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
See the discussion of Bohdan's patch on the mailing list for
further context:
https://lore.kernel.org/qemu-devel/CAA3Sv1LQ8yDUNLCB5WqLVZjsHffrU0uSbL_YYJW_m+Db2PhEeQ@mail.gmail.com/
---
 system/vl.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index 788d88ea03a..aa7953bdd7d 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1439,18 +1439,22 @@ static void qemu_create_default_devices(void)
 static int serial_parse(const char *devname)
 {
     int index = num_serial_hds;
-    char label[32];
 
-    if (strcmp(devname, "none") == 0)
-        return 0;
-    snprintf(label, sizeof(label), "serial%d", index);
     serial_hds = g_renew(Chardev *, serial_hds, index + 1);
 
-    serial_hds[index] = qemu_chr_new_mux_mon(label, devname, NULL);
-    if (!serial_hds[index]) {
-        error_report("could not connect serial device"
-                     " to character backend '%s'", devname);
-        return -1;
+    if (strcmp(devname, "none") == 0) {
+        /* Don't allocate a serial device for this index */
+        serial_hds[index] = NULL;
+    } else {
+        char label[32];
+        snprintf(label, sizeof(label), "serial%d", index);
+
+        serial_hds[index] = qemu_chr_new_mux_mon(label, devname, NULL);
+        if (!serial_hds[index]) {
+            error_report("could not connect serial device"
+                         " to character backend '%s'", devname);
+            return -1;
+        }
     }
     num_serial_hds++;
     return 0;
-- 
2.34.1

Re: [PATCH 1/2] system/vl.c: Fix handling of '-serial none -serial something'

[Daniel P. Berrangé]

On Mon, Jan 22, 2024 at 04:36:06PM +0000, Peter Maydell wrote:
> Currently if the user passes multiple -serial options on the command
> line, we mostly treat those as applying to the different serial
> devices in order, so that for example
>  -serial stdio -serial file:filename
> will connect the first serial port to stdio and the second to the
> named file.
> 
> The exception to this is the '-serial none' serial device type.  This
> means "don't allocate this serial device", but a bug means that
> following -serial options are not correctly handled, so that
>  -serial none -serial stdio
> has the unexpected effect that stdio is connected to the first serial
> port, not the second.
> 
> This is a very long-standing bug that dates back at least as far as
> commit 998bbd74b9d81 from 2009.
> 
> Make the 'none' serial type move forward in the indexing of serial
> devices like all the other serial types, so that any subsequent
> -serial options are correctly handled.
> 
> Note that if your commandline mistakenly had a '-serial none' that
> was being overridden by a following '-serial something' option, you
> should delete the unnecessary '-serial none'.  This will give you the
> same behaviour as before, on QEMU versions both with and without this
> bug fix.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Bohdan Kostiv <bohdan.kostiv@tii.ae>
> Fixes: 998bbd74b9d81 ("default devices: core code & serial lines")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> See the discussion of Bohdan's patch on the mailing list for
> further context:
> https://lore.kernel.org/qemu-devel/CAA3Sv1LQ8yDUNLCB5WqLVZjsHffrU0uSbL_YYJW_m+Db2PhEeQ@mail.gmail.com/
> ---
>  system/vl.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] system/vl.c: Fix handling of '-serial none -serial something'

[Richard Henderson]

On 1/23/24 02:36, Peter Maydell wrote:
> Currently if the user passes multiple -serial options on the command
> line, we mostly treat those as applying to the different serial
> devices in order, so that for example
>   -serial stdio -serialfile:filename
> will connect the first serial port to stdio and the second to the
> named file.
> 
> The exception to this is the '-serial none' serial device type.  This
> means "don't allocate this serial device", but a bug means that
> following -serial options are not correctly handled, so that
>   -serial none -serial stdio
> has the unexpected effect that stdio is connected to the first serial
> port, not the second.
> 
> This is a very long-standing bug that dates back at least as far as
> commit 998bbd74b9d81 from 2009.
> 
> Make the 'none' serial type move forward in the indexing of serial
> devices like all the other serial types, so that any subsequent
> -serial options are correctly handled.
> 
> Note that if your commandline mistakenly had a '-serial none' that
> was being overridden by a following '-serial something' option, you
> should delete the unnecessary '-serial none'.  This will give you the
> same behaviour as before, on QEMU versions both with and without this
> bug fix.
> 
> Cc:qemu-stable@nongnu.org
> Reported-by: Bohdan Kostiv<bohdan.kostiv@tii.ae>
> Fixes: 998bbd74b9d81 ("default devices: core code & serial lines")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> See the discussion of Bohdan's patch on the mailing list for
> further context:
> https://lore.kernel.org/qemu-devel/CAA3Sv1LQ8yDUNLCB5WqLVZjsHffrU0uSbL_YYJW_m+Db2PhEeQ@mail.gmail.com/
> ---
>   system/vl.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/2] qemu-options.hx: Improve -serial option documentation

[Peter Maydell]

The -serial option documentation is a bit brief about '-serial none'
and '-serial null'. In particular it's not very clear about the
difference between them, and it doesn't mention that it's up to
the machine model whether '-serial none' means "don't create the
serial port" or "don't wire the serial port up to anything".

Expand on these points.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-options.hx | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index ced82848637..d8c3fe91de1 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4129,7 +4129,8 @@ SRST
     This option can be used several times to simulate up to 4 serial
     ports.
 
-    Use ``-serial none`` to disable all serial ports.
+    You can use ``-serial none`` to suppress the creation of default
+    serial devices.
 
     Available character devices are:
 
@@ -4151,10 +4152,17 @@ SRST
         [Linux only] Pseudo TTY (a new PTY is automatically allocated)
 
     ``none``
-        No device is allocated.
+        No device is allocated. Note that for machine types which
+        emulate systems where a serial device is always present in
+        real hardware, this may be equivalent to the ``null`` option,
+        in that the serial device is still present but all output
+        is discarded. For boards where the number of serial ports is
+        truly variable, this suppresses the creation of the device.
 
     ``null``
-        void device
+        A guest will see the UART or serial device as present in the
+        machine, but all output is discarded, and there is no input.
+        Conceptually equivalent to redirecting the output to ``/dev/null``.
 
     ``chardev:id``
         Use a named character device defined with the ``-chardev``
-- 
2.34.1

Re: [PATCH 2/2] qemu-options.hx: Improve -serial option documentation

[Daniel P. Berrangé]

On Mon, Jan 22, 2024 at 04:36:07PM +0000, Peter Maydell wrote:
> The -serial option documentation is a bit brief about '-serial none'
> and '-serial null'. In particular it's not very clear about the
> difference between them, and it doesn't mention that it's up to
> the machine model whether '-serial none' means "don't create the
> serial port" or "don't wire the serial port up to anything".
> 
> Expand on these points.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-options.hx | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] qemu-options.hx: Improve -serial option documentation

[Philippe Mathieu-Daudé]

Hi,

On 22/1/24 17:36, Peter Maydell wrote:
> The -serial option documentation is a bit brief about '-serial none'
> and '-serial null'. In particular it's not very clear about the
> difference between them, and it doesn't mention that it's up to
> the machine model whether '-serial none' means "don't create the
> serial port" or "don't wire the serial port up to anything".
> 
> Expand on these points.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   qemu-options.hx | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ced82848637..d8c3fe91de1 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4129,7 +4129,8 @@ SRST
>       This option can be used several times to simulate up to 4 serial
>       ports.
>   
> -    Use ``-serial none`` to disable all serial ports.
> +    You can use ``-serial none`` to suppress the creation of default
> +    serial devices.
>   
>       Available character devices are:
>   
> @@ -4151,10 +4152,17 @@ SRST
>           [Linux only] Pseudo TTY (a new PTY is automatically allocated)
>   
>       ``none``
> -        No device is allocated.
> +        No device is allocated. Note that 

>          for machine types which
> +        emulate systems where a serial device is always present in
> +        real hardware, this may be equivalent to the ``null`` option,
> +        in that the serial device is still present but all output
> +        is discarded.

Should we deprecate this broken case, suggesting to use ``null``
instead?

>          For boards where the number of serial ports is
> +        truly variable, this suppresses the creation of the device.
>   
>       ``null``
> -        void device
> +        A guest will see the UART or serial device as present in the
> +        machine, but all output is discarded, and there is no input.
> +        Conceptually equivalent to redirecting the output to ``/dev/null``.
>   
>       ``chardev:id``
>           Use a named character device defined with the ``-chardev``

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/2] qemu-options.hx: Improve -serial option documentation

[Peter Maydell]

On Mon, 22 Jan 2024 at 17:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> On 22/1/24 17:36, Peter Maydell wrote:
> > The -serial option documentation is a bit brief about '-serial none'
> > and '-serial null'. In particular it's not very clear about the
> > difference between them, and it doesn't mention that it's up to
> > the machine model whether '-serial none' means "don't create the
> > serial port" or "don't wire the serial port up to anything".
> >
> > Expand on these points.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   qemu-options.hx | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index ced82848637..d8c3fe91de1 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4129,7 +4129,8 @@ SRST
> >       This option can be used several times to simulate up to 4 serial
> >       ports.
> >
> > -    Use ``-serial none`` to disable all serial ports.
> > +    You can use ``-serial none`` to suppress the creation of default
> > +    serial devices.
> >
> >       Available character devices are:
> >
> > @@ -4151,10 +4152,17 @@ SRST
> >           [Linux only] Pseudo TTY (a new PTY is automatically allocated)
> >
> >       ``none``
> > -        No device is allocated.
> > +        No device is allocated. Note that
>
> >          for machine types which
> > +        emulate systems where a serial device is always present in
> > +        real hardware, this may be equivalent to the ``null`` option,
> > +        in that the serial device is still present but all output
> > +        is discarded.
>
> Should we deprecate this broken case, suggesting to use ``null``
> instead?

It's machine specific. On systems with pluggable serial devices
it makes sense to use '-serial none' to get rid of them
entirely. On systems where the UARTs are hardwired into the
board, having '-serial none' literally delete the UART device
just breaks guests, which is why those boards make it behave
like '-serial null'. But users should still be able to use
'-serial none' to say "I don't really care about serial here".

(This is why in commit 12051d82f004024d5d we made the chardev
frontend functions cope with having a NULL backend, to avoid
boards having to say "oh, serial_hd(n) is NULL, I must create
a 'null' backend for it", which half of them didn't do.)

thanks
-- PMM

Re: [PATCH 0/2] system: Fix handling of '-serial none -serial something'

[Peter Maydell]

On Mon, 22 Jan 2024 at 16:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> (This patchset fixes a bug reported by Bohdan Kostiv at
> https://lore.kernel.org/qemu-devel/CAA3Sv1LQ8yDUNLCB5WqLVZjsHffrU0uSbL_YYJW_m+Db2PhEeQ@mail.gmail.com/
>  -- my patch 1 avoids a bug in his suggested change, and
> patch 2 is new, improving the documentation.)
>
> Currently if the user passes multiple -serial options on the command
> line, we mostly treat those as applying to the different serial
> devices in order, so that for example
>  -serial stdio -serial file:filename
> will connect the first serial port to stdio and the second to the
> named file.
>
> The exception to this is the '-serial none' serial device type.  This
> means "don't allocate this serial device", but a bug means that
> following -serial options are not correctly handled, so that
>  -serial none -serial stdio
> has the unexpected effect that stdio is connected to the first serial
> port, not the second.

I'll take this series via target-arm.next.

thanks
-- PMM