All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9] block/nvme: Rework error reporting
@ 2021-08-24 14:11 Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 1/9] block/nvme: Use safer trace format string Philippe Mathieu-Daudé
                   ` (8 more replies)
  0 siblings, 9 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

Hi,

This series contains various patches sent last year with
review comments addressed, few more cleanups, and a new
patch which remove the spurious "VFIO_MAP_DMA failed: No
space left on device" now poping up since commit 15a730e7a.
(it is the expected behavior, which is why we retry the
same call after flushing the DMA mappings).

Regards,

Phil.

Philippe Mathieu-Daudé (9):
  block/nvme: Use safer trace format string
  block/nvme: Have nvme_create_queue_pair() report errors consistently
  util/vfio-helpers: Let qemu_vfio_verify_mappings() use error_report()
  util/vfio-helpers: Replace qemu_mutex_lock() calls with
    QEMU_LOCK_GUARD
  util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map()
  util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map()
  util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error
  util/vfio-helpers: Let qemu_vfio_do_mapping() propagate Error
  block/nvme: Only report VFIO error on failed retry

 include/qemu/vfio-helpers.h |  2 +-
 block/nvme.c                | 29 +++++++++++------
 util/vfio-helpers.c         | 63 +++++++++++++++++--------------------
 block/trace-events          |  2 +-
 4 files changed, 50 insertions(+), 46 deletions(-)

-- 
2.31.1




^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/9] block/nvme: Use safer trace format string
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-25 11:25   ` Klaus Jensen
  2021-08-24 14:11 ` [PATCH 2/9] block/nvme: Have nvme_create_queue_pair() report errors consistently Philippe Mathieu-Daudé
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

Fix when building with -Wshorten-64-to-32:

  warning: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Wshorten-64-to-32]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/trace-events | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/trace-events b/block/trace-events
index b3d2b1e62cb..f4f1267c8c0 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -156,7 +156,7 @@ nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset 0x%"PRIx64" byte
 nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset 0x%"PRIx64" bytes %"PRId64" ret %d"
 nvme_dma_map_flush(void *s) "s %p"
 nvme_free_req_queue_wait(void *s, unsigned q_index) "s %p q #%u"
-nvme_create_queue_pair(unsigned q_index, void *q, unsigned size, void *aio_context, int fd) "index %u q %p size %u aioctx %p fd %d"
+nvme_create_queue_pair(unsigned q_index, void *q, size_t size, void *aio_context, int fd) "index %u q %p size %zu aioctx %p fd %d"
 nvme_free_queue_pair(unsigned q_index, void *q) "index %u q %p"
 nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
 nvme_cmd_map_qiov_pages(void *s, int i, uint64_t page) "s %p page[%d] 0x%"PRIx64
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/9] block/nvme: Have nvme_create_queue_pair() report errors consistently
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 1/9] block/nvme: Use safer trace format string Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-25 11:24   ` Klaus Jensen
  2021-08-24 14:11 ` [PATCH 3/9] util/vfio-helpers: Let qemu_vfio_verify_mappings() use error_report() Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

nvme_create_queue_pair() does not return a boolean value (indicating
eventual error) but a pointer, and is inconsistent in how it fills the
error handler. To fulfill callers expectations, always set an error
message on failure.

Reported-by: Auger Eric <eric.auger@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index e8dbbc23177..6642c104aa4 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -220,6 +220,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
 
     q = g_try_new0(NVMeQueuePair, 1);
     if (!q) {
+        error_setg(errp, "Cannot allocate queue pair");
         return NULL;
     }
     trace_nvme_create_queue_pair(idx, q, size, aio_context,
@@ -228,6 +229,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
                           qemu_real_host_page_size);
     q->prp_list_pages = qemu_try_memalign(qemu_real_host_page_size, bytes);
     if (!q->prp_list_pages) {
+        error_setg(errp, "Cannot allocate PRP page list");
         goto fail;
     }
     memset(q->prp_list_pages, 0, bytes);
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/9] util/vfio-helpers: Let qemu_vfio_verify_mappings() use error_report()
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 1/9] block/nvme: Use safer trace format string Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 2/9] block/nvme: Have nvme_create_queue_pair() report errors consistently Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 4/9] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

Instead of displaying the error on stderr, use error_report()
which also report to the monitor.

Reviewed-by: Fam Zheng <fam@euphon.net>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 911115b86e6..1d149136299 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -660,13 +660,13 @@ static bool qemu_vfio_verify_mappings(QEMUVFIOState *s)
     if (QEMU_VFIO_DEBUG) {
         for (i = 0; i < s->nr_mappings - 1; ++i) {
             if (!(s->mappings[i].host < s->mappings[i + 1].host)) {
-                fprintf(stderr, "item %d not sorted!\n", i);
+                error_report("item %d not sorted!", i);
                 qemu_vfio_dump_mappings(s);
                 return false;
             }
             if (!(s->mappings[i].host + s->mappings[i].size <=
                   s->mappings[i + 1].host)) {
-                fprintf(stderr, "item %d overlap with next!\n", i);
+                error_report("item %d overlap with next!", i);
                 qemu_vfio_dump_mappings(s);
                 return false;
             }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/9] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2021-08-24 14:11 ` [PATCH 3/9] util/vfio-helpers: Let qemu_vfio_verify_mappings() use error_report() Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-25 11:57   ` Klaus Jensen
  2021-08-24 14:11 ` [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map() Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

Simplify qemu_vfio_dma_[un]map() handlers by replacing a pair of
qemu_mutex_lock/qemu_mutex_unlock calls by the WITH_QEMU_LOCK_GUARD
macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 1d149136299..d956866b4e9 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -735,7 +735,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
     assert(QEMU_PTR_IS_ALIGNED(host, qemu_real_host_page_size));
     assert(QEMU_IS_ALIGNED(size, qemu_real_host_page_size));
     trace_qemu_vfio_dma_map(s, host, size, temporary, iova);
-    qemu_mutex_lock(&s->lock);
+    QEMU_LOCK_GUARD(&s->lock);
     mapping = qemu_vfio_find_mapping(s, host, &index);
     if (mapping) {
         iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
@@ -778,7 +778,6 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
         *iova = iova0;
     }
 out:
-    qemu_mutex_unlock(&s->lock);
     return ret;
 }
 
