[PATCH 00/12] hw/nvme: misc fixes and updates

[PATCH 00/12] hw/nvme: misc fixes and updates

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

This series includes a couple of misc fixes as well as some cleanup
pertaining to the aio handling in flush, dsm, copy and zone reset. As
Jinhao gets around to iothread stuff, it might come in handy to have
this stuff cleaned up a bit.

Dmitrys fix (nvme-next commit "hw/nvme: add missing return statement")
for dsm prompted me to audit the flush, dsm, zone reset and copy code
and that resulted in the discovery of some bugs and some general clean
up.

Klaus Jensen (12):
  hw/nvme: fix incorrect use of errp/local_err
  hw/nvme: remove redundant passing of PCIDevice
  hw/nvme: cleanup error reporting in nvme_init_pci()
  hw/nvme: fix numzrwa handling
  hw/nvme: fix accidental reintroduction of redundant code
  hw/nvme: fix cancellation of format operations
  hw/nvme: fix flush cancel
  hw/nvme: rework flush bh scheduling
  hw/nvme: improve cancellation handling in zone reset
  hw/nvme: improve cancellation handling in dsm
  hw/nvme: simplify copy command error handling
  hw/nvme: align logic of format with flush

 hw/nvme/ctrl.c | 252 +++++++++++++++++++++++--------------------------
 hw/nvme/ns.c   |   4 +-
 2 files changed, 119 insertions(+), 137 deletions(-)

-- 
2.36.1

[PATCH 01/12] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Make nvme_check_constraints() return an int and fix incorrect use of
errp/local_err.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index d349b3e42620..c8c4a0718fc4 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6808,7 +6808,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
     },
 };
 
