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

[Markus Armbruster]

Jean-Christophe DUBOIS <address@hidden> writes:

> Le 01/08/2015 09:57, Markus Armbruster a écrit :
>> Jean-Christophe Dubois <address@hidden> 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 <address@hidden>
>>> Reviewed-by: Peter Crosthwaite <address@hidden>
>>> ---
>>>
>>> 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.