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