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