Series comparison

-[Qemu-devel] [PULL 0/1] Block patches
+[PULL for-8.1 0/1] Block patches
-The following changes since commit 063833a6ec2a6747e27c5f9866bb44c7e8de1265:
+The following changes since commit ed8ad9728a9c0eec34db9dff61dfa2f1dd625637:
-  Merge remote-tracking branch 'remotes/mcayland/tags/qemu-sparc-signed' into staging (2017-10-19 18:42:51 +0100)
+  Merge tag 'pull-tpm-2023-07-14-1' of https://github.com/stefanberger/qemu-tpm into staging (2023-07-15 14:54:04 +0100)
-are available in the git repository at:
+are available in the Git repository at:
-  git://github.com/stefanha/qemu.git tags/block-pull-request
+  https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to e947d47da0b16e80d237c510e9d2e80799578c7f:
+for you to fetch changes up to 66547f416a61e0cb711dc76821890242432ba193:
-  oslib-posix: Fix compiler warning and some data types (2017-10-20 11:16:27 +0200)
+  block/nvme: invoke blk_io_plug_call() outside q->lock (2023-07-17 09:17:41 -0400)
 ----------------------------------------------------------------
 Pull request
 Fix the hang in the nvme:// block driver during startup.
 ----------------------------------------------------------------
-----------------------------------------------------------------
+Stefan Hajnoczi (1):
   block/nvme: invoke blk_io_plug_call() outside q->lock
-Stefan Weil (1):
+ block/nvme.c | 3 ++-
-  oslib-posix: Fix compiler warning and some data types
+file changed, 2 insertions(+), 1 deletion(-)
  util/oslib-posix.c | 15 ++++++++-------
 file changed, 8 insertions(+), 7 deletions(-)
 --
-.13.6
+.40.1

-[Qemu-devel] [PULL 1/1] oslib-posix: Fix compiler warning and some data types
+[PULL for-8.1 1/1] block/nvme: invoke blk_io_plug_call() outside q->lock
-From: Stefan Weil <sw@weilnetz.de>
+blk_io_plug_call() is invoked outside a blk_io_plug()/blk_io_unplug()
 section while opening the NVMe drive from:
-gcc warning:
+  nvme_file_open() ->
   nvme_init() ->
   nvme_identify() ->
   nvme_admin_cmd_sync() ->
   nvme_submit_command() ->
   blk_io_plug_call()
-/qemu/util/oslib-posix.c:304:11: error:
+blk_io_plug_call() immediately invokes the given callback when the
- variable ‘addr’ might be clobbered by ‘longjmp’ or ‘vfork’
+current thread is not plugged, as is the case during nvme_file_open().
  [-Werror=clobbered]
-Fix also some related data types:
+Unfortunately, nvme_submit_command() calls blk_io_plug_call() with
 q->lock still held:
-numpages, hpagesize are used as pointer offset.
+    ...
-Always use size_t for them and also for the derived
+    q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;
-numpages_per_thread and size_per_thread.
+    q->need_kick++;
     blk_io_plug_call(nvme_unplug_fn, q);
     qemu_mutex_unlock(&q->lock);
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
+nvme_unplug_fn() deadlocks trying to acquire q->lock because the lock is
-Signed-off-by: Stefan Weil <sw@weilnetz.de>
+already acquired by the same thread. The symptom is that QEMU hangs
-Message-id: 20171016202912.1117-1-sw@weilnetz.de
+during startup while opening the NVMe drive.
 Fix this by moving the blk_io_plug_call() outside q->lock. This is safe
 because no other thread runs code related to this queue and
 blk_io_plug_call()'s internal state is immune to thread safety issues
 since it is thread-local.
 Reported-by: Lukáš Doktor <ldoktor@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 Tested-by: Lukas Doktor <ldoktor@redhat.com>
 Message-id: 20230712191628.252806-1-stefanha@redhat.com
 Fixes: f2e590002bd6 ("block/nvme: convert to blk_io_plug_call() API")
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- util/oslib-posix.c | 15 ++++++++-------
+ block/nvme.c | 3 ++-
-file changed, 8 insertions(+), 7 deletions(-)
+file changed, 2 insertions(+), 1 deletion(-)
-diff --git a/util/oslib-posix.c b/util/oslib-posix.c
+diff --git a/block/nvme.c b/block/nvme.c
 index XXXXXXX..XXXXXXX 100644
