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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 4646 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]