[v2,14/18] hw/block/nvme: support identify namespace descriptor list

From: Klaus Jensen <k.jensen@samsung.com>

Since we are not providing the NGUID or EUI64 fields, we must support
the Namespace UUID. We do not have any way of storing a persistent
unique identifier, so conjure up a UUID that is just the namespace id.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 hw/block/nvme.c       | 41 +++++++++++++++++++++++++++++++++++++++++
 hw/block/trace-events |  1 +
 2 files changed, 42 insertions(+)

On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Since we are not providing the NGUID or EUI64 fields, we must support
> the Namespace UUID. We do not have any way of storing a persistent
> unique identifier, so conjure up a UUID that is just the namespace id.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  hw/block/nvme.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>  hw/block/trace-events |  1 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 8230e0e3826b..65c2fa3ac1f4 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -971,6 +971,45 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>      return ret;
>  }
>  
> +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c)
> +{
> +    uint32_t nsid = le32_to_cpu(c->nsid);
> +    uint64_t prp1 = le64_to_cpu(c->prp1);
> +    uint64_t prp2 = le64_to_cpu(c->prp2);
> +
> +    uint8_t list[NVME_IDENTIFY_DATA_SIZE];
> +
> +    struct data {
> +        struct {
> +            NvmeIdNsDescr hdr;
> +            uint8_t v[16];

You might consider to use QemuUUID from "qemu/uuid.h". The benefits
are you can use qemu_uuid_parse() qemu_uuid_unparse*() for tracing,
and DEFINE_PROP_UUID() in case you want to set a particular UUID
from command line, it case it is important to the guest.

> +        } uuid;
> +    };
> +
> +    struct data *ns_descrs = (struct data *)list;
> +
> +    trace_pci_nvme_identify_ns_descr_list(nsid);
> +
> +    if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> +        trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
> +        return NVME_INVALID_NSID | NVME_DNR;
> +    }
> +
> +    memset(list, 0x0, sizeof(list));
> +
> +    /*
> +     * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
> +     * structure, a Namespace UUID (nidt = 0x3) must be reported in the
> +     * Namespace Identification Descriptor. Add a very basic Namespace UUID
> +     * here.
> +     */
> +    ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
> +    ns_descrs->uuid.hdr.nidl = NVME_NIDT_UUID_LEN;
> +    stl_be_p(&ns_descrs->uuid.v, nsid);
> +
> +    return nvme_dma_read_prp(n, list, NVME_IDENTIFY_DATA_SIZE, prp1, prp2);
> +}
> +
>  static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
>  {
>      NvmeIdentify *c = (NvmeIdentify *)cmd;
> @@ -982,6 +1021,8 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
>          return nvme_identify_ctrl(n, c);
>      case NVME_ID_CNS_NS_ACTIVE_LIST:
>          return nvme_identify_nslist(n, c);
> +    case NVME_ID_CNS_NS_DESCR_LIST:
> +        return nvme_identify_ns_descr_list(n, c);
>      default:
>          trace_pci_nvme_err_invalid_identify_cns(le32_to_cpu(c->cns));
>          return NVME_INVALID_FIELD | NVME_DNR;
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 4a4ef34071df..7b7303cab1dd 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -45,6 +45,7 @@ pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
>  pci_nvme_identify_ctrl(void) "identify controller"
>  pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
>  pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> +pci_nvme_identify_ns_descr_list(uint32_t ns) "nsid %"PRIu32""
>  pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
>  pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint8_t sel, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" sel 0x%"PRIx8" cdw11 0x%"PRIx32""
>  pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint8_t save, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" save 0x%"PRIx8" cdw11 0x%"PRIx32""
>

On Jul  3 10:27, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Since we are not providing the NGUID or EUI64 fields, we must support
> > the Namespace UUID. We do not have any way of storing a persistent
> > unique identifier, so conjure up a UUID that is just the namespace id.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  hw/block/nvme.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> >  hw/block/trace-events |  1 +
> >  2 files changed, 42 insertions(+)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 8230e0e3826b..65c2fa3ac1f4 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -971,6 +971,45 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> >      return ret;
> >  }
> >  
> > +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c)
> > +{
> > +    uint32_t nsid = le32_to_cpu(c->nsid);
> > +    uint64_t prp1 = le64_to_cpu(c->prp1);
> > +    uint64_t prp2 = le64_to_cpu(c->prp2);
> > +
> > +    uint8_t list[NVME_IDENTIFY_DATA_SIZE];
> > +
> > +    struct data {
> > +        struct {
> > +            NvmeIdNsDescr hdr;
> > +            uint8_t v[16];
> 
> You might consider to use QemuUUID from "qemu/uuid.h". The benefits
> are you can use qemu_uuid_parse() qemu_uuid_unparse*() for tracing,
> and DEFINE_PROP_UUID() in case you want to set a particular UUID
> from command line, it case it is important to the guest.
> 

Yes, definitely. Niklas also does this in his patch for namespace types
support. And I think that it's very important that it can be made
persistent, which would require a device property.

Thus, if it is OK with the rest of you, I would like to defer this to
when we merge the multiple namespaces patch and add "uuid" as a nvme-ns
device property there. Then, we do not have to add the uuid property on
the nvme device now and then have to keep it around when the namespace
related properties moves to the nvme-ns device.

On 7/3/20 12:00 PM, Klaus Jensen wrote:
> On Jul  3 10:27, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Since we are not providing the NGUID or EUI64 fields, we must support
>>> the Namespace UUID. We do not have any way of storing a persistent
>>> unique identifier, so conjure up a UUID that is just the namespace id.
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
>>> ---
>>>  hw/block/nvme.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>>>  hw/block/trace-events |  1 +
>>>  2 files changed, 42 insertions(+)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index 8230e0e3826b..65c2fa3ac1f4 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -971,6 +971,45 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>>>      return ret;
>>>  }
>>>  
>>> +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c)
>>> +{
>>> +    uint32_t nsid = le32_to_cpu(c->nsid);
>>> +    uint64_t prp1 = le64_to_cpu(c->prp1);
>>> +    uint64_t prp2 = le64_to_cpu(c->prp2);
>>> +
>>> +    uint8_t list[NVME_IDENTIFY_DATA_SIZE];
>>> +
>>> +    struct data {
>>> +        struct {
>>> +            NvmeIdNsDescr hdr;
>>> +            uint8_t v[16];
>>
>> You might consider to use QemuUUID from "qemu/uuid.h". The benefits
>> are you can use qemu_uuid_parse() qemu_uuid_unparse*() for tracing,
>> and DEFINE_PROP_UUID() in case you want to set a particular UUID
>> from command line, it case it is important to the guest.
>>
> 
> Yes, definitely. Niklas also does this in his patch for namespace types
> support. And I think that it's very important that it can be made
> persistent, which would require a device property.
> 
> Thus, if it is OK with the rest of you, I would like to defer this to
> when we merge the multiple namespaces patch and add "uuid" as a nvme-ns
> device property there. Then, we do not have to add the uuid property on
> the nvme device now and then have to keep it around when the namespace
> related properties moves to the nvme-ns device.

No objection, it was a simple suggestion to consider for later ;)