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 <dmitry.fomic...@wdc.com>
> ---
> 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_invalid_log_page_offset(off) instead? Then
we can easily reuse it in the other log pages.
> + 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);
> + case NVME_LOG_CMD_EFFECTS:
> + return nvme_cmd_effects(n, len, off, req);
> default:
> trace_pci_nvme_err_invalid_log_page(nvme_cid(req), lid);
> return NVME_INVALID_FIELD | NVME_DNR;
> @@ -1920,6 +1977,11 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n,
> NvmeRequest *req)
> trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), req->cmd.opcode,
> nvme_adm_opc_str(req->cmd.opcode));
>
> + if (!(nvme_cse_acs[req->cmd.opcode] & NVME_CMD_EFF_CSUPP)) {
> + trace_pci_nvme_err_invalid_admin_opc(req->cmd.opcode);
> + return NVME_INVALID_OPCODE | NVME_DNR;
> + }
> +
This is the (redundant) check that effectively makes Abort an invalid
command.
> switch (req->cmd.opcode) {
> case NVME_ADM_CMD_DELETE_SQ:
> return nvme_del_sq(n, req);
> @@ -1942,8 +2004,7 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest
> *req)
> case NVME_ADM_CMD_ASYNC_EV_REQ:
> return nvme_aer(n, req);
> default:
> - trace_pci_nvme_err_invalid_admin_opc(req->cmd.opcode);
> - return NVME_INVALID_OPCODE | NVME_DNR;
> + assert(false);
> }
> }
>
> @@ -2031,6 +2092,23 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
> n->bar.cc = 0;
> }
>
> +static void nvme_select_ns_iocs(NvmeCtrl *n)
> +{
> + NvmeNamespace *ns;
> + int i;
> +
> + for (i = 1; i <= n->num_namespaces; i++) {
> + ns = nvme_ns(n, i);
> + if (!ns) {
> + continue;
> + }
> + ns->iocs = nvme_cse_iocs_none;
> + if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
> + ns->iocs = nvme_cse_iocs_nvm;
> + }
> + }
> +}
> +
> static int nvme_start_ctrl(NvmeCtrl *n)
> {
> uint32_t page_bits = NVME_CC_MPS(n->bar.cc) + 12;
> @@ -2129,6 +2207,8 @@ static int nvme_start_ctrl(NvmeCtrl *n)
>
> QTAILQ_INIT(&n->aer_queue);
>
> + nvme_select_ns_iocs(n);
> +
> return 0;
> }
>
> @@ -2737,7 +2817,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
> *pci_dev)
> id->acl = 3;
> id->aerl = n->params.aerl;
> id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO;
> - id->lpa = NVME_LPA_NS_SMART | NVME_LPA_EXTENDED;
> + id->lpa = NVME_LPA_NS_SMART | NVME_LPA_CSE | NVME_LPA_EXTENDED;
>
> /* recommended default value (~70 C) */
> id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING);
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index fac5995d94..0ae9cb0d35 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -85,6 +85,7 @@ pci_nvme_mmio_start_success(void) "setting controller
> enable bit succeeded"
> pci_nvme_mmio_stopped(void) "cleared controller enable bit"
> pci_nvme_mmio_shutdown_set(void) "shutdown bit set"
> pci_nvme_mmio_shutdown_cleared(void) "shutdown bit cleared"
> +pci_nvme_cmd_supp_and_effects_log_read(void) "commands supported and effects
> log read"
>
> # nvme traces for error conditions
> pci_nvme_err_mdts(uint16_t cid, size_t len) "cid %"PRIu16" len %zu"
> @@ -104,6 +105,7 @@ pci_nvme_err_invalid_prp(void) "invalid PRP"
> pci_nvme_err_invalid_opc(uint8_t opc) "invalid opcode 0x%"PRIx8""
> pci_nvme_err_invalid_admin_opc(uint8_t opc) "invalid admin opcode 0x%"PRIx8""
> pci_nvme_err_invalid_lba_range(uint64_t start, uint64_t len, uint64_t limit)
> "Invalid LBA start=%"PRIu64" len=%"PRIu64" limit=%"PRIu64""
> +pci_nvme_err_invalid_effects_log_offset(uint64_t ofs) "commands supported
> and effects log offset must be 0, got %"PRIu64""
> pci_nvme_err_invalid_del_sq(uint16_t qid) "invalid submission queue
> deletion, sid=%"PRIu16""
> pci_nvme_err_invalid_create_sq_cqid(uint16_t cqid) "failed creating
> submission queue, invalid cqid=%"PRIu16""
> pci_nvme_err_invalid_create_sq_sqid(uint16_t sqid) "failed creating
> submission queue, invalid sqid=%"PRIu16""
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 6de2d5aa75..4779495b7d 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -744,10 +744,27 @@ enum NvmeSmartWarn {
> NVME_SMART_FAILED_VOLATILE_MEDIA = 1 << 4,
> };
>
> +typedef struct NvmeEffectsLog {
> + uint32_t acs[256];
> + uint32_t iocs[256];
> + uint8_t resv[2048];
> +} NvmeEffectsLog;
> +
> +enum {
> + NVME_CMD_EFF_CSUPP = 1 << 0,
> + NVME_CMD_EFF_LBCC = 1 << 1,
> + NVME_CMD_EFF_NCC = 1 << 2,
> + NVME_CMD_EFF_NIC = 1 << 3,
> + NVME_CMD_EFF_CCC = 1 << 4,
> + NVME_CMD_EFF_CSE_MASK = 3 << 16,
> + NVME_CMD_EFF_UUID_SEL = 1 << 19,
> +};
> +
> enum NvmeLogIdentifier {
> NVME_LOG_ERROR_INFO = 0x01,
> NVME_LOG_SMART_INFO = 0x02,
> NVME_LOG_FW_SLOT_INFO = 0x03,
> + NVME_LOG_CMD_EFFECTS = 0x05,
> };
>
> typedef struct QEMU_PACKED NvmePSD {
> @@ -860,6 +877,7 @@ enum NvmeIdCtrlFrmw {
>
> enum NvmeIdCtrlLpa {
> NVME_LPA_NS_SMART = 1 << 0,
> + NVME_LPA_CSE = 1 << 1,
> NVME_LPA_EXTENDED = 1 << 2,
> };
>
> @@ -1059,6 +1077,7 @@ static inline void _nvme_check_size(void)
> QEMU_BUILD_BUG_ON(sizeof(NvmeErrorLog) != 64);
> QEMU_BUILD_BUG_ON(sizeof(NvmeFwSlotInfoLog) != 512);
> QEMU_BUILD_BUG_ON(sizeof(NvmeSmartLog) != 512);
> + QEMU_BUILD_BUG_ON(sizeof(NvmeEffectsLog) != 4096);
> QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrl) != 4096);
> QEMU_BUILD_BUG_ON(sizeof(NvmeIdNs) != 4096);
> QEMU_BUILD_BUG_ON(sizeof(NvmeSglDescriptor) != 16);
> --
> 2.21.0
>
>
--
One of us - No more doubt, silence or taboo about mental illness.
signature.asc
Description: PGP signature
