Re: [PATCH v5 05/26] nvme: populate the mandatory subnqn and ver fields

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> Required for compliance with NVMe revision 1.2.1 or later. See NVM
> Express 1.2.1, Section 5.11 ("Identify command"), Figure 90 and Section
> 7.9 ("NVMe Qualified Names").
> 
> This also bumps the supported version to 1.2.1.
> 
> Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
> ---
>  hw/block/nvme.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f05ebcce3f53..9abf74da20f2 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -9,9 +9,9 @@
>   */
>  
>  /**
> - * Reference Specs: http://www.nvmexpress.org, 1.2, 1.1, 1.0e
> + * Reference Specification: NVM Express 1.2.1
>   *
> - *  http://www.nvmexpress.org/resources/
> + *   https://nvmexpress.org/resources/specifications/
To be honest that redirects to https://nvmexpress.org/specifications/
Not a problem though.
>   */
>  
>  /**
> @@ -43,6 +43,8 @@
>  #include "trace.h"
>  #include "nvme.h"
>  
> +#define NVME_SPEC_VER 0x00010201
> +
>  #define NVME_GUEST_ERR(trace, fmt, ...) \
>      do { \
>          (trace_##trace)(__VA_ARGS__); \
> @@ -1365,6 +1367,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      id->ieee[0] = 0x00;
>      id->ieee[1] = 0x02;
>      id->ieee[2] = 0xb3;
> +    id->ver = cpu_to_le32(NVME_SPEC_VER);
This is indeed 1.2 addition
>      id->oacs = cpu_to_le16(0);
>      id->frmw = 7 << 1;
>      id->lpa = 1 << 0;
> @@ -1372,6 +1375,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      id->cqes = (0x4 << 4) | 0x4;
>      id->nn = cpu_to_le32(n->num_namespaces);
>      id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
> +
> +    strcpy((char *) id->subnqn, "nqn.2019-08.org.qemu:");
> +    pstrcat((char *) id->subnqn, sizeof(id->subnqn), n->params.serial);
Looks OK, this is first format according to the spec.
> +
>      id->psd[0].mp = cpu_to_le16(0x9c4);
>      id->psd[0].enlat = cpu_to_le32(0x10);
>      id->psd[0].exlat = cpu_to_le32(0x4);
> @@ -1386,7 +1393,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      NVME_CAP_SET_CSS(n->bar.cap, 1);
>      NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
>  
> -    n->bar.vs = 0x00010200;
> +    n->bar.vs = NVME_SPEC_VER;
>      n->bar.intmc = n->bar.intms = 0;
>  
>      if (n->params.cmb_size_mb) {

To be really pedantic, the 1.2 spec also requires at least:
  * wctemp and cctemp to be nonzero in Identify Controller (yea, this is stupid to report temperature for virtual controller)
  * NVME_ADM_CMD_GET_LOG_PAGE, with some mandatory log pages
  * NVME_ADM_CMD_SET_FEATURES/NVME_ADM_CMD_GET_FEATURES - The device currently doesn't implement some mandatory features.

And there are probably more. This is what I can recall from my nvme-mdev.

However I see that you implmented these in following patches, so I suggest you first put patches that implement all that features,
and then bump the NVME version.
Most of these features I mentioned were mandatory even in version 1.0 of the spec, so current version is not even
compliant with 1.0 IMHO.

Best regards,
	Maxim Levitsky