-static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
+static int nvme_check_params(NvmeCtrl *n, Error **errp)
 {
     NvmeParams *params = &n->params;
 
@@ -6822,38 +6822,38 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
     if (n->namespace.blkconf.blk && n->subsys) {
         error_setg(errp, "subsystem support is unavailable with legacy "
                    "namespace ('drive' property)");
-        return;
+        return -1;
     }
 
     if (params->max_ioqpairs < 1 ||
         params->max_ioqpairs > NVME_MAX_IOQPAIRS) {
         error_setg(errp, "max_ioqpairs must be between 1 and %d",
                    NVME_MAX_IOQPAIRS);
-        return;
+        return -1;
     }
 
     if (params->msix_qsize < 1 ||
         params->msix_qsize > PCI_MSIX_FLAGS_QSIZE + 1) {
         error_setg(errp, "msix_qsize must be between 1 and %d",
                    PCI_MSIX_FLAGS_QSIZE + 1);
-        return;
+        return -1;
     }
 
     if (!params->serial) {
         error_setg(errp, "serial property not set");
-        return;
+        return -1;
     }
 
     if (n->pmr.dev) {
         if (host_memory_backend_is_mapped(n->pmr.dev)) {
             error_setg(errp, "can't use already busy memdev: %s",
                        object_get_canonical_path_component(OBJECT(n->pmr.dev)));
-            return;
+            return -1;
         }
 
         if (!is_power_of_2(n->pmr.dev->size)) {
             error_setg(errp, "pmr backend size needs to be power of 2 in size");
-            return;
+            return -1;
         }
 
         host_memory_backend_set_mapped(n->pmr.dev, true);
@@ -6862,64 +6862,64 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
     if (n->params.zasl > n->params.mdts) {
         error_setg(errp, "zoned.zasl (Zone Append Size Limit) must be less "
                    "than or equal to mdts (Maximum Data Transfer Size)");
-        return;
+        return -1;
     }
 
     if (!n->params.vsl) {
         error_setg(errp, "vsl must be non-zero");
-        return;
+        return -1;
     }
 
     if (params->sriov_max_vfs) {
         if (!n->subsys) {
             error_setg(errp, "subsystem is required for the use of SR-IOV");
-            return;
+            return -1;
         }
 
         if (params->sriov_max_vfs > NVME_MAX_VFS) {
             error_setg(errp, "sriov_max_vfs must be between 0 and %d",
                        NVME_MAX_VFS);
-            return;
+            return -1;
         }
 
         if (params->cmb_size_mb) {
             error_setg(errp, "CMB is not supported with SR-IOV");
-            return;
+            return -1;
         }
 
         if (n->pmr.dev) {
             error_setg(errp, "PMR is not supported with SR-IOV");
-            return;
+            return -1;
         }
 
         if (!params->sriov_vq_flexible || !params->sriov_vi_flexible) {
             error_setg(errp, "both sriov_vq_flexible and sriov_vi_flexible"
                        " must be set for the use of SR-IOV");
-            return;
+            return -1;
         }
 
         if (params->sriov_vq_flexible < params->sriov_max_vfs * 2) {
             error_setg(errp, "sriov_vq_flexible must be greater than or equal"
                        " to %d (sriov_max_vfs * 2)", params->sriov_max_vfs * 2);
-            return;
+            return -1;
         }
 
         if (params->max_ioqpairs < params->sriov_vq_flexible + 2) {
             error_setg(errp, "(max_ioqpairs - sriov_vq_flexible) must be"
                        " greater than or equal to 2");
-            return;
+            return -1;
         }
 
         if (params->sriov_vi_flexible < params->sriov_max_vfs) {
             error_setg(errp, "sriov_vi_flexible must be greater than or equal"
                        " to %d (sriov_max_vfs)", params->sriov_max_vfs);
-            return;
+            return -1;
         }
 
         if (params->msix_qsize < params->sriov_vi_flexible + 1) {
             error_setg(errp, "(msix_qsize - sriov_vi_flexible) must be"
                        " greater than or equal to 1");
-            return;
+            return -1;
         }
 
         if (params->sriov_max_vi_per_vf &&
@@ -6927,7 +6927,7 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
             error_setg(errp, "sriov_max_vi_per_vf must meet:"
                        " (sriov_max_vi_per_vf - 1) %% %d == 0 and"
                        " sriov_max_vi_per_vf >= 1", NVME_VF_RES_GRANULARITY);
-            return;
+            return -1;
         }
 
         if (params->sriov_max_vq_per_vf &&
@@ -6936,9 +6936,11 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
             error_setg(errp, "sriov_max_vq_per_vf must meet:"
                        " (sriov_max_vq_per_vf - 1) %% %d == 0 and"
                        " sriov_max_vq_per_vf >= 2", NVME_VF_RES_GRANULARITY);
-            return;
+            return -1;
         }
     }
+
+    return 0;
 }
 
 static void nvme_init_state(NvmeCtrl *n)
@@ -7336,7 +7338,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeNamespace *ns;
-    Error *local_err = NULL;
     NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));
 
     if (pci_is_vf(pci_dev)) {
@@ -7348,9 +7349,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         n->subsys = pn->subsys;
     }
 
-    nvme_check_constraints(n, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (nvme_check_params(n, errp)) {
         return;
     }
 
@@ -7358,7 +7357,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
               &pci_dev->qdev, n->parent_obj.qdev.id);
 
     if (nvme_init_subsys(n, errp)) {
-        error_propagate(errp, local_err);
         return;
     }
     nvme_init_state(n);
-- 
2.36.1

[PATCH 02/12] hw/nvme: remove redundant passing of PCIDevice

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

The NvmeCtrl is a PCIDevice, so remove the redundant passing of the
PCIDevice parameter.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index c8c4a0718fc4..b688afccbe5a 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7007,8 +7007,9 @@ static void nvme_init_state(NvmeCtrl *n)
     }
 }
 
-static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
+static void nvme_init_cmb(NvmeCtrl *n)
 {
+    PCIDevice *pci_dev = PCI_DEVICE(n);
     uint64_t cmb_size = n->params.cmb_size_mb * MiB;
     uint64_t cap = ldq_le_p(&n->bar.cap);
 
@@ -7029,8 +7030,9 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
     }
 }
 
-static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
+static void nvme_init_pmr(NvmeCtrl *n)
 {
+    PCIDevice *pci_dev = PCI_DEVICE(n);
     uint32_t pmrcap = ldl_le_p(&n->bar.pmrcap);
 
     NVME_PMRCAP_SET_RDS(pmrcap, 1);
@@ -7116,8 +7118,9 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
     return 0;
 }
 
