Re: [PATCH v6 01/11] hw/block/nvme: Add Commands Supported and Effects log

[Niklas Cassel]

On Tue, Oct 13, 2020 at 05:50:34PM -0700, Keith Busch wrote:
> On Wed, Oct 14, 2020 at 06:42:02AM +0900, Dmitry Fomichev wrote:
> > +{
> > +    NvmeEffectsLog log = {};
> > +    uint32_t *dst_acs = log.acs, *dst_iocs = log.iocs;
> > +    uint32_t trans_len;
> > +    int i;
> > +
> > +    trace_pci_nvme_cmd_supp_and_effects_log_read();
> > +
> > +    if (off >= sizeof(log)) {
> > +        trace_pci_nvme_err_invalid_effects_log_offset(off);
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> > +    }
> > +
> > +    for (i = 0; i < 256; i++) {
> > +        dst_acs[i] = nvme_cse_acs[i];
> > +    }
> > +
> > +    for (i = 0; i < 256; i++) {
> > +        dst_iocs[i] = nvme_cse_iocs_nvm[i];
> > +    }
> 
> You're just copying the array, so let's do it like this:
> 
>     memcpy(log.acs, nvme_cse_acs, sizeof(nvme_cse_acs));
>     memcpy(log.iocs, nvme_cse_iocs_nvm, sizeof(nvme_cse_iocs_nvm));
> 
> I think you also need to check
> 
>     if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY)
> 
> before copying iocs.

So it seems Dmitry actually does this in the Namespace Types patch:


@@ -1051,10 +1054,6 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest 
*req)
     trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
                           req->cmd.opcode, nvme_io_opc_str(req->cmd.opcode));

-    if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_ADMIN_ONLY) {
-        return NVME_INVALID_OPCODE | NVME_DNR;
-    }
-
     if (!nvme_nsid_valid(n, nsid)) {
         return NVME_INVALID_NSID | NVME_DNR;
     }
@@ -1333,8 +1332,10 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint32_t 
buf_len,
         dst_acs[i] = nvme_cse_acs[i];
     }

-    for (i = 0; i < 256; i++) {
-        dst_iocs[i] = nvme_cse_iocs_nvm[i];
+    if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+        for (i = 0; i < 256; i++) {
+            dst_iocs[i] = nvme_cse_iocs_nvm[i];
+        }
     }


Which later in the ZNS patch is changed to:

@@ -1335,13 +2178,31 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint32_t 
buf_len,
         return NVME_INVALID_FIELD | NVME_DNR;
     }

+    switch (NVME_CC_CSS(n->bar.cc)) {
+    case NVME_CC_CSS_NVM:
+        src_iocs = nvme_cse_iocs_nvm;
+        break;
+    case NVME_CC_CSS_CSI:
+        switch (csi) {
+        case NVME_CSI_NVM:
+            src_iocs = nvme_cse_iocs_nvm;
+            break;
+        case NVME_CSI_ZONED:
+            src_iocs = nvme_cse_iocs_zoned;
+            break;
+        }
+        /* fall through */
+    case NVME_CC_CSS_ADMIN_ONLY:
+        break;
+    }
+
     for (i = 0; i < 256; i++) {
         dst_acs[i] = nvme_cse_acs[i];
     }

-    if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+    if (src_iocs) {
         for (i = 0; i < 256; i++) {
-            dst_iocs[i] = nvme_cse_iocs_nvm[i];
+            dst_iocs[i] = src_iocs[i];
         }
     }


Perhaps the nvme_io_cmd() if-statement removal from the NS types patch +
the switch from the ZNS patch (with out the NVME_CSI_ZONED) could be moved
to this patch instead?

The middle version of adding "+    if (NVME_CC_CSS(n->bar.cc) != 
NVME_CC_CSS_ADMIN_ONLY) {"
can then be dropped from the NS types patch, since it is not part of the final 
code anyway.


Kind regards,
Niklas