[PULL 0/2] Block patches

[PULL 0/2] Block patches

[Stefan Hajnoczi]

The following changes since commit 240ab11fb72049d6373cbbec8d788f8e411a00bc:

  Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20190924' into staging (2019-09-24 15:36:31 +0100)

are available in the Git repository at:

  https://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to f9a7e3698a737ee75a7b0af34203303df982550f:

  virtio-blk: schedule virtio_notify_config to run on main context (2019-09-25 18:06:36 +0100)

----------------------------------------------------------------
Pull request

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

Sergio Lopez (1):
  virtio-blk: schedule virtio_notify_config to run on main context

Vladimir Sementsov-Ogievskiy (1):
  util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended

 hw/block/virtio-blk.c | 16 +++++++++++++++-
 util/iov.c            |  3 ++-
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.21.0

[PULL 1/2] util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended

[Stefan Hajnoczi]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Make it more obvious, that filling qiov corresponds to qiov allocation,
which in turn corresponds to total_niov calculation, based on mid_niov
(not mid_len). Still add an assertion to show that there should be no
difference.

Reported-by: Coverity (CID 1405302)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20190910090310.14032-1-vsementsov@virtuozzo.com
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20190910090310.14032-1-vsementsov@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/iov.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/iov.c b/util/iov.c
index 5059e10431..a4689ff3c9 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -446,7 +446,8 @@ void qemu_iovec_init_extended(
         p++;
     }
 
-    if (mid_len) {
+    assert(!mid_niov == !mid_len);
+    if (mid_niov) {
         memcpy(p, mid_iov, mid_niov * sizeof(*p));
         p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head;
         p[0].iov_len -= mid_head;
-- 
2.21.0

Re: [PULL 1/2] util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended

[Vladimir Sementsov-Ogievskiy]

25.09.2019 20:43, Stefan Hajnoczi wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Make it more obvious, that filling qiov corresponds to qiov allocation,
> which in turn corresponds to total_niov calculation, based on mid_niov
> (not mid_len). Still add an assertion to show that there should be no
> difference.
> 
> Reported-by: Coverity (CID 1405302)
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Message-id: 20190910090310.14032-1-vsementsov@virtuozzo.com
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Message-Id: <20190910090310.14032-1-vsementsov@virtuozzo.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   util/iov.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/iov.c b/util/iov.c
> index 5059e10431..a4689ff3c9 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -446,7 +446,8 @@ void qemu_iovec_init_extended(
>           p++;
>       }
>   
> -    if (mid_len) {
> +    assert(!mid_niov == !mid_len);
> +    if (mid_niov) {
>           memcpy(p, mid_iov, mid_niov * sizeof(*p));
>           p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head;
>           p[0].iov_len -= mid_head;
> 


Hmm, seems we have to squash in:

--- a/util/iov.c
+++ b/util/iov.c
@@ -423,7 +423,7 @@ void qemu_iovec_init_extended(
  {
      size_t mid_head, mid_tail;
      int total_niov, mid_niov = 0;
-    struct iovec *p, *mid_iov;
+    struct iovec *p, *mid_iov = NULL;

      if (mid_len) {
          mid_iov = qiov_slice(mid_qiov, mid_offset, mid_len,


-- 
Best regards,
Vladimir

[PULL 2/2] virtio-blk: schedule virtio_notify_config to run on main context

[Stefan Hajnoczi]

From: Sergio Lopez <slp@redhat.com>

virtio_notify_config() needs to acquire the global mutex, which isn't
allowed from an iothread, and may lead to a deadlock like this:

 - main thead
  * Has acquired: qemu_global_mutex.
  * Is trying the acquire: iothread AioContext lock via
    AIO_WAIT_WHILE (after aio_poll).

 - iothread
  * Has acquired: AioContext lock.
  * Is trying to acquire: qemu_global_mutex (via
    virtio_notify_config->prepare_mmio_access).

If virtio_blk_resize() is called from an iothread, schedule
virtio_notify_config() to be run in the main context BH.

[Removed unnecessary newline as suggested by Kevin Wolf
<kwolf@redhat.com>.
--Stefan]

Signed-off-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20190916112411.21636-1-slp@redhat.com
Message-Id: <20190916112411.21636-1-slp@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 18851601cb..ed2ddebd2b 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -16,6 +16,7 @@
 #include "qemu/iov.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
+#include "qemu/main-loop.h"
 #include "trace.h"
 #include "hw/block/block.h"
 #include "hw/qdev-properties.h"
@@ -1086,11 +1087,24 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
     return 0;
 }
 
+static void virtio_resize_cb(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+    virtio_notify_config(vdev);
+}
+
 static void virtio_blk_resize(void *opaque)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
 
-    virtio_notify_config(vdev);
+    /*
+     * virtio_notify_config() needs to acquire the global mutex,
+     * so it can't be called from an iothread. Instead, schedule
+     * it to be run in the main context BH.
+     */
+    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
 }
 
 static const BlockDevOps virtio_block_ops = {
-- 
2.21.0

Re: [PULL 0/2] Block patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190925174400.8578-1-stefanha@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      authz/list.o
  CC      authz/listfile.o
/tmp/qemu-test/src/util/iov.c: In function 'qemu_iovec_init_extended':
/tmp/qemu-test/src/util/iov.c:451:9: error: 'mid_iov' may be used uninitialized in this function [-Werror=maybe-uninitialized]
         memcpy(p, mid_iov, mid_niov * sizeof(*p));
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


The full log is available at
http://patchew.org/logs/20190925174400.8578-1-stefanha@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PULL 0/2] Block patches

[Vladimir Sementsov-Ogievskiy]

26.09.2019 9:16, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190925174400.8578-1-stefanha@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-mingw@fedora build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #! /bin/bash
> export ARCH=x86_64
> make docker-image-fedora V=1 NETWORK=1
> time make docker-test-mingw@fedora J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>    CC      authz/list.o
>    CC      authz/listfile.o
> /tmp/qemu-test/src/util/iov.c: In function 'qemu_iovec_init_extended':
> /tmp/qemu-test/src/util/iov.c:451:9: error: 'mid_iov' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>           memcpy(p, mid_iov, mid_niov * sizeof(*p));
>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> 
> The full log is available at
> http://patchew.org/logs/20190925174400.8578-1-stefanha@redhat.com/testing.docker-mingw@fedora/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 

Actually it's obvious that it is initialized here:

We go here only if mid_niov, which may be set only in "if (mid_len)", and mid_iov is set in same "if (mid_len)".

My clang don't warn.

Still, we may just initialize mid_iov to NULL and don't care.

-- 
Best regards,
Vladimir

Re: [PULL 0/2] Block patches

[Peter Maydell]

On Wed, 25 Sep 2019 at 18:44, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The following changes since commit 240ab11fb72049d6373cbbec8d788f8e411a00bc:
>
>   Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20190924' into staging (2019-09-24 15:36:31 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to f9a7e3698a737ee75a7b0af34203303df982550f:
>
>   virtio-blk: schedule virtio_notify_config to run on main context (2019-09-25 18:06:36 +0100)
>
> ----------------------------------------------------------------
> Pull request
>
> ----------------------------------------------------------------

Hi; I'm dropping this pullreq as it makes one of the patchew build
configs fail to compile.

thanks
-- PMM