Re: [PATCH v6 03/11] hw/block/nvme: Add support for Namespace Types

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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.)


Kind regards,
Niklas