[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/5] hw: replace most use of qemu_chr_fe_write w
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH 4/5] hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all |
Date: |
Thu, 4 Aug 2016 10:23:39 +0200 |
On Wed, 3 Aug 2016 17:22:39 +0100
"Daniel P. Berrange" <address@hidden> wrote:
> The qemu_chr_fe_write method will return -1 on EAGAIN if the
> chardev backend write would block. Almost no callers of the
> qemu_chr_fe_write() method check the return value, instead
> blindly assuming data was successfully sent. In most cases
> this will lead to silent data loss on interactive consoles,
> but in some cases (eg RNG EGD) it'll just cause corruption
> of the protocol being spoken.
>
> We unfortunately can't fix the virtio-console code, due to
> a bug in the Linux guest drivers, which would cause the
> entire Linux kernel to hang if we delay processing of the
> incoming data in any way. Fixing this requires first fixing
> the guest driver to not hold spinlocks while writing to the
> hvc device backend.
>
> Fixes bug: https://bugs.launchpad.net/qemu/+bug/1586756
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
> backends/rng-egd.c | 4 +++-
> gdbstub.c | 4 +++-
> hw/arm/omap2.c | 8 +++++---
> hw/arm/pxa2xx.c | 4 +++-
> hw/arm/strongarm.c | 4 +++-
> hw/char/bcm2835_aux.c | 4 +++-
> hw/char/debugcon.c | 4 +++-
> hw/char/digic-uart.c | 2 ++
> hw/char/escc.c | 4 +++-
> hw/char/etraxfs_ser.c | 4 +++-
> hw/char/exynos4210_uart.c | 4 +++-
> hw/char/grlib_apbuart.c | 4 +++-
> hw/char/imx_serial.c | 4 +++-
> hw/char/ipoctal232.c | 4 +++-
> hw/char/lm32_juart.c | 2 ++
> hw/char/lm32_uart.c | 2 ++
> hw/char/mcf_uart.c | 4 +++-
> hw/char/parallel.c | 4 +++-
> hw/char/pl011.c | 4 +++-
> hw/char/sclpconsole-lm.c | 4 +++-
> hw/char/sclpconsole.c | 2 ++
ack for the sclp consoles
> hw/char/sh_serial.c | 4 +++-
> hw/char/spapr_vty.c | 5 +++--
> hw/char/stm32f2xx_usart.c | 2 ++
> hw/char/virtio-console.c | 21 +++++++++++++++++++++
> hw/char/xilinx_uartlite.c | 4 +++-
> hw/usb/ccid-card-passthru.c | 7 +++++--
> hw/usb/dev-serial.c | 4 +++-
> slirp/slirp.c | 4 +++-
> 29 files changed, 104 insertions(+), 27 deletions(-)
>
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 4f0e03d..d44c18c 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -68,6 +68,27 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
> */
> if (ret < 0)
> ret = 0;
> +
> + /* XXX we should be queuing data to send later for the
> + * console devices too rather than silently dropping
> + * console data on EAGAIN. The Linux virtio-console
> + * hvc driver though does sends with spinlocks held,
> + * so if we enable throttling that'll stall the entire
> + * guest kernel, not merely the process writing to the
> + * console.
> + *
> + * While we could queue data for later write without
> + * enabling throttling, this would result in the guest
> + * being able to trigger arbitrary memory usage in QEMU
> + * buffering data for later writes.
> + *
> + * So fixing this problem likely requires fixing the
> + * Linux virtio-console hvc driver to not hold spinlocks
> + * while writing, and instead merely block the process
> + * that's writing. QEMU would then need some way to detect
> + * if the guest had the fixed driver too, before we can
> + * use throttling on host side.
Probably best done via a new virtio-console feature bit, as this would
be OS agnostic.
> + */
> if (!k->is_console) {
> virtio_serial_throttle_port(port, true);
> if (!vcon->watch) {
- [Qemu-devel] [PATCH 0/5] Global fix / workaround usage of qemu_chr_fe_write, Daniel P. Berrange, 2016/08/03
- [Qemu-devel] [PATCH 4/5] hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all, Daniel P. Berrange, 2016/08/03
- Re: [Qemu-devel] [PATCH 4/5] hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all,
Cornelia Huck <=
- [Qemu-devel] [PATCH 1/5] virtio-console: set frontend open permanently for console devs, Daniel P. Berrange, 2016/08/03
- [Qemu-devel] [PATCH 2/5] impi: check return of qemu_chr_fe_write() for errors, Daniel P. Berrange, 2016/08/03
- [Qemu-devel] [PATCH 3/5] sclpconsole: remove bogus check for -EAGAIN, Daniel P. Berrange, 2016/08/03
- [Qemu-devel] [PATCH 5/5] char: convert qemu_chr_fe_write to qemu_chr_fe_write_all, Daniel P. Berrange, 2016/08/03