diff mbox series

[v6,01/11] hw/block/nvme: Add Commands Supported and Effects log

Message ID 20201013214212.2152-2-dmitry.fomichev@wdc.com (mailing list archive)
State New, archived
Headers show
Series hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set | expand

Commit Message

Dmitry Fomichev Oct. 13, 2020, 9:42 p.m. UTC
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       | 69 +++++++++++++++++++++++++++++++++++++++----
 hw/block/trace-events |  2 ++
 include/block/nvme.h  | 19 ++++++++++++
 4 files changed, 86 insertions(+), 5 deletions(-)

Comments

Keith Busch Oct. 14, 2020, 12:50 a.m. UTC | #1
On Wed, Oct 14, 2020 at 06:42:02AM +0900, Dmitry Fomichev wrote:
> +{
> +    NvmeEffectsLog log = {};
> +    uint32_t *dst_acs = log.acs, *dst_iocs = log.iocs;
> +    uint32_t trans_len;
> +    int i;
> +
> +    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;
> +    }
> +
> +    for (i = 0; i < 256; i++) {
> +        dst_acs[i] = nvme_cse_acs[i];
> +    }
> +
> +    for (i = 0; i < 256; i++) {
> +        dst_iocs[i] = nvme_cse_iocs_nvm[i];
> +    }

You're just copying the array, so let's do it like this:

    memcpy(log.acs, nvme_cse_acs, sizeof(nvme_cse_acs));
    memcpy(log.iocs, nvme_cse_iocs_nvm, sizeof(nvme_cse_iocs_nvm));

I think you also need to check

    if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY)

before copying iocs.
Niklas Cassel Oct. 14, 2020, 12:13 p.m. UTC | #2
On Tue, Oct 13, 2020 at 05:50:34PM -0700, Keith Busch wrote:
> On Wed, Oct 14, 2020 at 06:42:02AM +0900, Dmitry Fomichev wrote:
> > +{
> > +    NvmeEffectsLog log = {};
> > +    uint32_t *dst_acs = log.acs, *dst_iocs = log.iocs;
> > +    uint32_t trans_len;
> > +    int i;
> > +
> > +    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;
> > +    }
> > +
> > +    for (i = 0; i < 256; i++) {
> > +        dst_acs[i] = nvme_cse_acs[i];
> > +    }
> > +
> > +    for (i = 0; i < 256; i++) {
> > +        dst_iocs[i] = nvme_cse_iocs_nvm[i];
> > +    }
> 
> You're just copying the array, so let's do it like this:
> 
>     memcpy(log.acs, nvme_cse_acs, sizeof(nvme_cse_acs));
>     memcpy(log.iocs, nvme_cse_iocs_nvm, sizeof(nvme_cse_iocs_nvm));
> 
> I think you also need to check
> 
>     if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY)
> 
> before copying iocs.

So it seems Dmitry actually does this in the Namespace Types patch:


@@ -1051,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;
     }
@@ -1333,8 +1332,10 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint32_t buf_len,
         dst_acs[i] = nvme_cse_acs[i];
     }

-    for (i = 0; i < 256; i++) {
-        dst_iocs[i] = nvme_cse_iocs_nvm[i];
+    if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+        for (i = 0; i < 256; i++) {
+            dst_iocs[i] = nvme_cse_iocs_nvm[i];
+        }
     }


Which later in the ZNS patch is changed to:

@@ -1335,13 +2178,31 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint32_t buf_len,
         return NVME_INVALID_FIELD | NVME_DNR;
     }

+    switch (NVME_CC_CSS(n->bar.cc)) {
+    case NVME_CC_CSS_NVM:
+        src_iocs = nvme_cse_iocs_nvm;
+        break;
+    case NVME_CC_CSS_CSI:
+        switch (csi) {
+        case NVME_CSI_NVM:
+            src_iocs = nvme_cse_iocs_nvm;
+            break;
+        case NVME_CSI_ZONED:
+            src_iocs = nvme_cse_iocs_zoned;
+            break;
+        }
+        /* fall through */
+    case NVME_CC_CSS_ADMIN_ONLY:
+        break;
+    }
+
     for (i = 0; i < 256; i++) {
         dst_acs[i] = nvme_cse_acs[i];
     }