---- a/util/oslib-posix.c
+--- a/block/nvme.c
-+++ b/util/oslib-posix.c
++++ b/block/nvme.c
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ static void nvme_submit_command(NVMeQueuePair *q, NVMeRequest *req,
+            q->sq.tail * NVME_SQ_ENTRY_BYTES, cmd, sizeof(*cmd));
- struct MemsetThread {
+     q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;
-     char *addr;
+     q->need_kick++;
--    uint64_t numpages;
++    qemu_mutex_unlock(&q->lock);
--    uint64_t hpagesize;
++
-+    size_t numpages;
+     blk_io_plug_call(nvme_unplug_fn, q);
-+    size_t hpagesize;
+-    qemu_mutex_unlock(&q->lock);
-     QemuThread pgthread;
+ }
-     sigjmp_buf env;
- };
+ static void nvme_admin_cmd_sync_cb(void *opaque, int ret)
@@ -XXX,XX +XXX,XX @@ static void sigbus_handler(int signal)
  static void *do_touch_pages(void *arg)
  {
      MemsetThread *memset_args = (MemsetThread *)arg;
 -    char *addr = memset_args->addr;
 -    uint64_t numpages = memset_args->numpages;
 -    uint64_t hpagesize = memset_args->hpagesize;
      sigset_t set, oldset;
 -    int i = 0;
      /* unblock SIGBUS */
      sigemptyset(&set);
@@ -XXX,XX +XXX,XX @@ static void *do_touch_pages(void *arg)
      if (sigsetjmp(memset_args->env, 1)) {
          memset_thread_failed = true;
      } else {
 +        char *addr = memset_args->addr;
 +        size_t numpages = memset_args->numpages;
 +        size_t hpagesize = memset_args->hpagesize;
 +        size_t i;
          for (i = 0; i < numpages; i++) {
              /*
               * Read & write back the same value, so we don't
@@ -XXX,XX +XXX,XX @@ static inline int get_memset_num_threads(int smp_cpus)
  static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
                              int smp_cpus)
  {
 -    uint64_t numpages_per_thread, size_per_thread;
 +    size_t numpages_per_thread;
 +    size_t size_per_thread;
      char *addr = area;
      int i = 0;
 --
-.13.6
+.40.1

From: Stefan Weil <sw@weilnetz.de>

gcc warning:

/qemu/util/oslib-posix.c:304:11: error:
 variable ‘addr’ might be clobbered by ‘longjmp’ or ‘vfork’
 [-Werror=clobbered]

Fix also some related data types:

numpages, hpagesize are used as pointer offset.
Always use size_t for them and also for the derived
numpages_per_thread and size_per_thread.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Message-id: 20171016202912.1117-1-sw@weilnetz.de
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/oslib-posix.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index XXXXXXX..XXXXXXX 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -XXX,XX +XXX,XX @@
 
 struct MemsetThread {
     char *addr;
-    uint64_t numpages;
-    uint64_t hpagesize;
+    size_t numpages;
+    size_t hpagesize;
     QemuThread pgthread;
     sigjmp_buf env;
 };
@@ -XXX,XX +XXX,XX @@ static void sigbus_handler(int signal)
 static void *do_touch_pages(void *arg)
 {
     MemsetThread *memset_args = (MemsetThread *)arg;
-    char *addr = memset_args->addr;
-    uint64_t numpages = memset_args->numpages;
-    uint64_t hpagesize = memset_args->hpagesize;
     sigset_t set, oldset;
-    int i = 0;
 
     /* unblock SIGBUS */
     sigemptyset(&set);
@@ -XXX,XX +XXX,XX @@ static void *do_touch_pages(void *arg)
     if (sigsetjmp(memset_args->env, 1)) {
         memset_thread_failed = true;
     } else {
+        char *addr = memset_args->addr;
+        size_t numpages = memset_args->numpages;
+        size_t hpagesize = memset_args->hpagesize;
+        size_t i;
         for (i = 0; i < numpages; i++) {
             /*
              * Read & write back the same value, so we don't
@@ -XXX,XX +XXX,XX @@ static inline int get_memset_num_threads(int smp_cpus)
 static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
                             int smp_cpus)
 {
-    uint64_t numpages_per_thread, size_per_thread;
+    size_t numpages_per_thread;
+    size_t size_per_thread;
     char *addr = area;
     int i = 0;
 
-- 
2.13.6

blk_io_plug_call() is invoked outside a blk_io_plug()/blk_io_unplug()
section while opening the NVMe drive from:

nvme_file_open() ->
  nvme_init() ->
  nvme_identify() ->
  nvme_admin_cmd_sync() ->
  nvme_submit_command() ->
  blk_io_plug_call()

blk_io_plug_call() immediately invokes the given callback when the
current thread is not plugged, as is the case during nvme_file_open().

Unfortunately, nvme_submit_command() calls blk_io_plug_call() with
q->lock still held:

...
    q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;
    q->need_kick++;
    blk_io_plug_call(nvme_unplug_fn, q);
    qemu_mutex_unlock(&q->lock);
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^

nvme_unplug_fn() deadlocks trying to acquire q->lock because the lock is
already acquired by the same thread. The symptom is that QEMU hangs
during startup while opening the NVMe drive.

Fix this by moving the blk_io_plug_call() outside q->lock. This is safe
because no other thread runs code related to this queue and
blk_io_plug_call()'s internal state is immune to thread safety issues
since it is thread-local.

Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Lukas Doktor <ldoktor@redhat.com>
Message-id: 20230712191628.252806-1-stefanha@redhat.com
Fixes: f2e590002bd6 ("block/nvme: convert to blk_io_plug_call() API")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/nvme.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index XXXXXXX..XXXXXXX 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -XXX,XX +XXX,XX @@ static void nvme_submit_command(NVMeQueuePair *q, NVMeRequest *req,
            q->sq.tail * NVME_SQ_ENTRY_BYTES, cmd, sizeof(*cmd));
     q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;
     q->need_kick++;
+    qemu_mutex_unlock(&q->lock);
+
     blk_io_plug_call(nvme_unplug_fn, q);
-    qemu_mutex_unlock(&q->lock);
 }
 
 static void nvme_admin_cmd_sync_cb(void *opaque, int ret)
-- 
2.40.1