[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v6 03/11] hw/block/nvme: Add support for Namespace Types
From: |
Dmitry Fomichev |
Subject: |
RE: [PATCH v6 03/11] hw/block/nvme: Add support for Namespace Types |
Date: |
Mon, 19 Oct 2020 02:03:40 +0000 |
> -----Original Message-----
> From: Niklas Cassel <Niklas.Cassel@wdc.com>
> Sent: Wednesday, October 14, 2020 9:01 AM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: Keith Busch <kbusch@kernel.org>; Klaus Jensen
> <k.jensen@samsung.com>; Kevin Wolf <kwolf@redhat.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Maxim Levitsky
> <mlevitsk@redhat.com>; Fam Zheng <fam@euphon.net>; Alistair Francis
> <Alistair.Francis@wdc.com>; Matias Bjorling <Matias.Bjorling@wdc.com>;
> Damien Le Moal <Damien.LeMoal@wdc.com>; qemu-block@nongnu.org;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH v6 03/11] hw/block/nvme: Add support for Namespace
> Types
>
> On Wed, Oct 14, 2020 at 06:42:04AM +0900, Dmitry Fomichev wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > Define the structures and constants required to implement
> > Namespace Types support.
> >
> > Namespace Types introduce a new command set, "I/O Command Sets",
> > that allows the host to retrieve the command sets associated with
> > a namespace. Introduce support for the command set and enable
> > detection for the NVM Command Set.
> >
> > The new workflows for identify commands rely heavily on zero-filled
> > identify structs. E.g., certain CNS commands are defined to return
> > a zero-filled identify struct when an inactive namespace NSID
> > is supplied.
> >
> > Add a helper function in order to avoid code duplication when
> > reporting zero-filled identify structures.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
>
> (snip)
>
> > @@ -2090,6 +2199,27 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
> > n->bar.cc = 0;
> > }
> >
> > +static void nvme_select_ns_iocs(NvmeCtrl *n)
> > +{
> > + NvmeNamespace *ns;
> > + int i;
> > +
> > + for (i = 1; i <= n->num_namespaces; i++) {
> > + ns = nvme_ns(n, i);
> > + if (!ns) {
> > + continue;
> > + }
> > + ns->iocs = nvme_cse_iocs_none;
> > + switch (ns->csi) {
> > + case NVME_CSI_NVM:
> > + if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
> > + ns->iocs = nvme_cse_iocs_nvm;
> > + }
> > + break;
> > + }
> > + }
> > +}
> > +
> > static int nvme_start_ctrl(NvmeCtrl *n)
> > {
> > uint32_t page_bits = NVME_CC_MPS(n->bar.cc) + 12;
> > @@ -2188,6 +2318,8 @@ static int nvme_start_ctrl(NvmeCtrl *n)
> >
> > QTAILQ_INIT(&n->aer_queue);
> >
> > + nvme_select_ns_iocs(n);
> > +
> > return 0;
> > }
> >
> > @@ -2655,7 +2787,6 @@ int nvme_register_namespace(NvmeCtrl *n,
> NvmeNamespace *ns, Error **errp)
> > trace_pci_nvme_register_namespace(nsid);
> >
> > n->namespaces[nsid - 1] = ns;
> > - ns->iocs = nvme_cse_iocs_nvm;
> >
> > return 0;
> > }
>
> Considering how tightly coupled the three above diffs are with the
> Commands Supported and Effects log, and since patch 1 already adds
> the ns->iocs checking in nvme_admin_cmd() and nvme_io_cmd(),
> and since these three diffs are not really related to NS types,
> I think they should be moved to patch 1.
>
> It really helps the reviewer if both the ns->iocs assignment
> and checking is done in the same patch, and introduced as early
> as possible. And since this code is needed/valid simply to check
> if ADMIN_ONLY is selected (even before NS Types were introduced),
> I don't see any reason not to introduce them in to patch 1
> together with the other ns->iocs stuff.
>
> (We were always able to select a I/O Command Set using CC.CSS
> (Admin only/None, or NVM), NS types simply introduced the ability
> to select/enable more than one command set at the same time.)
>
OK, perhaps it is better to introduce nvme_select_ns_iocs() earlier,
in the CSE Log patch. This way there will be no need to modify
nvme_start_ctrl() again in this patch. Since ns->csi is not defined
until this commit, the initial code for nvme_select_ns_iocs() will
be simpler.
>
> 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
- [PATCH v6 06/11] hw/block/nvme: Introduce max active and open zone limits, Dmitry Fomichev, 2020/10/13
- [PATCH v6 07/11] hw/block/nvme: Support Zone Descriptor Extensions, Dmitry Fomichev, 2020/10/13
- [PATCH v6 08/11] hw/block/nvme: Add injection of Offline/Read-Only zones, Dmitry Fomichev, 2020/10/13
- [PATCH v6 09/11] hw/block/nvme: Document zoned parameters in usage text, Dmitry Fomichev, 2020/10/13
- [PATCH v6 10/11] hw/block/nvme: Separate read and write handlers, Dmitry Fomichev, 2020/10/13
- [PATCH v6 11/11] hw/block/nvme: Merge nvme_write_zeroes() with nvme_write(), Dmitry Fomichev, 2020/10/13