[Qemu-devel] [PATCH 0/5] Global fix / workaround usage of qemu_chr_fe_write

[Qemu-devel] [PATCH 0/5] Global fix / workaround usage of qemu_chr_fe_write

[Daniel P. Berrange]

This series does a global fix and/or workaround for bad usage of
qemu_chr_fe_write(). Essentially only about 4/5 places in the code
got the usage correct, by handling errors. Everything else would
silently loose data if used with a chardev backend that can return
EAGAIN. One specific instance of that bug was highlighted when I
fixed the socket backend, such that in connect mode, it would be
non-blocking. https://bugs.launchpad.net/qemu/+bug/1586756

The "correct" fix is to check for short writes, or EAGAIN and
then schedule an event callback to re-try the write later. A
couple of places do that correctly (for example hw/char/serial.c
and hw/char/virtioconsole.c).  Changing existing serial port
impls todo this though is a fair amount of amount, so as a
temporary fix, this series changes all the broken code to simply
use qemu_chr_fe_write_all() instead. Thus we at least stop
silently loosing data, albeit at the cost of blocking the guest
execution while we wait. So we still need to do a proper fix
in all these devices models at some point, hence I've left
comments in the code as a reminder.

This series also includes a patch to virtio-console I sent
previously, but with a fixed commit message that more accurately
describes what is going on.

Daniel P. Berrange (5):
  virtio-console: set frontend open permanently for console devs
  impi: check return of qemu_chr_fe_write() for errors
  sclpconsole: remove bogus check for -EAGAIN
  hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all
  char: convert qemu_chr_fe_write to qemu_chr_fe_write_all

 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    | 22 ++++++----------------
 hw/char/sclpconsole.c       |  2 ++
 hw/char/sh_serial.c         |  4 +++-
 hw/char/spapr_vty.c         |  5 +++--
 hw/char/stm32f2xx_usart.c   |  2 ++
 hw/char/virtio-console.c    | 46 +++++++++++++++++++++++++++++++++++++++++----
 hw/char/xilinx_uartlite.c   |  4 +++-
 hw/ipmi/ipmi_bmc_extern.c   |  8 ++++++--
 hw/usb/ccid-card-passthru.c |  7 +++++--
 hw/usb/dev-serial.c         |  4 +++-
 qemu-char.c                 | 18 ++++++++++++------
 slirp/slirp.c               |  4 +++-
 31 files changed, 146 insertions(+), 54 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/5] virtio-console: set frontend open permanently for console devs

[Daniel P. Berrange]

The virtio-console.c file handles both serial consoles
and interactive consoles, since they're backed by the
same device model.

Since serial devices are expected to be reliable and
need to notify the guest when the backend is opened
or closed, the virtio-console.c file wires up support
for chardev events. This affects both serial consoles
and interactive consoles, using a network connection
based chardev backend such as 'socket', but not when
using a PTY based backend or plain 'file' backends.

When the host side is not connected the handle_output()
method in virtio-serial-bus.c will drop any data sent
by the guest, before it even reaches the virtio-console.c
code. This means that if the chardev has a logfile
configured, the data will never get logged.

Consider for example, configuring a x86_64 guest with a
plain UART serial port

  -chardev socket,id=charserial1,host=127.0.0.1,port=9001,server,nowait,logfile=console1.log,logappend=on
  -device isa-serial,chardev=charserial1,id=serial1

vs a s390 guest which has to use the virtio-console port

  -chardev socket,id=charconsole1,host=127.0.0.1,port=9000,server,nowait,logfile=console2.log,logappend=on
  -device virtconsole,chardev=charconsole1,id=console1

The isa-serial one gets data written to the log regardless
of whether a client is connected, while the virtioconsole
one only gets data written to the log when a client is
connected.

There is no need for virtio-serial-bus.c to aggressively
drop the data for console devices, as the chardev code is
prefectly capable of discarding the data itself.

So this patch changes virtconsole devices so that they
are always marked as having the host side open. This
ensures that the guest OS will always send any data it
has (Linux virtio-console hvc driver actually ignores
the host open state and sends data regardless, but we
should not rely on that), and also prevents the
virtio-serial-bus code prematurely discarding data.

The behaviour of virtserialport devices is *not* changed,
only virtconsole, because for the former, it is important
that the guest OSknow exactly when the host side is opened
/ closed so it can do any protocol re-negotiation that may
be required.

