Re: [PATCH v5 15/26] nvme: bump supported specification to 1.3
On Mon, 2020-03-16 at 00:50 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 12:35, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Add new fields to the Identify Controller and Identify Namespace data
> > > structures accoding to NVM Express 1.3d.
> > >
> > > NVM Express 1.3d requires the following additional features:
> > > - addition of the Namespace Identification Descriptor List (CNS 03h)
> > > for the Identify command
> > > - support for returning Command Sequence Error if a Set Features
> > > command is submitted for the Number of Queues feature after any I/O
> > > queues have been created.
> > > - The addition of the Log Specific Field (LSP) in the Get Log Page
> > > command.
> > >
> > > Signed-off-by: Klaus Jensen
> > > ---
> > > hw/block/nvme.c | 57 ---
> > > hw/block/nvme.h | 1 +
> > > hw/block/trace-events | 3 ++-
> > > include/block/nvme.h | 20 ++-
> > > 4 files changed, 71 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 900732bb2f38..4acfc85b56a2 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -9,7 +9,7 @@
> > > */
> > >
> > > /**
> > > - * Reference Specification: NVM Express 1.2.1
> > > + * Reference Specification: NVM Express 1.3d
> > > *
> > > * https://nvmexpress.org/resources/specifications/
> > > */
> > > @@ -43,7 +43,7 @@
> > > #include "trace.h"
> > > #include "nvme.h"
> > >
> > > -#define NVME_SPEC_VER 0x00010201
> > > +#define NVME_SPEC_VER 0x00010300
> > > #define NVME_MAX_QS PCI_MSIX_FLAGS_QSIZE
> > > #define NVME_TEMPERATURE 0x143
> > > #define NVME_TEMPERATURE_WARNING 0x157
> > > @@ -735,6 +735,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd
> > > *cmd, NvmeRequest *req)
> > > uint32_t dw12 = le32_to_cpu(cmd->cdw12);
> > > uint32_t dw13 = le32_to_cpu(cmd->cdw13);
> > > uint8_t lid = dw10 & 0xff;
> > > +uint8_t lsp = (dw10 >> 8) & 0xf;
> > > uint8_t rae = (dw10 >> 15) & 0x1;
> > > uint32_t numdl, numdu;
> > > uint64_t off, lpol, lpou;
> > > @@ -752,7 +753,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd
> > > *cmd, NvmeRequest *req)
> > > return NVME_INVALID_FIELD | NVME_DNR;
> > > }
> > >
> > > -trace_nvme_dev_get_log(nvme_cid(req), lid, rae, len, off);
> > > +trace_nvme_dev_get_log(nvme_cid(req), lid, lsp, rae, len, off);
> > >
> > > switch (lid) {
> > > case NVME_LOG_ERROR_INFO:
> > > @@ -863,6 +864,8 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd
> > > *cmd)
> > > cq = g_malloc0(sizeof(*cq));
> > > nvme_init_cq(cq, n, prp1, cqid, vector, qsize + 1,
> > > NVME_CQ_FLAGS_IEN(qflags));
> >
> > Code alignment on that '('
> > > +
> > > +n->qs_created = true;
> >
> > Should be done also at nvme_create_sq
>
> No, because you can't create a SQ without a matching CQ:
True, I missed that.
>
> if (unlikely(!cqid || nvme_check_cqid(n, cqid))) {
> trace_nvme_dev_err_invalid_create_sq_cqid(cqid);
> return NVME_INVALID_CQID | NVME_DNR;
> }
>
>
> So if there is a matching cq, then qs_created = true.
>
> > > return NVME_SUCCESS;
> > > }
> > >
> > > @@ -924,6 +927,47 @@ static uint16_t nvme_identify_ns_list(NvmeCtrl *n,
> > > NvmeIdentify *c)
> > > return ret;
> > > }
> > >
> > > +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeCmd *c)
> > > +{
> > > +static const int len = 4096;
> >
> > The spec caps the Identify payload size to 4K,
> > thus this should go to nvme.h
>
> Done.
>
> > > +
> > > +struct ns_descr {
> > > +uint8_t nidt;
> > > +uint8_t nidl;
> > > +uint8_t rsvd2[2];
> > > +uint8_t nid[16];
> > > +};
> >
> > This is also part of the spec, thus should
> > move to nvme.h
> >
>
> Done - and cleaned up.
Perfect, thanks!
>
> > > +
> > > +uint32_t nsid = le32_to_cpu(c->nsid);
> > > +uint64_t prp1 = le64_to_cpu(c->prp1);
> > > +uint64_t prp2 = le64_to_cpu(c->prp2);
> > > +
> > > +struct ns_descr *list;
> > > +uint16_t ret;
> > > +
> > > +trace_nvme_dev_identify_ns_descr_list(nsid);
> > > +
> > > +if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> > > +trace_nvme_dev_err_invalid_ns(nsid, n->num_namespaces);
> > > +return NVME_INVALID_NSID | NVME_DNR;
> > > +}
> > > +
> > > +/*
> > > + * 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.
> >
> > Some per namespace uuid qemu property will be very nice to have to have a
> > uuid that
> > is at least somewhat unique.
> > Linux kernel I think might complain if it detects namespaces with duplicate
> > uuids.
>
Re: [PATCH v5 15/26] nvme: bump supported specification to 1.3
On Feb 12 12:35, Maxim Levitsky wrote:
> On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > Add new fields to the Identify Controller and Identify Namespace data
> > structures accoding to NVM Express 1.3d.
> >
> > NVM Express 1.3d requires the following additional features:
> > - addition of the Namespace Identification Descriptor List (CNS 03h)
> > for the Identify command
> > - support for returning Command Sequence Error if a Set Features
> > command is submitted for the Number of Queues feature after any I/O
> > queues have been created.
> > - The addition of the Log Specific Field (LSP) in the Get Log Page
> > command.
>
> >
> > Signed-off-by: Klaus Jensen
> > ---
> > hw/block/nvme.c | 57 ---
> > hw/block/nvme.h | 1 +
> > hw/block/trace-events | 3 ++-
> > include/block/nvme.h | 20 ++-
> > 4 files changed, 71 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 900732bb2f38..4acfc85b56a2 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -9,7 +9,7 @@
> > */
> >
> > /**
> > - * Reference Specification: NVM Express 1.2.1
> > + * Reference Specification: NVM Express 1.3d
> > *
> > * https://nvmexpress.org/resources/specifications/
> > */
> > @@ -43,7 +43,7 @@
> > #include "trace.h"
> > #include "nvme.h"
> >
> > -#define NVME_SPEC_VER 0x00010201
> > +#define NVME_SPEC_VER 0x00010300
> > #define NVME_MAX_QS PCI_MSIX_FLAGS_QSIZE
> > #define NVME_TEMPERATURE 0x143
> > #define NVME_TEMPERATURE_WARNING 0x157
> > @@ -735,6 +735,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd,
> > NvmeRequest *req)
> > uint32_t dw12 = le32_to_cpu(cmd->cdw12);
> > uint32_t dw13 = le32_to_cpu(cmd->cdw13);
> > uint8_t lid = dw10 & 0xff;
> > +uint8_t lsp = (dw10 >> 8) & 0xf;
> > uint8_t rae = (dw10 >> 15) & 0x1;
> > uint32_t numdl, numdu;
> > uint64_t off, lpol, lpou;
> > @@ -752,7 +753,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd,
> > NvmeRequest *req)
> > return NVME_INVALID_FIELD | NVME_DNR;
> > }
> >
> > -trace_nvme_dev_get_log(nvme_cid(req), lid, rae, len, off);
> > +trace_nvme_dev_get_log(nvme_cid(req), lid, lsp, rae, len, off);
> >
> > switch (lid) {
> > case NVME_LOG_ERROR_INFO:
> > @@ -863,6 +864,8 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd
> > *cmd)
> > cq = g_malloc0(sizeof(*cq));
> > nvme_init_cq(cq, n, prp1, cqid, vector, qsize + 1,
> > NVME_CQ_FLAGS_IEN(qflags));
> Code alignment on that '('
> > +
> > +n->qs_created = true;
> Should be done also at nvme_create_sq
No, because you can't create a SQ without a matching CQ:
if (unlikely(!cqid || nvme_check_cqid(n, cqid))) {
trace_nvme_dev_err_invalid_create_sq_cqid(cqid);
return NVME_INVALID_CQID | NVME_DNR;
}
So if there is a matching cq, then qs_created = true.
> > return NVME_SUCCESS;
> > }
> >
> > @@ -924,6 +927,47 @@ static uint16_t nvme_identify_ns_list(NvmeCtrl *n,
> > NvmeIdentify *c)
> > return ret;
> > }
> >
> > +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeCmd *c)
> > +{
> > +static const int len = 4096;
> The spec caps the Identify payload size to 4K,
> thus this should go to nvme.h
Done.
> > +
> > +struct ns_descr {
> > +uint8_t nidt;
> > +uint8_t nidl;
> > +uint8_t rsvd2[2];
> > +uint8_t nid[16];
> > +};
> This is also part of the spec, thus should
> move to nvme.h
>
Done - and cleaned up.
> > +
> > +uint32_t nsid = le32_to_cpu(c->nsid);
> > +uint64_t prp1 = le64_to_cpu(c->prp1);
> > +uint64_t prp2 = le64_to_cpu(c->prp2);
> > +
> > +struct ns_descr *list;
> > +uint16_t ret;
> > +
> > +trace_nvme_dev_identify_ns_descr_list(nsid);
> > +
> > +if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> > +trace_nvme_dev_err_invalid_ns(nsid, n->num_namespaces);
> > +return NVME_INVALID_NSID | NVME_DNR;
> > +}
> > +
> > +/*
> > + * 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.
> Some per namespace uuid qemu property will be very nice to have to have a
> uuid that
> is at least somewhat unique.
> Linux kernel I think might complain if it detects namespaces with duplicate
> uuids.
It will be "unique" per controller (because it's just the namespace id).
The spec also says that it should be fixed for the lifetime of the
namespace, but I'm not sure how to ensure that without keeping that
state on disk somehow. I have a solution for this in a later series, but
for now, I think this is ok.
But since we actually support multiple controllers, there certainly is
an issue here. Maybe we can
Re: [PATCH v5 15/26] nvme: bump supported specification to 1.3
On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> Add new fields to the Identify Controller and Identify Namespace data
> structures accoding to NVM Express 1.3d.
>
> NVM Express 1.3d requires the following additional features:
> - addition of the Namespace Identification Descriptor List (CNS 03h)
> for the Identify command
> - support for returning Command Sequence Error if a Set Features
> command is submitted for the Number of Queues feature after any I/O
> queues have been created.
> - The addition of the Log Specific Field (LSP) in the Get Log Page
> command.
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 57 ---
> hw/block/nvme.h | 1 +
> hw/block/trace-events | 3 ++-
> include/block/nvme.h | 20 ++-
> 4 files changed, 71 insertions(+), 10 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 900732bb2f38..4acfc85b56a2 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -9,7 +9,7 @@
> */
>
> /**
> - * Reference Specification: NVM Express 1.2.1
> + * Reference Specification: NVM Express 1.3d
> *
> * https://nvmexpress.org/resources/specifications/
> */
> @@ -43,7 +43,7 @@
> #include "trace.h"
> #include "nvme.h"
>
> -#define NVME_SPEC_VER 0x00010201
> +#define NVME_SPEC_VER 0x00010300
> #define NVME_MAX_QS PCI_MSIX_FLAGS_QSIZE
> #define NVME_TEMPERATURE 0x143
> #define NVME_TEMPERATURE_WARNING 0x157
> @@ -735,6 +735,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd,
> NvmeRequest *req)
> uint32_t dw12 = le32_to_cpu(cmd->cdw12);
> uint32_t dw13 = le32_to_cpu(cmd->cdw13);
> uint8_t lid = dw10 & 0xff;
> +uint8_t lsp = (dw10 >> 8) & 0xf;
> uint8_t rae = (dw10 >> 15) & 0x1;
> uint32_t numdl, numdu;
> uint64_t off, lpol, lpou;
> @@ -752,7 +753,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd,
> NvmeRequest *req)
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> -trace_nvme_dev_get_log(nvme_cid(req), lid, rae, len, off);
> +trace_nvme_dev_get_log(nvme_cid(req), lid, lsp, rae, len, off);
>
> switch (lid) {
> case NVME_LOG_ERROR_INFO:
> @@ -863,6 +864,8 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
> cq = g_malloc0(sizeof(*cq));
> nvme_init_cq(cq, n, prp1, cqid, vector, qsize + 1,
> NVME_CQ_FLAGS_IEN(qflags));
Code alignment on that '('
> +
> +n->qs_created = true;
Should be done also at nvme_create_sq
> return NVME_SUCCESS;
> }
>
> @@ -924,6 +927,47 @@ static uint16_t nvme_identify_ns_list(NvmeCtrl *n,
> NvmeIdentify *c)
> return ret;
> }
>
> +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeCmd *c)
> +{
> +static const int len = 4096;
The spec caps the Identify payload size to 4K,
thus this should go to nvme.h
> +
> +struct ns_descr {
> +uint8_t nidt;
> +uint8_t nidl;
> +uint8_t rsvd2[2];
> +uint8_t nid[16];
> +};
This is also part of the spec, thus should
move to nvme.h
> +
> +uint32_t nsid = le32_to_cpu(c->nsid);
> +uint64_t prp1 = le64_to_cpu(c->prp1);
> +uint64_t prp2 = le64_to_cpu(c->prp2);
> +
> +struct ns_descr *list;
> +uint16_t ret;
> +
> +trace_nvme_dev_identify_ns_descr_list(nsid);
> +
> +if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> +trace_nvme_dev_err_invalid_ns(nsid, n->num_namespaces);
> +return NVME_INVALID_NSID | NVME_DNR;
> +}
> +
> +/*
> + * 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.
Some per namespace uuid qemu property will be very nice to have to have a uuid
that
is at least somewhat unique.
Linux kernel I think might complain if it detects namespaces with duplicate
uuids.
> + */
> +list = g_malloc0(len);
> +list->nidt = 0x3;
> +list->nidl = 0x10;
Those should also be #defined in nvme.h
> +*(uint32_t *) >nid[12] = cpu_to_be32(nsid);
> +
> +ret = nvme_dma_read_prp(n, (uint8_t *) list, len, prp1, prp2);
> +g_free(list);
> +return ret;
> +}
> +
> static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> {
> NvmeIdentify *c = (NvmeIdentify *)cmd;
> @@ -935,6 +979,8 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> return nvme_identify_ctrl(n, c);
> case 0x02:
> return nvme_identify_ns_list(n, c);
> +case 0x03:
The CNS values should be defined in nvme.h.
> +return nvme_identify_ns_descr_list(n, cmd);
> default:
> trace_nvme_dev_err_invalid_identify_cns(le32_to_cpu(c->cns));
> return NVME_INVALID_FIELD | NVME_DNR;
> @@ -1133,6 +1179,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
>
