From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZM9il-0007aR-Ob for qemu-devel@nongnu.org; Mon, 03 Aug 2015 02:59:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZM9ih-0006u1-UO for qemu-devel@nongnu.org; Mon, 03 Aug 2015 02:59:39 -0400 From: Markus Armbruster References: <1438342461-18967-1-git-send-email-jcd@tribudubois.net> <871tfno2z7.fsf@blackfin.pond.sub.org> <55BCF27F.50901@tribudubois.net> Date: Mon, 03 Aug 2015 08:59:31 +0200 In-Reply-To: <55BCF27F.50901@tribudubois.net> (Jean-Christophe DUBOIS's message of "Sat, 01 Aug 2015 18:23:27 +0200") Message-ID: <87fv40luwc.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jean-Christophe DUBOIS Cc: Peter Chubb , qemu-devel@nongnu.org, qemu-stable@nongnu.org Jean-Christophe DUBOIS writes: > Le 01/08/2015 09:57, Markus Armbruster a =C3=A9crit : >> Jean-Christophe Dubois writes: >> >>> The "chardev" property initialization might have failed (for example be= cause >>> 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 >>> Reviewed-by: Peter Crosthwaite >>> --- >>> >>> 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, hwadd= r offset, >>> s->usr2 &=3D ~USR2_RDR; >>> s->uts1 |=3D 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 =3D 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.