Fixes bug: https://bugs.launchpad.net/qemu/+bug/1599214

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 hw/char/virtio-console.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 2e36481..4f0e03d 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -85,8 +85,9 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
 {
     VirtConsole *vcon = VIRTIO_CONSOLE(port);
     DeviceState *dev = DEVICE(port);
+    VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
-    if (vcon->chr) {
+    if (vcon->chr && !k->is_console) {
         qemu_chr_fe_set_open(vcon->chr, guest_connected);
     }
 
@@ -156,9 +157,25 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
     }
 
     if (vcon->chr) {
-        vcon->chr->explicit_fe_open = 1;
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
+        /*
+         * For consoles we don't block guest data transfer just
+         * because nothing is connected - we'll just let it go
+         * whetherever the chardev wants - /dev/null probably.
+         *
+         * For serial ports we need 100% reliable data transfer
+         * so we use the opened/closed signals from chardev to
+         * trigger open/close of the device
+         */
+        if (k->is_console) {
+            vcon->chr->explicit_fe_open = 0;
+            qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
+                                  NULL, vcon);
+            virtio_serial_open(port);
+        } else {
+            vcon->chr->explicit_fe_open = 1;
+            qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
+                                  chr_event, vcon);
+        }
     }
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 2/5] impi: check return of qemu_chr_fe_write() for errors

[Daniel P. Berrange]

The continue_send() method in ipmi_bmc_extern.c directly
assigns the return value of qemu_chr_fe_write() to the
variable tracking the I/O buffer offset. This ignores the
possibility that the return value could be -1 and so will
cause I/O go backwards on EAGAIN. Fortunately 'outpos' is
unsigned, so can't go negative - it will become MAX_INT
which will cause the loop to stop, and avoid an accidental
out of bounds array access.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 hw/ipmi/ipmi_bmc_extern.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index 157879e..5d5b4e4 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -100,12 +100,16 @@ ipmb_checksum(const unsigned char *data, int size, unsigned char start)
 
 static void continue_send(IPMIBmcExtern *ibe)
 {
+    int ret;
     if (ibe->outlen == 0) {
         goto check_reset;
     }
  send:
-    ibe->outpos += qemu_chr_fe_write(ibe->chr, ibe->outbuf + ibe->outpos,
-                                     ibe->outlen - ibe->outpos);
+    ret = qemu_chr_fe_write(ibe->chr, ibe->outbuf + ibe->outpos,
+                            ibe->outlen - ibe->outpos);
+    if (ret > 0) {
+        ibe->outpos += ret;
+    }
     if (ibe->outpos < ibe->outlen) {
         /* Not fully transmitted, try again in a 10ms */
         timer_mod_ns(ibe->extern_timer,
-- 
2.7.4

[Qemu-devel] [PATCH 3/5] sclpconsole: remove bogus check for -EAGAIN

[Daniel P. Berrange]

The write_console_data() method in sclpconsole-lm.c checks
whether the return value of qemu_chr_fe_write() has the
value of -EAGAIN and if so then increments the buffer offset
by the value of EAGAIN. Fortunately qemu_chr_fe_write() will
never return EAGAIN directly, rather it returns -1 with
errno set to EAGAIN, so this broken code path was not
reachable. The behaviour on EAGAIN was stil bad though,
causing the write_console_data() to busy_wait repeatedly
calling qemu_chr_fe_write() with no sleep between iters.

Just remove all this loop logic and replace with a call
to qemu_chr_fe_write_all().

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 hw/char/sclpconsole-lm.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
index a22ad8d..8cb0026 100644
--- a/hw/char/sclpconsole-lm.c
+++ b/hw/char/sclpconsole-lm.c
@@ -201,21 +201,9 @@ static int write_console_data(SCLPEvent *event, const uint8_t *buf, int len)
         return len;
     }
 
-    buf_offset = buf;
-    while (len > 0) {
-        ret = qemu_chr_fe_write(scon->chr, buf, len);
-        if (ret == 0) {
-            /* a pty doesn't seem to be connected - no error */
-            len = 0;
-        } else if (ret == -EAGAIN || (ret > 0 && ret < len)) {
-            len -= ret;
-            buf_offset += ret;
-        } else {
-            len = 0;
-        }
-    }
-
-    return ret;
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    return qemu_chr_fe_write_all(scon->chr, buf, len);
 }
 
 static int process_mdb(SCLPEvent *event, MDBO *mdbo)
-- 
2.7.4

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

[Daniel P. Berrange]

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 ++
 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/backends/rng-egd.c b/backends/rng-egd.c
index 7a1b924..ba17c07 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -41,7 +41,9 @@ static void rng_egd_request_entropy(RngBackend *b, RngRequest *req)
         header[0] = 0x02;
         header[1] = len;
 
-        qemu_chr_fe_write(s->chr, header, sizeof(header));
+        /* XXX this blocks entire thread. Rewrite to use
+         * qemu_chr_fe_write and background I/O callbacks */
+        qemu_chr_fe_write_all(s->chr, header, sizeof(header));
 
         size -= len;
     }
