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