[PATCH v2 00/11] block/nvme: Rework error reporting

[PATCH v2 00/11] block/nvme: Rework error reporting

[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).

Since v1:
- Addressed Klaus review comments (cleaner Error* handling)
- Add Klaus's R-b

Regards,

Phil.

Philippe Mathieu-Daudé (11):
  block/nvme: Use safer trace format string
  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()
  block/nvme: Have nvme_create_queue_pair() report errors consistently
  util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map()
  util/vfio-helpers: Extract qemu_vfio_water_mark_reached()
  util/vfio-helpers: Use error_setg in qemu_vfio_find_[fixed/temp]_iova
  util/vfio-helpers: Simplify qemu_vfio_dma_map() returning directly
  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         | 89 +++++++++++++++++++++----------------
 block/trace-events          |  2 +-
 4 files changed, 72 insertions(+), 50 deletions(-)

-- 
2.31.1

[PATCH v2 01/11] block/nvme: Use safer trace format string

[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]

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
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

[PATCH v2 02/11] util/vfio-helpers: Let qemu_vfio_verify_mappings() use error_report()

[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

Re: [PATCH v2 02/11] util/vfio-helpers: Let qemu_vfio_verify_mappings() use error_report()

[Klaus Jensen]

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

On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
> 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>

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

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

[PATCH v2 03/11] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD

[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.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
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

[PATCH v2 04/11] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map()

[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.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
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

[PATCH v2 05/11] block/nvme: Have nvme_create_queue_pair() report errors consistently

[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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index e8dbbc23177..0786c501e46 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);
@@ -239,6 +241,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);
     if (r) {
+        error_setg_errno(errp, -r, "Cannot map buffer for DMA");
         goto fail;
     }
     q->free_req_head = -1;
-- 
2.31.1

Re: [PATCH v2 05/11] block/nvme: Have nvme_create_queue_pair() report errors consistently

[Klaus Jensen]

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

On Aug 26 21:50, 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>

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

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

[PATCH v2 06/11] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map()

[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>
---
 include/qemu/vfio-helpers.h |  2 +-
 block/nvme.c                | 22 +++++++++++-----------
 util/vfio-helpers.c         | 10 ++++++----
 3 files changed, 18 insertions(+), 16 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 0786c501e46..80546b0babd 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,9 +238,9 @@ 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) {
-        error_setg_errno(errp, -r, "Cannot map buffer for DMA");
+        error_prepend(errp, "Cannot map buffer for DMA: ");
         goto fail;
     }
     q->free_req_head = -1;
@@ -534,9 +533,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;
     }
 
@@ -1032,7 +1031,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
@@ -1524,14 +1523,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..77cdec845d9 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;
-- 
2.31.1

Re: [PATCH v2 06/11] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map()

[Klaus Jensen]

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

On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  include/qemu/vfio-helpers.h |  2 +-
>  block/nvme.c                | 22 +++++++++++-----------
>  util/vfio-helpers.c         | 10 ++++++----
>  3 files changed, 18 insertions(+), 16 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 0786c501e46..80546b0babd 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,9 +238,9 @@ 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) {
> -        error_setg_errno(errp, -r, "Cannot map buffer for DMA");
> +        error_prepend(errp, "Cannot map buffer for DMA: ");
>          goto fail;
>      }
>      q->free_req_head = -1;
> @@ -534,9 +533,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;
>      }
>  
> @@ -1032,7 +1031,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
> @@ -1524,14 +1523,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..77cdec845d9 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)

Won't this break at this point? You add an Error** to qemu_vfio_dma_map,
but it is never used (so it probably stays NULL). So, above, if
qemu_vfio_dma_map() returns something not zero, you use error_prepend(),
which I think expects errp to be non-NULL?

I realize that later patches fixes this, but this sticks out when
reviewing this on its own.