diff --git a/gdbstub.c b/gdbstub.c
index 5da66f1..ecea8c4 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -402,7 +402,9 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int len)
         }
     }
 #else
-    qemu_chr_fe_write(s->chr, buf, len);
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(s->chr, buf, len);
 #endif
 }
 
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index 3a0d777..7e11c65 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -769,14 +769,16 @@ static void omap_sti_fifo_write(void *opaque, hwaddr addr,
 
     if (ch == STI_TRACE_CONTROL_CHANNEL) {
         /* Flush channel <i>value</i>.  */
-        qemu_chr_fe_write(s->chr, (const uint8_t *) "\r", 1);
+        /* XXX this blocks entire thread. Rewrite to use
+         * qemu_chr_fe_write and background I/O callbacks */
+        qemu_chr_fe_write_all(s->chr, (const uint8_t *) "\r", 1);
     } else if (ch == STI_TRACE_CONSOLE_CHANNEL || 1) {
         if (value == 0xc0 || value == 0xc3) {
             /* Open channel <i>ch</i>.  */
         } else if (value == 0x00)
-            qemu_chr_fe_write(s->chr, (const uint8_t *) "\n", 1);
+            qemu_chr_fe_write_all(s->chr, (const uint8_t *) "\n", 1);
         else
-            qemu_chr_fe_write(s->chr, &byte, 1);
+            qemu_chr_fe_write_all(s->chr, &byte, 1);
     }
 }
 
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index cb55704..0241e07 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1903,7 +1903,9 @@ static void pxa2xx_fir_write(void *opaque, hwaddr addr,
         else
             ch = ~value;
         if (s->chr && s->enable && (s->control[0] & (1 << 3)))	/* TXE */
-            qemu_chr_fe_write(s->chr, &ch, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->chr, &ch, 1);
         break;
     case ICSR0:
         s->status[0] &= ~(value & 0x66);
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index f1b2c6c..021cbf9 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -1108,7 +1108,9 @@ static void strongarm_uart_tx(void *opaque)
     if (s->utcr3 & UTCR3_LBM) /* loopback */ {
         strongarm_uart_receive(s, &s->tx_fifo[s->tx_start], 1);
     } else if (s->chr) {
-        qemu_chr_fe_write(s->chr, &s->tx_fifo[s->tx_start], 1);
+        /* XXX this blocks entire thread. Rewrite to use
+         * qemu_chr_fe_write and background I/O callbacks */
+        qemu_chr_fe_write_all(s->chr, &s->tx_fifo[s->tx_start], 1);
     }
 
     s->tx_start = (s->tx_start + 1) % 8;
diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
index 319f165..f7a845d 100644
--- a/hw/char/bcm2835_aux.c
+++ b/hw/char/bcm2835_aux.c
@@ -169,7 +169,9 @@ static void bcm2835_aux_write(void *opaque, hwaddr offset, uint64_t value,
         /* "DLAB bit set means access baudrate register" is NYI */
         ch = value;
         if (s->chr) {
-            qemu_chr_fe_write(s->chr, &ch, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->chr, &ch, 1);
         }
         break;
 
diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
index e7f025e..4402033 100644
--- a/hw/char/debugcon.c
+++ b/hw/char/debugcon.c
@@ -60,7 +60,9 @@ static void debugcon_ioport_write(void *opaque, hwaddr addr, uint64_t val,
     printf(" [debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02" PRIx64 "]\n", addr, val);
 #endif
 
-    qemu_chr_fe_write(s->chr, &ch, 1);
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(s->chr, &ch, 1);
 }
 
 
diff --git a/hw/char/digic-uart.c b/hw/char/digic-uart.c
index c7604e6..e96a9b2 100644
--- a/hw/char/digic-uart.c
+++ b/hw/char/digic-uart.c
@@ -77,6 +77,8 @@ static void digic_uart_write(void *opaque, hwaddr addr, uint64_t value,
     switch (addr) {
     case R_TX:
         if (s->chr) {
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
             qemu_chr_fe_write_all(s->chr, &ch, 1);
         }
         break;
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 31a5f90..aa17397 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -557,7 +557,9 @@ static void escc_mem_write(void *opaque, hwaddr addr,
         s->tx = val;
         if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
             if (s->chr)
-                qemu_chr_fe_write(s->chr, &s->tx, 1);
+                /* XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks */
+                qemu_chr_fe_write_all(s->chr, &s->tx, 1);
             else if (s->type == kbd && !s->disabled) {
                 handle_kbd_command(s, val);
             }
diff --git a/hw/char/etraxfs_ser.c b/hw/char/etraxfs_ser.c
index 04ca04f..c99cc5d 100644
--- a/hw/char/etraxfs_ser.c
+++ b/hw/char/etraxfs_ser.c
@@ -126,7 +126,9 @@ ser_write(void *opaque, hwaddr addr,
     switch (addr)
     {
         case RW_DOUT:
-            qemu_chr_fe_write(s->chr, &ch, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->chr, &ch, 1);
             s->regs[R_INTR] |= 3;
             s->pending_tx = 1;
             s->regs[addr] = value;
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 885ecc0..1107578 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -387,7 +387,9 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
             s->reg[I_(UTRSTAT)] &= ~(UTRSTAT_TRANSMITTER_EMPTY |
                     UTRSTAT_Tx_BUFFER_EMPTY);
             ch = (uint8_t)val;
-            qemu_chr_fe_write(s->chr, &ch, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->chr, &ch, 1);
 #if DEBUG_Tx_DATA
             fprintf(stderr, "%c", ch);
 #endif
diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
index 871524c..778148a 100644
--- a/hw/char/grlib_apbuart.c
+++ b/hw/char/grlib_apbuart.c
@@ -203,7 +203,9 @@ static void grlib_apbuart_write(void *opaque, hwaddr addr,
         /* Transmit when character device available and transmitter enabled */
         if ((uart->chr) && (uart->control & UART_TRANSMIT_ENABLE)) {
             c = value & 0xFF;
-            qemu_chr_fe_write(uart->chr, &c, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(uart->chr, &c, 1);
             /* Generate interrupt */
             if (uart->control & UART_TRANSMIT_INTERRUPT) {
                 qemu_irq_pulse(uart->irq);
diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index 44856d6..5c3fa61 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -182,7 +182,9 @@ static void imx_serial_write(void *opaque, hwaddr offset,
         ch = value;
         if (s->ucr2 & UCR2_TXEN) {
             if (s->chr) {
-                qemu_chr_fe_write(s->chr, &ch, 1);
+                /* XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks */
+                qemu_chr_fe_write_all(s->chr, &ch, 1);
             }
             s->usr1 &= ~USR1_TRDY;
             imx_update(s);
diff --git a/hw/char/ipoctal232.c b/hw/char/ipoctal232.c
index 9ead32a..2859fdd 100644
--- a/hw/char/ipoctal232.c
+++ b/hw/char/ipoctal232.c
@@ -360,7 +360,9 @@ static void io_write(IPackDevice *ip, uint8_t addr, uint16_t val)
             DPRINTF("Write THR%c (0x%x)\n", channel + 'a', reg);
             if (ch->dev) {
                 uint8_t thr = reg;
-                qemu_chr_fe_write(ch->dev, &thr, 1);
+                /* XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks */
+                qemu_chr_fe_write_all(ch->dev, &thr, 1);
             }
         } else {
             DPRINTF("Write THR%c (0x%x), Tx disabled\n", channel + 'a', reg);
diff --git a/hw/char/lm32_juart.c b/hw/char/lm32_juart.c
index 28c2cf7..cb1ac76 100644
--- a/hw/char/lm32_juart.c
+++ b/hw/char/lm32_juart.c
@@ -76,6 +76,8 @@ void lm32_juart_set_jtx(DeviceState *d, uint32_t jtx)
 
     s->jtx = jtx;
     if (s->chr) {
+        /* XXX this blocks entire thread. Rewrite to use
+         * qemu_chr_fe_write and background I/O callbacks */
         qemu_chr_fe_write_all(s->chr, &ch, 1);
     }
 }
diff --git a/hw/char/lm32_uart.c b/hw/char/lm32_uart.c
index b5c760d..be93697 100644
--- a/hw/char/lm32_uart.c
+++ b/hw/char/lm32_uart.c
@@ -178,6 +178,8 @@ static void uart_write(void *opaque, hwaddr addr,
     switch (addr) {
     case R_RXTX:
         if (s->chr) {
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
             qemu_chr_fe_write_all(s->chr, &ch, 1);
         }
         break;
diff --git a/hw/char/mcf_uart.c b/hw/char/mcf_uart.c
index 3c0438f..c184859 100644
--- a/hw/char/mcf_uart.c
+++ b/hw/char/mcf_uart.c
@@ -114,7 +114,9 @@ static void mcf_uart_do_tx(mcf_uart_state *s)
 {
     if (s->tx_enabled && (s->sr & MCF_UART_TxEMP) == 0) {
         if (s->chr)
-            qemu_chr_fe_write(s->chr, (unsigned char *)&s->tb, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->chr, (unsigned char *)&s->tb, 1);
         s->sr |= MCF_UART_TxEMP;
     }
     if (s->tx_enabled) {
diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index 11c78fe..66fe60e 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -128,7 +128,9 @@ parallel_ioport_write_sw(void *opaque, uint32_t addr, uint32_t val)
             if (val & PARA_CTR_STROBE) {
                 s->status &= ~PARA_STS_BUSY;
                 if ((s->control & PARA_CTR_STROBE) == 0)
-                    qemu_chr_fe_write(s->chr, &s->dataw, 1);
+                    /* XXX this blocks entire thread. Rewrite to use
+                     * qemu_chr_fe_write and background I/O callbacks */
+                    qemu_chr_fe_write_all(s->chr, &s->dataw, 1);
             } else {
                 if (s->control & PARA_CTR_INTEN) {
                     s->irq_pending = 1;
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c0fbf8a..786e605 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -146,7 +146,9 @@ static void pl011_write(void *opaque, hwaddr offset,
         /* ??? Check if transmitter is enabled.  */
         ch = value;
         if (s->chr)
-            qemu_chr_fe_write(s->chr, &ch, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->chr, &ch, 1);
         s->int_level |= PL011_INT_TX;
         pl011_update(s);
         break;
diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
index 8cb0026..8e1d74d 100644
--- a/hw/char/sclpconsole-lm.c
+++ b/hw/char/sclpconsole-lm.c
@@ -89,7 +89,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     scon->buf[scon->length] = *buf;
     scon->length += 1;
     if (scon->echo) {
-        qemu_chr_fe_write(scon->chr, buf, size);
+        /* XXX this blocks entire thread. Rewrite to use
+         * qemu_chr_fe_write and background I/O callbacks */
+        qemu_chr_fe_write_all(scon->chr, buf, size);
     }
 }
 
diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
index d224648..a75ad4f 100644
--- a/hw/char/sclpconsole.c
+++ b/hw/char/sclpconsole.c
@@ -168,6 +168,8 @@ static ssize_t write_console_data(SCLPEvent *event, const uint8_t *buf,
         return len;
     }
 
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
     return qemu_chr_fe_write_all(scon->chr, buf, len);
 }
 
diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 4c55dcb..97ce562 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -111,7 +111,9 @@ static void sh_serial_write(void *opaque, hwaddr offs,
     case 0x0c: /* FTDR / TDR */
         if (s->chr) {
             ch = val;
-            qemu_chr_fe_write(s->chr, &ch, 1);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->chr, &ch, 1);
 	}
 	s->dr = val;
 	s->flags &= ~SH_SERIAL_FLAG_TDE;
diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index 3498d7b..9aeafc0 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -60,8 +60,9 @@ void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
 {
     VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
 
-    /* FIXME: should check the qemu_chr_fe_write() return value */
-    qemu_chr_fe_write(dev->chardev, buf, len);
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(dev->chardev, buf, len);
 }
 
 static void spapr_vty_realize(VIOsPAPRDevice *sdev, Error **errp)
diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
index 15657ab..4c6640d 100644
--- a/hw/char/stm32f2xx_usart.c
+++ b/hw/char/stm32f2xx_usart.c
@@ -153,6 +153,8 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
         if (value < 0xF000) {
             ch = value;
             if (s->chr) {
+                /* XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks */
                 qemu_chr_fe_write_all(s->chr, &ch, 1);
             }
             s->usart_sr |= USART_SR_TC;
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.
+         */
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
             if (!vcon->watch) {
diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c
index 4847efb..3766dc2 100644
--- a/hw/char/xilinx_uartlite.c
+++ b/hw/char/xilinx_uartlite.c
@@ -144,7 +144,9 @@ uart_write(void *opaque, hwaddr addr,
 
         case R_TX:
             if (s->chr)
-                qemu_chr_fe_write(s->chr, &ch, 1);
+                /* XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks */
+                qemu_chr_fe_write_all(s->chr, &ch, 1);
 
             s->regs[addr] = value;
 
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index c0e90e5..2eacea7 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -75,8 +75,11 @@ static void ccid_card_vscard_send_msg(PassthruState *s,
     scr_msg_header.type = htonl(type);
     scr_msg_header.reader_id = htonl(reader_id);
     scr_msg_header.length = htonl(length);
-    qemu_chr_fe_write(s->cs, (uint8_t *)&scr_msg_header, sizeof(VSCMsgHeader));
-    qemu_chr_fe_write(s->cs, payload, length);
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(s->cs, (uint8_t *)&scr_msg_header,
+                          sizeof(VSCMsgHeader));
+    qemu_chr_fe_write_all(s->cs, payload, length);
 }
 
 static void ccid_card_vscard_send_apdu(PassthruState *s,
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index ba8538e..966ad84 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -366,7 +366,9 @@ static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
             goto fail;
         for (i = 0; i < p->iov.niov; i++) {
             iov = p->iov.iov + i;
-            qemu_chr_fe_write(s->cs, iov->iov_base, iov->iov_len);
+            /* XXX this blocks entire thread. Rewrite to use
+             * qemu_chr_fe_write and background I/O callbacks */
+            qemu_chr_fe_write_all(s->cs, iov->iov_base, iov->iov_len);
         }
         p->actual_length = p->iov.size;
         break;
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 7eb183d..49c6c95 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1072,7 +1072,9 @@ int slirp_add_exec(Slirp *slirp, int do_pty, const void *args,
 ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags)
 {
     if (so->s == -1 && so->extra) {
-        qemu_chr_fe_write(so->extra, buf, len);
+        /* XXX this blocks entire thread. Rewrite to use
+         * qemu_chr_fe_write and background I/O callbacks */
+        qemu_chr_fe_write_all(so->extra, buf, len);
         return len;
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH 5/5] char: convert qemu_chr_fe_write to qemu_chr_fe_write_all

[Daniel P. Berrange]

The mux chardev was not checking the return value of any
qemu_chr_fe_write() call so would silently loose data
on EAGAIN.

Similarly the qemu_chr_fe_printf method would not check
errors and was not in a position to retry even if it
could check.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index a50b8fb..e8dde2d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -440,7 +440,9 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
     va_list ap;
     va_start(ap, fmt);
     vsnprintf(buf, sizeof(buf), fmt, ap);
-    qemu_chr_fe_write(s, (uint8_t *)buf, strlen(buf));
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(s, (uint8_t *)buf, strlen(buf));
     va_end(ap);
 }
 
@@ -556,7 +558,9 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
                          (secs / 60) % 60,
                          secs % 60,
                          (int)(ti % 1000));
-                qemu_chr_fe_write(d->drv, (uint8_t *)buf1, strlen(buf1));
+                /* XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks */
+                qemu_chr_fe_write_all(d->drv, (uint8_t *)buf1, strlen(buf1));
                 d->linestart = 0;
             }
             ret += qemu_chr_fe_write(d->drv, buf+i, 1);
@@ -594,13 +598,15 @@ static void mux_print_help(CharDriverState *chr)
                  "\n\rEscape-Char set to Ascii: 0x%02x\n\r\n\r",
                  term_escape_char);
     }
-    qemu_chr_fe_write(chr, (uint8_t *)cbuf, strlen(cbuf));
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(chr, (uint8_t *)cbuf, strlen(cbuf));
     for (i = 0; mux_help[i] != NULL; i++) {
         for (j=0; mux_help[i][j] != '\0'; j++) {
             if (mux_help[i][j] == '%')
-                qemu_chr_fe_write(chr, (uint8_t *)ebuf, strlen(ebuf));
+                qemu_chr_fe_write_all(chr, (uint8_t *)ebuf, strlen(ebuf));
             else
-                qemu_chr_fe_write(chr, (uint8_t *)&mux_help[i][j], 1);
+                qemu_chr_fe_write_all(chr, (uint8_t *)&mux_help[i][j], 1);
         }
     }
 }
@@ -625,7 +631,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
         case 'x':
             {
                  const char *term =  "QEMU: Terminated\n\r";
-                 qemu_chr_fe_write(chr, (uint8_t *)term, strlen(term));
+                 qemu_chr_fe_write_all(chr, (uint8_t *)term, strlen(term));
                  exit(0);
                  break;
             }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/5] virtio-console: set frontend open permanently for console devs

[Paolo Bonzini]

On 03/08/2016 18:22, Daniel P. Berrange wrote:
> The virtio-console.c file handles both serial consoles
> and interactive consoles, since they're backed by the
> same device model.
> 
> Since serial devices are expected to be reliable and
> need to notify the guest when the backend is opened
> or closed, the virtio-console.c file wires up support
> for chardev events. This affects both serial consoles
> and interactive consoles, using a network connection
> based chardev backend such as 'socket', but not when
> using a PTY based backend or plain 'file' backends.
> 
> When the host side is not connected the handle_output()
> method in virtio-serial-bus.c will drop any data sent
> by the guest, before it even reaches the virtio-console.c
> code. This means that if the chardev has a logfile
> configured, the data will never get logged.
> 
> Consider for example, configuring a x86_64 guest with a
> plain UART serial port
> 
>   -chardev socket,id=charserial1,host=127.0.0.1,port=9001,server,nowait,logfile=console1.log,logappend=on
>   -device isa-serial,chardev=charserial1,id=serial1
> 
> vs a s390 guest which has to use the virtio-console port
> 
>   -chardev socket,id=charconsole1,host=127.0.0.1,port=9000,server,nowait,logfile=console2.log,logappend=on
>   -device virtconsole,chardev=charconsole1,id=console1
> 
> The isa-serial one gets data written to the log regardless
> of whether a client is connected, while the virtioconsole
> one only gets data written to the log when a client is
> connected.
> 
> There is no need for virtio-serial-bus.c to aggressively
> drop the data for console devices, as the chardev code is
> prefectly capable of discarding the data itself.
> 
> So this patch changes virtconsole devices so that they
> are always marked as having the host side open. This
> ensures that the guest OS will always send any data it
> has (Linux virtio-console hvc driver actually ignores
> the host open state and sends data regardless, but we
> should not rely on that), and also prevents the
> virtio-serial-bus code prematurely discarding data.
> 
> The behaviour of virtserialport devices is *not* changed,
> only virtconsole, because for the former, it is important
> that the guest OSknow exactly when the host side is opened
> / closed so it can do any protocol re-negotiation that may
> be required.
> 
> Fixes bug: https://bugs.launchpad.net/qemu/+bug/1599214
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/char/virtio-console.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 2e36481..4f0e03d 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -85,8 +85,9 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
>  {
>      VirtConsole *vcon = VIRTIO_CONSOLE(port);
>      DeviceState *dev = DEVICE(port);
> +    VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>  
> -    if (vcon->chr) {
> +    if (vcon->chr && !k->is_console) {
>          qemu_chr_fe_set_open(vcon->chr, guest_connected);
>      }
>  
> @@ -156,9 +157,25 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (vcon->chr) {
> -        vcon->chr->explicit_fe_open = 1;
> -        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
> -                              vcon);
> +        /*
> +         * For consoles we don't block guest data transfer just
> +         * because nothing is connected - we'll just let it go
> +         * whetherever the chardev wants - /dev/null probably.
> +         *
> +         * For serial ports we need 100% reliable data transfer
> +         * so we use the opened/closed signals from chardev to
> +         * trigger open/close of the device
> +         */
> +        if (k->is_console) {
> +            vcon->chr->explicit_fe_open = 0;
> +            qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
> +                                  NULL, vcon);
> +            virtio_serial_open(port);
> +        } else {
> +            vcon->chr->explicit_fe_open = 1;
> +            qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
> +                                  chr_event, vcon);
> +        }
>      }
>  }
>  
> 

I think this patch should go in 2.7, perhaps patch 2 too.  Everything
else can wait for 2.8.

Paolo

Re: [Qemu-devel] [PATCH 1/5] virtio-console: set frontend open permanently for console devs

[Cornelia Huck]

On Wed,  3 Aug 2016 17:22:36 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> The virtio-console.c file handles both serial consoles
> and interactive consoles, since they're backed by the
> same device model.
> 
> Since serial devices are expected to be reliable and
> need to notify the guest when the backend is opened
> or closed, the virtio-console.c file wires up support
> for chardev events. This affects both serial consoles
> and interactive consoles, using a network connection
> based chardev backend such as 'socket', but not when
> using a PTY based backend or plain 'file' backends.
> 
> When the host side is not connected the handle_output()
> method in virtio-serial-bus.c will drop any data sent
> by the guest, before it even reaches the virtio-console.c
> code. This means that if the chardev has a logfile
> configured, the data will never get logged.
> 
> Consider for example, configuring a x86_64 guest with a
> plain UART serial port
> 
>   -chardev socket,id=charserial1,host=127.0.0.1,port=9001,server,nowait,logfile=console1.log,logappend=on
>   -device isa-serial,chardev=charserial1,id=serial1
> 
> vs a s390 guest which has to use the virtio-console port
> 
>   -chardev socket,id=charconsole1,host=127.0.0.1,port=9000,server,nowait,logfile=console2.log,logappend=on
>   -device virtconsole,chardev=charconsole1,id=console1
> 
> The isa-serial one gets data written to the log regardless
> of whether a client is connected, while the virtioconsole
> one only gets data written to the log when a client is
> connected.
> 
> There is no need for virtio-serial-bus.c to aggressively
> drop the data for console devices, as the chardev code is
> prefectly capable of discarding the data itself.
> 
> So this patch changes virtconsole devices so that they
> are always marked as having the host side open. This
> ensures that the guest OS will always send any data it
> has (Linux virtio-console hvc driver actually ignores
> the host open state and sends data regardless, but we
> should not rely on that), and also prevents the
> virtio-serial-bus code prematurely discarding data.
> 
> The behaviour of virtserialport devices is *not* changed,
> only virtconsole, because for the former, it is important
> that the guest OSknow exactly when the host side is opened
> / closed so it can do any protocol re-negotiation that may
> be required.
> 
> Fixes bug: https://bugs.launchpad.net/qemu/+bug/1599214
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/char/virtio-console.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)

From my limited understanding of the character layer this sounds
reasonable, and I agree that this is 2.7 material.

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 3/5] sclpconsole: remove bogus check for -EAGAIN

[Cornelia Huck]

On Wed,  3 Aug 2016 17:22:38 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

s/sclpconsole/sclpconsolelm/ in the subject, as there are two sclp
consoles :)

> The write_console_data() method in sclpconsole-lm.c checks
> whether the return value of qemu_chr_fe_write() has the
> value of -EAGAIN and if so then increments the buffer offset
> by the value of EAGAIN. Fortunately qemu_chr_fe_write() will
> never return EAGAIN directly, rather it returns -1 with
> errno set to EAGAIN, so this broken code path was not
> reachable. The behaviour on EAGAIN was stil bad though,
> causing the write_console_data() to busy_wait repeatedly
> calling qemu_chr_fe_write() with no sleep between iters.
> 
> Just remove all this loop logic and replace with a call
> to qemu_chr_fe_write_all().
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/char/sclpconsole-lm.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index a22ad8d..8cb0026 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -201,21 +201,9 @@ static int write_console_data(SCLPEvent *event, const uint8_t *buf, int len)
>          return len;
>      }
> 
> -    buf_offset = buf;
> -    while (len > 0) {
> -        ret = qemu_chr_fe_write(scon->chr, buf, len);
> -        if (ret == 0) {
> -            /* a pty doesn't seem to be connected - no error */
> -            len = 0;
> -        } else if (ret == -EAGAIN || (ret > 0 && ret < len)) {
> -            len -= ret;
> -            buf_offset += ret;
> -        } else {
> -            len = 0;
> -        }
> -    }
> -
> -    return ret;
> +    /* XXX this blocks entire thread. Rewrite to use
> +     * qemu_chr_fe_write and background I/O callbacks */
> +    return qemu_chr_fe_write_all(scon->chr, buf, len);

This is basically the same change that had been done for the sclp vt220
console already in 2e14211 ("s390/sclpconsole: handle char layer busy
conditions"), so this looks fine.

>  }
> 
>  static int process_mdb(SCLPEvent *event, MDBO *mdbo)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

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

[Cornelia Huck]

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) {