-static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
+static int nvme_init_pci(NvmeCtrl *n, Error **errp)
 {
+    PCIDevice *pci_dev = PCI_DEVICE(n);
     uint8_t *pci_conf = pci_dev->config;
     uint64_t bar_size;
     unsigned msix_table_offset, msix_pba_offset;
@@ -7174,11 +7177,11 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
     nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
 
     if (n->params.cmb_size_mb) {
-        nvme_init_cmb(n, pci_dev);
+        nvme_init_cmb(n);
     }
 
     if (n->pmr.dev) {
-        nvme_init_pmr(n, pci_dev);
+        nvme_init_pmr(n);
     }
 
     if (!pci_is_vf(pci_dev) && n->params.sriov_max_vfs) {
@@ -7201,8 +7204,9 @@ static void nvme_init_subnqn(NvmeCtrl *n)
     }
 }
 
-static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
+static void nvme_init_ctrl(NvmeCtrl *n)
 {
+    PCIDevice *pci_dev = PCI_DEVICE(n);
     NvmeIdCtrl *id = &n->id_ctrl;
     uint8_t *pci_conf = pci_dev->config;
     uint64_t cap = ldq_le_p(&n->bar.cap);
@@ -7360,10 +7364,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
     nvme_init_state(n);
-    if (nvme_init_pci(n, pci_dev, errp)) {
+    if (nvme_init_pci(n, errp)) {
         return;
     }
-    nvme_init_ctrl(n, pci_dev);
+
+    nvme_init_ctrl(n);
 
     /* setup a namespace if the controller drive property was given */
     if (n->namespace.blkconf.blk) {
-- 
2.36.1

[PATCH 03/12] hw/nvme: cleanup error reporting in nvme_init_pci()

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Replace the local Error variable with errp and ERRP_GUARD().

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index b688afccbe5a..af82daa304bf 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7120,14 +7120,14 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
 
 static int nvme_init_pci(NvmeCtrl *n, Error **errp)
 {
+    ERRP_GUARD();
+
     PCIDevice *pci_dev = PCI_DEVICE(n);
     uint8_t *pci_conf = pci_dev->config;
     uint64_t bar_size;
     unsigned msix_table_offset, msix_pba_offset;
     int ret;
 
-    Error *err = NULL;
-
     pci_conf[PCI_INTERRUPT_PIN] = 1;
     pci_config_set_prog_interface(pci_conf, 0x2);
 
@@ -7164,13 +7164,13 @@ static int nvme_init_pci(NvmeCtrl *n, Error **errp)
     }
     ret = msix_init(pci_dev, n->params.msix_qsize,
                     &n->bar0, 0, msix_table_offset,
-                    &n->bar0, 0, msix_pba_offset, 0, &err);
+                    &n->bar0, 0, msix_pba_offset, 0, errp);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
-            warn_report_err(err);
+            warn_report_err(*errp);
+            *errp = NULL;
         } else {
-            error_propagate(errp, err);
-            return ret;
+            return -1;
         }
     }
 
-- 
2.36.1

[PATCH 04/12] hw/nvme: fix numzrwa handling

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Number of ZRWA Resources should be initialized to Max Active Resources,
and not the total number of zones.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 8 ++------
 hw/nvme/ns.c   | 4 ++--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index af82daa304bf..a09700455c02 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1765,9 +1765,7 @@ static uint16_t nvme_zrm_finish(NvmeNamespace *ns, NvmeZone *zone)
 
         if (zone->d.za & NVME_ZA_ZRWA_VALID) {
             zone->d.za &= ~NVME_ZA_ZRWA_VALID;
-            if (ns->params.numzrwa) {
-                ns->zns.numzrwa++;
-            }
+            ns->zns.numzrwa++;
         }
 
         /* fallthrough */