>  {
>      int ret = 0;
>      int index;
> -- 
> 2.31.1
> 
> 


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

Re: [PATCH v2 06/11] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map()

[Philippe Mathieu-Daudé]

+Markus

On 8/27/21 7:48 AM, Klaus Jensen wrote:
> On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
>> 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>
>> ---
>>  include/qemu/vfio-helpers.h |  2 +-
>>  block/nvme.c                | 22 +++++++++++-----------
>>  util/vfio-helpers.c         | 10 ++++++----
>>  3 files changed, 18 insertions(+), 16 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 0786c501e46..80546b0babd 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,9 +238,9 @@ 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) {
>> -        error_setg_errno(errp, -r, "Cannot map buffer for DMA");
>> +        error_prepend(errp, "Cannot map buffer for DMA: ");
>>          goto fail;
>>      }
>>      q->free_req_head = -1;
>> @@ -534,9 +533,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;
>>      }
>>  
>> @@ -1032,7 +1031,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
>> @@ -1524,14 +1523,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..77cdec845d9 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)
> 
> Won't this break at this point? You add an Error** to qemu_vfio_dma_map,
> but it is never used (so it probably stays NULL). So, above, if
> qemu_vfio_dma_map() returns something not zero, you use error_prepend(),
> which I think expects errp to be non-NULL?

I can't find such restriction documented in "qapi/error.h".

In error.c:

