[PATCH 0/3] hw/block/nvme: allow cmb and pmr to coexist

[PATCH 0/3] hw/block/nvme: allow cmb and pmr to coexist

[Klaus Jensen]

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

This is a resurrection of Andrzej's series[1] from back July.

Andrzej's main patch basically moved the the CMB from BAR 2 into an
offset in BAR 4 (located after the MSI-X table and PBA). Having an
offset on the CMB causes a bunch of calculations related to address
mapping to change.

So, since I couldn't get the patch to apply cleanly I took a stab at
implementing the suggestion I originally came up with: simply move the
MSI-X table and PBA from BAR 4 into BAR 0 (up-aligned to a 4 KiB
boundary, after the main NVMe controller registers). This way we can
keep the CMB at offset zero in its own BAR and free up BAR 4 for use by
PMR. This makes the patch simpler and does not impact any of the
existing address mapping code.

Andrzej, I would prefer an Ack from you, since I pretty much voided your
main patch.

  [1]: https://lore.kernel.org/qemu-devel/20200729220107.37758-1-andrzej.jakowski@linux.intel.com/

CC: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>

Andrzej Jakowski (1):
  hw/block/nvme: indicate CMB support through controller capabilities
    register

Klaus Jensen (2):
  hw/block/nvme: move msix table and pba to BAR 0
  hw/block/nvme: allow cmb and pmr to coexist

 hw/block/nvme.h      |  1 +
 include/block/nvme.h | 10 +++++++---
 hw/block/nvme.c      | 42 +++++++++++++++++++++++++++++++-----------
 3 files changed, 39 insertions(+), 14 deletions(-)

-- 
2.29.2

[PATCH 1/3] hw/block/nvme: indicate CMB support through controller capabilities register

[Klaus Jensen]

From: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>

This patch sets CMBS bit in controller capabilities register when user
configures NVMe driver with CMB support, so capabilites are correctly
reported to guest OS.

Signed-off-by: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
Reviewed-by: Maxim Levitsky <mlevitsky@gmail.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 include/block/nvme.h | 10 +++++++---
 hw/block/nvme.c      |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 8a46d9cf015f..c7cba786b491 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -36,6 +36,7 @@ enum NvmeCapShift {
     CAP_MPSMIN_SHIFT   = 48,
     CAP_MPSMAX_SHIFT   = 52,
     CAP_PMR_SHIFT      = 56,
+    CAP_CMB_SHIFT      = 57,
 };
 
 enum NvmeCapMask {
@@ -49,6 +50,7 @@ enum NvmeCapMask {
     CAP_MPSMIN_MASK    = 0xf,
     CAP_MPSMAX_MASK    = 0xf,
     CAP_PMR_MASK       = 0x1,
+    CAP_CMB_MASK       = 0x1,
 };
 
 #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
@@ -78,9 +80,11 @@ enum NvmeCapMask {
 #define NVME_CAP_SET_MPSMIN(cap, val) (cap |= (uint64_t)(val & CAP_MPSMIN_MASK)\
                                                            << CAP_MPSMIN_SHIFT)
 #define NVME_CAP_SET_MPSMAX(cap, val) (cap |= (uint64_t)(val & CAP_MPSMAX_MASK)\
-                                                            << CAP_MPSMAX_SHIFT)
-#define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)\
-                                                            << CAP_PMR_SHIFT)
+                                                           << CAP_MPSMAX_SHIFT)
+#define NVME_CAP_SET_PMRS(cap, val)   (cap |= (uint64_t)(val & CAP_PMR_MASK)   \
+                                                           << CAP_PMR_SHIFT)
+#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMB_MASK)   \
+                                                           << CAP_CMB_SHIFT)
 
 enum NvmeCapCss {
     NVME_CAP_CSS_NVM        = 1 << 0,
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 01b657b1c5e2..4c599159f533 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2754,6 +2754,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_NVM);
     NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_ADMIN_ONLY);
     NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
+    NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
 
     n->bar.vs = NVME_SPEC_VER;
     n->bar.intmc = n->bar.intms = 0;
-- 
2.29.2

[PATCH 2/3] hw/block/nvme: move msix table and pba to BAR 0

[Klaus Jensen]

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

In the interest of supporting both CMB and PMR to be enabled on the same
device, move the MSI-X table and pending bit out of BAR 4 and into BAR
0.

This is a simplified version of the patch contributed by Andrzej
Jakowski (see [1]). Leaving the CMB at offset 0 removes the need for
changes to CMB address mapping code.

  [1]: https://lore.kernel.org/qemu-devel/20200729220107.37758-3-andrzej.jakowski@linux.intel.com/

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.h |  1 +
 hw/block/nvme.c | 24 ++++++++++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index e080a2318a50..b7b42e70f97f 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -116,6 +116,7 @@ typedef struct NvmeFeatureVal {
 
 typedef struct NvmeCtrl {
     PCIDevice    parent_obj;
+    MemoryRegion bar0;
     MemoryRegion iomem;
     MemoryRegion ctrl_mem;
     NvmeBar      bar;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 4c599159f533..db8c5ae2f527 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2660,6 +2660,8 @@ static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
 static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 {
     uint8_t *pci_conf = pci_dev->config;
+    uint64_t bar_size, msix_table_size, msix_pba_size;
+    unsigned msix_table_offset, msix_pba_offset;
 
     pci_conf[PCI_INTERRUPT_PIN] = 1;
     pci_config_set_prog_interface(pci_conf, 0x2);
@@ -2675,11 +2677,29 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
     pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS);
     pcie_endpoint_cap_init(pci_dev, 0x80);
 
+    bar_size = QEMU_ALIGN_UP(n->reg_size, 4 * KiB);
+    msix_table_offset = bar_size;
+    msix_table_size = PCI_MSIX_ENTRY_SIZE * n->params.msix_qsize;
+
+    bar_size += msix_table_size;
+    bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB);
+    msix_pba_offset = bar_size;
+    msix_pba_size = QEMU_ALIGN_UP(n->params.msix_qsize, 64) / 8;
+
+    bar_size += msix_pba_size;
+    bar_size = pow2ceil(bar_size);
+
+    memory_region_init(&n->bar0, OBJECT(n), "nvme-bar0", bar_size);
     memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n, "nvme",
                           n->reg_size);
