[PATCH 0/9] block/nvme: Rework error reporting

[PATCH 0/9] 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).

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

[PATCH 1/9] 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]

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

Re: [PATCH 1/9] block/nvme: Use safer trace format string

[Klaus Jensen]

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

[PATCH 2/9] 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 | 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

Re: [PATCH 2/9] block/nvme: Have nvme_create_queue_pair() report errors consistently

[Klaus Jensen]

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

[PATCH 3/9] 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

[PATCH 4/9] 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.

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

Re: [PATCH 4/9] util/vfio-helpers: Replace qemu_mutex_lock() calls with QEMU_LOCK_GUARD

[Klaus Jensen]

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

[PATCH 5/9] 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.

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

Re: [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map()

[Klaus Jensen]

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

Re: [PATCH 5/9] util/vfio-helpers: Remove unreachable code in qemu_vfio_dma_map()

[Klaus Jensen]

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

[PATCH 6/9] 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>
---
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

[PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error

[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

Re: [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error

[Klaus Jensen]

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

Re: [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error

[Klaus Jensen]

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

[PATCH 8/9] 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 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

[PATCH 9/9] 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>
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

Re: [PATCH 9/9] block/nvme: Only report VFIO error on failed retry

[Klaus Jensen]

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