void error_prepend(Error *const *errp, const char *fmt, ...)
{
    va_list ap;

    va_start(ap, fmt);
    error_vprepend(errp, fmt, ap);
    ...

void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
{
    GString *newmsg;

    if (!errp) {
        return;
    }
    ...

> 
> I realize that later patches fixes this, but this sticks out when
> reviewing this on its own.
> 
>>  {
>>      int ret = 0;
>>      int index;
>> -- 
>> 2.31.1
>>
>>
> 

Re: [PATCH v2 06/11] util/vfio-helpers: Pass Error handle to qemu_vfio_dma_map()

[Klaus Jensen]

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

On Aug 27 08:24, Philippe Mathieu-Daudé wrote:
> +Markus
> 
> On 8/27/21 7:48 AM, Klaus Jensen wrote:
> > On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
> >> 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>
> >> ---
> >>  include/qemu/vfio-helpers.h |  2 +-
> >>  block/nvme.c                | 22 +++++++++++-----------
> >>  util/vfio-helpers.c         | 10 ++++++----
> >>  3 files changed, 18 insertions(+), 16 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 0786c501e46..80546b0babd 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,9 +238,9 @@ 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) {
> >> -        error_setg_errno(errp, -r, "Cannot map buffer for DMA");
> >> +        error_prepend(errp, "Cannot map buffer for DMA: ");
> >>          goto fail;
> >>      }
> >>      q->free_req_head = -1;
> >> @@ -534,9 +533,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;
> >>      }
> >>  
> >> @@ -1032,7 +1031,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
> >> @@ -1524,14 +1523,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..77cdec845d9 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)
> > 
> > Won't this break at this point? You add an Error** to qemu_vfio_dma_map,
> > but it is never used (so it probably stays NULL). So, above, if
> > qemu_vfio_dma_map() returns something not zero, you use error_prepend(),
> > which I think expects errp to be non-NULL?
> 
> I can't find such restriction documented in "qapi/error.h".
> 
> In error.c:
> 
> void error_prepend(Error *const *errp, const char *fmt, ...)
> {
>     va_list ap;
> 
>     va_start(ap, fmt);
>     error_vprepend(errp, fmt, ap);
>     ...
> 
> void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
> {
>     GString *newmsg;
> 
>     if (!errp) {
>         return;
>     }
>     ...
> 

Sorry, my bad - I should have checked myself. In that case,

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

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

[PATCH v2 07/11] util/vfio-helpers: Extract qemu_vfio_water_mark_reached()

[Philippe Mathieu-Daudé]

Extract qemu_vfio_water_mark_reached() for readability,
and have it provide an error hint it its Error* handle.

Suggested-by: Klaus Jensen <k.jensen@samsung.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 77cdec845d9..306b5a83e48 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -721,6 +721,21 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
     return -ENOMEM;
 }
 
+/**
+ * qemu_vfio_water_mark_reached:
+ *
+ * Returns %true if high watermark has been reached, %false otherwise.
+ */
+static bool qemu_vfio_water_mark_reached(QEMUVFIOState *s, size_t size,
+                                         Error **errp)
+{
+    if (s->high_water_mark - s->low_water_mark + 1 < size) {
+        error_setg(errp, "iova exhausted (water mark reached)");
+        return true;
+    }
+    return false;
+}
+
 /* Map [host, host + size) area into a contiguous IOVA address space, and store
  * the result in @iova if not NULL. The caller need to make sure the area is
  * aligned to page size, and mustn't overlap with existing mapping areas (split
@@ -742,7 +757,7 @@ 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 {
-        if (s->high_water_mark - s->low_water_mark + 1 < size) {
+        if (qemu_vfio_water_mark_reached(s, size, errp)) {
             ret = -ENOMEM;
             goto out;
         }
-- 
2.31.1

Re: [PATCH v2 07/11] util/vfio-helpers: Extract qemu_vfio_water_mark_reached()

[Klaus Jensen]

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

On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
> Extract qemu_vfio_water_mark_reached() for readability,
> and have it provide an error hint it its Error* handle.
> 
> Suggested-by: Klaus Jensen <k.jensen@samsung.com>
> 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 --]

[PATCH v2 08/11] util/vfio-helpers: Use error_setg in qemu_vfio_find_[fixed/temp]_iova

[Philippe Mathieu-Daudé]

Have qemu_vfio_find_fixed_iova() and qemu_vfio_find_temp_iova()
propagate eventual errors to callers.

Suggested-by: Klaus Jensen <k.jensen@samsung.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 306b5a83e48..7de5081dbd3 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -678,7 +678,8 @@ static bool qemu_vfio_verify_mappings(QEMUVFIOState *s)
 }
 
 static int
-qemu_vfio_find_fixed_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
+qemu_vfio_find_fixed_iova(QEMUVFIOState *s, size_t size, uint64_t *iova,
+                          Error **errp)
 {
     int i;
 
@@ -696,11 +697,14 @@ qemu_vfio_find_fixed_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
             return 0;
         }
     }
+    error_setg(errp, "fixed iova range not found");
+
     return -ENOMEM;
 }
 
 static int
-qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
+qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova,
+                         Error **errp)
 {
     int i;
 
@@ -718,6 +722,8 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
             return 0;
         }
     }
+    error_setg(errp, "temporary iova range not found");
+
     return -ENOMEM;
 }
 
@@ -762,7 +768,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
             goto out;
         }
         if (!temporary) {
-            if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
+            if (qemu_vfio_find_fixed_iova(s, size, &iova0, errp) < 0) {
                 ret = -ENOMEM;
                 goto out;
             }
@@ -776,7 +782,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
             }
             qemu_vfio_dump_mappings(s);
         } else {
-            if (qemu_vfio_find_temp_iova(s, size, &iova0)) {
+            if (qemu_vfio_find_temp_iova(s, size, &iova0, errp)) {
                 ret = -ENOMEM;
                 goto out;
             }
-- 
2.31.1

Re: [PATCH v2 08/11] util/vfio-helpers: Use error_setg in qemu_vfio_find_[fixed/temp]_iova

[Klaus Jensen]

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

On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
> Have qemu_vfio_find_fixed_iova() and qemu_vfio_find_temp_iova()
> propagate eventual errors to callers.
> 
> Suggested-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  util/vfio-helpers.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 306b5a83e48..7de5081dbd3 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -678,7 +678,8 @@ static bool qemu_vfio_verify_mappings(QEMUVFIOState *s)
>  }
>  
>  static int
> -qemu_vfio_find_fixed_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
> +qemu_vfio_find_fixed_iova(QEMUVFIOState *s, size_t size, uint64_t *iova,
> +                          Error **errp)
>  {
>      int i;
>  
> @@ -696,11 +697,14 @@ qemu_vfio_find_fixed_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
>              return 0;
>          }
>      }
> +    error_setg(errp, "fixed iova range not found");
> +
>      return -ENOMEM;
>  }
>  
>  static int
> -qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
> +qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova,
> +                         Error **errp)
>  {
>      int i;
>  
> @@ -718,6 +722,8 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
>              return 0;
>          }
>      }
> +    error_setg(errp, "temporary iova range not found");
> +
>      return -ENOMEM;
>  }
>  
> @@ -762,7 +768,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>              goto out;
>          }
>          if (!temporary) {
> -            if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
> +            if (qemu_vfio_find_fixed_iova(s, size, &iova0, errp) < 0) {
>                  ret = -ENOMEM;
>                  goto out;
>              }
> @@ -776,7 +782,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>              }
>              qemu_vfio_dump_mappings(s);
>          } else {
> -            if (qemu_vfio_find_temp_iova(s, size, &iova0)) {
> +            if (qemu_vfio_find_temp_iova(s, size, &iova0, errp)) {
>                  ret = -ENOMEM;
>                  goto out;
>              }

Not related to this patch, but it is slightly confusing that these
functions actually already return a negative errno, but then we
overwrite it.

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

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

Re: [PATCH v2 08/11] util/vfio-helpers: Use error_setg in qemu_vfio_find_[fixed/temp]_iova

[Philippe Mathieu-Daudé]

On 8/27/21 7:57 AM, Klaus Jensen wrote:
> On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
>> Have qemu_vfio_find_fixed_iova() and qemu_vfio_find_temp_iova()
>> propagate eventual errors to callers.
>>
>> Suggested-by: Klaus Jensen <k.jensen@samsung.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  util/vfio-helpers.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index 306b5a83e48..7de5081dbd3 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c

>>  static int
>> -qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
>> +qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova,
>> +                         Error **errp)
>>  {
>>      int i;
>>  
>> @@ -718,6 +722,8 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
>>              return 0;
>>          }
>>      }
>> +    error_setg(errp, "temporary iova range not found");
>> +
>>      return -ENOMEM;
>>  }

>> @@ -776,7 +782,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>>              }
>>              qemu_vfio_dump_mappings(s);
>>          } else {
>> -            if (qemu_vfio_find_temp_iova(s, size, &iova0)) {
>> +            if (qemu_vfio_find_temp_iova(s, size, &iova0, errp)) {
>>                  ret = -ENOMEM;
>>                  goto out;
>>              }
> 
> Not related to this patch, but it is slightly confusing that these
> functions actually already return a negative errno, but then we
> overwrite it.

Good point. I'll make it return a boolean.

[PATCH v2 09/11] util/vfio-helpers: Simplify qemu_vfio_dma_map() returning directly

[Philippe Mathieu-Daudé]

To simplify qemu_vfio_dma_map():
- reduce 'ret' (returned value) scope by returning errno directly,
- remove the goto 'out' label.

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

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 7de5081dbd3..48b4384e8c8 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -750,7 +750,6 @@ static bool qemu_vfio_water_mark_reached(QEMUVFIOState *s, size_t size,
 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;
@@ -763,32 +762,31 @@ 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 (qemu_vfio_water_mark_reached(s, size, errp)) {
-            ret = -ENOMEM;
-            goto out;
+            return -ENOMEM;
         }
         if (!temporary) {
             if (qemu_vfio_find_fixed_iova(s, size, &iova0, errp) < 0) {
-                ret = -ENOMEM;
-                goto out;
+                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, errp)) {
-                ret = -ENOMEM;
-                goto out;
+                return -ENOMEM;
             }
             ret = qemu_vfio_do_mapping(s, host, size, iova0);
-            if (ret) {
-                goto out;
+            if (ret < 0) {
+                return ret;
             }
         }
     }
@@ -796,8 +794,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
     if (iova) {
         *iova = iova0;
     }
-out:
-    return ret;
+    return 0;
 }
 
 /* Reset the high watermark and free all "temporary" mappings. */
-- 
2.31.1

Re: [PATCH v2 09/11] util/vfio-helpers: Simplify qemu_vfio_dma_map() returning directly

[Klaus Jensen]

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

On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
> To simplify qemu_vfio_dma_map():
> - reduce 'ret' (returned value) scope by returning errno directly,
> - remove the goto 'out' label.
> 
> 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 --]

[PATCH v2 10/11] util/vfio-helpers: Let qemu_vfio_do_mapping() propagate Error

[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 48b4384e8c8..0c011b3dbc8 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;
@@ -774,7 +774,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;
@@ -784,7 +784,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
             if (qemu_vfio_find_temp_iova(s, size, &iova0, errp)) {
                 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

Re: [PATCH v2 10/11] util/vfio-helpers: Let qemu_vfio_do_mapping() propagate Error

[Klaus Jensen]

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

On Aug 26 21:50, Philippe Mathieu-Daudé wrote:
> 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>

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

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

[PATCH v2 11/11] block/nvme: Only report VFIO error on failed retry

[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>
Reviewed-by: Klaus Jensen <k.jensen@samsung.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