[Qemu-devel] [PATCH 0/2] hw/superio: Fix inconsistent use of Chardev->be

[Qemu-devel] [PATCH 0/2] hw/superio: Fix inconsistent use of Chardev->be

[Philippe Mathieu-Daudé]

My commit cd9526ab7c0 break something in the chardev frontend (serial).
Also 4c3119a6e3e does the same with the parallel device.

Fix the broken code and restore Marc-André patch which fixes the ctrl-a+b.
Tested with my usual setup.

Philippe Mathieu-Daudé (2):
  hw/isa/superio: Fix inconsistent use of Chardev->be
  Revert "Revert "mux: fix ctrl-a b again"" again

 chardev/char-mux.c   | 1 +
 hw/isa/isa-superio.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Philippe Mathieu-Daudé]

4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent
use of Chardev->be. Also, this CharBackend member is private and is
not supposed to be accessible.

Fix it by removing the inconsistent check.

Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/isa-superio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index b95608a003..08afe44731 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
         if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) {
             /* FIXME use a qdev chardev prop instead of parallel_hds[] */
             chr = parallel_hds[i];
-            if (chr == NULL || chr->be) {
+            if (chr == NULL) {
                 name = g_strdup_printf("discarding-parallel%d", i);
                 chr = qemu_chr_new(name, "null");
             } else {
@@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
         if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) {
             /* FIXME use a qdev chardev prop instead of serial_hds[] */
             chr = serial_hds[i];
-            if (chr == NULL || chr->be) {
+            if (chr == NULL) {
                 name = g_strdup_printf("discarding-serial%d", i);
                 chr = qemu_chr_new(name, "null");
             } else {
-- 
2.17.0

Re: [Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Peter Maydell]

On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent
> use of Chardev->be. Also, this CharBackend member is private and is
> not supposed to be accessible.
>
> Fix it by removing the inconsistent check.
>
> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/isa/isa-superio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
> index b95608a003..08afe44731 100644
> --- a/hw/isa/isa-superio.c
> +++ b/hw/isa/isa-superio.c
> @@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
>          if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) {
>              /* FIXME use a qdev chardev prop instead of parallel_hds[] */
>              chr = parallel_hds[i];
> -            if (chr == NULL || chr->be) {
> +            if (chr == NULL) {
>                  name = g_strdup_printf("discarding-parallel%d", i);
>                  chr = qemu_chr_new(name, "null");
>              } else {
> @@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
>          if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) {
>              /* FIXME use a qdev chardev prop instead of serial_hds[] */
>              chr = serial_hds[i];
> -            if (chr == NULL || chr->be) {
> +            if (chr == NULL) {
>                  name = g_strdup_printf("discarding-serial%d", i);
>                  chr = qemu_chr_new(name, "null");
>              } else {

You should not need to create fake null devices like this. The
device using the chardev should just cope with having a NULL
pointer now we have commit 12051d82f004024.

Also consider having cc:stable on this patchset?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Philippe Mathieu-Daudé]

Hi Peter,

On 04/20/2018 05:43 AM, Peter Maydell wrote:
> On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent
>> use of Chardev->be. Also, this CharBackend member is private and is
>> not supposed to be accessible.
>>
>> Fix it by removing the inconsistent check.
>>
>> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/isa/isa-superio.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
>> index b95608a003..08afe44731 100644
>> --- a/hw/isa/isa-superio.c
>> +++ b/hw/isa/isa-superio.c
>> @@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
>>          if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) {
>>              /* FIXME use a qdev chardev prop instead of parallel_hds[] */
>>              chr = parallel_hds[i];
>> -            if (chr == NULL || chr->be) {
>> +            if (chr == NULL) {
>>                  name = g_strdup_printf("discarding-parallel%d", i);
>>                  chr = qemu_chr_new(name, "null");
>>              } else {
>> @@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)
>>          if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) {
>>              /* FIXME use a qdev chardev prop instead of serial_hds[] */
>>              chr = serial_hds[i];
>> -            if (chr == NULL || chr->be) {
>> +            if (chr == NULL) {
>>                  name = g_strdup_printf("discarding-serial%d", i);
>>                  chr = qemu_chr_new(name, "null");
>>              } else {
> 
> You should not need to create fake null devices like this. The
> device using the chardev should just cope with having a NULL
> pointer now we have commit 12051d82f004024.

I am trying to model a SoC with 4 uarts, and started using the current
pattern:

  for (i = 0; i < MAX_SERIAL_PORTS; i+++) {
    if (serial_hds[i]) {
      serial_mm_init(..., serial_hds[i], ...);

Then the test firmware failed due to unassigned access after rebasing,
due to:

  commit ed860129acd3fcd0b1e47884e810212aaca4d21b
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   Thu Sep 7 13:54:54 2017 +0100

    boards.h: Define new flag ignore_memory_transaction_failures

    Define a new MachineClass field ignore_memory_transaction_failures.
    If this is flag is true then the CPU will ignore memory transaction
    failures which should cause the CPU to take an exception due to an
    access to an unassigned physical address; the transaction will
    instead return zero (for a read) or be ignored (for a write).  This
    should be set only by legacy board models which rely on the old
    RAZ/WI behaviour for handling devices that QEMU does not yet model.
    New board models should instead use "unimplemented-device" for all
    memory ranges where the guest will attempt to probe for a device that
    QEMU doesn't implement and a stub device is required.

    We need this for ARM boards, where we're about to implement support for
    generating external aborts on memory transaction failures. Too many
    of our legacy board models rely on the RAZ/WI behaviour and we
    would break currently working guests when their "probe for device"
    code provoked an external abort rather than a RAZ.

Which is fine, I understand new boards shouldn't use the
ignore_memory_transaction_failures flag.

The chardev subsystem is still confuse to me.
I think than checking if a chardev backend is connected to instantiate
and mmap a device and his irqs is incorrect, since boards/SoCs always
come with the same hardware.

Indeed previous to 12051d82f004024, qemu_chr_fe_init(s=NULL) was doing

     if (CHARDEV_IS_MUX(s)) {
         ...
     } else {
         s->be = b; /* NULL deref... */
     }

Which seems the original reason of my bogus "if (chr == NULL || chr->be)
..." check.

Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll
work on simplifying and fixing this.

> Also consider having cc:stable on this patchset?

Once we get a consensus on the series, OK.

Thanks,

Phil.

Re: [Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Peter Maydell]

On 20 April 2018 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 04/20/2018 05:43 AM, Peter Maydell wrote:
>> On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent
>>> use of Chardev->be. Also, this CharBackend member is private and is
>>> not supposed to be accessible.

>> You should not need to create fake null devices like this. The
>> device using the chardev should just cope with having a NULL
>> pointer now we have commit 12051d82f004024.
>
> I am trying to model a SoC with 4 uarts, and started using the current
> pattern:
>
>   for (i = 0; i < MAX_SERIAL_PORTS; i+++) {
>     if (serial_hds[i]) {
>       serial_mm_init(..., serial_hds[i], ...);

This is wrong, because it means that the UART device will
only be visible to the guest if the user connected it to
something (with -serial whatever). You should always create
the same number of devices, which correspond to what
hard wired UARTs the board has.

(The exception would I guess be if the devices were pluggable,
so each serial device was a pluggable PCI UART or something
weird; in that case there's an argument that -serial means
"create and plug in one of these devices". x86 is probably
also different for legacy reasons.)

> Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll
> work on simplifying and fixing this.

Correct. There are a bunch of dubious workarounds in the
codebase which try to handle the problem at different levels,
but we've decided that the right thing is for the NULL pointer
to be dealt with in one place at the bottom (in the qemu_chr_fe_*
functions).

Incidentally I was wondering if we could sensibly get rid of
the compile time MAX_SERIAL_PORTS. Right now there's no way
to model a device with five UARTs such that they can all
be sent to interesting places. I think there's no reason
for this except that the PC traditionally has 4 UARTs and
nobody's ever changed the code...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Philippe Mathieu-Daudé]

On 04/20/2018 09:59 AM, Peter Maydell wrote:
> On 20 April 2018 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 04/20/2018 05:43 AM, Peter Maydell wrote:
>>> On 19 April 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> 4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent
>>>> use of Chardev->be. Also, this CharBackend member is private and is
>>>> not supposed to be accessible.
> 
>>> You should not need to create fake null devices like this. The
>>> device using the chardev should just cope with having a NULL
>>> pointer now we have commit 12051d82f004024.
>>
>> I am trying to model a SoC with 4 uarts, and started using the current
>> pattern:
>>
>>   for (i = 0; i < MAX_SERIAL_PORTS; i+++) {
>>     if (serial_hds[i]) {
>>       serial_mm_init(..., serial_hds[i], ...);
> 
> This is wrong, because it means that the UART device will
> only be visible to the guest if the user connected it to
> something (with -serial whatever). You should always create
> the same number of devices, which correspond to what
> hard wired UARTs the board has.

Agreed!

> (The exception would I guess be if the devices were pluggable,
> so each serial device was a pluggable PCI UART or something
> weird; in that case there's an argument that -serial means
> "create and plug in one of these devices". x86 is probably
> also different for legacy reasons.)

Good example, also USB uarts.

>> Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll
>> work on simplifying and fixing this.
> 
> Correct. There are a bunch of dubious workarounds in the
> codebase which try to handle the problem at different levels,
> but we've decided that the right thing is for the NULL pointer
> to be dealt with in one place at the bottom (in the qemu_chr_fe_*
> functions).

OK, I'll happily clean that :)

> Incidentally I was wondering if we could sensibly get rid of
> the compile time MAX_SERIAL_PORTS. Right now there's no way
> to model a device with five UARTs such that they can all
> be sent to interesting places. I think there's no reason
> for this except that the PC traditionally has 4 UARTs and
> nobody's ever changed the code...

Yes, I thought the same and planned to relax this limit (and few others)
in my "remove i386/pc dependency from non-PC world" series...

Re: [Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Peter Maydell]

On 20 April 2018 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 04/20/2018 09:59 AM, Peter Maydell wrote:
>> Incidentally I was wondering if we could sensibly get rid of
>> the compile time MAX_SERIAL_PORTS. Right now there's no way
>> to model a device with five UARTs such that they can all
>> be sent to interesting places. I think there's no reason
>> for this except that the PC traditionally has 4 UARTs and
>> nobody's ever changed the code...
>
> Yes, I thought the same and planned to relax this limit (and few others)
> in my "remove i386/pc dependency from non-PC world" series...

It seems fairly straightforward so I've put some patches
together, might be able to get them onto the list this
afternoon.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Philippe Mathieu-Daudé]

>>> On 04/20/2018 05:43 AM, Peter Maydell wrote:
>>> Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll
>>> work on simplifying and fixing this.
>>
>> Correct. There are a bunch of dubious workarounds in the
>> codebase which try to handle the problem at different levels,
>> but we've decided that the right thing is for the NULL pointer
>> to be dealt with in one place at the bottom (in the qemu_chr_fe_*
>> functions).
> 
> OK, I'll happily clean that :)

Hmmm removing the "if (serial_hds[0])" check I reach:

void serial_realize_core(SerialState *s, Error **errp)
{
    if (!qemu_chr_fe_backend_connected(&s->chr)) {
        error_setg(errp, "Can't create serial device, empty char device");
        return;
    }

Re: [Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Peter Maydell]

On 20 April 2018 at 15:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> On 04/20/2018 05:43 AM, Peter Maydell wrote:
>>>> Since we can now safely call serial_mm_init(..., chr=NULL, ...), I'll
>>>> work on simplifying and fixing this.
>>>
>>> Correct. There are a bunch of dubious workarounds in the
>>> codebase which try to handle the problem at different levels,
>>> but we've decided that the right thing is for the NULL pointer
>>> to be dealt with in one place at the bottom (in the qemu_chr_fe_*
>>> functions).
>>
>> OK, I'll happily clean that :)
>
> Hmmm removing the "if (serial_hds[0])" check I reach:
>
> void serial_realize_core(SerialState *s, Error **errp)
> {
>     if (!qemu_chr_fe_backend_connected(&s->chr)) {
>         error_setg(errp, "Can't create serial device, empty char device");
>         return;
>     }

Yeah. That check is unnecessary; nothing fails to work if it is
removed. I've got a patch deleting it in my "drop MAX_SERIAL_PORTS"
series.

thanks
-- PMM

[Qemu-devel] [PATCH 2/2] Revert "Revert "mux: fix ctrl-a b again"" again

[Philippe Mathieu-Daudé]

This reverts commit 6f660996f1623034344cc37a1d430099067b755b to
reintroduce commit 1b2503fcf7b5932c5a3779ca2ceb92bd403c4ee7.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 chardev/char-mux.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 1b925c8dec..6055e76293 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
     }
 
     d->focus = focus;
+    chr->be = d->backends[focus];
     mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
 }
 
-- 
2.17.0

Re: [Qemu-devel] [PATCH 2/2] Revert "Revert "mux: fix ctrl-a b again"" again

[Marc-André Lureau]

Hi

On Fri, Apr 20, 2018 at 12:09 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> This reverts commit 6f660996f1623034344cc37a1d430099067b755b to
> reintroduce commit 1b2503fcf7b5932c5a3779ca2ceb92bd403c4ee7.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I think we agreed this would go after 2.12.

I was going to send an updated patch with additional tests. I can take
your patch for the next series perhaps, instead of doing the double
revert ;)

> ---
>  chardev/char-mux.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 1b925c8dec..6055e76293 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
>      }
>
>      d->focus = focus;
> +    chr->be = d->backends[focus];
>      mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
>  }
>
> --
> 2.17.0
>