Re: [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List

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

On Wed, Jan 20, 2021 at 02:01:47AM +0900, Minwoo Im wrote:
> Spec v1.4b 6.1.4 "Active and Inactive NSID Types" says:
> 
> "Active NSIDs for a controller refer to namespaces that are attached to
> that controller. Allocated NSIDs that are inactive for a controller refer
> to namespaces that are not attached to that controller."
> 
> This patch introduced for Identify Active Namespace ID List (CNS 02h).
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  hw/block/nvme.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2b2c07b36c2b..7247167b0ee6 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2883,6 +2883,39 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
>      return NVME_INVALID_FIELD | NVME_DNR;
>  }
>  
> +static uint16_t nvme_identify_nslist_active(NvmeCtrl *n, NvmeRequest *req)
> +{
> +    NvmeNamespace *ns;
> +    NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> +    uint32_t min_nsid = le32_to_cpu(c->nsid);
> +    uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> +    static const int data_len = sizeof(list);
> +    uint32_t *list_ptr = (uint32_t *)list;
> +    int i, j = 0;
> +
> +    if (min_nsid >= NVME_NSID_BROADCAST - 1) {
> +        return NVME_INVALID_NSID | NVME_DNR;
> +    }
> +
> +    for (i = 1; i <= n->num_namespaces; i++) {
> +        ns = nvme_ns(n, i);
> +        if (!ns || ns->params.nsid <= min_nsid) {
> +            continue;
> +        }
> +
> +        if (!nvme_ns_is_attached(n, ns)) {
> +            continue;
> +        }
> +
> +        list_ptr[j++] = cpu_to_le32(ns->params.nsid);
> +        if (j == data_len / sizeof(uint32_t)) {
> +            break;
> +        }
> +    }
> +
> +    return nvme_dma(n, list, data_len, DMA_DIRECTION_FROM_DEVICE, req);
> +}
> +
>  static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
>  {
>      NvmeNamespace *ns;
> @@ -2914,10 +2947,6 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
>              continue;
>          }
>  
> -        if (!nvme_ns_is_attached(n, ns)) {
> -            continue;
> -        }
> -
>          list_ptr[j++] = cpu_to_le32(ns->params.nsid);
>          if (j == data_len / sizeof(uint32_t)) {
>              break;
> @@ -3045,7 +3074,7 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
>      case NVME_ID_CNS_CS_CTRL:
>          return nvme_identify_ctrl_csi(n, req);
>      case NVME_ID_CNS_NS_ACTIVE_LIST:
> -         /* fall through */
> +         return nvme_identify_nslist_active(n, req);
>      case NVME_ID_CNS_NS_PRESENT_LIST:
>          return nvme_identify_nslist(n, req);
>      case NVME_ID_CNS_CS_NS_ACTIVE_LIST:
> -- 
> 2.17.1
> 
> 

Hello Minwoo,

By introducing a detached parameter,
you are also implicitly making the following
NVMe commands no longer be spec compliant:

NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST

When these commands are called on a detached namespace,
they should usually return a zero filled data struct.

Dmitry and I had a patch on V8 on the ZNS series
that tried to fix some the existing NVMe commands
to be spec compliant, by handling detached namespaces
properly. In the end, in order to make it easier to
get the ZNS series accepted, we decided to drop the
detached related stuff from the series.

Feel free to look at that patch for inspiration:
https://github.com/dmitry-fomichev/qemu/commit/251c0ffee5149c739b1347811fa7e32a1c36bf7c

I'm not sure if you want to modify all the functions that
our patch modifies, but I think that you should at least
modify the following nvme functions:

nvme_identify_ns()
nvme_identify_ns_csi()
nvme_identify_nslist()
nvme_identify_nslist_csi()

So they handle detached namespaces correctly for both:
NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
as well as for:
NVME_ID_CNS_NS_PRESENT, NVME_ID_CNS_CS_NS_PRESENT,
NVME_ID_CNS_NS_PRESENT_LIST, NVME_ID_CNS_CS_NS_PRESENT_LIST


Kind regards,
Niklas