@@ -1807,9 +1805,7 @@ static uint16_t nvme_zrm_reset(NvmeNamespace *ns, NvmeZone *zone)
         nvme_aor_dec_active(ns);
 
         if (zone->d.za & NVME_ZA_ZRWA_VALID) {
-            if (ns->params.numzrwa) {
-                ns->zns.numzrwa++;
-            }
+            ns->zns.numzrwa++;
         }
 
         /* fallthrough */
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 870c3ca1a2f0..b9b35b3c3bc9 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -286,7 +286,7 @@ static void nvme_ns_init_zoned(NvmeNamespace *ns)
 
     if (ns->params.zrwas) {
         ns->zns.numzrwa = ns->params.numzrwa ?
-            ns->params.numzrwa : ns->num_zones;
+            ns->params.numzrwa : ns->params.max_active_zones;
 
         ns->zns.zrwas = ns->params.zrwas >> ns->lbaf.ds;
         ns->zns.zrwafg = ns->params.zrwafg >> ns->lbaf.ds;
@@ -294,7 +294,7 @@ static void nvme_ns_init_zoned(NvmeNamespace *ns)
         id_ns_z->ozcs |= NVME_ID_NS_ZONED_OZCS_ZRWASUP;
         id_ns_z->zrwacap = NVME_ID_NS_ZONED_ZRWACAP_EXPFLUSHSUP;
 
-        id_ns_z->numzrwa = cpu_to_le32(ns->params.numzrwa);
+        id_ns_z->numzrwa = cpu_to_le32(ns->zns.numzrwa - 1);
         id_ns_z->zrwas = cpu_to_le16(ns->zns.zrwas);
         id_ns_z->zrwafg = cpu_to_le16(ns->zns.zrwafg);
     }
-- 
2.36.1

[PATCH 05/12] hw/nvme: fix accidental reintroduction of redundant code

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Commit 44219b6029fc ("hw/nvme: 64-bit pi support") accidentially
reintroduced code that was removed in commit a6de6ed5092c ("hw/nvme:
move format parameter parsing").

It is beneign, but get rid of it anyway.

Fixes: 44219b6029fc ("hw/nvme: 64-bit pi support")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index a09700455c02..f10334fc1d3f 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5725,9 +5725,6 @@ static void nvme_format_bh(void *opaque)
     NvmeFormatAIOCB *iocb = opaque;
     NvmeRequest *req = iocb->req;
     NvmeCtrl *n = nvme_ctrl(req);
-    uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
-    uint8_t lbaf = dw10 & 0xf;
-    uint8_t pi = (dw10 >> 5) & 0x7;
     uint16_t status;
     int i;
 
@@ -5749,7 +5746,7 @@ static void nvme_format_bh(void *opaque)
         goto done;
     }
 
-    status = nvme_format_check(iocb->ns, lbaf, pi);
+    status = nvme_format_check(iocb->ns, iocb->lbaf, iocb->pi);
     if (status) {
         req->status = status;
         goto done;
-- 
2.36.1

[PATCH 06/12] hw/nvme: fix cancellation of format operations

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Cancelling a format operation neglects to set iocb->ret as well as
clearing the iocb->aiocb after cancelling the underlying aiocb.

Fix this.

Fixes: 3bcf26d3d619 ("hw/nvme: reimplement format nvm to allow cancellation")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index f10334fc1d3f..a85eabfa8bfd 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5640,8 +5640,11 @@ static void nvme_format_cancel(BlockAIOCB *aiocb)
 {
     NvmeFormatAIOCB *iocb = container_of(aiocb, NvmeFormatAIOCB, common);
 
+    iocb->ret = -ECANCELED;
+
     if (iocb->aiocb) {
         blk_aio_cancel_async(iocb->aiocb);
+        iocb->aiocb = NULL;
     }
 }
 
-- 
2.36.1

[PATCH 07/12] hw/nvme: fix flush cancel

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Make sure that iocb->aiocb is NULL'ed when cancelling.

Fixes: 38f4ac65ac88 ("hw/nvme: reimplement flush to allow cancellation")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index a85eabfa8bfd..cea90cf65ce8 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3158,6 +3158,7 @@ static void nvme_flush_cancel(BlockAIOCB *acb)
 
     if (iocb->aiocb) {
         blk_aio_cancel_async(iocb->aiocb);
+        iocb->aiocb = NULL;
     }
 }
 
-- 
2.36.1