@@ -813,14 +812,12 @@ void qemu_vfio_dma_unmap(QEMUVFIOState *s, void *host)
     }
 
     trace_qemu_vfio_dma_unmap(s, host);
-    qemu_mutex_lock(&s->lock);
+    QEMU_LOCK_GUARD(&s->lock);
     m = qemu_vfio_find_mapping(s, host, &index);
     if (!m) {
-        goto out;
+        return;
     }
     qemu_vfio_undo_mapping(s, m, NULL);
-out:
-    qemu_mutex_unlock(&s->lock);
 }
 
 static void qemu_vfio_reset(QEMUVFIOState *s)
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map()
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2021-08-24 14:11 ` [PATCH 4/9] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-25 11:53   ` Klaus Jensen
  2021-08-24 14:11 ` [PATCH 6/9] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map() Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

qemu_vfio_add_mapping() returns a pointer to an indexed entry
in pre-allocated QEMUVFIOState::mappings[], thus can not be NULL.
Remove the pointless check.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index d956866b4e9..e7909222cfd 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -751,10 +751,6 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
             }
 
             mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
-            if (!mapping) {
-                ret = -ENOMEM;
-                goto out;
-            }
             assert(qemu_vfio_verify_mappings(s));
             ret = qemu_vfio_do_mapping(s, host, size, iova0);
             if (ret) {
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 6/9] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map()
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2021-08-24 14:11 ` [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map() Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

Currently qemu_vfio_dma_map() displays errors on stderr.
When using management interface, this information is simply
lost. Pass qemu_vfio_dma_map() an Error** handle so it can
propagate the error to callers.

Reviewed-by: Fam Zheng <fam@euphon.net>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Changed:
- Initialize *err before calling error_prepend (Eric Auger)
- use error_reportf_err() in nvme_register_buf()
---
 include/qemu/vfio-helpers.h |  2 +-
 block/nvme.c                | 20 ++++++++++----------
 util/vfio-helpers.c         | 13 +++++++++----
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 4491c8e1a6e..bde9495b254 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -18,7 +18,7 @@ typedef struct QEMUVFIOState QEMUVFIOState;
 QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp);
 void qemu_vfio_close(QEMUVFIOState *s);
 int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
-                      bool temporary, uint64_t *iova_list);
+                      bool temporary, uint64_t *iova_list, Error **errp);
 int qemu_vfio_dma_reset_temporary(QEMUVFIOState *s);
 void qemu_vfio_dma_unmap(QEMUVFIOState *s, void *host);
 void *qemu_vfio_pci_map_bar(QEMUVFIOState *s, int index,
diff --git a/block/nvme.c b/block/nvme.c
index 6642c104aa4..663e5d918fa 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -176,12 +176,11 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
         return false;
     }
     memset(q->queue, 0, bytes);
