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

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

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

On Jan 20 14:07, Niklas Cassel wrote:
> 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
> 

Definitely - it makes sense to reintroduce your patch here, with a
replacement of `ns->attached` with `nvme_ns_is_attach()`. Looks like
that should be sufficient.

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

Agreed.

> 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've seen this patch and as Klaus said, only thing patches need go with
is to put some of nvme_ns_is_attached() helper among the Identify
handlers.

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

Yes, pretty makes sense to update them.  But now it seems like
'attach/detach' scheme should have been separated out of this series
which just introduced the multi-path for controllers and namespace
sharing.  I will drop this 'detach' scheme out of this series and make
another series to support all of the Identify you mentioned above
cleanly.

> 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

On Thu, Jan 21, 2021 at 06:58:19AM +0900, Minwoo Im wrote:
> > 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.
> 
> Agreed.
> 
> > 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've seen this patch and as Klaus said, only thing patches need go with
> is to put some of nvme_ns_is_attached() helper among the Identify
> handlers.
> 
> > 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()
> 
> Yes, pretty makes sense to update them.  But now it seems like
> 'attach/detach' scheme should have been separated out of this series
> which just introduced the multi-path for controllers and namespace
> sharing.  I will drop this 'detach' scheme out of this series and make
> another series to support all of the Identify you mentioned above
> cleanly.

Hello Minwoo,

thank you for putting in work on this!

Kind regards,
Niklas