Jean-Christophe DUBOIS <j...@tribudubois.net> writes: > 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)
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.