[PATCH 08/12] hw/nvme: rework flush bh scheduling

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Remove an initial bh scheduling and move the completion invocation into
a separate bh. Also, make sure that iocb->aiocb is NULL'ed when
cancelling.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index cea90cf65ce8..1acf88d9e8c7 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3168,6 +3168,19 @@ static const AIOCBInfo nvme_flush_aiocb_info = {
     .get_aio_context = nvme_get_aio_context,
 };
 
+static void nvme_flush_bh(void *opaque)
+{
+    NvmeFlushAIOCB *iocb = opaque;
+
+    iocb->common.cb(iocb->common.opaque, iocb->ret);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+    qemu_aio_unref(iocb);
+}
+
+static void nvme_do_flush(NvmeFlushAIOCB *iocb);
+
 static void nvme_flush_ns_cb(void *opaque, int ret)
 {
     NvmeFlushAIOCB *iocb = opaque;
@@ -3189,13 +3202,11 @@ static void nvme_flush_ns_cb(void *opaque, int ret)
     }
 
 out:
-    iocb->aiocb = NULL;
-    qemu_bh_schedule(iocb->bh);
+    nvme_do_flush(iocb);
 }
 
-static void nvme_flush_bh(void *opaque)
+static void nvme_do_flush(NvmeFlushAIOCB *iocb)
 {
-    NvmeFlushAIOCB *iocb = opaque;
     NvmeRequest *req = iocb->req;
     NvmeCtrl *n = nvme_ctrl(req);
     int i;
@@ -3222,14 +3233,9 @@ static void nvme_flush_bh(void *opaque)
     return;
 
 done:
-    qemu_bh_delete(iocb->bh);
-    iocb->bh = NULL;
-
-    iocb->common.cb(iocb->common.opaque, iocb->ret);
-
-    qemu_aio_unref(iocb);
-
-    return;
+    if (iocb->bh) {
+        qemu_bh_schedule(iocb->bh);
+    }
 }
 
 static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
@@ -3263,7 +3269,7 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
     }
 
     req->aiocb = &iocb->common;
-    qemu_bh_schedule(iocb->bh);
+    nvme_do_flush(iocb);
 
     return NVME_NO_COMPLETE;
 
-- 
2.36.1

[PATCH 09/12] hw/nvme: improve cancellation handling in zone reset

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

If the zone reset operation is cancelled but the block unmap operation
completes normally, the callback will continue resetting the next zone
since it neglects to check iocb->ret which will have been set to
-ECANCELED. Make sure that this is checked and bail out if an error is
present.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 1acf88d9e8c7..9add74753fd8 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3760,14 +3760,8 @@ static void nvme_zone_reset_epilogue_cb(void *opaque, int ret)
     int64_t moff;
     int count;
 
-    if (ret < 0) {
-        nvme_zone_reset_cb(iocb, ret);
-        return;
-    }
-
-    if (!ns->lbaf.ms) {
-        nvme_zone_reset_cb(iocb, 0);
-        return;
+    if (ret < 0 || iocb->ret < 0 || !ns->lbaf.ms) {
+        goto out;
     }
 
     moff = nvme_moff(ns, iocb->zone->d.zslba);
@@ -3777,6 +3771,9 @@ static void nvme_zone_reset_epilogue_cb(void *opaque, int ret)
                                         BDRV_REQ_MAY_UNMAP,
                                         nvme_zone_reset_cb, iocb);
     return;
+
+out:
+    nvme_zone_reset_cb(iocb, ret);
 }
 
 static void nvme_zone_reset_cb(void *opaque, int ret)