-    r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
+    r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova, errp);
     if (r) {
-        error_setg(errp, "Cannot map queue");
-        return false;
+        error_prepend(errp, "Cannot map queue: ");
     }
-    return true;
+    return r == 0;
 }
 
 static void nvme_free_queue_pair(NVMeQueuePair *q)
@@ -239,7 +238,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
     qemu_co_queue_init(&q->free_req_queue);
     q->completion_bh = aio_bh_new(aio_context, nvme_process_completion_bh, q);
     r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
-                          false, &prp_list_iova);
+                          false, &prp_list_iova, errp);
     if (r) {
         goto fail;
     }
@@ -533,9 +532,9 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
     }
-    r = qemu_vfio_dma_map(s->vfio, id, id_size, true, &iova);
+    r = qemu_vfio_dma_map(s->vfio, id, id_size, true, &iova, errp);
     if (r) {
-        error_setg(errp, "Cannot map buffer for DMA");
+        error_prepend(errp, "Cannot map buffer for DMA: ");
         goto out;
     }
 
@@ -1031,7 +1030,7 @@ static coroutine_fn int nvme_cmd_map_qiov(BlockDriverState *bs, NvmeCmd *cmd,
 try_map:
         r = qemu_vfio_dma_map(s->vfio,
                               qiov->iov[i].iov_base,
-                              len, true, &iova);
+                              len, true, &iova, NULL);
         if (r == -ENOSPC) {
             /*
              * In addition to the -ENOMEM error, the VFIO_IOMMU_MAP_DMA
@@ -1523,14 +1522,15 @@ static void nvme_aio_unplug(BlockDriverState *bs)
 static void nvme_register_buf(BlockDriverState *bs, void *host, size_t size)
 {
     int ret;
+    Error *local_err = NULL;
     BDRVNVMeState *s = bs->opaque;
 
-    ret = qemu_vfio_dma_map(s->vfio, host, size, false, NULL);
+    ret = qemu_vfio_dma_map(s->vfio, host, size, false, NULL, &local_err);
     if (ret) {
         /* FIXME: we may run out of IOVA addresses after repeated
          * bdrv_register_buf/bdrv_unregister_buf, because nvme_vfio_dma_unmap
          * doesn't reclaim addresses for fixed mappings. */
-        error_report("nvme_register_buf failed: %s", strerror(-ret));
+        error_reportf_err(local_err, "nvme_register_buf failed: ");
     }
 }
 
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index e7909222cfd..3e1a49bea15 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -463,13 +463,15 @@ static void qemu_vfio_ram_block_added(RAMBlockNotifier *n, void *host,
                                       size_t size, size_t max_size)
 {
     QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier);
+    Error *local_err = NULL;
     int ret;
 
     trace_qemu_vfio_ram_block_added(s, host, max_size);
-    ret = qemu_vfio_dma_map(s, host, max_size, false, NULL);
+    ret = qemu_vfio_dma_map(s, host, max_size, false, NULL, &local_err);
     if (ret) {
-        error_report("qemu_vfio_dma_map(%p, %zu) failed: %s", host, max_size,
-                     strerror(-ret));
+        error_reportf_err(local_err,
+                          "qemu_vfio_dma_map(%p, %zu) failed: ",
+                          host, max_size);
     }
 }
 
@@ -725,7 +727,7 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
  * mapping status within this area is not allowed).
  */
 int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
-                      bool temporary, uint64_t *iova)
+                      bool temporary, uint64_t *iova, Error **errp)
 {
     int ret = 0;
     int index;
@@ -774,6 +776,9 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
         *iova = iova0;
     }
 out:
