Re: [PATCH v2 04/20] nvme: populate the mandatory subnqn and ver fields

[Beata Michalska]

Hi Klaus

On Tue, 15 Oct 2019 at 11:42, Klaus Jensen <address@hidden> 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 <address@hidden>
> ---
>  hw/block/nvme.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 277700fdcc58..16f0fba10b08 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/
>   */
>
>  /**
> @@ -1366,6 +1366,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(0x00010201);
>      id->oacs = cpu_to_le16(0);
>      id->frmw = 7 << 1;
>      id->lpa = 1 << 0;
> @@ -1373,6 +1374,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);
> +
>      id->psd[0].mp = cpu_to_le16(0x9c4);
>      id->psd[0].enlat = cpu_to_le32(0x10);
>      id->psd[0].exlat = cpu_to_le32(0x4);
> @@ -1387,7 +1392,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 = 0x00010201;

Very minor:

The version number is being set twice in the patch series already.
And it is being set in two places.
It might be worth to make a #define out of it so that only one
needs to be changed.

BR


Beata
>      n->bar.intmc = n->bar.intms = 0;
>
>      if (n->params.cmb_size_mb) {
> --
> 2.23.0
>
>