@@ -3788,6 +3785,8 @@ static void nvme_zone_reset_cb(void *opaque, int ret)
     if (ret < 0) {
         iocb->ret = ret;
         goto done;
+    } else if (iocb->ret < 0) {
+        goto done;
     }
 
     if (iocb->zone) {
-- 
2.36.1

[PATCH 10/12] hw/nvme: improve cancellation handling in dsm

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

When the DSM operation is cancelled asynchronously, we set iocb->ret to
-ECANCELED. However, the callback function only checks the return value
of the completed aio, which may have completed succesfully prior to the
cancellation and thus the callback ends up continuing the dsm operation
instead of bailing out. Fix this.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 9add74753fd8..5eee730ed0cf 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2367,16 +2367,10 @@ static void nvme_dsm_md_cb(void *opaque, int ret)
     uint64_t slba;
     uint32_t nlb;
 
-    if (ret < 0) {
-        iocb->ret = ret;
+    if (ret < 0 || iocb->ret < 0 || !ns->lbaf.ms) {
         goto done;
     }
 
-    if (!ns->lbaf.ms) {
-        nvme_dsm_cb(iocb, 0);
-        return;
-    }
-
     range = &iocb->range[iocb->idx - 1];
     slba = le64_to_cpu(range->slba);
     nlb = le32_to_cpu(range->nlb);
@@ -2389,7 +2383,6 @@ static void nvme_dsm_md_cb(void *opaque, int ret)
     ret = nvme_block_status_all(ns, slba, nlb, BDRV_BLOCK_ZERO);
     if (ret) {
         if (ret < 0) {
-            iocb->ret = ret;
             goto done;
         }
 
@@ -2403,8 +2396,7 @@ static void nvme_dsm_md_cb(void *opaque, int ret)
     return;
 
 done:
-    iocb->aiocb = NULL;
-    qemu_bh_schedule(iocb->bh);
+    nvme_dsm_cb(iocb, ret);
 }
 
 static void nvme_dsm_cb(void *opaque, int ret)
@@ -2420,6 +2412,8 @@ static void nvme_dsm_cb(void *opaque, int ret)
     if (ret < 0) {
         iocb->ret = ret;
         goto done;
+    } else if (iocb->ret < 0) {
+        goto done;
     }
 
 next:
-- 
2.36.1

[PATCH 11/12] hw/nvme: simplify copy command error handling

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Move error handling down in the call stack to simplify the control flow.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 50 ++++++++++++--------------------------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 5eee730ed0cf..4d6b4c9f00ba 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2614,7 +2614,7 @@ static void nvme_copy_bh(void *opaque)
     qemu_aio_unref(iocb);
 }
 
-static void nvme_copy_cb(void *opaque, int ret);
+static void nvme_do_copy(NvmeCopyAIOCB *iocb);
 
 static void nvme_copy_source_range_parse_format0(void *ranges, int idx,
                                                  uint64_t *slba, uint32_t *nlb,
@@ -2726,7 +2726,7 @@ static void nvme_copy_out_completed_cb(void *opaque, int ret)
     iocb->idx++;
     iocb->slba += nlb;
 out:
-    nvme_copy_cb(iocb, iocb->ret);
+    nvme_do_copy(iocb);
 }
 
 static void nvme_copy_out_cb(void *opaque, int ret)
@@ -2738,16 +2738,8 @@ static void nvme_copy_out_cb(void *opaque, int ret)
     size_t mlen;
     uint8_t *mbounce;
 
-    if (ret < 0) {
-        iocb->ret = ret;
+    if (ret < 0 || iocb->ret < 0 || !ns->lbaf.ms) {
         goto out;
-    } else if (iocb->ret < 0) {
-        goto out;
-    }
-
-    if (!ns->lbaf.ms) {
-        nvme_copy_out_completed_cb(iocb, 0);
-        return;
     }
 
     nvme_copy_source_range_parse(iocb->ranges, iocb->idx, iocb->format, NULL,
@@ -2766,7 +2758,7 @@ static void nvme_copy_out_cb(void *opaque, int ret)
     return;
 
 out:
-    nvme_copy_cb(iocb, ret);
+    nvme_copy_out_completed_cb(iocb, ret);
 }
 
 static void nvme_copy_in_completed_cb(void *opaque, int ret)
@@ -2860,15 +2852,9 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret)
 
 invalid:
     req->status = status;
-    iocb->aiocb = NULL;
-    if (iocb->bh) {
-        qemu_bh_schedule(iocb->bh);
-    }
-
-    return;
-
+    iocb->ret = -1;
 out:
-    nvme_copy_cb(iocb, ret);
+    nvme_do_copy(iocb);
 }
 
 static void nvme_copy_in_cb(void *opaque, int ret)
@@ -2879,16 +2865,8 @@ static void nvme_copy_in_cb(void *opaque, int ret)
     uint64_t slba;
     uint32_t nlb;
 