+    if (ret) {
+        error_setg_errno(errp, -ret, "Cannot map buffer for DMA");
+    }
     return ret;
 }
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2021-08-24 14:11 ` [PATCH 6/9] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map() Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-25 11:34   ` Klaus Jensen
  2021-08-24 14:11 ` [PATCH 8/9] util/vfio-helpers: Let qemu_vfio_do_mapping() " Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 9/9] block/nvme: Only report VFIO error on failed retry Philippe Mathieu-Daudé
  8 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

Now that all qemu_vfio_dma_map() callers provide an Error* argument,
fill it with relevant / more descriptive message. Reduce 'ret'
(returned value) scope by returning errno directly to simplify
(removing the goto / 'out' label).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c        |  1 +
 util/vfio-helpers.c | 31 ++++++++++++++-----------------
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 663e5d918fa..80546b0babd 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
     r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
                           false, &prp_list_iova, errp);
     if (r) {
+        error_prepend(errp, "Cannot map buffer for DMA: ");
         goto fail;
     }
     q->free_req_head = -1;
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 3e1a49bea15..f4c16e1dae5 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
 int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
                       bool temporary, uint64_t *iova, Error **errp)
 {
-    int ret = 0;
     int index;
     IOVAMapping *mapping;
     uint64_t iova0;
@@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
     if (mapping) {
         iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
     } else {
+        int ret;
+
         if (s->high_water_mark - s->low_water_mark + 1 < size) {
-            ret = -ENOMEM;
-            goto out;
+            error_setg(errp, "iova exhausted (water mark reached)");
+            return -ENOMEM;
         }
         if (!temporary) {
-            if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
-                ret = -ENOMEM;
-                goto out;
+            if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
+                error_setg(errp, "iova range not found");
+                return -ENOMEM;
             }
 
             mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
             assert(qemu_vfio_verify_mappings(s));
             ret = qemu_vfio_do_mapping(s, host, size, iova0);
-            if (ret) {
+            if (ret < 0) {
                 qemu_vfio_undo_mapping(s, mapping, NULL);
-                goto out;
+                return ret;
             }
             qemu_vfio_dump_mappings(s);
         } else {
             if (qemu_vfio_find_temp_iova(s, size, &iova0)) {
-                ret = -ENOMEM;
-                goto out;
+                error_setg(errp, "iova range not found");
+                return -ENOMEM;
             }
             ret = qemu_vfio_do_mapping(s, host, size, iova0);
-            if (ret) {
-                goto out;
+            if (ret < 0) {
+                return ret;
             }
         }
     }
@@ -775,11 +776,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
     if (iova) {
         *iova = iova0;
     }
-out:
-    if (ret) {
-        error_setg_errno(errp, -ret, "Cannot map buffer for DMA");
-    }
-    return ret;
+    return 0;
 }
 
 /* Reset the high watermark and free all "temporary" mappings. */
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 8/9] util/vfio-helpers: Let qemu_vfio_do_mapping() propagate Error
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2021-08-24 14:11 ` [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-24 14:11 ` [PATCH 9/9] block/nvme: Only report VFIO error on failed retry Philippe Mathieu-Daudé
  8 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Auger Eric, Hanna Reitz,
	Stefan Hajnoczi, Philippe Mathieu-Daudé

Pass qemu_vfio_do_mapping() an Error* argument so it can propagate
any error to callers. Replace error_report() which only report
to the monitor by the more generic error_setg_errno().

Reviewed-by: Fam Zheng <fam@euphon.net>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index f4c16e1dae5..613f3db3745 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -610,7 +610,7 @@ static IOVAMapping *qemu_vfio_add_mapping(QEMUVFIOState *s,
 
 /* Do the DMA mapping with VFIO. */
 static int qemu_vfio_do_mapping(QEMUVFIOState *s, void *host, size_t size,
-                                uint64_t iova)
+                                uint64_t iova, Error **errp)
 {
     struct vfio_iommu_type1_dma_map dma_map = {
         .argsz = sizeof(dma_map),
@@ -622,7 +622,7 @@ static int qemu_vfio_do_mapping(QEMUVFIOState *s, void *host, size_t size,
     trace_qemu_vfio_do_mapping(s, host, iova, size);
 
     if (ioctl(s->container, VFIO_IOMMU_MAP_DMA, &dma_map)) {
-        error_report("VFIO_MAP_DMA failed: %s", strerror(errno));
+        error_setg_errno(errp, errno, "VFIO_MAP_DMA failed");
         return -errno;
     }
     return 0;
@@ -755,7 +755,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
 
             mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
             assert(qemu_vfio_verify_mappings(s));
-            ret = qemu_vfio_do_mapping(s, host, size, iova0);
+            ret = qemu_vfio_do_mapping(s, host, size, iova0, errp);
             if (ret < 0) {
                 qemu_vfio_undo_mapping(s, mapping, NULL);
                 return ret;
@@ -766,7 +766,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
                 error_setg(errp, "iova range not found");
                 return -ENOMEM;
             }
-            ret = qemu_vfio_do_mapping(s, host, size, iova0);
+            ret = qemu_vfio_do_mapping(s, host, size, iova0, errp);
             if (ret < 0) {
                 return ret;
             }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 9/9] block/nvme: Only report VFIO error on failed retry
  2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
                   ` (7 preceding siblings ...)
  2021-08-24 14:11 ` [PATCH 8/9] util/vfio-helpers: Let qemu_vfio_do_mapping() " Philippe Mathieu-Daudé
@ 2021-08-24 14:11 ` Philippe Mathieu-Daudé
  2021-08-25 12:18   ` Klaus Jensen
  8 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Tingting Mao, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi, Philippe Mathieu-Daudé

We expect the first qemu_vfio_dma_map() to fail (indicating
DMA mappings exhaustion, see commit 15a730e7a3a). Do not
report the first failure as error, since we are going to
flush the mappings and retry.

This removes spurious error message displayed on the monitor:

  (qemu) c
  (qemu) qemu-kvm: VFIO_MAP_DMA failed: No space left on device
  (qemu) info status
  VM status: running

Reported-by: Tingting Mao <timao@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 80546b0babd..abfe305baf2 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1019,6 +1019,7 @@ static coroutine_fn int nvme_cmd_map_qiov(BlockDriverState *bs, NvmeCmd *cmd,
     uint64_t *pagelist = req->prp_list_page;
     int i, j, r;
     int entries = 0;
+    Error *local_err = NULL, **errp = NULL;
 
     assert(qiov->size);
     assert(QEMU_IS_ALIGNED(qiov->size, s->page_size));
@@ -1031,7 +1032,7 @@ static coroutine_fn int nvme_cmd_map_qiov(BlockDriverState *bs, NvmeCmd *cmd,
 try_map:
         r = qemu_vfio_dma_map(s->vfio,
                               qiov->iov[i].iov_base,
-                              len, true, &iova, NULL);
+                              len, true, &iova, errp);
         if (r == -ENOSPC) {
             /*
              * In addition to the -ENOMEM error, the VFIO_IOMMU_MAP_DMA
@@ -1066,6 +1067,8 @@ try_map:
                     goto fail;
                 }
             }
+            errp = &local_err;
+
             goto try_map;
         }
         if (r) {
@@ -1109,6 +1112,9 @@ fail:
      * because they are already mapped before calling this function; for
      * temporary mappings, a later nvme_cmd_(un)map_qiov will reclaim by
      * calling qemu_vfio_dma_reset_temporary when necessary. */
+    if (local_err) {
+        error_reportf_err(local_err, "Cannot map buffer for DMA: ");
+    }
     return r;
 }
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/9] block/nvme: Have nvme_create_queue_pair() report errors consistently
  2021-08-24 14:11 ` [PATCH 2/9] block/nvme: Have nvme_create_queue_pair() report errors consistently Philippe Mathieu-Daudé
@ 2021-08-25 11:24   ` Klaus Jensen
  0 siblings, 0 replies; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 11:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> nvme_create_queue_pair() does not return a boolean value (indicating
> eventual error) but a pointer, and is inconsistent in how it fills the
> error handler. To fulfill callers expectations, always set an error
> message on failure.
> 
> Reported-by: Auger Eric <eric.auger@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index e8dbbc23177..6642c104aa4 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -220,6 +220,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>  
>      q = g_try_new0(NVMeQueuePair, 1);
>      if (!q) {
> +        error_setg(errp, "Cannot allocate queue pair");
>          return NULL;
>      }
>      trace_nvme_create_queue_pair(idx, q, size, aio_context,
> @@ -228,6 +229,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>                            qemu_real_host_page_size);
>      q->prp_list_pages = qemu_try_memalign(qemu_real_host_page_size, bytes);
>      if (!q->prp_list_pages) {
> +        error_setg(errp, "Cannot allocate PRP page list");
>          goto fail;
>      }
>      memset(q->prp_list_pages, 0, bytes);
> -- 
> 2.31.1
> 

Looks like

    r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
                          false, &prp_list_iova);
    if (r) {
        goto fail;
    }

should set an error message as well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/9] block/nvme: Use safer trace format string
  2021-08-24 14:11 ` [PATCH 1/9] block/nvme: Use safer trace format string Philippe Mathieu-Daudé
@ 2021-08-25 11:25   ` Klaus Jensen
  0 siblings, 0 replies; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 11:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> Fix when building with -Wshorten-64-to-32:
> 
>   warning: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Wshorten-64-to-32]
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error
  2021-08-24 14:11 ` [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error Philippe Mathieu-Daudé
@ 2021-08-25 11:34   ` Klaus Jensen
  2021-08-25 13:08     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 11:34 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 3916 bytes --]

On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> Now that all qemu_vfio_dma_map() callers provide an Error* argument,
> fill it with relevant / more descriptive message. Reduce 'ret'
> (returned value) scope by returning errno directly to simplify
> (removing the goto / 'out' label).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c        |  1 +
>  util/vfio-helpers.c | 31 ++++++++++++++-----------------
>  2 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 663e5d918fa..80546b0babd 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
>                            false, &prp_list_iova, errp);
>      if (r) {
> +        error_prepend(errp, "Cannot map buffer for DMA: ");

Ah. Here is the missing error message that I noticed in patch 2 ;)

>          goto fail;
>      }
>      q->free_req_head = -1;
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 3e1a49bea15..f4c16e1dae5 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
>  int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>                        bool temporary, uint64_t *iova, Error **errp)
>  {
> -    int ret = 0;
>      int index;
>      IOVAMapping *mapping;
>      uint64_t iova0;
> @@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>      if (mapping) {
>          iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
>      } else {
> +        int ret;
> +
>          if (s->high_water_mark - s->low_water_mark + 1 < size) {
> -            ret = -ENOMEM;
> -            goto out;
> +            error_setg(errp, "iova exhausted (water mark reached)");
> +            return -ENOMEM;
>          }
>          if (!temporary) {
> -            if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
> -                ret = -ENOMEM;
> -                goto out;
> +            if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
> +                error_setg(errp, "iova range not found");
> +                return -ENOMEM;

Just curious.

Previously this did error_setg_errno in out. Why don't we want to do that here?

>              }
>  
>              mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
>              assert(qemu_vfio_verify_mappings(s));
>              ret = qemu_vfio_do_mapping(s, host, size, iova0);
> -            if (ret) {
> +            if (ret < 0) {
>                  qemu_vfio_undo_mapping(s, mapping, NULL);
> -                goto out;
> +                return ret;
>              }
>              qemu_vfio_dump_mappings(s);
>          } else {
>              if (qemu_vfio_find_temp_iova(s, size, &iova0)) {
> -                ret = -ENOMEM;
> -                goto out;
> +                error_setg(errp, "iova range not found");
> +                return -ENOMEM;
>              }
>              ret = qemu_vfio_do_mapping(s, host, size, iova0);
> -            if (ret) {
> -                goto out;
> +            if (ret < 0) {
> +                return ret;
>              }
>          }
>      }
> @@ -775,11 +776,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>      if (iova) {
>          *iova = iova0;
>      }
> -out:
> -    if (ret) {
> -        error_setg_errno(errp, -ret, "Cannot map buffer for DMA");
> -    }
> -    return ret;
> +    return 0;
>  }
>  
>  /* Reset the high watermark and free all "temporary" mappings. */
> -- 
> 2.31.1
> 
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map()
  2021-08-24 14:11 ` [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map() Philippe Mathieu-Daudé
@ 2021-08-25 11:53   ` Klaus Jensen
  2021-08-25 11:55     ` Klaus Jensen
  0 siblings, 1 reply; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 11:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> qemu_vfio_add_mapping() returns a pointer to an indexed entry
> in pre-allocated QEMUVFIOState::mappings[], thus can not be NULL.
> Remove the pointless check.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  util/vfio-helpers.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index d956866b4e9..e7909222cfd 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -751,10 +751,6 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>              }
>  
>              mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
> -            if (!mapping) {
> -                ret = -ENOMEM;
> -                goto out;
> -            }
>              assert(qemu_vfio_verify_mappings(s));
>              ret = qemu_vfio_do_mapping(s, host, size, iova0);
>              if (ret) {
> -- 
> 2.31.1
> 
> 

This looks OK.

But maybe it would be prudent to assert that index is within bounds of
s->mappings in qemu_vfio_add_mapping? E.g.,

   assert(index >= 0 && index < s->nr_mappings + 1);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map()
  2021-08-25 11:53   ` Klaus Jensen
@ 2021-08-25 11:55     ` Klaus Jensen
  0 siblings, 0 replies; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 11:55 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Aug 25 13:53, Klaus Jensen wrote:
> On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> > qemu_vfio_add_mapping() returns a pointer to an indexed entry
> > in pre-allocated QEMUVFIOState::mappings[], thus can not be NULL.
> > Remove the pointless check.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  util/vfio-helpers.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> > index d956866b4e9..e7909222cfd 100644
> > --- a/util/vfio-helpers.c
> > +++ b/util/vfio-helpers.c
> > @@ -751,10 +751,6 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> >              }
> >  
> >              mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
> > -            if (!mapping) {
> > -                ret = -ENOMEM;
> > -                goto out;
> > -            }
> >              assert(qemu_vfio_verify_mappings(s));
> >              ret = qemu_vfio_do_mapping(s, host, size, iova0);
> >              if (ret) {
> > -- 
> > 2.31.1
> > 
> > 
> 
> This looks OK.
> 
> But maybe it would be prudent to assert that index is within bounds of
> s->mappings in qemu_vfio_add_mapping? E.g.,
> 
>    assert(index >= 0 && index < s->nr_mappings + 1);

It's not like `if (!mapping) {` would have cought and out-of-bounds
index value anyway, so

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/9] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD
  2021-08-24 14:11 ` [PATCH 4/9] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD Philippe Mathieu-Daudé
@ 2021-08-25 11:57   ` Klaus Jensen
  0 siblings, 0 replies; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 11:57 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> Simplify qemu_vfio_dma_[un]map() handlers by replacing a pair of
> qemu_mutex_lock/qemu_mutex_unlock calls by the WITH_QEMU_LOCK_GUARD
> macro.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 9/9] block/nvme: Only report VFIO error on failed retry
  2021-08-24 14:11 ` [PATCH 9/9] block/nvme: Only report VFIO error on failed retry Philippe Mathieu-Daudé
@ 2021-08-25 12:18   ` Klaus Jensen
  0 siblings, 0 replies; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 12:18 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, Tingting Mao, qemu-devel,
	Auger Eric, Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> We expect the first qemu_vfio_dma_map() to fail (indicating
> DMA mappings exhaustion, see commit 15a730e7a3a). Do not
> report the first failure as error, since we are going to
> flush the mappings and retry.
> 
> This removes spurious error message displayed on the monitor:
> 
>   (qemu) c
>   (qemu) qemu-kvm: VFIO_MAP_DMA failed: No space left on device
>   (qemu) info status
>   VM status: running
> 
> Reported-by: Tingting Mao <timao@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Neat.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error
  2021-08-25 11:34   ` Klaus Jensen
@ 2021-08-25 13:08     ` Philippe Mathieu-Daudé
  2021-08-25 14:53       ` Klaus Jensen
  0 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-25 13:08 UTC (permalink / raw)
  To: Klaus Jensen
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

On 8/25/21 1:34 PM, Klaus Jensen wrote:
> On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
>> Now that all qemu_vfio_dma_map() callers provide an Error* argument,
>> fill it with relevant / more descriptive message. Reduce 'ret'
>> (returned value) scope by returning errno directly to simplify
>> (removing the goto / 'out' label).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c        |  1 +
>>  util/vfio-helpers.c | 31 ++++++++++++++-----------------
>>  2 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 663e5d918fa..80546b0babd 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
>>                            false, &prp_list_iova, errp);
>>      if (r) {
>> +        error_prepend(errp, "Cannot map buffer for DMA: ");
> 
> Ah. Here is the missing error message that I noticed in patch 2 ;)

So I should use error_setg() in patch 2 and replace it with
error_prepend() here? I'm trying to address Eric's comments
from:

https://lore.kernel.org/qemu-devel/2cbd5471-4611-ae6b-d79f-db6ff19db5bf@redhat.com/
https://lore.kernel.org/qemu-devel/bd1cc1c9-3553-d252-3ca9-a23bc1035170@redhat.com/

> 
>>          goto fail;
>>      }
>>      q->free_req_head = -1;
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index 3e1a49bea15..f4c16e1dae5 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
>>  int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>>                        bool temporary, uint64_t *iova, Error **errp)
>>  {
>> -    int ret = 0;
>>      int index;
>>      IOVAMapping *mapping;
>>      uint64_t iova0;
>> @@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>>      if (mapping) {
>>          iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
>>      } else {
>> +        int ret;
>> +
>>          if (s->high_water_mark - s->low_water_mark + 1 < size) {
>> -            ret = -ENOMEM;
>> -            goto out;
>> +            error_setg(errp, "iova exhausted (water mark reached)");
>> +            return -ENOMEM;
>>          }
>>          if (!temporary) {
>> -            if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
>> -                ret = -ENOMEM;
>> -                goto out;
>> +            if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
>> +                error_setg(errp, "iova range not found");
>> +                return -ENOMEM;
> 
> Just curious.
> 
> Previously this did error_setg_errno in out. Why don't we want to do that here?

I didn't thought about it but indeed you are right.

Thanks for the review!

Phil.



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error
  2021-08-25 13:08     ` Philippe Mathieu-Daudé
@ 2021-08-25 14:53       ` Klaus Jensen
  0 siblings, 0 replies; 19+ messages in thread
From: Klaus Jensen @ 2021-08-25 14:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Auger Eric,
	Hanna Reitz, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]

On Aug 25 15:08, Philippe Mathieu-Daudé wrote:
> On 8/25/21 1:34 PM, Klaus Jensen wrote:
> > On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> >> Now that all qemu_vfio_dma_map() callers provide an Error* argument,
> >> fill it with relevant / more descriptive message. Reduce 'ret'
> >> (returned value) scope by returning errno directly to simplify
> >> (removing the goto / 'out' label).
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  block/nvme.c        |  1 +
> >>  util/vfio-helpers.c | 31 ++++++++++++++-----------------
> >>  2 files changed, 15 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index 663e5d918fa..80546b0babd 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
> >>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
> >>                            false, &prp_list_iova, errp);
> >>      if (r) {
> >> +        error_prepend(errp, "Cannot map buffer for DMA: ");
> > 
> > Ah. Here is the missing error message that I noticed in patch 2 ;)
> 
> So I should use error_setg() in patch 2 and replace it with
> error_prepend() here? I'm trying to address Eric's comments
> from:
> 
> https://lore.kernel.org/qemu-devel/2cbd5471-4611-ae6b-d79f-db6ff19db5bf@redhat.com/
> https://lore.kernel.org/qemu-devel/bd1cc1c9-3553-d252-3ca9-a23bc1035170@redhat.com/
> 

It's not like its breaking anything that it is missing from patch 2, so
I'm fine with it not being there. I agree that it feels weird to add it,
then replace it a few patches further down ;)

> > 
> >>          goto fail;
> >>      }
> >>      q->free_req_head = -1;
> >> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> >> index 3e1a49bea15..f4c16e1dae5 100644
> >> --- a/util/vfio-helpers.c
> >> +++ b/util/vfio-helpers.c
> >> @@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
> >>  int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> >>                        bool temporary, uint64_t *iova, Error **errp)
> >>  {
> >> -    int ret = 0;
> >>      int index;
> >>      IOVAMapping *mapping;
> >>      uint64_t iova0;
> >> @@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> >>      if (mapping) {
> >>          iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
> >>      } else {
> >> +        int ret;
> >> +
> >>          if (s->high_water_mark - s->low_water_mark + 1 < size) {
> >> -            ret = -ENOMEM;
> >> -            goto out;
> >> +            error_setg(errp, "iova exhausted (water mark reached)");
> >> +            return -ENOMEM;
> >>          }
> >>          if (!temporary) {
> >> -            if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
> >> -                ret = -ENOMEM;
> >> -                goto out;
> >> +            if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
> >> +                error_setg(errp, "iova range not found");
> >> +                return -ENOMEM;
> > 
> > Just curious.
> > 
> > Previously this did error_setg_errno in out. Why don't we want to do that here?
> 
> I didn't thought about it but indeed you are right.
> 
> Thanks for the review!
> 
> Phil.
> 
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-08-25 14:57 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 14:11 [PATCH 0/9] block/nvme: Rework error reporting Philippe Mathieu-Daudé
2021-08-24 14:11 ` [PATCH 1/9] block/nvme: Use safer trace format string Philippe Mathieu-Daudé
2021-08-25 11:25   ` Klaus Jensen
2021-08-24 14:11 ` [PATCH 2/9] block/nvme: Have nvme_create_queue_pair() report errors consistently Philippe Mathieu-Daudé
2021-08-25 11:24   ` Klaus Jensen
2021-08-24 14:11 ` [PATCH 3/9] util/vfio-helpers: Let qemu_vfio_verify_mappings() use error_report() Philippe Mathieu-Daudé
2021-08-24 14:11 ` [PATCH 4/9] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD Philippe Mathieu-Daudé
2021-08-25 11:57   ` Klaus Jensen
2021-08-24 14:11 ` [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map() Philippe Mathieu-Daudé
2021-08-25 11:53   ` Klaus Jensen
2021-08-25 11:55     ` Klaus Jensen
2021-08-24 14:11 ` [PATCH 6/9] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map() Philippe Mathieu-Daudé
2021-08-24 14:11 ` [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error Philippe Mathieu-Daudé
2021-08-25 11:34   ` Klaus Jensen
2021-08-25 13:08     ` Philippe Mathieu-Daudé
2021-08-25 14:53       ` Klaus Jensen
2021-08-24 14:11 ` [PATCH 8/9] util/vfio-helpers: Let qemu_vfio_do_mapping() " Philippe Mathieu-Daudé
2021-08-24 14:11 ` [PATCH 9/9] block/nvme: Only report VFIO error on failed retry Philippe Mathieu-Daudé
2021-08-25 12:18   ` Klaus Jensen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.