Re: [Qemu-devel] [PATCH 4/5] hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Wed,  3 Aug 2016 17:22:39 +0100
"Daniel P. Berrange" <berrange@redhat.com> 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 <berrange@redhat.com>
> ---
>  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) {