-    if (ret < 0) {
-        iocb->ret = ret;
+    if (ret < 0 || iocb->ret < 0 || !ns->lbaf.ms) {
         goto out;
-    } else if (iocb->ret < 0) {
-        goto out;
-    }
-
-    if (!ns->lbaf.ms) {
-        nvme_copy_in_completed_cb(iocb, 0);
-        return;
     }
 
     nvme_copy_source_range_parse(iocb->ranges, iocb->idx, iocb->format, &slba,
@@ -2904,12 +2882,11 @@ static void nvme_copy_in_cb(void *opaque, int ret)
     return;
 
 out:
-    nvme_copy_cb(iocb, iocb->ret);
+    nvme_copy_in_completed_cb(iocb, ret);
 }
 
-static void nvme_copy_cb(void *opaque, int ret)
+static void nvme_do_copy(NvmeCopyAIOCB *iocb)
 {
-    NvmeCopyAIOCB *iocb = opaque;
     NvmeRequest *req = iocb->req;
     NvmeNamespace *ns = req->ns;
     uint64_t slba;
@@ -2917,10 +2894,7 @@ static void nvme_copy_cb(void *opaque, int ret)
     size_t len;
     uint16_t status;
 
-    if (ret < 0) {
-        iocb->ret = ret;
-        goto done;
-    } else if (iocb->ret < 0) {
+    if (iocb->ret < 0) {
         goto done;
     }
 
@@ -2967,6 +2941,7 @@ static void nvme_copy_cb(void *opaque, int ret)
 
 invalid:
     req->status = status;
+    iocb->ret = -1;
 done:
     iocb->aiocb = NULL;
     if (iocb->bh) {
@@ -2974,7 +2949,6 @@ done:
     }
 }
 
-
 static uint16_t nvme_copy(NvmeCtrl *n, NvmeRequest *req)
 {
     NvmeNamespace *ns = req->ns;
@@ -3060,7 +3034,7 @@ static uint16_t nvme_copy(NvmeCtrl *n, NvmeRequest *req)
                      BLOCK_ACCT_WRITE);
 
     req->aiocb = &iocb->common;
-    nvme_copy_cb(iocb, 0);
+    nvme_do_copy(iocb);
 
     return NVME_NO_COMPLETE;
 
-- 
2.36.1

[PATCH 12/12] hw/nvme: align logic of format with flush

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Use the same logic in format as in flush, saving a bh scheduling at the
start of the operation and moving completion handling to a separately
invoked bottom halve.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 4d6b4c9f00ba..0e6f4d71134b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5608,8 +5608,6 @@ typedef struct NvmeFormatAIOCB {
     uint8_t pil;
 } NvmeFormatAIOCB;
 
-static void nvme_format_bh(void *opaque);
-
 static void nvme_format_cancel(BlockAIOCB *aiocb)
 {
     NvmeFormatAIOCB *iocb = container_of(aiocb, NvmeFormatAIOCB, common);
@@ -5628,6 +5626,17 @@ static const AIOCBInfo nvme_format_aiocb_info = {
     .get_aio_context = nvme_get_aio_context,
 };
 
+static void nvme_format_bh(void *opaque)
+{
+    NvmeFormatAIOCB *iocb = opaque;
+
+    iocb->common.cb(iocb->common.opaque, iocb->ret);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+    qemu_aio_unref(iocb);
+}
+
 static void nvme_format_set(NvmeNamespace *ns, uint8_t lbaf, uint8_t mset,
                             uint8_t pi, uint8_t pil)
 {
@@ -5642,6 +5651,8 @@ static void nvme_format_set(NvmeNamespace *ns, uint8_t lbaf, uint8_t mset,
     nvme_ns_init_format(ns);
 }
 
+static void nvme_do_format(NvmeFormatAIOCB *iocb);
+
 static void nvme_format_ns_cb(void *opaque, int ret)
 {
     NvmeFormatAIOCB *iocb = opaque;
@@ -5651,6 +5662,8 @@ static void nvme_format_ns_cb(void *opaque, int ret)
     if (ret < 0) {
         iocb->ret = ret;
         goto done;
+    } else if (iocb->ret < 0) {
+        goto done;
     }
 
     assert(ns);
@@ -5672,8 +5685,7 @@ static void nvme_format_ns_cb(void *opaque, int ret)
     iocb->offset = 0;
 
 done:
-    iocb->aiocb = NULL;
-    qemu_bh_schedule(iocb->bh);
+    nvme_do_format(iocb);
 }
 
 static uint16_t nvme_format_check(NvmeNamespace *ns, uint8_t lbaf, uint8_t pi)
@@ -5697,16 +5709,15 @@ static uint16_t nvme_format_check(NvmeNamespace *ns, uint8_t lbaf, uint8_t pi)
     return NVME_SUCCESS;
 }
 
-static void nvme_format_bh(void *opaque)
+static void nvme_do_format(NvmeFormatAIOCB *iocb)
 {
-    NvmeFormatAIOCB *iocb = opaque;
     NvmeRequest *req = iocb->req;
     NvmeCtrl *n = nvme_ctrl(req);
     uint16_t status;
     int i;
 
     if (iocb->ret < 0) {
-        goto done;
+        goto out;
     }
 
     if (iocb->broadcast) {
@@ -5720,26 +5731,23 @@ static void nvme_format_bh(void *opaque)
     }
 
     if (!iocb->ns) {
-        goto done;
+        goto out;
     }
 
     status = nvme_format_check(iocb->ns, iocb->lbaf, iocb->pi);
     if (status) {
         req->status = status;
-        goto done;
+        goto out;
     }
 
     iocb->ns->status = NVME_FORMAT_IN_PROGRESS;
     nvme_format_ns_cb(iocb, 0);
     return;
 
-done:
-    qemu_bh_delete(iocb->bh);
-    iocb->bh = NULL;
-
-    iocb->common.cb(iocb->common.opaque, iocb->ret);
-
-    qemu_aio_unref(iocb);
+out:
+    if (iocb->bh) {
+        qemu_bh_schedule(iocb->bh);
+    }
 }
 
 static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest *req)
@@ -5786,7 +5794,7 @@ static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest *req)
     }
 
     req->aiocb = &iocb->common;
