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