| Message ID | 1438342461-18967-1-git-send-email-jcd@tribudubois.net |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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;