[PATCH for-7.2 v2 0/6] hw/nvme: misc fixes

[PATCH for-7.2 v2 0/6] hw/nvme: misc fixes

[Klaus Jensen]

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

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

Klaus Jensen (6):
  hw/nvme: fix accidental reintroduction of redundant code
  hw/nvme: fix cancellation of format operations
  hw/nvme: fix flush cancel
  hw/nvme: fix cancellation handling in zone reset
  hw/nvme: fix cancellation handling in dsm
  hw/nvme: fix numzrwa handling

 hw/nvme/ctrl.c | 50 ++++++++++++++++++++------------------------------
 hw/nvme/ns.c   |  4 ++--
 2 files changed, 22 insertions(+), 32 deletions(-)

-- 
2.38.1

[PATCH for-7.2 v2 1/6] 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 e23af4db91ae..918af03d32be 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5847,9 +5847,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;
 
@@ -5871,7 +5868,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.38.1

[PATCH for-7.2 v2 2/6] 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 918af03d32be..819c02067191 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5762,8 +5762,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.38.1

[PATCH for-7.2 v2 3/6] 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 819c02067191..fe748fc18a2e 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3176,6 +3176,7 @@ static void nvme_flush_cancel(BlockAIOCB *acb)
 
     if (iocb->aiocb) {
         blk_aio_cancel_async(iocb->aiocb);
+        iocb->aiocb = NULL;
     }
 }
 
-- 
2.38.1

[PATCH for-7.2 v2 4/6] hw/nvme: fix 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.

Fixes: 63d96e4ffd71 ("hw/nvme: reimplement zone reset to allow cancellation")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index fe748fc18a2e..fa8e4b8dd53a 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3772,14 +3772,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);
@@ -3789,6 +3783,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)
@@ -3797,7 +3794,9 @@ static void nvme_zone_reset_cb(void *opaque, int ret)
     NvmeRequest *req = iocb->req;
     NvmeNamespace *ns = req->ns;
 
-    if (ret < 0) {
+    if (iocb->ret < 0) {
+        goto done;
+    } else if (ret < 0) {
         iocb->ret = ret;
         goto done;
     }
-- 
2.38.1

[PATCH for-7.2 v2 5/6] hw/nvme: fix 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.

Fixes: d7d1474fd85d ("hw/nvme: reimplement dsm to allow cancellation")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index fa8e4b8dd53a..6f217c3951bd 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2384,16 +2384,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);
@@ -2406,7 +2400,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;
         }
 
@@ -2420,8 +2413,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)
@@ -2434,7 +2426,9 @@ static void nvme_dsm_cb(void *opaque, int ret)
     uint64_t slba;
     uint32_t nlb;
 
-    if (ret < 0) {
+    if (iocb->ret < 0) {
+        goto done;
+    } else if (ret < 0) {
         iocb->ret = ret;
         goto done;
     }
-- 
2.38.1

[PATCH for-7.2 v2 6/6] 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 6f217c3951bd..b47289ecf16a 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1778,9 +1778,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 */
@@ -1820,9 +1818,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 62a1f97be010..6beeacca94ea 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.38.1

Re: [PATCH for-7.2 v2 2/6] hw/nvme: fix cancellation of format operations

[Philippe Mathieu-Daudé]

On 10/11/22 08:05, Klaus Jensen wrote:
> 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 918af03d32be..819c02067191 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -5762,8 +5762,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;
>       }
>   }
>   

What about nvme_flush_cancel()?

Re: [PATCH for-7.2 v2 2/6] hw/nvme: fix cancellation of format operations

[Philippe Mathieu-Daudé]

On 10/11/22 10:53, Philippe Mathieu-Daudé wrote:
> On 10/11/22 08:05, Klaus Jensen wrote:
>> 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 918af03d32be..819c02067191 100644
>> --- a/hw/nvme/ctrl.c
>> +++ b/hw/nvme/ctrl.c
>> @@ -5762,8 +5762,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;
>>       }
>>   }
> 
> What about nvme_flush_cancel()?

Ah, this is what the next patch fixes...