[Qemu-devel] [PULL for-2.1 0/5] Serial + SCSI fixes for 2014-07-14

[Qemu-devel] [PULL for-2.1 0/5] Serial + SCSI fixes for 2014-07-14

[Paolo Bonzini]

The following changes since commit ab6d3749c4915cd5692633e321f7745dce06fe77:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-vga-20140711-1' into staging (2014-07-11 17:50:38 +0100)

are available in the git repository at:


  git://github.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to 7497bce6c2561f1215fe179e40837774f6243799:

  serial-pci: remove memory regions from BAR before destroying them (2014-07-14 16:14:15 +0200)

----------------------------------------------------------------
Misc 2.1 fixes regarding character/serial devices and SCSI.

----------------------------------------------------------------

Now with signed tags. :)

Paolo

Fam Zheng (1):
      scsi: Report error when lun number is in use

Kirill Batuzov (1):
      serial: change retry logic to avoid concurrency

Paolo Bonzini (3):
      qemu-char: fix deadlock with "-monitor pty"
      virtio-scsi: fix with -M pc-i440fx-2.0
      serial-pci: remove memory regions from BAR before destroying them

 hw/char/serial-pci.c            |  1 +
 hw/char/serial.c                | 59 +++++++++++++++++++++++------------------
 hw/scsi/scsi-bus.c              |  3 ++-
 include/hw/virtio/virtio-scsi.h |  2 ++
 qemu-char.c                     | 23 ++++++++++++++--
 5 files changed, 59 insertions(+), 29 deletions(-)
-- 
1.9.3

[Qemu-devel] [PULL 1/5] scsi: Report error when lun number is in use

[Paolo Bonzini]

From: Fam Zheng <famz@redhat.com>

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 <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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 ea1ac09..4341754 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;
         }
     }
 
-- 
1.9.3

[Qemu-devel] [PULL 2/5] qemu-char: fix deadlock with "-monitor pty"

[Paolo Bonzini]

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=<optimized out>, 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=<optimized out>, ap=<optimized out>)
    0x0000555555624414 in monitor_printf (mon=<optimized out>, fmt=fmt@entry=0x5555559105a0 "QEMU %s monitor - type 'help' for more information\n")
    0x0000555555629806 in monitor_event (opaque=0x555556251fd0, event=<optimized out>)
    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 <liangx.z.li@intel.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 55e372c..7acc03f 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);
 }
-- 
1.9.3

[Qemu-devel] [PULL 3/5] serial: change retry logic to avoid concurrency

[Paolo Bonzini]

From: Kirill Batuzov <batuzovk@ispras.ru>

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 <batuzovk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 59 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 54180a9..764e184 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;
-            }
-            s->tsr = fifo8_pop(&s->xmit_fifo);
-            if (!s->xmit_fifo.num) {
+            } else {
+                s->tsr = s->thr;
                 s->lsr |= UART_LSR_THRE;
+                s->lsr &= ~UART_LSR_TEMT;
             }
-        } 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;
+        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;
         }
-        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:
-- 
1.9.3

Re: [Qemu-devel] [PULL 3/5] serial: change retry logic to avoid concurrency

[Pavel Hrdina]

On 14.7.2014 17:49, Paolo Bonzini wrote:
> From: Kirill Batuzov <batuzovk@ispras.ru>
> 
> 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 <batuzovk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


Hi, this commit introduced a regression with serial console. The issue
is that if you start a guest with serial console:

-chardev pty,id=charserial0 -device
isa-serial,chardev=charserial0,id=serial0

the guest hang during boot for a long time and I'm not even sure it
it ever boot up. The last message printed out by kernel is:

"[   0.000000] console [tty0] enabled"

If you connect to the serial console than the guest continue
booting immediately.

Pavel

Re: [Qemu-devel] [PULL 3/5] serial: change retry logic to avoid concurrency

[Paolo Bonzini]

Il 24/07/2014 14:57, Pavel Hrdina ha scritto:
> On 14.7.2014 17:49, Paolo Bonzini wrote:
>> From: Kirill Batuzov <batuzovk@ispras.ru>
>>
>> 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 <batuzovk@ispras.ru>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> 
> Hi, this commit introduced a regression with serial console. The issue
> is that if you start a guest with serial console:
> 
> -chardev pty,id=charserial0 -device
> isa-serial,chardev=charserial0,id=serial0
> 
> the guest hang during boot for a long time and I'm not even sure it
> it ever boot up. The last message printed out by kernel is:
> 
> "[   0.000000] console [tty0] enabled"
> 
> If you connect to the serial console than the guest continue
> booting immediately.

Interesting, the patch is actually doing exactly what it was meant to
do, but in the wrong circumstances. :)

The bug is that G_IO_HUP is not supported by ptys.  I have just sent a
patch.

Paolo

[Qemu-devel] [PULL 4/5] virtio-scsi: fix with -M pc-i440fx-2.0

[Paolo Bonzini]

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 <dgilbert@redhat.com>
Reviewed-by: David Gilbert <dgilbert@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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 0419ee4..188a2d9 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,                    \
-- 
1.9.3

[Qemu-devel] [PULL 5/5] serial-pci: remove memory regions from BAR before destroying them

[Paolo Bonzini]

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 <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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 f53bb9c..c133c33 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]);
     }
-- 
1.9.3

Re: [Qemu-devel] [PULL for-2.1 0/5] Serial + SCSI fixes for 2014-07-14

[Peter Maydell]

On 14 July 2014 16:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The following changes since commit ab6d3749c4915cd5692633e321f7745dce06fe77:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-vga-20140711-1' into staging (2014-07-11 17:50:38 +0100)
>
> are available in the git repository at:
>
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 7497bce6c2561f1215fe179e40837774f6243799:
>
>   serial-pci: remove memory regions from BAR before destroying them (2014-07-14 16:14:15 +0200)
>
> ----------------------------------------------------------------
> Misc 2.1 fixes regarding character/serial devices and SCSI.

Applied, thanks.

-- PMM