[PATCH 0/3] hw/block/nvme: misc fixes

[PATCH 0/3] hw/block/nvme: misc fixes

[Klaus Jensen]

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

Small set of misc fixes from Gollu.

Gollu Appalanaidu (3):
  hw/block/nvme: nvme_identify fixes
  hw/block/nvme: fix potential compilation error
  hw/block/nvme: report non-mdts command size limit for dsm

 hw/block/nvme.h       |  1 +
 include/block/nvme.h  | 11 +++++++++++
 hw/block/nvme.c       | 46 +++++++++++++++++++++++++++----------------
 hw/block/trace-events |  2 ++
 4 files changed, 43 insertions(+), 17 deletions(-)

-- 
2.30.1

[PATCH 1/3] hw/block/nvme: nvme_identify fixes

[Klaus Jensen]

From: Gollu Appalanaidu <anaidu.gollu@samsung.com>

Remove an unnecessary le_to_cpu conversion and add trace event for
Identify.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c       | 5 ++++-
 hw/block/trace-events | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1cd82fa3c9fe..ddc83f7f7a19 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3415,7 +3415,10 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
 {
     NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
 
-    switch (le32_to_cpu(c->cns)) {
+    trace_pci_nvme_identify(nvme_cid(req), c->cns, le16_to_cpu(c->ctrlid),
+                            c->csi);
+
+    switch (c->cns) {
     case NVME_ID_CNS_NS:
          /* fall through */
     case NVME_ID_CNS_NS_PRESENT:
diff --git a/hw/block/trace-events b/hw/block/trace-events
index b04f7a3e1890..1f958d09d2a9 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -61,6 +61,7 @@ pci_nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize,
 pci_nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
 pci_nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
 pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
+pci_nvme_identify(uint16_t cid, uint8_t cns, uint16_t ctrlid, uint8_t csi) "cid %"PRIu16" cns 0x%"PRIx8" ctrlid %"PRIu16" csi 0x%"PRIx8""
 pci_nvme_identify_ctrl(void) "identify controller"
 pci_nvme_identify_ctrl_csi(uint8_t csi) "identify controller, csi=0x%"PRIx8""
 pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
-- 
2.30.1

[PATCH 2/3] hw/block/nvme: fix potential compilation error

[Klaus Jensen]

From: Gollu Appalanaidu <anaidu.gollu@samsung.com>

assert may be compiled to a noop and we could end up returning an
uninitialized status.

Fix this by always returning Internal Device Error as a fallback.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index ddc83f7f7a19..897b9ff0db91 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1232,7 +1232,7 @@ static uint16_t nvme_check_zone_write(NvmeNamespace *ns, NvmeZone *zone,
 
 static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
 {
-    uint16_t status;
+    uint64_t zslba = zone->d.zslba;
 
     switch (nvme_get_zone_state(zone)) {
     case NVME_ZONE_STATE_EMPTY:
@@ -1241,16 +1241,15 @@ static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
     case NVME_ZONE_STATE_FULL:
     case NVME_ZONE_STATE_CLOSED:
     case NVME_ZONE_STATE_READ_ONLY:
-        status = NVME_SUCCESS;
-        break;
+        return NVME_SUCCESS;
     case NVME_ZONE_STATE_OFFLINE:
-        status = NVME_ZONE_OFFLINE;
-        break;
+        trace_pci_nvme_err_zone_is_offline(zslba);
+        return NVME_ZONE_OFFLINE;
     default:
         assert(false);
     }
 
-    return status;
+    return NVME_INTERNAL_DEV_ERROR;
 }
 
 static uint16_t nvme_check_zone_read(NvmeNamespace *ns, uint64_t slba,
-- 
2.30.1

[PATCH 3/3] hw/block/nvme: report non-mdts command size limit for dsm

[Klaus Jensen]

From: Gollu Appalanaidu <anaidu.gollu@samsung.com>

Dataset Management is not subject to MDTS, but exceeded a certain size
per range causes internal looping. Report this limit (DMRSL) in the NVM
command set specific identify controller data structure.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.h       |  1 +
 include/block/nvme.h  | 11 +++++++++++
 hw/block/nvme.c       | 30 ++++++++++++++++++++----------
 hw/block/trace-events |  1 +
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index cb2b5175f1a1..3046b82b3da1 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -172,6 +172,7 @@ typedef struct NvmeCtrl {
     int         aer_queued;
 
     uint8_t     zasl;
+    uint32_t    dmrsl;
 
     NvmeSubsystem   *subsys;
 
diff --git a/include/block/nvme.h b/include/block/nvme.h
index b23f3ae2279f..16d8c4c90f7e 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1041,6 +1041,16 @@ typedef struct NvmeIdCtrlZoned {
     uint8_t     rsvd1[4095];
 } NvmeIdCtrlZoned;
 
+typedef struct NvmeIdCtrlNvm {
+    uint8_t     vsl;
+    uint8_t     wzsl;
+    uint8_t     wusl;
+    uint8_t     dmrl;
+    uint32_t    dmrsl;
+    uint64_t    dmsl;
+    uint8_t     rsvd16[4080];
+} NvmeIdCtrlNvm;
+
 enum NvmeIdCtrlOacs {
     NVME_OACS_SECURITY  = 1 << 0,
     NVME_OACS_FORMAT    = 1 << 1,
@@ -1396,6 +1406,7 @@ static inline void _nvme_check_size(void)
     QEMU_BUILD_BUG_ON(sizeof(NvmeEffectsLog) != 4096);
     QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrl) != 4096);
     QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlZoned) != 4096);
+    QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlNvm) != 4096);
     QEMU_BUILD_BUG_ON(sizeof(NvmeLBAF) != 4);
     QEMU_BUILD_BUG_ON(sizeof(NvmeLBAFE) != 16);
     QEMU_BUILD_BUG_ON(sizeof(NvmeIdNs) != 4096);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 897b9ff0db91..5d6bba5fcb0d 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1777,6 +1777,10 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
             trace_pci_nvme_dsm_deallocate(nvme_cid(req), nvme_nsid(ns), slba,
                                           nlb);
 
+            if (nlb > n->dmrsl) {
+                trace_pci_nvme_dsm_single_range_limit_exceeded(nlb, n->dmrsl);
+            }
+
             offset = nvme_l2b(ns, slba);
             len = nvme_l2b(ns, nlb);
 
@@ -3202,21 +3206,24 @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeRequest *req)
 static uint16_t nvme_identify_ctrl_csi(NvmeCtrl *n, NvmeRequest *req)
 {
     NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
-    NvmeIdCtrlZoned id = {};
+    uint8_t id[NVME_IDENTIFY_DATA_SIZE] = {};
 
     trace_pci_nvme_identify_ctrl_csi(c->csi);
 
-    if (c->csi == NVME_CSI_NVM) {
-        return nvme_rpt_empty_id_struct(n, req);
-    } else if (c->csi == NVME_CSI_ZONED) {
-        if (n->params.zasl_bs) {
-            id.zasl = n->zasl;
-        }
-        return nvme_dma(n, (uint8_t *)&id, sizeof(id),
-                        DMA_DIRECTION_FROM_DEVICE, req);
+    switch (c->csi) {
+    case NVME_CSI_NVM:
+        ((NvmeIdCtrlNvm *)&id)->dmrsl = cpu_to_le32(n->dmrsl);
+        break;
+
+    case NVME_CSI_ZONED:
+        ((NvmeIdCtrlZoned *)&id)->zasl = n->zasl;
+        break;
+
+    default:
+        return NVME_INVALID_FIELD | NVME_DNR;
     }
 
-    return NVME_INVALID_FIELD | NVME_DNR;
+    return nvme_dma(n, id, sizeof(id), DMA_DIRECTION_FROM_DEVICE, req);
 }
 
 static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req)
