qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Keith Busch <kbusch@kernel.org>
Cc: "Fam Zheng" <fam@euphon.net>, "Kevin Wolf" <kwolf@redhat.com>,
	"Damien Le Moal" <Damien.LeMoal@wdc.com>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
	"Dmitry Fomichev" <Dmitry.Fomichev@wdc.com>,
	"Klaus Jensen" <k.jensen@samsung.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"Maxim Levitsky" <mlevitsk@redhat.com>,
	"Alistair Francis" <Alistair.Francis@wdc.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Matias Bjorling" <Matias.Bjorling@wdc.com>
Subject: Re: [PATCH v6 01/11] hw/block/nvme: Add Commands Supported and Effects log
Date: Wed, 14 Oct 2020 12:13:06 +0000	[thread overview]
Message-ID: <20201014121305.GB122299@localhost.localdomain> (raw)
In-Reply-To: <20201014005034.GA1264232@dhcp-10-100-145-180.wdl.wdc.com>

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

  reply	other threads:[~2020-10-14 12:23 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-13 21:42 [PATCH v6 00/11] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 01/11] hw/block/nvme: Add Commands Supported and Effects log Dmitry Fomichev
2020-10-14  0:50   ` Keith Busch
2020-10-14 12:13     ` Niklas Cassel [this message]
2020-10-19  2:01     ` Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 02/11] hw/block/nvme: Generate namespace UUIDs Dmitry Fomichev
2020-10-14 11:40   ` Klaus Jensen
2020-10-13 21:42 ` [PATCH v6 03/11] hw/block/nvme: Add support for Namespace Types Dmitry Fomichev
2020-10-14 13:01   ` Niklas Cassel
2020-10-19  2:03     ` Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 04/11] hw/block/nvme: Support allocated CNS command variants Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 05/11] hw/block/nvme: Support Zoned Namespace Command Set Dmitry Fomichev
2020-10-14 11:59   ` Niklas Cassel
2020-10-19  2:02     ` Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 06/11] hw/block/nvme: Introduce max active and open zone limits Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 07/11] hw/block/nvme: Support Zone Descriptor Extensions Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 08/11] hw/block/nvme: Add injection of Offline/Read-Only zones Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 09/11] hw/block/nvme: Document zoned parameters in usage text Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 10/11] hw/block/nvme: Separate read and write handlers Dmitry Fomichev
2020-10-13 21:42 ` [PATCH v6 11/11] hw/block/nvme: Merge nvme_write_zeroes() with nvme_write() Dmitry Fomichev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201014121305.GB122299@localhost.localdomain \
    --to=niklas.cassel@wdc.com \
    --cc=Alistair.Francis@wdc.com \
    --cc=Damien.LeMoal@wdc.com \
    --cc=Dmitry.Fomichev@wdc.com \
    --cc=Matias.Bjorling@wdc.com \
    --cc=fam@euphon.net \
    --cc=k.jensen@samsung.com \
    --cc=kbusch@kernel.org \
    --cc=kwolf@redhat.com \
    --cc=mlevitsk@redhat.com \
    --cc=philmd@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).