Re: [PATCH v7 11/48] nvme: refactor device realization

Philippe Mathieu-Daudé Wed, 15 Apr 2020 00:14:27 -0700

Hi Klaus,

On 4/15/20 7:51 AM, Klaus Jensen wrote:

From: Klaus Jensen <k.jen...@samsung.com>


This patch splits up nvme_realize into multiple individual functions,
each initializing a different subset of the device.

Signed-off-by: Klaus Jensen <klaus.jen...@cnexlabs.com>
Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
Acked-by: Keith Busch <kbu...@kernel.org>
---
  hw/block/nvme.c | 178 +++++++++++++++++++++++++++++++-----------------
  hw/block/nvme.h |  21 ++++++
  2 files changed, 136 insertions(+), 63 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 123539a5d0ae..d1c42ee4765c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -44,6 +44,8 @@
  #include "trace.h"
  #include "nvme.h"

+#define NVME_CMB_BIR 2

+
  #define NVME_GUEST_ERR(trace, fmt, ...) \
      do { \
          (trace_##trace)(__VA_ARGS__); \
@@ -1322,73 +1324,112 @@ static const MemoryRegionOps nvme_cmb_ops = {
      },
  };

-static void nvme_realize(PCIDevice *pci_dev, Error **errp)

+static int nvme_check_constraints(NvmeCtrl *n, Error **errp)
  {
-    NvmeCtrl *n = NVME(pci_dev);
-    NvmeIdCtrl *id = &n->id_ctrl;
+    NvmeParams *params = &n->params;

- int i;

-    int64_t bs_size;
-    uint8_t *pci_conf;
-
-    if (n->params.num_queues) {
+    if (params->num_queues) {
          warn_report("nvme: num_queues is deprecated; please use max_ioqpairs "
                      "instead");

- n->params.max_ioqpairs = n->params.num_queues - 1;

+        params->max_ioqpairs = params->num_queues - 1;
      }

- if (!n->params.max_ioqpairs) {

-        error_setg(errp, "max_ioqpairs can't be less than 1");
+    if (params->max_ioqpairs < 1 ||
+        params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
+        error_setg(errp, "nvme: max_ioqpairs must be between 1 and %d",
+                   PCI_MSIX_FLAGS_QSIZE);
+        return -1;
      }

if (!n->conf.blk) {

-        error_setg(errp, "drive property not set");
-        return;
+        error_setg(errp, "nvme: block backend not configured");
+        return -1;
      }

- bs_size = blk_getlength(n->conf.blk);

-    if (bs_size < 0) {
-        error_setg(errp, "could not get backing file size");
-        return;
+    if (!params->serial) {
+        error_setg(errp, "nvme: serial not configured");
+        return -1;
      }

- if (!n->params.serial) {

-        error_setg(errp, "serial property not set");
-        return;
-    }
+    return 0;
+}
+
+static int nvme_init_blk(NvmeCtrl *n, Error **errp)
+{
      blkconf_blocksizes(&n->conf);
      if (!blkconf_apply_backend_options(&n->conf, 
blk_is_read_only(n->conf.blk),
                                         false, errp)) {
-        return;
+        return -1;
      }

- pci_conf = pci_dev->config;

-    pci_conf[PCI_INTERRUPT_PIN] = 1;
-    pci_config_set_prog_interface(pci_dev->config, 0x2);
-    pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
-    pcie_endpoint_cap_init(pci_dev, 0x80);
+    return 0;
+}

+static void nvme_init_state(NvmeCtrl *n)

+{
      n->num_namespaces = 1;
      n->reg_size = pow2ceil(0x1008 + 2 * (n->params.max_ioqpairs) * 4);
-    n->ns_size = bs_size / (uint64_t)n->num_namespaces;
-
      n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
      n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
      n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1);
+}

- memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n,

-                          "nvme", n->reg_size);
-    pci_register_bar(pci_dev, 0,
-        PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
-        &n->iomem);
+static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
+{
+    NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
+    NVME_CMBLOC_SET_OFST(n->bar.cmbloc, 0);
+
+    NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
+    NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
+    NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
+    NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
+    NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
+    NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2);
+    NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb);
+
+    n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
+    memory_region_init_io(&n->ctrl_mem, OBJECT(n), &nvme_cmb_ops, n,
+                          "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
+    pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc),
+                     PCI_BASE_ADDRESS_SPACE_MEMORY |
+                     PCI_BASE_ADDRESS_MEM_TYPE_64 |
+                     PCI_BASE_ADDRESS_MEM_PREFETCH, &n->ctrl_mem);
+}
+
+static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev)
+{
+    uint8_t *pci_conf = pci_dev->config;
+
+    pci_conf[PCI_INTERRUPT_PIN] = 1;
+    pci_config_set_prog_interface(pci_conf, 0x2);
+    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
+    pci_config_set_device_id(pci_conf, 0x5845);
+    pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS);
+    pcie_endpoint_cap_init(pci_dev, 0x80);
+
+    memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n, "nvme",
+                          n->reg_size);
+    pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
+                     PCI_BASE_ADDRESS_MEM_TYPE_64, &n->iomem);
      msix_init_exclusive_bar(pci_dev, n->params.max_ioqpairs + 1, 4, NULL);

