]> git.proxmox.com Git - mirror_qemu.git/commitdiff
hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all
authorDaniel P. Berrange <berrange@redhat.com>
Tue, 6 Sep 2016 13:56:04 +0000 (14:56 +0100)
committerPaolo Bonzini <pbonzini@redhat.com>
Tue, 13 Sep 2016 17:09:42 +0000 (19:09 +0200)
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>
Message-Id: <1473170165-540-4-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
29 files changed:
backends/rng-egd.c
gdbstub.c
hw/arm/omap2.c
hw/arm/pxa2xx.c
hw/arm/strongarm.c
hw/char/bcm2835_aux.c
hw/char/debugcon.c
hw/char/digic-uart.c
hw/char/escc.c
hw/char/etraxfs_ser.c
hw/char/exynos4210_uart.c
hw/char/grlib_apbuart.c
hw/char/imx_serial.c
hw/char/ipoctal232.c
hw/char/lm32_juart.c
hw/char/lm32_uart.c
hw/char/mcf_uart.c
hw/char/parallel.c
hw/char/pl011.c
hw/char/sclpconsole-lm.c
hw/char/sclpconsole.c
hw/char/sh_serial.c
hw/char/spapr_vty.c
hw/char/stm32f2xx_usart.c
hw/char/virtio-console.c
hw/char/xilinx_uartlite.c
hw/usb/ccid-card-passthru.c
hw/usb/dev-serial.c
slirp/slirp.c

index 7a1b9242d8d322cf81f7a68892d77d2320579d68..ba17c075cf189471da37feca8ba02d905513cc0b 100644 (file)
@@ -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;
     }
index 5da66f1794173740e5f85071a44d76725e18a89b..ecea8c42cffd871544b05f3097f3b431a3f797e7 100644 (file)
--- 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
 }
 
index 3a0d77714a257a89fbece047968cec4fe83ac923..7e11c65cbaee1f4861daa5aaaf8e022240f9f5d2 100644 (file)
@@ -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);
     }
 }
 
index cb55704687d5a63297d324274f3b1242a7e60a93..0241e07d8410351571d67fdcbd0d3f6b4f8c3bf3 100644 (file)
@@ -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);
index f1b2c6c9665c20c8bb60767c5055e000bc4d8f96..021cbf9a0f13904b956de8e4dfab7dc1ba59def6 100644 (file)
@@ -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;
index 319f1652f68f4a2461489c521747e3c62ebdc536..f7a845d3e2ce614bbe9f80a975bf352e5298bf95 100644 (file)
@@ -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;
 
index e7f025ec671807ad33c50f054fc02e44655afd1e..4402033861fe2e05b7a74cac9a7383764b7be081 100644 (file)
@@ -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);
 }
 
 
index c7604e6766aaed23ac1e7425912433893d7bc33b..e96a9b2d8d8b6fc315dfdedfa47b15301c223426 100644 (file)
@@ -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;
index 31a5f902f99546fa8510baa70506f2741cd0ff3e..aa1739762bb59c494bb4a92d585398630219d048 100644 (file)
@@ -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);
             }
index 04ca04fe2cf554e7340b91bb12cc90bc4a3d35df..c99cc5d13050204492829c9ba50dcd15cbb9f7ed 100644 (file)
@@ -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;
index 885ecc027b231a5cafdf9d740215a221a9c5999b..1107578138d24dfe8453cee00220965882be1d81 100644 (file)
@@ -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
index 871524c82f56dfec2c39af49c794c1af1b39315d..778148a15e38bd5a733f755aef0df43a5dfabdb5 100644 (file)
@@ -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);
index 44856d671e75e672e3c7bbd89a2ce4d06ddbdf40..5c3fa61e4c4bf9bf62ec8640acbff7ad1f8fa16a 100644 (file)
@@ -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);
index 9ead32af60010806b9a9bb7b04e2355facefaff1..2859fdd7fbfee698e6b1c81c49cabf49969f324f 100644 (file)
@@ -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);
index 28c2cf702d3a757e5d82baaae508c0bb1d8cc550..cb1ac76731dcf3fab213ae284d8684e9f071d777 100644 (file)
@@ -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);
     }
 }
index b5c760dda3ca779e752e4f933f72e250fd501af3..be93697a393696408819bb2e2c9149807437fc5f 100644 (file)
@@ -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;
index 3c0438fd79150488c110cc78be04d961456ef69c..c184859c837d2eea5b2240d197662d4517b043c1 100644 (file)
@@ -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) {
index fa085667ff05dbcf879794411cb1e29f56c59d25..da22e363560279b85ff20c6ab20ca7f46921feb6 100644 (file)
@@ -129,7 +129,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;
index c0fbf8a87428c564551b7baa9653a434f4ca82c1..786e605fdd6166b3703726e0335179bed3fa6a70 100644 (file)
@@ -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;
index dbe753147b0d6653f5a93039d17af3dea3b3bb12..9a563269e639b0f9ad53816eeeec55e40a174b9b 100644 (file)
@@ -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);
     }
 }
 
index d22464826b65ef46eaf5482287e76ff4f761ee09..a75ad4f60adfa6becde015cf47875621da54a944 100644 (file)
@@ -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);
 }
 
index 4c55dcb7dc8a0f993a028a85448ad78f3464e8ea..97ce5629a49043b0f4cd87bb6e6961e499c052b7 100644 (file)
@@ -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;
index 3498d7b05202a838b6748b60779d032a9bfb3229..9aeafc0c42b3297b6ccdb3696f4b86ec3ea8f0a8 100644 (file)
@@ -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)
index 15657abda9f687ee60c2f5a733effd9a439f2eab..4c6640dbe91829a6f028911144bb9686f8df9631 100644 (file)
@@ -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;
index 4f0e03d3b70f5fc492127d1497bfdc647913ac45..d44c18c12818bdf12415546eb6eb397d8b0e1274 100644 (file)
@@ -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) {
index 4847efb29f612cde3ce6f60ab72d8beed26f5594..3766dc2c5b5c081d7cf7ce8e5034275466a005db 100644 (file)
@@ -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;
 
index c0e90e501c996bd510ee22a3dbcc7da0a9712757..2eacea72f3f3ecf9a0056b647c9285ebb35757d7 100644 (file)
@@ -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,
index ba8538e60ea110e6f802d197b29cc0ef3ded8213..966ad84b90154766a2b637ec83561dff81cffedf 100644 (file)
@@ -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;
index d67eda12f4774e5b14dab9ee8e137a6048335501..6e2b4e5a90108000ebc8db55a8e9347f96f76f2e 100644 (file)
@@ -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;
     }