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
next prev parent 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).