@@ -4670,6 +4677,9 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
 
     n->namespaces[nsid - 1] = ns;
 
+    n->dmrsl = MIN_NON_ZERO(n->dmrsl,
+                            BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
+
     return 0;
 }
 
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 1f958d09d2a9..27940fe2e98a 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -51,6 +51,7 @@ pci_nvme_copy_cb(uint16_t cid) "cid %"PRIu16""
 pci_nvme_block_status(int64_t offset, int64_t bytes, int64_t pnum, int ret, bool zeroed) "offset %"PRId64" bytes %"PRId64" pnum %"PRId64" ret 0x%x zeroed %d"
 pci_nvme_dsm(uint16_t cid, uint32_t nsid, uint32_t nr, uint32_t attr) "cid %"PRIu16" nsid %"PRIu32" nr %"PRIu32" attr 0x%"PRIx32""
 pci_nvme_dsm_deallocate(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba %"PRIu64" nlb %"PRIu32""
+pci_nvme_dsm_single_range_limit_exceeded(uint32_t nlb, uint32_t dmrsl) "nlb %"PRIu32" dmrsl %"PRIu32""
 pci_nvme_compare(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba 0x%"PRIx64" nlb %"PRIu32""
 pci_nvme_compare_cb(uint16_t cid) "cid %"PRIu16""
 pci_nvme_aio_discard_cb(uint16_t cid) "cid %"PRIu16""
-- 
2.30.1

Re: [PATCH 1/3] hw/block/nvme: nvme_identify fixes

[Minwoo Im]

On 21-02-22 08:06:13, Klaus Jensen wrote:
> From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> 
> Remove an unnecessary le_to_cpu conversion and add trace event for
> Identify.
> 
> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c       | 5 ++++-
>  hw/block/trace-events | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 1cd82fa3c9fe..ddc83f7f7a19 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3415,7 +3415,10 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
>  {
>      NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
>  
> -    switch (le32_to_cpu(c->cns)) {
> +    trace_pci_nvme_identify(nvme_cid(req), c->cns, le16_to_cpu(c->ctrlid),
> +                            c->csi);

I think it would be great if it can be separated into two.
Anyway, changes look good to me.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

> +
> +    switch (c->cns) {
>      case NVME_ID_CNS_NS:
>           /* fall through */
>      case NVME_ID_CNS_NS_PRESENT:
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index b04f7a3e1890..1f958d09d2a9 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -61,6 +61,7 @@ pci_nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize,
>  pci_nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
>  pci_nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
>  pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
> +pci_nvme_identify(uint16_t cid, uint8_t cns, uint16_t ctrlid, uint8_t csi) "cid %"PRIu16" cns 0x%"PRIx8" ctrlid %"PRIu16" csi 0x%"PRIx8""
>  pci_nvme_identify_ctrl(void) "identify controller"
>  pci_nvme_identify_ctrl_csi(uint8_t csi) "identify controller, csi=0x%"PRIx8""
>  pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> -- 
> 2.30.1
> 
> 

Re: [PATCH 2/3] hw/block/nvme: fix potential compilation error

[Minwoo Im]

On 21-02-22 08:06:14, Klaus Jensen wrote:
> From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> 
> assert may be compiled to a noop and we could end up returning an
> uninitialized status.
> 
> Fix this by always returning Internal Device Error as a fallback.
> 
> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index ddc83f7f7a19..897b9ff0db91 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1232,7 +1232,7 @@ static uint16_t nvme_check_zone_write(NvmeNamespace *ns, NvmeZone *zone,
>  
>  static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
>  {
> -    uint16_t status;
> +    uint64_t zslba = zone->d.zslba;
>  
>      switch (nvme_get_zone_state(zone)) {
>      case NVME_ZONE_STATE_EMPTY:
> @@ -1241,16 +1241,15 @@ static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
>      case NVME_ZONE_STATE_FULL:
>      case NVME_ZONE_STATE_CLOSED:
>      case NVME_ZONE_STATE_READ_ONLY:
> -        status = NVME_SUCCESS;
> -        break;
> +        return NVME_SUCCESS;
>      case NVME_ZONE_STATE_OFFLINE:
> -        status = NVME_ZONE_OFFLINE;
> -        break;
> +        trace_pci_nvme_err_zone_is_offline(zslba);

This also is a tiny addition to the potential error fix.  Anyway, it can
be shorten to: (if zslba is used in a place only)

	trace_pci_nvme_err_zone_is_offline(zone->d.zslba);

> +        return NVME_ZONE_OFFLINE;
>      default:
>          assert(false);
>      }
>  
> -    return status;
> +    return NVME_INTERNAL_DEV_ERROR;
>  }
>  
>  static uint16_t nvme_check_zone_read(NvmeNamespace *ns, uint64_t slba,
> -- 
> 2.30.1
> 
> 

Re: [PATCH 3/3] hw/block/nvme: report non-mdts command size limit for dsm

[Minwoo Im]

On 21-02-22 08:06:15, Klaus Jensen wrote:
> From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> 
> Dataset Management is not subject to MDTS, but exceeded a certain size
> per range causes internal looping. Report this limit (DMRSL) in the NVM
> command set specific identify controller data structure.
> 
> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.h       |  1 +
>  include/block/nvme.h  | 11 +++++++++++
>  hw/block/nvme.c       | 30 ++++++++++++++++++++----------
>  hw/block/trace-events |  1 +
>  4 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index cb2b5175f1a1..3046b82b3da1 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -172,6 +172,7 @@ typedef struct NvmeCtrl {
>      int         aer_queued;
>  
>      uint8_t     zasl;
> +    uint32_t    dmrsl;
>  
>      NvmeSubsystem   *subsys;
>  
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index b23f3ae2279f..16d8c4c90f7e 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -1041,6 +1041,16 @@ typedef struct NvmeIdCtrlZoned {
>      uint8_t     rsvd1[4095];
>  } NvmeIdCtrlZoned;
>  
> +typedef struct NvmeIdCtrlNvm {
> +    uint8_t     vsl;
> +    uint8_t     wzsl;
> +    uint8_t     wusl;
> +    uint8_t     dmrl;
> +    uint32_t    dmrsl;
> +    uint64_t    dmsl;
> +    uint8_t     rsvd16[4080];
> +} NvmeIdCtrlNvm;
> +
>  enum NvmeIdCtrlOacs {
>      NVME_OACS_SECURITY  = 1 << 0,
>      NVME_OACS_FORMAT    = 1 << 1,
> @@ -1396,6 +1406,7 @@ static inline void _nvme_check_size(void)
>      QEMU_BUILD_BUG_ON(sizeof(NvmeEffectsLog) != 4096);
>      QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrl) != 4096);
>      QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlZoned) != 4096);
> +    QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlNvm) != 4096);
>      QEMU_BUILD_BUG_ON(sizeof(NvmeLBAF) != 4);
>      QEMU_BUILD_BUG_ON(sizeof(NvmeLBAFE) != 16);
>      QEMU_BUILD_BUG_ON(sizeof(NvmeIdNs) != 4096);
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 897b9ff0db91..5d6bba5fcb0d 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1777,6 +1777,10 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
>              trace_pci_nvme_dsm_deallocate(nvme_cid(req), nvme_nsid(ns), slba,
>                                            nlb);
>  
> +            if (nlb > n->dmrsl) {
> +                trace_pci_nvme_dsm_single_range_limit_exceeded(nlb, n->dmrsl);
> +            }
> +
>              offset = nvme_l2b(ns, slba);
>              len = nvme_l2b(ns, nlb);
>  
> @@ -3202,21 +3206,24 @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeRequest *req)
>  static uint16_t nvme_identify_ctrl_csi(NvmeCtrl *n, NvmeRequest *req)
>  {
>      NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> -    NvmeIdCtrlZoned id = {};
> +    uint8_t id[NVME_IDENTIFY_DATA_SIZE] = {};
>  
>      trace_pci_nvme_identify_ctrl_csi(c->csi);
>  
> -    if (c->csi == NVME_CSI_NVM) {
> -        return nvme_rpt_empty_id_struct(n, req);
> -    } else if (c->csi == NVME_CSI_ZONED) {
> -        if (n->params.zasl_bs) {
> -            id.zasl = n->zasl;
> -        }
> -        return nvme_dma(n, (uint8_t *)&id, sizeof(id),
> -                        DMA_DIRECTION_FROM_DEVICE, req);
> +    switch (c->csi) {
> +    case NVME_CSI_NVM:
> +        ((NvmeIdCtrlNvm *)&id)->dmrsl = cpu_to_le32(n->dmrsl);
> +        break;
> +
> +    case NVME_CSI_ZONED:
> +        ((NvmeIdCtrlZoned *)&id)->zasl = n->zasl;

Question.  Are we okay without checking this like above ? :)

	if (n->params.zasl_bs) {
		((NvmeIdCtrlZoned *)&id)->zasl = n->zasl;
	}

> +        break;
> +
> +    default:
> +        return NVME_INVALID_FIELD | NVME_DNR;
>      }
>  
> -    return NVME_INVALID_FIELD | NVME_DNR;
> +    return nvme_dma(n, id, sizeof(id), DMA_DIRECTION_FROM_DEVICE, req);
>  }
>  
>  static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req)
> @@ -4670,6 +4677,9 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
>  
>      n->namespaces[nsid - 1] = ns;
>  
> +    n->dmrsl = MIN_NON_ZERO(n->dmrsl,
> +                            BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
> +
>      return 0;
>  }
>  
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 1f958d09d2a9..27940fe2e98a 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -51,6 +51,7 @@ pci_nvme_copy_cb(uint16_t cid) "cid %"PRIu16""
>  pci_nvme_block_status(int64_t offset, int64_t bytes, int64_t pnum, int ret, bool zeroed) "offset %"PRId64" bytes %"PRId64" pnum %"PRId64" ret 0x%x zeroed %d"
>  pci_nvme_dsm(uint16_t cid, uint32_t nsid, uint32_t nr, uint32_t attr) "cid %"PRIu16" nsid %"PRIu32" nr %"PRIu32" attr 0x%"PRIx32""
>  pci_nvme_dsm_deallocate(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba %"PRIu64" nlb %"PRIu32""
> +pci_nvme_dsm_single_range_limit_exceeded(uint32_t nlb, uint32_t dmrsl) "nlb %"PRIu32" dmrsl %"PRIu32""
>  pci_nvme_compare(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba 0x%"PRIx64" nlb %"PRIu32""
>  pci_nvme_compare_cb(uint16_t cid) "cid %"PRIu16""
>  pci_nvme_aio_discard_cb(uint16_t cid) "cid %"PRIu16""
> -- 
> 2.30.1
> 
> 

Re: [PATCH 1/3] hw/block/nvme: nvme_identify fixes

[Philippe Mathieu-Daudé]

On 2/22/21 1:00 PM, Minwoo Im wrote:
> On 21-02-22 08:06:13, Klaus Jensen wrote:
>> From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>>
>> Remove an unnecessary le_to_cpu conversion and add trace event for
>> Identify.
>>
>> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>>  hw/block/nvme.c       | 5 ++++-
>>  hw/block/trace-events | 1 +
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 1cd82fa3c9fe..ddc83f7f7a19 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -3415,7 +3415,10 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
>>  {
>>      NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
>>  
>> -    switch (le32_to_cpu(c->cns)) {
>> +    trace_pci_nvme_identify(nvme_cid(req), c->cns, le16_to_cpu(c->ctrlid),
>> +                            c->csi);
> 
> I think it would be great if it can be separated into two.

Agreed.

> Anyway, changes look good to me.
> 
> Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH 2/3] hw/block/nvme: fix potential compilation error

[Philippe Mathieu-Daudé]

On 2/22/21 8:06 AM, Klaus Jensen wrote:
> From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> 
> assert may be compiled to a noop and we could end up returning an
> uninitialized status.

Per commit 262a69f4282 ("osdep.h: Prohibit disabling assert()
in supported builds") this shouldn't be possible. Anyhow cleanup
is good.

> 
> Fix this by always returning Internal Device Error as a fallback.
> 
> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index ddc83f7f7a19..897b9ff0db91 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1232,7 +1232,7 @@ static uint16_t nvme_check_zone_write(NvmeNamespace *ns, NvmeZone *zone,
>  
>  static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
>  {
> -    uint16_t status;
> +    uint64_t zslba = zone->d.zslba;
>  
>      switch (nvme_get_zone_state(zone)) {
>      case NVME_ZONE_STATE_EMPTY:
> @@ -1241,16 +1241,15 @@ static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
>      case NVME_ZONE_STATE_FULL:
>      case NVME_ZONE_STATE_CLOSED:
>      case NVME_ZONE_STATE_READ_ONLY:
> -        status = NVME_SUCCESS;
> -        break;
> +        return NVME_SUCCESS;
>      case NVME_ZONE_STATE_OFFLINE:
> -        status = NVME_ZONE_OFFLINE;
> -        break;
> +        trace_pci_nvme_err_zone_is_offline(zslba);
> +        return NVME_ZONE_OFFLINE;
>      default:
>          assert(false);
>      }
>  
> -    return status;
> +    return NVME_INTERNAL_DEV_ERROR;
>  }
>  
>  static uint16_t nvme_check_zone_read(NvmeNamespace *ns, uint64_t slba,
> 

Re: [PATCH 3/3] hw/block/nvme: report non-mdts command size limit for dsm

[Klaus Jensen]

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

On Feb 22 21:11, Minwoo Im wrote:
> On 21-02-22 08:06:15, Klaus Jensen wrote:
> > From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > 
> > Dataset Management is not subject to MDTS, but exceeded a certain size
> > per range causes internal looping. Report this limit (DMRSL) in the NVM
> > command set specific identify controller data structure.
> > 
> > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.h       |  1 +
> >  include/block/nvme.h  | 11 +++++++++++
> >  hw/block/nvme.c       | 30 ++++++++++++++++++++----------
> >  hw/block/trace-events |  1 +
> >  4 files changed, 33 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index cb2b5175f1a1..3046b82b3da1 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -172,6 +172,7 @@ typedef struct NvmeCtrl {
> >      int         aer_queued;
> >  
> >      uint8_t     zasl;
> > +    uint32_t    dmrsl;
> >  
> >      NvmeSubsystem   *subsys;
> >  
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index b23f3ae2279f..16d8c4c90f7e 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -1041,6 +1041,16 @@ typedef struct NvmeIdCtrlZoned {
> >      uint8_t     rsvd1[4095];
> >  } NvmeIdCtrlZoned;
> >  
> > +typedef struct NvmeIdCtrlNvm {
> > +    uint8_t     vsl;
> > +    uint8_t     wzsl;
> > +    uint8_t     wusl;
> > +    uint8_t     dmrl;
> > +    uint32_t    dmrsl;
> > +    uint64_t    dmsl;
> > +    uint8_t     rsvd16[4080];
> > +} NvmeIdCtrlNvm;
> > +
> >  enum NvmeIdCtrlOacs {
> >      NVME_OACS_SECURITY  = 1 << 0,
> >      NVME_OACS_FORMAT    = 1 << 1,
> > @@ -1396,6 +1406,7 @@ static inline void _nvme_check_size(void)
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeEffectsLog) != 4096);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrl) != 4096);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlZoned) != 4096);
> > +    QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlNvm) != 4096);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeLBAF) != 4);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeLBAFE) != 16);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeIdNs) != 4096);
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 897b9ff0db91..5d6bba5fcb0d 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1777,6 +1777,10 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
> >              trace_pci_nvme_dsm_deallocate(nvme_cid(req), nvme_nsid(ns), slba,
> >                                            nlb);
> >  
> > +            if (nlb > n->dmrsl) {
> > +                trace_pci_nvme_dsm_single_range_limit_exceeded(nlb, n->dmrsl);
> > +            }
> > +
> >              offset = nvme_l2b(ns, slba);
> >              len = nvme_l2b(ns, nlb);
> >  
> > @@ -3202,21 +3206,24 @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeRequest *req)
> >  static uint16_t nvme_identify_ctrl_csi(NvmeCtrl *n, NvmeRequest *req)
> >  {
> >      NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> > -    NvmeIdCtrlZoned id = {};
> > +    uint8_t id[NVME_IDENTIFY_DATA_SIZE] = {};
> >  
> >      trace_pci_nvme_identify_ctrl_csi(c->csi);
> >  
> > -    if (c->csi == NVME_CSI_NVM) {
> > -        return nvme_rpt_empty_id_struct(n, req);
> > -    } else if (c->csi == NVME_CSI_ZONED) {
> > -        if (n->params.zasl_bs) {
> > -            id.zasl = n->zasl;
> > -        }
> > -        return nvme_dma(n, (uint8_t *)&id, sizeof(id),
> > -                        DMA_DIRECTION_FROM_DEVICE, req);
> > +    switch (c->csi) {
> > +    case NVME_CSI_NVM:
> > +        ((NvmeIdCtrlNvm *)&id)->dmrsl = cpu_to_le32(n->dmrsl);
> > +        break;
> > +
> > +    case NVME_CSI_ZONED:
> > +        ((NvmeIdCtrlZoned *)&id)->zasl = n->zasl;
> 
> Question.  Are we okay without checking this like above ? :)
> 
> 	if (n->params.zasl_bs) {
> 		((NvmeIdCtrlZoned *)&id)->zasl = n->zasl;
> 	}
> 

Ah yeah. Thanks!

I forgot about that zasl_bs vs zasl quirkiness.

> > +        break;
> > +
> > +    default:
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> >      }
> >  
> > -    return NVME_INVALID_FIELD | NVME_DNR;
> > +    return nvme_dma(n, id, sizeof(id), DMA_DIRECTION_FROM_DEVICE, req);
> >  }
> >  
> >  static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req)
> > @@ -4670,6 +4677,9 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> >  
> >      n->namespaces[nsid - 1] = ns;
> >  
> > +    n->dmrsl = MIN_NON_ZERO(n->dmrsl,
> > +                            BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
> > +
> >      return 0;
> >  }
> >  
> > diff --git a/hw/block/trace-events b/hw/block/trace-events
> > index 1f958d09d2a9..27940fe2e98a 100644
> > --- a/hw/block/trace-events
> > +++ b/hw/block/trace-events
> > @@ -51,6 +51,7 @@ pci_nvme_copy_cb(uint16_t cid) "cid %"PRIu16""
> >  pci_nvme_block_status(int64_t offset, int64_t bytes, int64_t pnum, int ret, bool zeroed) "offset %"PRId64" bytes %"PRId64" pnum %"PRId64" ret 0x%x zeroed %d"
> >  pci_nvme_dsm(uint16_t cid, uint32_t nsid, uint32_t nr, uint32_t attr) "cid %"PRIu16" nsid %"PRIu32" nr %"PRIu32" attr 0x%"PRIx32""
> >  pci_nvme_dsm_deallocate(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba %"PRIu64" nlb %"PRIu32""
> > +pci_nvme_dsm_single_range_limit_exceeded(uint32_t nlb, uint32_t dmrsl) "nlb %"PRIu32" dmrsl %"PRIu32""
> >  pci_nvme_compare(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba 0x%"PRIx64" nlb %"PRIu32""
> >  pci_nvme_compare_cb(uint16_t cid) "cid %"PRIu16""
> >  pci_nvme_aio_discard_cb(uint16_t cid) "cid %"PRIu16""
> > -- 
> > 2.30.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 0/3] hw/block/nvme: misc fixes

[Klaus Jensen]

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

On Jan 25 10:02, Keith Busch wrote:
> On Mon, Jan 25, 2021 at 09:22:24AM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Misc fixes from Gollu.
> 
> Looks good.
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>

Thanks, applied!

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

Re: [PATCH 0/3] hw/block/nvme: misc fixes

[Keith Busch]

On Mon, Jan 25, 2021 at 09:22:24AM +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Misc fixes from Gollu.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

[PATCH 0/3] hw/block/nvme: misc fixes

[Klaus Jensen]

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

Misc fixes from Gollu.

Gollu Appalanaidu (3):
  hw/block/nvme: fix set feature for error recovery
  hw/block/nvme: fix set feature save field check
  hw/block/nvme: align with existing style

 hw/block/nvme.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

-- 
2.30.0