-    qemu_bh_schedule(iocb->bh);
+    nvme_do_format(iocb);
 
     return NVME_NO_COMPLETE;
 
@@ -5794,6 +5802,7 @@ out:
     qemu_bh_delete(iocb->bh);
     iocb->bh = NULL;
     qemu_aio_unref(iocb);
+
     return status;
 }
 
-- 
2.36.1

Re: [PATCH 00/12] hw/nvme: misc fixes and updates

[Klaus Jensen]

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

On Jun 23 23:18, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> This series includes a couple of misc fixes as well as some cleanup
> pertaining to the aio handling in flush, dsm, copy and zone reset. As
> Jinhao gets around to iothread stuff, it might come in handy to have
> this stuff cleaned up a bit.
> 
> Dmitrys fix (nvme-next commit "hw/nvme: add missing return statement")
> for dsm prompted me to audit the flush, dsm, zone reset and copy code
> and that resulted in the discovery of some bugs and some general clean
> up.
> 
> Klaus Jensen (12):
>   hw/nvme: fix incorrect use of errp/local_err
>   hw/nvme: remove redundant passing of PCIDevice
>   hw/nvme: cleanup error reporting in nvme_init_pci()
>   hw/nvme: fix numzrwa handling
>   hw/nvme: fix accidental reintroduction of redundant code
>   hw/nvme: fix cancellation of format operations
>   hw/nvme: fix flush cancel
>   hw/nvme: rework flush bh scheduling
>   hw/nvme: improve cancellation handling in zone reset
>   hw/nvme: improve cancellation handling in dsm
>   hw/nvme: simplify copy command error handling
>   hw/nvme: align logic of format with flush
> 
>  hw/nvme/ctrl.c | 252 +++++++++++++++++++++++--------------------------
>  hw/nvme/ns.c   |   4 +-
>  2 files changed, 119 insertions(+), 137 deletions(-)
> 
> -- 
> 2.36.1
> 

Ping,

We are coming up on the 7.1 soft-freeze. Most of the above are fixes, so
they can go in after, but I'd like to get a couple of the other non-fix
patches in if possile ;)

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