[Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device

[Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device

[Jean-Christophe Dubois]

The "chardev" property initialization might have failed (for example because
there are not enough chardevs provided by QEMU).

The serial device emulator need to be able to work with an uninitialized
(NULL) chardev device pointer.

This patch add some missing tests on the chr pointer value before
using it.

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

Changes since V1:
 * Grammar sweep on patch description
 * Added Peter's "Reviewed-by"

 hw/char/imx_serial.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index f3fbc77..383e50c 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -203,7 +203,9 @@ static uint64_t imx_serial_read(void *opaque, hwaddr offset,
             s->usr2 &= ~USR2_RDR;
             s->uts1 |= UTS1_RXEMPTY;
             imx_update(s);
-            qemu_chr_accept_input(s->chr);
+            if (s->chr) {
+                qemu_chr_accept_input(s->chr);
+            }
         }
         return c;
 
@@ -290,7 +292,9 @@ static void imx_serial_write(void *opaque, hwaddr offset,
         }
         if (value & UCR2_RXEN) {
             if (!(s->ucr2 & UCR2_RXEN)) {
-                qemu_chr_accept_input(s->chr);
+                if (s->chr) {
+                    qemu_chr_accept_input(s->chr);
+                }
             }
         }
         s->ucr2 = value & 0xffff;
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device

[Markus Armbruster]

Jean-Christophe Dubois <jcd@tribudubois.net> writes:

> The "chardev" property initialization might have failed (for example because
> there are not enough chardevs provided by QEMU).
>
> The serial device emulator need to be able to work with an uninitialized
> (NULL) chardev device pointer.
>
> This patch add some missing tests on the chr pointer value before
> using it.
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
> Changes since V1:
>  * Grammar sweep on patch description
>  * Added Peter's "Reviewed-by"
>
>  hw/char/imx_serial.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index f3fbc77..383e50c 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -203,7 +203,9 @@ static uint64_t imx_serial_read(void *opaque, hwaddr offset,
>              s->usr2 &= ~USR2_RDR;
>              s->uts1 |= UTS1_RXEMPTY;
>              imx_update(s);
> -            qemu_chr_accept_input(s->chr);
> +            if (s->chr) {
> +                qemu_chr_accept_input(s->chr);
> +            }
>          }
>          return c;
>  
> @@ -290,7 +292,9 @@ static void imx_serial_write(void *opaque, hwaddr offset,
>          }
>          if (value & UCR2_RXEN) {
>              if (!(s->ucr2 & UCR2_RXEN)) {
> -                qemu_chr_accept_input(s->chr);
> +                if (s->chr) {
> +                    qemu_chr_accept_input(s->chr);
> +                }
>              }
>          }
>          s->ucr2 = value & 0xffff;

I'm afraid this approach is inconsistent with existing practice, namely
that if a device requires a backend, and the backend is missing, the
realize() method fails.

Example: serial_realize_core() in hw/char/serial.c, used by
serial_pci_realize() for device "pci-serial", multi_serial_pci_realize()
for devices "pci-serial-2x" and "pci-serial-4x", serial_isa_realizefn()
for device "isa-serial".  The latter has a bug: when the backend is
missing, realize fails, but the actual device is created anyway.

Example: virtio_blk_device_realize() in hw/block/virtio-blk.c.

Note that NICs don't require a backend.  I guess they behave like "no
carrier" then.  net_check_clients() warns "has no peer", however.

Character devices without a backend could be changed not to require a
backend, and behave as if they had a null backend then.  Whether that's
a good user interface needs some thought.  Regardless, it should be
consistent: either all character devices do, or none.

If you can quote another character device that already does it like your
patch, then your patch doesn't create a new inconsistency, it merely
adds to an existing one.  Acceptable.

If you can't, you should either make your device behave like all the
others, or make all the others behave like yours.

Re: [Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device

[Jean-Christophe DUBOIS]

Le 01/08/2015 09:57, Markus Armbruster a écrit :
> Jean-Christophe Dubois <jcd@tribudubois.net> writes:
>
>> The "chardev" property initialization might have failed (for example because
>> there are not enough chardevs provided by QEMU).
>>
>> The serial device emulator need to be able to work with an uninitialized
>> (NULL) chardev device pointer.
>>
>> This patch add some missing tests on the chr pointer value before
>> using it.
>>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>> Changes since V1:
>>   * Grammar sweep on patch description
>>   * Added Peter's "Reviewed-by"
>>
>>   hw/char/imx_serial.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
>> index f3fbc77..383e50c 100644
>> --- a/hw/char/imx_serial.c
>> +++ b/hw/char/imx_serial.c
>> @@ -203,7 +203,9 @@ static uint64_t imx_serial_read(void *opaque, hwaddr offset,
>>               s->usr2 &= ~USR2_RDR;
>>               s->uts1 |= UTS1_RXEMPTY;
>>               imx_update(s);
>> -            qemu_chr_accept_input(s->chr);
>> +            if (s->chr) {
>> +                qemu_chr_accept_input(s->chr);
>> +            }
>>           }
>>           return c;
>>   
>> @@ -290,7 +292,9 @@ static void imx_serial_write(void *opaque, hwaddr offset,
>>           }
>>           if (value & UCR2_RXEN) {
>>               if (!(s->ucr2 & UCR2_RXEN)) {
>> -                qemu_chr_accept_input(s->chr);
>> +                if (s->chr) {
>> +                    qemu_chr_accept_input(s->chr);
>> +                }
>>               }
>>           }
>>           s->ucr2 = value & 0xffff;
> I'm afraid this approach is inconsistent with existing practice, namely
> that if a device requires a backend, and the backend is missing, the
> realize() method fails.
>
> Example: serial_realize_core() in hw/char/serial.c, used by
> serial_pci_realize() for device "pci-serial", multi_serial_pci_realize()
> for devices "pci-serial-2x" and "pci-serial-4x", serial_isa_realizefn()
> for device "isa-serial".  The latter has a bug: when the backend is
> missing, realize fails, but the actual device is created anyway.
>
> Example: virtio_blk_device_realize() in hw/block/virtio-blk.c.
>
> Note that NICs don't require a backend.  I guess they behave like "no
> carrier" then.  net_check_clients() warns "has no peer", however.
>
> Character devices without a backend could be changed not to require a
> backend, and behave as if they had a null backend then.  Whether that's
> a good user interface needs some thought.  Regardless, it should be
> consistent: either all character devices do, or none.
>
> If you can quote another character device that already does it like your
> patch, then your patch doesn't create a new inconsistency, it merely
> adds to an existing one.  Acceptable.
>
> If you can't, you should either make your device behave like all the
> others, or make all the others behave like yours.
As of today it seems to me that most serial emulators (pl011.c, 
cadence_uart.c, digic-uart.c, milkymist-uart.c, ...) are calling 
qemu_char_get_next_serial() (which is a crude method that should be 
replaced with chardev prop) to retreive their backend device. If this 
call fails (returning NULL because MAX_SERIAL_PORTS serial devices are 
already initialized/used), then qemu_chr_add_handlers() is not called 
but the realize function does not fail as very few serial drivers are 
calling error_setg(). So in effect the serial devices are running 
without backend.

This is the case for the existing i.MX serial driver. So without this 
patch qemu would crash (on a NULL pointer) when the 5th ( > 
MAX_SERIAL_PORT) serial device is initialized by the guest operating system.

if the i.MX serial driver was using error_setg() then qemu would fail to 
initialize (which certainly prevent the future crash initiated by the 
guest OS). For now the i.MX serial driver does like most other serial 
drivers and just ignore the fact that there is no backend in the 
realize() call. But it does not protect itself everywhere when the 
backend is used as a NULL pointer.

Now some SOCs (like i.MX25) have more than MAX_SERIAL_PORTS (which for 
now is set to 4). For example, the linux "device tree" file is 
advertising the 5 serial devices of the i.MX25 SOC.

So what is the solution:
  * Make sure that no more than MAX_SERIAL_PORTS serial devices are 
realized() and expect that the guest OS will cope with the missing 
devices (that should be present on the SOC)
  * Realize all devices (> MAX_SERIAL_PORTS) but allow some to have no 
backend.

Regards

JC

Re: [Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device

[Markus Armbruster]

Jean-Christophe DUBOIS <jcd@tribudubois.net> writes:

> Le 01/08/2015 09:57, Markus Armbruster a écrit :
>> Jean-Christophe Dubois <jcd@tribudubois.net> writes:
>>
>>> The "chardev" property initialization might have failed (for example because
>>> there are not enough chardevs provided by QEMU).
>>>
>>> The serial device emulator need to be able to work with an uninitialized
>>> (NULL) chardev device pointer.
>>>
>>> This patch add some missing tests on the chr pointer value before
>>> using it.
>>>
>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>
>>> Changes since V1:
>>>   * Grammar sweep on patch description
>>>   * Added Peter's "Reviewed-by"
>>>
>>>   hw/char/imx_serial.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
>>> index f3fbc77..383e50c 100644
>>> --- a/hw/char/imx_serial.c
>>> +++ b/hw/char/imx_serial.c
>>> @@ -203,7 +203,9 @@ static uint64_t imx_serial_read(void *opaque, hwaddr offset,
>>>               s->usr2 &= ~USR2_RDR;
>>>               s->uts1 |= UTS1_RXEMPTY;
>>>               imx_update(s);
>>> -            qemu_chr_accept_input(s->chr);
>>> +            if (s->chr) {
>>> +                qemu_chr_accept_input(s->chr);
>>> +            }
>>>           }
>>>           return c;
>>>   @@ -290,7 +292,9 @@ static void imx_serial_write(void *opaque,
>>> hwaddr offset,
>>>           }
>>>           if (value & UCR2_RXEN) {
>>>               if (!(s->ucr2 & UCR2_RXEN)) {
>>> -                qemu_chr_accept_input(s->chr);
>>> +                if (s->chr) {
>>> +                    qemu_chr_accept_input(s->chr);
>>> +                }
>>>               }
>>>           }
>>>           s->ucr2 = value & 0xffff;
>> I'm afraid this approach is inconsistent with existing practice, namely
>> that if a device requires a backend, and the backend is missing, the
>> realize() method fails.
>>
>> Example: serial_realize_core() in hw/char/serial.c, used by
>> serial_pci_realize() for device "pci-serial", multi_serial_pci_realize()
>> for devices "pci-serial-2x" and "pci-serial-4x", serial_isa_realizefn()
>> for device "isa-serial".  The latter has a bug: when the backend is
>> missing, realize fails, but the actual device is created anyway.
>>
>> Example: virtio_blk_device_realize() in hw/block/virtio-blk.c.
>>
>> Note that NICs don't require a backend.  I guess they behave like "no
>> carrier" then.  net_check_clients() warns "has no peer", however.
>>
>> Character devices without a backend could be changed not to require a
>> backend, and behave as if they had a null backend then.  Whether that's
>> a good user interface needs some thought.  Regardless, it should be
>> consistent: either all character devices do, or none.
>>
>> If you can quote another character device that already does it like your
>> patch, then your patch doesn't create a new inconsistency, it merely
>> adds to an existing one.  Acceptable.
>>
>> If you can't, you should either make your device behave like all the
>> others, or make all the others behave like yours.
> As of today it seems to me that most serial emulators (pl011.c,
> cadence_uart.c, digic-uart.c, milkymist-uart.c, ...) are calling
> qemu_char_get_next_serial() (which is a crude method that should be
> replaced with chardev prop)

Yes.

>                             to retreive their backend device. If this
> call fails (returning NULL because MAX_SERIAL_PORTS serial devices are
> already initialized/used),

Actually, it fails when serial_hds[] has no more backends.  By default,
it has one, and with -nodefaults, it has none.  You can add more with
-serial, up to MAX_SERIAL_PORTS.

>                            then qemu_chr_add_handlers() is not called
> but the realize function does not fail as very few serial drivers are
> calling error_setg(). So in effect the serial devices are running
> without backend.

Okay, the devices still using qemu_char_get_next_serial() appear to do
it like your patch.  Therefore, your patch doesn't create a new
inconsistency.

> This is the case for the existing i.MX serial driver. So without this
> patch qemu would crash (on a NULL pointer) when the 5th ( >
> MAX_SERIAL_PORT) serial device is initialized by the guest operating
> system.

I guess it crashes for the other serial devices as well when the backend
is missing.

> if the i.MX serial driver was using error_setg() then qemu would fail
> to initialize (which certainly prevent the future crash initiated by
> the guest OS). For now the i.MX serial driver does like most other
> serial drivers and just ignore the fact that there is no backend in
> the realize() call. But it does not protect itself everywhere when the
> backend is used as a NULL pointer.
>
> Now some SOCs (like i.MX25) have more than MAX_SERIAL_PORTS (which for
> now is set to 4). For example, the linux "device tree" file is
> advertising the 5 serial devices of the i.MX25 SOC.
>
> So what is the solution:
>  * Make sure that no more than MAX_SERIAL_PORTS serial devices are
> realized() and expect that the guest OS will cope with the missing
> devices (that should be present on the SOC)
>  * Realize all devices (> MAX_SERIAL_PORTS) but allow some to have no
> backend.

* Fix the devices to use device properties instead of serial_hd[], with
  backward-compatibility code to create devices for serial_hd[], like
  serial_hds_init() does.  MAX_SERIAL_PORTS is pretty much irrelevant
  then.

Regarding the question what a character device model can do when it
misses a backend:

* It can behave like a null backend.  Unlike the real null backend, this
  one is done by sprinking device model code with if (!s->chr)
  conditionals.  Ugh.

* It can fail realize().  Something (user or system) must configure
  backends for all mandatory onboard serial devices, or else QEMU won't
  start.

  Example: PC boards have four optional onboard devices.
  pc_basic_device_init() calls serial_hds_init(), which creates the i-th
  device iff serial_hds[i].  No device is created without a backend.

  Example: A hypothetical board with five mandatory devices.  Board code
  should set the backend to serial_hds[i] when that's non-null, or else
  set it to a null character device.  Again, no device is created
  without a backend.

I'm not objecting to either solution (I dislike the extra conditionals,
though).  I'm objecting to inconsistency, i.e. having *both* solutions.

Since the inconsistency already exists, my objection does *not* extend
to your patch.

Re: [Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device

[Peter Maydell]

On 31 July 2015 at 12:34, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
> The "chardev" property initialization might have failed (for example because
> there are not enough chardevs provided by QEMU).
>
> The serial device emulator need to be able to work with an uninitialized
> (NULL) chardev device pointer.
>
> This patch add some missing tests on the chr pointer value before
> using it.
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---

Thanks, applied to target-arm.next.

-- PMM