> -----Original Message----- > From: Niklas Cassel <niklas.cas...@wdc.com> > Sent: Wednesday, October 14, 2020 9:01 AM > To: Dmitry Fomichev <dmitry.fomic...@wdc.com> > Cc: Keith Busch <kbu...@kernel.org>; Klaus Jensen > <k.jen...@samsung.com>; Kevin Wolf <kw...@redhat.com>; Philippe > Mathieu-Daudé <phi...@redhat.com>; Maxim Levitsky > <mlevi...@redhat.com>; Fam Zheng <f...@euphon.net>; Alistair Francis > <alistair.fran...@wdc.com>; Matias Bjorling <matias.bjorl...@wdc.com>; > Damien Le Moal <damien.lem...@wdc.com>; qemu-bl...@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.cas...@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.cas...@wdc.com> > > Signed-off-by: Dmitry Fomichev <dmitry.fomic...@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