RE: [PATCH v7 01/11] hw/block/nvme: Add Commands Supported and Effects log
> -Original Message-
> From: Klaus Jensen
> Sent: Monday, October 19, 2020 4:16 PM
> To: Dmitry Fomichev
> Cc: Keith Busch ; Klaus Jensen
> ; Kevin Wolf ; Philippe
> Mathieu-Daudé ; Maxim Levitsky
> ; Fam Zheng ; Niklas Cassel
> ; Damien Le Moal ;
> qemu-block@nongnu.org; qemu-de...@nongnu.org; Alistair Francis
> ; Matias Bjorling
> Subject: Re: [PATCH v7 01/11] hw/block/nvme: Add Commands Supported
> and Effects log
>
> On Oct 19 11:17, Dmitry Fomichev wrote:
> > This log page becomes necessary to implement to allow checking for
> > Zone Append command support in Zoned Namespace Command Set.
> >
> > This commit adds the code to report this log page for NVM Command
> > Set only. The parts that are specific to zoned operation will be
> > added later in the series.
> >
> > All incoming admin and i/o commands are now only processed if their
> > corresponding support bits are set in this log. This provides an
> > easy way to control what commands to support and what not to
> > depending on set CC.CSS.
> >
> > Signed-off-by: Dmitry Fomichev
> > ---
> > hw/block/nvme-ns.h| 1 +
> > hw/block/nvme.c | 98 +++--
> --
> > hw/block/trace-events | 2 +
> > include/block/nvme.h | 19 +
> > 4 files changed, 111 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> > index 83734f4606..ea8c2f785d 100644
> > --- a/hw/block/nvme-ns.h
> > +++ b/hw/block/nvme-ns.h
> > @@ -29,6 +29,7 @@ typedef struct NvmeNamespace {
> > int32_t bootindex;
> > int64_t size;
> > NvmeIdNs id_ns;
> > +const uint32_t *iocs;
> >
> > NvmeNamespaceParams params;
> > } NvmeNamespace;
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 9d30ca69dc..5a9493d89f 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -111,6 +111,28 @@ static const uint32_t
> nvme_feature_cap[NVME_FID_MAX] = {
> > [NVME_TIMESTAMP]= NVME_FEAT_CAP_CHANGE,
> > };
> >
> > +static const uint32_t nvme_cse_acs[256] = {
> > +[NVME_ADM_CMD_DELETE_SQ]= NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_CREATE_SQ]= NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_DELETE_CQ]= NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_CREATE_CQ]= NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_IDENTIFY] = NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_SET_FEATURES] = NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_GET_FEATURES] = NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_GET_LOG_PAGE] = NVME_CMD_EFF_CSUPP,
> > +[NVME_ADM_CMD_ASYNC_EV_REQ] = NVME_CMD_EFF_CSUPP,
> > +};
>
> NVME_ADM_CMD_ABORT is missing. And since you added a (redundant)
> check
> in nvme_admin_cmd that cheks this table, Abort is now an invalid
> command.
Adding the ABORT, thanks. I think this code was conceived before abort was
merged, this is why it is missing.
>
> Also, can you reorder it according to opcode instead of
> pseudo-lexicographically?
Ok, will move ...GET_LOG_PAGE that is now out or order.
>
> > +
> > +static const uint32_t nvme_cse_iocs_none[256] = {
> > +};
>
> [-pedantic] no need for the '= {}'
OK.
>
> > +
> > +static const uint32_t nvme_cse_iocs_nvm[256] = {
> > +[NVME_CMD_FLUSH]= NVME_CMD_EFF_CSUPP |
> NVME_CMD_EFF_LBCC,
> > +[NVME_CMD_WRITE_ZEROES] = NVME_CMD_EFF_CSUPP |
> NVME_CMD_EFF_LBCC,
> > +[NVME_CMD_WRITE]= NVME_CMD_EFF_CSUPP |
> NVME_CMD_EFF_LBCC,
> > +[NVME_CMD_READ] = NVME_CMD_EFF_CSUPP,
> > +};
> > +
> > static void nvme_process_sq(void *opaque);
> >
> > static uint16_t nvme_cid(NvmeRequest *req)
> > @@ -1032,10 +1054,6 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n,
> NvmeRequest *req)
> > trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
> >req->cmd.opcode,
> > nvme_io_opc_str(req->cmd.opcode));
> >
> > -if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_ADMIN_ONLY) {
> > -return NVME_INVALID_OPCODE | NVME_DNR;
> > -}
> > -
>
> I would assume the device to respond with invalid opcode before
> validating the nsid if it is an admin only device.
The host can't make any assumptions about the ordering of validation
checks performed by the controller. In the case of receiving an i/o
command with invalid NSID when CC.CSS == ADMIN_ONLY, both
Invalid Opcode and Invalid NSID error status
Re: [PATCH v7 01/11] hw/block/nvme: Add Commands Supported and Effects log
On Oct 19 11:17, Dmitry Fomichev wrote:
> This log page becomes necessary to implement to allow checking for
> Zone Append command support in Zoned Namespace Command Set.
>
> This commit adds the code to report this log page for NVM Command
> Set only. The parts that are specific to zoned operation will be
> added later in the series.
>
> All incoming admin and i/o commands are now only processed if their
> corresponding support bits are set in this log. This provides an
> easy way to control what commands to support and what not to
> depending on set CC.CSS.
>
> Signed-off-by: Dmitry Fomichev
> ---
> hw/block/nvme-ns.h| 1 +
> hw/block/nvme.c | 98 +++
> hw/block/trace-events | 2 +
> include/block/nvme.h | 19 +
> 4 files changed, 111 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> index 83734f4606..ea8c2f785d 100644
> --- a/hw/block/nvme-ns.h
> +++ b/hw/block/nvme-ns.h
> @@ -29,6 +29,7 @@ typedef struct NvmeNamespace {
> int32_t bootindex;
> int64_t size;
> NvmeIdNs id_ns;
> +const uint32_t *iocs;
>
> NvmeNamespaceParams params;
> } NvmeNamespace;
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9d30ca69dc..5a9493d89f 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -111,6 +111,28 @@ static const uint32_t nvme_feature_cap[NVME_FID_MAX] = {
> [NVME_TIMESTAMP]= NVME_FEAT_CAP_CHANGE,
> };
>
> +static const uint32_t nvme_cse_acs[256] = {
> +[NVME_ADM_CMD_DELETE_SQ]= NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_CREATE_SQ]= NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_DELETE_CQ]= NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_CREATE_CQ]= NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_IDENTIFY] = NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_SET_FEATURES] = NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_GET_FEATURES] = NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_GET_LOG_PAGE] = NVME_CMD_EFF_CSUPP,
> +[NVME_ADM_CMD_ASYNC_EV_REQ] = NVME_CMD_EFF_CSUPP,
> +};
NVME_ADM_CMD_ABORT is missing. And since you added a (redundant) check
in nvme_admin_cmd that cheks this table, Abort is now an invalid
command.
Also, can you reorder it according to opcode instead of
pseudo-lexicographically?
> +
> +static const uint32_t nvme_cse_iocs_none[256] = {
> +};
[-pedantic] no need for the '= {}'
> +
> +static const uint32_t nvme_cse_iocs_nvm[256] = {
> +[NVME_CMD_FLUSH]= NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
> +[NVME_CMD_WRITE_ZEROES] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
> +[NVME_CMD_WRITE]= NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
> +[NVME_CMD_READ] = NVME_CMD_EFF_CSUPP,
> +};
> +
> static void nvme_process_sq(void *opaque);
>
> static uint16_t nvme_cid(NvmeRequest *req)
> @@ -1032,10 +1054,6 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest
> *req)
> trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
>req->cmd.opcode, nvme_io_opc_str(req->cmd.opcode));
>
> -if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_ADMIN_ONLY) {
> -return NVME_INVALID_OPCODE | NVME_DNR;
> -}
> -
I would assume the device to respond with invalid opcode before
validating the nsid if it is an admin only device.
> if (!nvme_nsid_valid(n, nsid)) {
> return NVME_INVALID_NSID | NVME_DNR;
> }
> @@ -1045,6 +1063,11 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest
> *req)
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> +if (!(req->ns->iocs[req->cmd.opcode] & NVME_CMD_EFF_CSUPP)) {
> +trace_pci_nvme_err_invalid_opc(req->cmd.opcode);
> +return NVME_INVALID_OPCODE | NVME_DNR;
> +}
> +
> switch (req->cmd.opcode) {
> case NVME_CMD_FLUSH:
> return nvme_flush(n, req);
> @@ -1054,8 +1077,7 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest
> *req)
> case NVME_CMD_READ:
> return nvme_rw(n, req);
> default:
> -trace_pci_nvme_err_invalid_opc(req->cmd.opcode);
> -return NVME_INVALID_OPCODE | NVME_DNR;
> +assert(false);
> }
> }
>
> @@ -1291,6 +1313,39 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t
> rae, uint32_t buf_len,
> DMA_DIRECTION_FROM_DEVICE, req);
> }
>
> +static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint32_t buf_len,
> + uint64_t off, NvmeRequest *req)
> +{
> +NvmeEffectsLog log = {};
[-pedantic] and empty initializer list is not allowed, should be '{0}'.
> +const uint32_t *src_iocs = NULL;
> +uint32_t trans_len;
> +
> +trace_pci_nvme_cmd_supp_and_effects_log_read();
This has just been traced in nvme_admin_cmd and this doesn't add any
additional info.
> +
> +if (off >= sizeof(log)) {
> +trace_pci_nvme_err_invalid_effects_log_offset(off);
Can we do `trace_pci_nvme_err_
Re: [PATCH v7 01/11] hw/block/nvme: Add Commands Supported and Effects log
On Mon, Oct 19, 2020 at 11:17:16AM +0900, Dmitry Fomichev wrote: > This log page becomes necessary to implement to allow checking for > Zone Append command support in Zoned Namespace Command Set. > > This commit adds the code to report this log page for NVM Command > Set only. The parts that are specific to zoned operation will be > added later in the series. > > All incoming admin and i/o commands are now only processed if their > corresponding support bits are set in this log. This provides an > easy way to control what commands to support and what not to > depending on set CC.CSS. > > Signed-off-by: Dmitry Fomichev Looks good to me. Reviewed-by: Keith Busch
[PATCH v7 01/11] hw/block/nvme: Add Commands Supported and Effects log
This log page becomes necessary to implement to allow checking for
Zone Append command support in Zoned Namespace Command Set.
This commit adds the code to report this log page for NVM Command
Set only. The parts that are specific to zoned operation will be
added later in the series.
All incoming admin and i/o commands are now only processed if their
corresponding support bits are set in this log. This provides an
easy way to control what commands to support and what not to
depending on set CC.CSS.
Signed-off-by: Dmitry Fomichev
---
hw/block/nvme-ns.h| 1 +
hw/block/nvme.c | 98 +++
hw/block/trace-events | 2 +
include/block/nvme.h | 19 +
4 files changed, 111 insertions(+), 9 deletions(-)
diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index 83734f4606..ea8c2f785d 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -29,6 +29,7 @@ typedef struct NvmeNamespace {
int32_t bootindex;
int64_t size;
NvmeIdNs id_ns;
+const uint32_t *iocs;
NvmeNamespaceParams params;
} NvmeNamespace;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9d30ca69dc..5a9493d89f 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -111,6 +111,28 @@ static const uint32_t nvme_feature_cap[NVME_FID_MAX] = {
[NVME_TIMESTAMP]= NVME_FEAT_CAP_CHANGE,
};
+static const uint32_t nvme_cse_acs[256] = {
+[NVME_ADM_CMD_DELETE_SQ]= NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_CREATE_SQ]= NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_DELETE_CQ]= NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_CREATE_CQ]= NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_IDENTIFY] = NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_SET_FEATURES] = NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_GET_FEATURES] = NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_GET_LOG_PAGE] = NVME_CMD_EFF_CSUPP,
+[NVME_ADM_CMD_ASYNC_EV_REQ] = NVME_CMD_EFF_CSUPP,
+};
+
+static const uint32_t nvme_cse_iocs_none[256] = {
+};
+
+static const uint32_t nvme_cse_iocs_nvm[256] = {
+[NVME_CMD_FLUSH]= NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
+[NVME_CMD_WRITE_ZEROES] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
+[NVME_CMD_WRITE]= NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
+[NVME_CMD_READ] = NVME_CMD_EFF_CSUPP,
+};
+
static void nvme_process_sq(void *opaque);
static uint16_t nvme_cid(NvmeRequest *req)
@@ -1032,10 +1054,6 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest
*req)
trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
req->cmd.opcode, nvme_io_opc_str(req->cmd.opcode));
-if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_ADMIN_ONLY) {
-return NVME_INVALID_OPCODE | NVME_DNR;
-}
-
if (!nvme_nsid_valid(n, nsid)) {
return NVME_INVALID_NSID | NVME_DNR;
}
@@ -1045,6 +1063,11 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest
*req)
return NVME_INVALID_FIELD | NVME_DNR;
}
+if (!(req->ns->iocs[req->cmd.opcode] & NVME_CMD_EFF_CSUPP)) {
+trace_pci_nvme_err_invalid_opc(req->cmd.opcode);
+return NVME_INVALID_OPCODE | NVME_DNR;
+}
+
switch (req->cmd.opcode) {
case NVME_CMD_FLUSH:
return nvme_flush(n, req);
@@ -1054,8 +1077,7 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
case NVME_CMD_READ:
return nvme_rw(n, req);
default:
-trace_pci_nvme_err_invalid_opc(req->cmd.opcode);
-return NVME_INVALID_OPCODE | NVME_DNR;
+assert(false);
}
}
@@ -1291,6 +1313,39 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t
rae, uint32_t buf_len,
DMA_DIRECTION_FROM_DEVICE, req);
}
+static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint32_t buf_len,
+ uint64_t off, NvmeRequest *req)
+{
+NvmeEffectsLog log = {};
+const uint32_t *src_iocs = NULL;
+uint32_t trans_len;
+
+trace_pci_nvme_cmd_supp_and_effects_log_read();
+
+if (off >= sizeof(log)) {
+trace_pci_nvme_err_invalid_effects_log_offset(off);
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+switch (NVME_CC_CSS(n->bar.cc)) {
+case NVME_CC_CSS_NVM:
+src_iocs = nvme_cse_iocs_nvm;
+case NVME_CC_CSS_ADMIN_ONLY:
+break;
+}
+
+memcpy(log.acs, nvme_cse_acs, sizeof(nvme_cse_acs));
+
+if (src_iocs) {
+memcpy(log.iocs, src_iocs, sizeof(log.iocs));
+}
+
+trans_len = MIN(sizeof(log) - off, buf_len);
+
+return nvme_dma(n, ((uint8_t *)&log) + off, trans_len,
+DMA_DIRECTION_FROM_DEVICE, req);
+}
+
static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
{
NvmeCmd *cmd = &req->cmd;
@@ -1334,6 +1389,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest
*req)
return nvme_smart_info(n, rae, len, off, req);
case NVME_LOG_FW_SLOT_INFO:
return nvme_fw_log_info(n, len, off, req
