[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 01/11] hw/block/nvme: Add Commands Supported and Effects l
From: |
Niklas Cassel |
Subject: |
Re: [PATCH v6 01/11] hw/block/nvme: Add Commands Supported and Effects log |
Date: |
Wed, 14 Oct 2020 12:13:06 +0000 |
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
- [PATCH v6 00/11] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Dmitry Fomichev, 2020/10/13
- [PATCH v6 01/11] hw/block/nvme: Add Commands Supported and Effects log, Dmitry Fomichev, 2020/10/13
- [PATCH v6 02/11] hw/block/nvme: Generate namespace UUIDs, Dmitry Fomichev, 2020/10/13
- [PATCH v6 03/11] hw/block/nvme: Add support for Namespace Types, Dmitry Fomichev, 2020/10/13
- [PATCH v6 04/11] hw/block/nvme: Support allocated CNS command variants, Dmitry Fomichev, 2020/10/13
- [PATCH v6 05/11] hw/block/nvme: Support Zoned Namespace Command Set, Dmitry Fomichev, 2020/10/13