+ if (n->params.cmb_size_mb) {

+        nvme_init_cmb(n, pci_dev);
+    }
+}
+
+static void nvme_init_ctrl(NvmeCtrl *n)
+{
+    NvmeIdCtrl *id = &n->id_ctrl;
+    NvmeParams *params = &n->params;
+    uint8_t *pci_conf = n->parent_obj.config;
+
      id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
      id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
      strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' ');
      strpadcpy((char *)id->fr, sizeof(id->fr), "1.0", ' ');
-    strpadcpy((char *)id->sn, sizeof(id->sn), n->params.serial, ' ');
+    strpadcpy((char *)id->sn, sizeof(id->sn), params->serial, ' ');
      id->rab = 6;
      id->ieee[0] = 0x00;
      id->ieee[1] = 0x02;
@@ -1429,43 +1470,54 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
**errp)

n->bar.vs = 0x00010200;

      n->bar.intmc = n->bar.intms = 0;
+}

- if (n->params.cmb_size_mb) {

+static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
+{
+    int64_t bs_size;
+    NvmeIdNs *id_ns = &ns->id_ns;

- NVME_CMBLOC_SET_BIR(n->bar.cmbloc, 2);

-        NVME_CMBLOC_SET_OFST(n->bar.cmbloc, 0);
+    bs_size = blk_getlength(n->conf.blk);
+    if (bs_size < 0) {
+        error_setg_errno(errp, -bs_size, "blk_getlength");
+        return -1;
+    }

- NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);

-        NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
-        NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
-        NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
-        NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
-        NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2); /* MBs */
-        NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb);
+    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
+    n->ns_size = bs_size;

- n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));

-        memory_region_init_io(&n->ctrl_mem, OBJECT(n), &nvme_cmb_ops, n,
-                              "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
-        pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc),
-            PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64 |
-            PCI_BASE_ADDRESS_MEM_PREFETCH, &n->ctrl_mem);
+    id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns));

+ /* no thin provisioning */

+    id_ns->ncap = id_ns->nsze;
+    id_ns->nuse = id_ns->ncap;
+
+    return 0;
+}
+
+static void nvme_realize(PCIDevice *pci_dev, Error **errp)
+{
+    NvmeCtrl *n = NVME(pci_dev);
+    int i;
+
+    if (nvme_check_constraints(n, errp)) {
+        return;
+    }
+
+    nvme_init_state(n);
+
+    if (nvme_init_blk(n, errp)) {
+        return;
      }

for (i = 0; i < n->num_namespaces; i++) {

-        NvmeNamespace *ns = &n->namespaces[i];
-        NvmeIdNs *id_ns = &ns->id_ns;
-        id_ns->nsfeat = 0;
-        id_ns->nlbaf = 0;
-        id_ns->flbas = 0;
-        id_ns->mc = 0;
-        id_ns->dpc = 0;
-        id_ns->dps = 0;
-        id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
-        id_ns->ncap  = id_ns->nuse = id_ns->nsze =
-            cpu_to_le64(n->ns_size >>
-                id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
+        if (nvme_init_namespace(n, &n->namespaces[i], errp)) {
+            return;
+        }
      }
+
+    nvme_init_pci(n, pci_dev);
+    nvme_init_ctrl(n);

This patch is a pain to review... Could you split it? I'd use onetrivial patch for each function extracted from nvme_realize().

static void nvme_exit(PCIDevice *pci_dev)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index ad1786953be9..b7c465560eea 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -67,6 +67,22 @@ typedef struct NvmeNamespace {
      NvmeIdNs        id_ns;
  } NvmeNamespace;

+static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)

+{
+    NvmeIdNs *id_ns = &ns->id_ns;
+    return &id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)];
+}
+
+static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
+{
+    return nvme_ns_lbaf(ns)->ds;
+}
+
+static inline size_t nvme_ns_lbads_bytes(NvmeNamespace *ns)
+{
+    return 1 << nvme_ns_lbads(ns);
+}
+
  #define TYPE_NVME "nvme"
  #define NVME(obj) \
          OBJECT_CHECK(NvmeCtrl, (obj), TYPE_NVME)
@@ -101,4 +117,9 @@ typedef struct NvmeCtrl {
      NvmeIdCtrl      id_ctrl;
  } NvmeCtrl;

+static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns)

+{
+    return n->ns_size >> nvme_ns_lbads(ns);
+}
+
  #endif /* HW_NVME_H */

Re: [PATCH v7 11/48] nvme: refactor device realization

Reply via email to