-    if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+    if (src_iocs) {
         for (i = 0; i < 256; i++) {
-            dst_iocs[i] = nvme_cse_iocs_nvm[i];
+            dst_iocs[i] = src_iocs[i];
         }
     }


Perhaps the nvme_io_cmd() if-statement removal from the NS types patch +
the switch from the ZNS patch (with out the NVME_CSI_ZONED) could be moved
to this patch instead?

The middle version of adding "+    if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {"
can then be dropped from the NS types patch, since it is not part of the final code anyway.


Kind regards,
Niklas
Dmitry Fomichev Oct. 19, 2020, 2:01 a.m. UTC | #3
> -----Original Message-----
> From: Keith Busch <kbusch@kernel.org>
> Sent: Tuesday, October 13, 2020 8:51 PM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: Klaus Jensen <k.jensen@samsung.com>; Kevin Wolf
> <kwolf@redhat.com>; Philippe Mathieu-Daudé <philmd@redhat.com>;
> Maxim Levitsky <mlevitsk@redhat.com>; Fam Zheng <fam@euphon.net>;
> Alistair Francis <Alistair.Francis@wdc.com>; Matias Bjorling
> <Matias.Bjorling@wdc.com>; Niklas Cassel <Niklas.Cassel@wdc.com>;
> Damien Le Moal <Damien.LeMoal@wdc.com>; qemu-block@nongnu.org;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH v6 01/11] hw/block/nvme: Add Commands Supported
> and Effects log
> 
> On Wed, Oct 14, 2020 at 06:42:02AM +0900, Dmitry Fomichev wrote:
> > +{
> > +    NvmeEffectsLog log = {};
> > +    uint32_t *dst_acs = log.acs, *dst_iocs = log.iocs;
> > +    uint32_t trans_len;
> > +    int i;
> > +
> > +    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;
> > +    }
> > +
> > +    for (i = 0; i < 256; i++) {
> > +        dst_acs[i] = nvme_cse_acs[i];
> > +    }
> > +
> > +    for (i = 0; i < 256; i++) {
> > +        dst_iocs[i] = nvme_cse_iocs_nvm[i];
> > +    }
> 
> You're just copying the array, so let's do it like this:
> 
>     memcpy(log.acs, nvme_cse_acs, sizeof(nvme_cse_acs));
>     memcpy(log.iocs, nvme_cse_iocs_nvm, sizeof(nvme_cse_iocs_nvm));
> 

Sure, this is a lot cleaner.

> I think you also need to check
> 
>     if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY)
> 
> before copying iocs.

Yes, thanks.
diff mbox series

Patch

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..ee0eff98da 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -111,6 +111,25 @@  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_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)
@@ -1045,6 +1064,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 +1078,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 +1314,35 @@  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 = {};
+    uint32_t *dst_acs = log.acs, *dst_iocs = log.iocs;
+    uint32_t trans_len;
+    int i;
+
+    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;
+    }
+
+    for (i = 0; i < 256; i++) {
+        dst_acs[i] = nvme_cse_acs[i];
+    }
+
+    for (i = 0; i < 256; i++) {
+        dst_iocs[i] = nvme_cse_iocs_nvm[i];
+    }
+
+    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 +1386,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 +1974,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;
+    }
+
     switch (req->cmd.opcode) {
     case NVME_ADM_CMD_DELETE_SQ:
         return nvme_del_sq(n, req);
@@ -1942,8 +2001,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);
     }
 }
 
@@ -2596,6 +2654,7 @@  int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
     trace_pci_nvme_register_namespace(nsid);
 
     n->namespaces[nsid - 1] = ns;
+    ns->iocs = nvme_cse_iocs_nvm;
 
     return 0;
 }
@@ -2737,7 +2796,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);