Re: [PATCH] hw/block/nvme: fix csi field for cns 0x00 and 0x11

[Klaus Jensen <its@irrelevant.dk>]

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

On Apr 26 13:16, Gollu Appalanaidu wrote:
>As per the TP 4056d Namespace types CNS 0x00 and CNS 0x11
>CSI field shouldn't use but it is being used for these two
>Identify command CNS values, fix that.
>
>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>---
> hw/nvme/ctrl.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 2e7498a73e..1657b1d04a 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -4244,11 +4244,16 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req, bool active)
>         }
>     }
>
>-    if (c->csi == NVME_CSI_NVM && nvme_csi_has_nvm_support(ns)) {
>-        return nvme_c2h(n, (uint8_t *)&ns->id_ns, sizeof(NvmeIdNs), req);
>+    if (active && nvme_csi_has_nvm_support(ns)) {
>+        goto out;
>+    } else if (!active && ns->csi == NVME_CSI_NVM) {
>+        goto out;
>+    } else {
>+        return NVME_INVALID_CMD_SET | NVME_DNR;
>     }
>
>-    return NVME_INVALID_CMD_SET | NVME_DNR;
>+out:
>+    return nvme_c2h(n, (uint8_t *)&ns->id_ns, sizeof(NvmeIdNs), req);
> }
>
> static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
>-- 
>2.17.1
>
>

Looking closer at this, since we only support the NVM and Zoned command 
sets, we can get rid of the `nvme_csi_has_nvm_support()` helper and just 
assume NVM command set support for all namespaces. The way different 
command sets are handled doesn't scale anyway, so we might as well 
simplify the logic a bit.

Something like this (compile-tested only) patch maybe?

diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 2e7498a73e70..7fcd6992358d 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -4178,16 +4178,6 @@ static uint16_t nvme_rpt_empty_id_struct(NvmeCtrl *n, NvmeRequest *req)
      return nvme_c2h(n, id, sizeof(id), req);
  }

-static inline bool nvme_csi_has_nvm_support(NvmeNamespace *ns)
-{
-    switch (ns->csi) {
-    case NVME_CSI_NVM:
-    case NVME_CSI_ZONED:
-        return true;
-    }
-    return false;
-}
-
  static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeRequest *req)
  {
      trace_pci_nvme_identify_ctrl();
@@ -4244,7 +4234,7 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req, bool active)
          }
      }

-    if (c->csi == NVME_CSI_NVM && nvme_csi_has_nvm_support(ns)) {
+    if (active || ns->csi == NVME_CSI_NVM) {
          return nvme_c2h(n, (uint8_t *)&ns->id_ns, sizeof(NvmeIdNs), req);
      }

@@ -4315,7 +4305,7 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req,
          }
      }

-    if (c->csi == NVME_CSI_NVM && nvme_csi_has_nvm_support(ns)) {
+    if (c->csi == NVME_CSI_NVM) {
          return nvme_rpt_empty_id_struct(n, req);
      } else if (c->csi == NVME_CSI_ZONED && ns->csi == NVME_CSI_ZONED) {
          return nvme_c2h(n, (uint8_t *)ns->id_ns_zoned, sizeof(NvmeIdNsZoned),


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