From 2039511b8f573165420c86380762ae829dc398d9 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 10 Jun 2014 14:55:58 +0800 Subject: [PATCH 1/5] scsi: Report error when lun number is in use In the case that the lun number is taken by another scsi device, don't release the existing device siliently, but report an error to user. Signed-off-by: Fam Zheng Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-bus.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index ea1ac09c8a..4341754253 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -177,7 +177,8 @@ static int scsi_qdev_init(DeviceState *qdev) d = scsi_device_find(bus, dev->channel, dev->id, dev->lun); assert(d); if (d->lun == dev->lun && dev != d) { - object_unparent(OBJECT(d)); + error_report("lun already used by '%s'", d->qdev.id); + goto err; } } From 7b3621f47a990c5099c6385728347f69a8d0e55c Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 11 Jul 2014 12:11:38 +0200 Subject: [PATCH 2/5] qemu-char: fix deadlock with "-monitor pty" qemu_chr_be_generic_open cannot be called with the write lock taken, because it calls client code that may call qemu_chr_fe_write. This actually happens for the monitor: 0x00007ffff27dbf79 in __GI_raise (sig=sig@entry=6) 0x00007ffff27df388 in __GI_abort () 0x00005555555ef489 in error_exit (err=, msg=msg@entry=0x5555559796d0 <__func__.5959> "qemu_mutex_lock") 0x00005555558f9080 in qemu_mutex_lock (mutex=mutex@entry=0x555556248a30) 0x0000555555713936 in qemu_chr_fe_write (s=0x555556248a30, buf=buf@entry=0x5555563d8870 "QEMU 2.0.90 monitor - type 'help' for more information\r\n", len=56) 0x00005555556217fd in monitor_flush_locked (mon=mon@entry=0x555556251fd0) 0x0000555555621a12 in monitor_flush_locked (mon=0x555556251fd0) monitor_puts (mon=mon@entry=0x555556251fd0, str=0x55555634bfa7 "", str@entry=0x55555634bf70 "QEMU 2.0.90 monitor - type 'help' for more information\n") 0x0000555555624359 in monitor_vprintf (mon=0x555556251fd0, fmt=, ap=) 0x0000555555624414 in monitor_printf (mon=, fmt=fmt@entry=0x5555559105a0 "QEMU %s monitor - type 'help' for more information\n") 0x0000555555629806 in monitor_event (opaque=0x555556251fd0, event=) 0x000055555571343c in qemu_chr_be_generic_open (s=0x555556248a30) To avoid this, defer the call to an idle callback, which will be called as soon as the main loop is re-entered. In order to simplify the cleanup and do it in one place only, change pty_chr_close to call pty_chr_state. To reproduce, run with "-monitor pty", then try to read from the slave /dev/pts/FOO that it creates. Fixes: 9005b2a7589540a3733b3abdcfbccfe7746cd1a1 Reported-by: Li Liang Reviewed-by: Fam Zheng Signed-off-by: Paolo Bonzini --- qemu-char.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/qemu-char.c b/qemu-char.c index 55e372cf32..7acc03f161 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -1089,6 +1089,7 @@ typedef struct { /* Protected by the CharDriverState chr_write_lock. */ int connected; guint timer_tag; + guint open_tag; } PtyCharDriver; static void pty_chr_update_read_handler_locked(CharDriverState *chr); @@ -1101,6 +1102,7 @@ static gboolean pty_chr_timer(gpointer opaque) qemu_mutex_lock(&chr->chr_write_lock); s->timer_tag = 0; + s->open_tag = 0; if (!s->connected) { /* Next poll ... */ pty_chr_update_read_handler_locked(chr); @@ -1203,12 +1205,26 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque) return TRUE; } +static gboolean qemu_chr_be_generic_open_func(gpointer opaque) +{ + CharDriverState *chr = opaque; + PtyCharDriver *s = chr->opaque; + + s->open_tag = 0; + qemu_chr_be_generic_open(chr); + return FALSE; +} + /* Called with chr_write_lock held. */ static void pty_chr_state(CharDriverState *chr, int connected) { PtyCharDriver *s = chr->opaque; if (!connected) { + if (s->open_tag) { + g_source_remove(s->open_tag); + s->open_tag = 0; + } remove_fd_in_watch(chr); s->connected = 0; /* (re-)connect poll interval for idle guests: once per second. @@ -1221,8 +1237,9 @@ static void pty_chr_state(CharDriverState *chr, int connected) s->timer_tag = 0; } if (!s->connected) { + g_assert(s->open_tag == 0); s->connected = 1; - qemu_chr_be_generic_open(chr); + s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr); } if (!chr->fd_in_tag) { chr->fd_in_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, @@ -1236,7 +1253,8 @@ static void pty_chr_close(struct CharDriverState *chr) PtyCharDriver *s = chr->opaque; int fd; - remove_fd_in_watch(chr); + qemu_mutex_lock(&chr->chr_write_lock); + pty_chr_state(chr, 0); fd = g_io_channel_unix_get_fd(s->fd); g_io_channel_unref(s->fd); close(fd); @@ -1244,6 +1262,7 @@ static void pty_chr_close(struct CharDriverState *chr) g_source_remove(s->timer_tag); s->timer_tag = 0; } + qemu_mutex_unlock(&chr->chr_write_lock); g_free(s); qemu_chr_be_event(chr, CHR_EVENT_CLOSED); } From f702e62a193e9ddb41cef95068717e5582b39a64 Mon Sep 17 00:00:00 2001 From: Kirill Batuzov Date: Fri, 11 Jul 2014 13:41:08 +0400 Subject: [PATCH 3/5] serial: change retry logic to avoid concurrency Whenever serial_xmit fails to transmit a byte it adds a watch that would call it again when the "line" becomes ready. This results in a retry chain: serial_xmit -> add_watch -> serial_xmit Each chain is able to transmit one character, and for every character passed to serial by the guest driver a new chain is spawned. The problem lays with the fact that a new chain is spawned even when there is one already waiting on the watch. So there can be several retry chains waiting concurrently on one "line". Every chain tries to transmit current character, so character order is not messed up. But also every chain increases retry counter (tsr_retry). If there are enough concurrent chains this counter will hit MAX_XMIT_RETRY value and the character will be dropped. To reproduce this bug you need to feed serial output to some program consuming it slowly enough. A python script from bug #1335444 description is an example of such program. This commit changes retry logic in the following way to avoid concurrency: instead of spawning a new chain for each character being transmitted spawn only one and make it transmit characters until FIFO is empty. The change consists of two parts: - add a do {} while () loop in serial_xmit (diff is a bit erratic for this part, diff -w will show actual change), - do not call serial_xmit from serial_ioport_write if there is one waiting on the watch already. This should fix another issue causing bug #1335444. Signed-off-by: Kirill Batuzov Signed-off-by: Paolo Bonzini --- hw/char/serial.c | 63 +++++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/hw/char/serial.c b/hw/char/serial.c index 54180a9cba..764e1846cd 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -223,37 +223,42 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque) { SerialState *s = opaque; - if (s->tsr_retry <= 0) { - if (s->fcr & UART_FCR_FE) { - if (fifo8_is_empty(&s->xmit_fifo)) { + do { + if (s->tsr_retry <= 0) { + if (s->fcr & UART_FCR_FE) { + if (fifo8_is_empty(&s->xmit_fifo)) { + return FALSE; + } + s->tsr = fifo8_pop(&s->xmit_fifo); + if (!s->xmit_fifo.num) { + s->lsr |= UART_LSR_THRE; + } + } else if ((s->lsr & UART_LSR_THRE)) { + return FALSE; + } else { + s->tsr = s->thr; + s->lsr |= UART_LSR_THRE; + s->lsr &= ~UART_LSR_TEMT; + } + } + + if (s->mcr & UART_MCR_LOOP) { + /* in loopback mode, say that we just received a char */ + serial_receive1(s, &s->tsr, 1); + } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { + if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY && + qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, + serial_xmit, s) > 0) { + s->tsr_retry++; return FALSE; } - s->tsr = fifo8_pop(&s->xmit_fifo); - if (!s->xmit_fifo.num) { - s->lsr |= UART_LSR_THRE; - } - } else if ((s->lsr & UART_LSR_THRE)) { - return FALSE; + s->tsr_retry = 0; } else { - s->tsr = s->thr; - s->lsr |= UART_LSR_THRE; - s->lsr &= ~UART_LSR_TEMT; + s->tsr_retry = 0; } - } - - if (s->mcr & UART_MCR_LOOP) { - /* in loopback mode, say that we just received a char */ - serial_receive1(s, &s->tsr, 1); - } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { - if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY && - qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s) > 0) { - s->tsr_retry++; - return FALSE; - } - s->tsr_retry = 0; - } else { - s->tsr_retry = 0; - } + /* Transmit another byte if it is already available. It is only + possible when FIFO is enabled and not empty. */ + } while ((s->fcr & UART_FCR_FE) && !fifo8_is_empty(&s->xmit_fifo)); s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); @@ -293,7 +298,9 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val, s->thr_ipending = 0; s->lsr &= ~UART_LSR_THRE; serial_update_irq(s); - serial_xmit(NULL, G_IO_OUT, s); + if (s->tsr_retry <= 0) { + serial_xmit(NULL, G_IO_OUT, s); + } } break; case 1: From 1f4e6a069b07869eb5fd3ab711b703b95d363652 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 14 Jul 2014 12:03:09 +0200 Subject: [PATCH 4/5] virtio-scsi: fix with -M pc-i440fx-2.0 Right now starting a machine with virtio-scsi and a <= 2.0 machine type fails with: qemu-system-x86_64: -device virtio-scsi-pci: Property .any_layout not found This is because the any_layout bit was actually never set after virtio-scsi was changed to support arbitrary layout for virtio buffers. (This was just a cleanup and a preparation for virtio 1.0; no guest actually checks the bit, but the new request parsing algorithms are tested even with old guest). Reported-by: David Gilbert Reviewed-by: David Gilbert Signed-off-by: Paolo Bonzini --- include/hw/virtio/virtio-scsi.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 0419ee4252..188a2d9144 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -178,6 +178,8 @@ typedef struct { DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128) #define DEFINE_VIRTIO_SCSI_FEATURES(_state, _feature_field) \ + DEFINE_PROP_BIT("any_layout", _state, _feature_field, \ + VIRTIO_F_ANY_LAYOUT, true), \ DEFINE_PROP_BIT("hotplug", _state, _feature_field, VIRTIO_SCSI_F_HOTPLUG, \ true), \ DEFINE_PROP_BIT("param_change", _state, _feature_field, \ From 7497bce6c2561f1215fe179e40837774f6243799 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 25 Jun 2014 20:21:37 +0200 Subject: [PATCH 5/5] serial-pci: remove memory regions from BAR before destroying them Otherwise, hot-unplug of pci-serial-2x trips the assertion in memory_region_destroy: (qemu) device_del gg (qemu) qemu-system-x86_64: /work/armbru/tmp/qemu/memory.c:1021: memory_region_destroy: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed. Aborted (core dumped) Reported-by: Markus Armbruster Reviewed-by: Markus Armbruster Signed-off-by: Paolo Bonzini --- hw/char/serial-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c index f53bb9c5d0..c133c33e7a 100644 --- a/hw/char/serial-pci.c +++ b/hw/char/serial-pci.c @@ -148,6 +148,7 @@ static void multi_serial_pci_exit(PCIDevice *dev) for (i = 0; i < pci->ports; i++) { s = pci->state + i; serial_exit_core(s); + memory_region_del_subregion(&pci->iobar, &s->io); memory_region_destroy(&s->io); g_free(pci->name[i]); }