+    memory_region_add_subregion(&n->bar0, 0, &n->iomem);
+
     pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
-                     PCI_BASE_ADDRESS_MEM_TYPE_64, &n->iomem);
-    if (msix_init_exclusive_bar(pci_dev, n->params.msix_qsize, 4, errp)) {
+                     PCI_BASE_ADDRESS_MEM_TYPE_64, &n->bar0);
+
+    if (msix_init(pci_dev, n->params.msix_qsize,
+                  &n->bar0, 0, msix_table_offset,
+                  &n->bar0, 0, msix_pba_offset, 0, errp)) {
         return;
     }
 
-- 
2.29.2

[PATCH 3/3] hw/block/nvme: allow cmb and pmr to coexist

[Klaus Jensen]

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

With BAR 4 now free to use, allow PMR and CMB to be enabled
simultaneously.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index db8c5ae2f527..72d5449121c7 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -28,14 +28,13 @@
  * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
  * offset 0 in BAR2 and supports only WDS, RDS and SQS for now.
  *
- * cmb_size_mb= and pmrdev= options are mutually exclusive due to limitation
- * in available BAR's. cmb_size_mb= will take precedence over pmrdev= when
- * both provided.
  * Enabling pmr emulation can be achieved by pointing to memory-backend-file.
  * For example:
  * -object memory-backend-file,id=<mem_id>,share=on,mem-path=<file_path>, \
  *  size=<size> .... -device nvme,...,pmrdev=<mem_id>
  *
+ * The PMR will use BAR 4/5 exclusively.
+ *
  *
  * nvme device parameters
  * ~~~~~~~~~~~~~~~~~~~~~~
@@ -76,7 +75,7 @@
 #define NVME_DB_SIZE  4
 #define NVME_SPEC_VER 0x00010300
 #define NVME_CMB_BIR 2
-#define NVME_PMR_BIR 2
+#define NVME_PMR_BIR 4
 #define NVME_TEMPERATURE 0x143
 #define NVME_TEMPERATURE_WARNING 0x157
 #define NVME_TEMPERATURE_CRITICAL 0x175
@@ -2520,7 +2519,7 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
         return;
     }
 
-    if (!n->params.cmb_size_mb && n->pmrdev) {
+    if (n->pmrdev) {
         if (host_memory_backend_is_mapped(n->pmrdev)) {
             error_setg(errp, "can't use already busy memdev: %s",
                        object_get_canonical_path_component(OBJECT(n->pmrdev)));
@@ -2610,9 +2609,6 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
 
 static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
 {
-    /* Controller Capabilities register */
-    NVME_CAP_SET_PMRS(n->bar.cap, 1);
-
     /* PMR Capabities register */
     n->bar.pmrcap = 0;
     NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 0);
@@ -2705,7 +2701,9 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 
     if (n->params.cmb_size_mb) {
         nvme_init_cmb(n, pci_dev);
-    } else if (n->pmrdev) {
+    }
+
+    if (n->pmrdev) {
         nvme_init_pmr(n, pci_dev);
     }
 }
@@ -2775,6 +2773,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_ADMIN_ONLY);
     NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
     NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
+    NVME_CAP_SET_PMRS(n->bar.cap, n->pmrdev ? 1 : 0);
 
     n->bar.vs = NVME_SPEC_VER;
     n->bar.intmc = n->bar.intms = 0;
-- 
2.29.2

Re: [PATCH 0/3] hw/block/nvme: allow cmb and pmr to coexist

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

On Nov 23 07:59, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> This is a resurrection of Andrzej's series[1] from back July.
> 
> Andrzej's main patch basically moved the the CMB from BAR 2 into an
> offset in BAR 4 (located after the MSI-X table and PBA). Having an
> offset on the CMB causes a bunch of calculations related to address
> mapping to change.
> 
> So, since I couldn't get the patch to apply cleanly I took a stab at
> implementing the suggestion I originally came up with: simply move the
> MSI-X table and PBA from BAR 4 into BAR 0 (up-aligned to a 4 KiB
> boundary, after the main NVMe controller registers). This way we can
> keep the CMB at offset zero in its own BAR and free up BAR 4 for use by
> PMR. This makes the patch simpler and does not impact any of the
> existing address mapping code.
> 
> Andrzej, I would prefer an Ack from you, since I pretty much voided your
> main patch.
> 
>   [1]: https://lore.kernel.org/qemu-devel/20200729220107.37758-1-andrzej.jakowski@linux.intel.com/
> 
> CC: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> 
> Andrzej Jakowski (1):
>   hw/block/nvme: indicate CMB support through controller capabilities
>     register
> 
> Klaus Jensen (2):
>   hw/block/nvme: move msix table and pba to BAR 0
>   hw/block/nvme: allow cmb and pmr to coexist
> 
>  hw/block/nvme.h      |  1 +
>  include/block/nvme.h | 10 +++++++---
>  hw/block/nvme.c      | 42 +++++++++++++++++++++++++++++++-----------
>  3 files changed, 39 insertions(+), 14 deletions(-)
> 
> -- 
> 2.29.2
> 
> 

Gentle ping on this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]