[PATCH v2 00/12] hw/block/nvme: misc cmb/pmr patches and bump to v1.4

[PATCH v2 00/12] hw/block/nvme: misc cmb/pmr patches and bump to v1.4

[Klaus Jensen]

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

This is a resend of "hw/block/nvme: allow cmb and pmr to coexist" with
some more PMR work added (PMR RDS/WDS support).

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

Andrzej's main patch basically moved 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.

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

Changes for v2
~~~~~~~~~~~~~~

  - Rebased on nvme-next
  - Added a fix for 64 bit register hi/lo split writes
  - Added the patches from "hw/block/nvme: cmb enhancements and bump to
    v1.4" to the back of this.
  - As suggested by Keith, I removed "legacy CMB" support - the patch
    now exclusively bumps the support to the "v1.4 variant", so the
    linux kernel nvme gang have to get their game on ;)

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

Klaus Jensen (9):
  hw/block/nvme: add size to mmio read/write trace events
  hw/block/nvme: fix 64 bit register hi/lo split writes
  hw/block/nvme: move msix table and pba to BAR 0
  hw/block/nvme: allow cmb and pmr to coexist
  hw/block/nvme: rename PMR/CMB shift/mask fields
  hw/block/nvme: remove redundant zeroing of PMR registers
  hw/block/nvme: disable PMR at boot up
  hw/block/nvme: bump to v1.4
  hw/block/nvme: lift cmb restrictions

Naveen Nagar (1):
  hw/block/nvme: add PMR RDS/WDS support

Padmakar Kalghatgi (1):
  hw/block/nvme: move cmb logic to v1.4

 hw/block/nvme.h       |   3 +
 include/block/nvme.h  | 125 ++++++++++++++---
 hw/block/nvme.c       | 307 +++++++++++++++++++++++++++---------------
 hw/block/trace-events |   6 +-
 4 files changed, 314 insertions(+), 127 deletions(-)

-- 
2.30.0

[PATCH v2 01/12] hw/block/nvme: add size to mmio read/write trace events

[Klaus Jensen]

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

Add the size of the mmio read/write to the trace event.

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 47da73ce2364..bd7e258c3c26 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3840,7 +3840,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
     uint8_t *ptr = (uint8_t *)&n->bar;
     uint64_t val = 0;
 
-    trace_pci_nvme_mmio_read(addr);
+    trace_pci_nvme_mmio_read(addr, size);
 
     if (unlikely(addr & (sizeof(uint32_t) - 1))) {
         NVME_GUEST_ERR(pci_nvme_ub_mmiord_misaligned32,
@@ -4004,7 +4004,7 @@ static void nvme_mmio_write(void *opaque, hwaddr addr, uint64_t data,
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
 
-    trace_pci_nvme_mmio_write(addr, data);
+    trace_pci_nvme_mmio_write(addr, data, size);
 
     if (addr < sizeof(n->bar)) {
         nvme_write_bar(n, addr, data, size);
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 78d76b0a71c1..a104d7f4da80 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -80,8 +80,8 @@ pci_nvme_enqueue_event_noqueue(int queued) "queued %d"
 pci_nvme_enqueue_event_masked(uint8_t typ) "type 0x%"PRIx8""
 pci_nvme_no_outstanding_aers(void) "ignoring event; no outstanding AERs"
 pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
-pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
-pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64""
+pci_nvme_mmio_read(uint64_t addr, unsigned size) "addr 0x%"PRIx64" size %d"
+pci_nvme_mmio_write(uint64_t addr, uint64_t data, unsigned size) "addr 0x%"PRIx64" data 0x%"PRIx64" size %d"
 pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16" new_head %"PRIu16""
 pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "sqid %"PRIu16" new_tail %"PRIu16""
 pci_nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
-- 
2.30.0

[PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Klaus Jensen]

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

64 bit registers like ASQ and ACQ should be writable by both a hi/lo 32
bit write combination as well as a plain 64 bit write. The spec does not
define ordering on the hi/lo split, but the code currently assumes that
the low order bits are written first. Additionally, the code does not
consider that another address might already have been written into the
register, causing the OR'ing to result in a bad address.

Fix this by explicitly overwriting only the low or high order bits for
32 bit writes.

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index bd7e258c3c26..40b9f8ccfc0e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3781,19 +3781,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
         break;
     case 0x28:  /* ASQ */
-        n->bar.asq = data;
+        n->bar.asq = size == 8 ? data :
+            (n->bar.asq & ~0xffffffff) | (data & 0xffffffff);
         trace_pci_nvme_mmio_asqaddr(data);
         break;
     case 0x2c:  /* ASQ hi */
-        n->bar.asq |= data << 32;
+        n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
         trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
         break;
     case 0x30:  /* ACQ */
         trace_pci_nvme_mmio_acqaddr(data);
-        n->bar.acq = data;
+        n->bar.acq = size == 8 ? data :
+            (n->bar.acq & ~0xffffffff) | (data & 0xffffffff);
         break;
     case 0x34:  /* ACQ hi */
-        n->bar.acq |= data << 32;
+        n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
         trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
         break;
     case 0x38:  /* CMBLOC */
-- 
2.30.0

[PATCH v2 03/12] 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 45b2678db1f0..86d7fc2f905c 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 40b9f8ccfc0e..606006c549bc 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4336,6 +4336,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_CSI_SUPP);
     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.30.0

[PATCH v2 04/12] 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 array 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 | 23 +++++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 65540b650e1d..2a25bc84f3f9 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -126,6 +126,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 606006c549bc..ec2104fcf3b6 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4230,6 +4230,8 @@ static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
 static int 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;
     int ret;
 
     Error *err = NULL;
@@ -4248,11 +4250,28 @@ static int 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);
-    ret = msix_init_exclusive_bar(pci_dev, n->params.msix_qsize, 4, &err);
+                     PCI_BASE_ADDRESS_MEM_TYPE_64, &n->bar0);
+    ret = msix_init(pci_dev, n->params.msix_qsize,
+                    &n->bar0, 0, msix_table_offset,
+                    &n->bar0, 0, msix_pba_offset, 0, &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             warn_report_err(err);
-- 
2.30.0

[PATCH v2 05/12] 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 ec2104fcf3b6..f3bea582b3c0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -29,14 +29,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
  * ~~~~~~~~~~~~~~~~~~~~~~
@@ -109,7 +108,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
@@ -4083,7 +4082,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)));
@@ -4180,9 +4179,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);
@@ -4283,7 +4279,9 @@ static int 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);
     }
 
@@ -4356,6 +4354,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.30.0

[PATCH v2 06/12] hw/block/nvme: rename PMR/CMB shift/mask fields

[Klaus Jensen]

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

Use the correct field names.

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

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 86d7fc2f905c..f3cbe17d0971 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -35,8 +35,8 @@ enum NvmeCapShift {
     CAP_CSS_SHIFT      = 37,
     CAP_MPSMIN_SHIFT   = 48,
     CAP_MPSMAX_SHIFT   = 52,
-    CAP_PMR_SHIFT      = 56,
-    CAP_CMB_SHIFT      = 57,
+    CAP_PMRS_SHIFT     = 56,
+    CAP_CMBS_SHIFT     = 57,
 };
 
 enum NvmeCapMask {
@@ -49,8 +49,8 @@ enum NvmeCapMask {
     CAP_CSS_MASK       = 0xff,
     CAP_MPSMIN_MASK    = 0xf,
     CAP_MPSMAX_MASK    = 0xf,
-    CAP_PMR_MASK       = 0x1,
-    CAP_CMB_MASK       = 0x1,
+    CAP_PMRS_MASK      = 0x1,
+    CAP_CMBS_MASK      = 0x1,
 };
 
 #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
@@ -81,10 +81,10 @@ enum NvmeCapMask {
                                                            << 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)
-#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMB_MASK)   \
-                                                           << CAP_CMB_SHIFT)
+#define NVME_CAP_SET_PMRS(cap, val)   (cap |= (uint64_t)(val & CAP_PMRS_MASK)  \
+                                                           << CAP_PMRS_SHIFT)
+#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMBS_MASK)  \
+                                                           << CAP_CMBS_SHIFT)
 
 enum NvmeCapCss {
     NVME_CAP_CSS_NVM        = 1 << 0,
-- 
2.30.0

[PATCH v2 07/12] hw/block/nvme: remove redundant zeroing of PMR registers

[Klaus Jensen]

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

The controller registers are initially zero. Remove the redundant
zeroing.

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f3bea582b3c0..9ee9570bb65c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4179,43 +4179,8 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
 
 static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
 {
-    /* PMR Capabities register */
-    n->bar.pmrcap = 0;
-    NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 0);
-    NVME_PMRCAP_SET_WDS(n->bar.pmrcap, 0);
     NVME_PMRCAP_SET_BIR(n->bar.pmrcap, NVME_PMR_BIR);
-    NVME_PMRCAP_SET_PMRTU(n->bar.pmrcap, 0);
-    /* Turn on bit 1 support */
     NVME_PMRCAP_SET_PMRWBM(n->bar.pmrcap, 0x02);
-    NVME_PMRCAP_SET_PMRTO(n->bar.pmrcap, 0);
-    NVME_PMRCAP_SET_CMSS(n->bar.pmrcap, 0);
-
-    /* PMR Control register */
-    n->bar.pmrctl = 0;
-    NVME_PMRCTL_SET_EN(n->bar.pmrctl, 0);
-
-    /* PMR Status register */
-    n->bar.pmrsts = 0;
-    NVME_PMRSTS_SET_ERR(n->bar.pmrsts, 0);
-    NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 0);
-    NVME_PMRSTS_SET_HSTS(n->bar.pmrsts, 0);
-    NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 0);
-
-    /* PMR Elasticity Buffer Size register */
-    n->bar.pmrebs = 0;
-    NVME_PMREBS_SET_PMRSZU(n->bar.pmrebs, 0);
-    NVME_PMREBS_SET_RBB(n->bar.pmrebs, 0);
-    NVME_PMREBS_SET_PMRWBZ(n->bar.pmrebs, 0);
-
-    /* PMR Sustained Write Throughput register */
-    n->bar.pmrswtp = 0;
-    NVME_PMRSWTP_SET_PMRSWTU(n->bar.pmrswtp, 0);
-    NVME_PMRSWTP_SET_PMRSWTV(n->bar.pmrswtp, 0);
-
-    /* PMR Memory Space Control register */
-    n->bar.pmrmsc = 0;
-    NVME_PMRMSC_SET_CMSE(n->bar.pmrmsc, 0);
-    NVME_PMRMSC_SET_CBA(n->bar.pmrmsc, 0);
 
     pci_register_bar(pci_dev, NVME_PMRCAP_BIR(n->bar.pmrcap),
                      PCI_BASE_ADDRESS_SPACE_MEMORY |
-- 
2.30.0

[PATCH v2 08/12] hw/block/nvme: disable PMR at boot up

[Klaus Jensen]

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

The PMR should not be enabled at boot up. Disable the PMR MemoryRegion
initially and implement MMIO for PMRCTL, allowing the host to enable the
PMR explicitly.

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9ee9570bb65c..937a6ed0a70d 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3810,8 +3810,16 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
                        "invalid write to PMRCAP register, ignored");
         return;
-    case 0xE04: /* TODO PMRCTL */
-        break;
+    case 0xE04: /* PMRCTL */
+        n->bar.pmrctl = data;
+        if (NVME_PMRCTL_EN(data)) {
+            memory_region_set_enabled(&n->pmrdev->mr, true);
+            n->bar.pmrsts = 0;
+        } else {
+            memory_region_set_enabled(&n->pmrdev->mr, false);
+            NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 1);
+        }
+        return;
     case 0xE08: /* PMRSTS */
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
                        "invalid write to PMRSTS register, ignored");
@@ -4186,6 +4194,8 @@ static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
                      PCI_BASE_ADDRESS_SPACE_MEMORY |
                      PCI_BASE_ADDRESS_MEM_TYPE_64 |
                      PCI_BASE_ADDRESS_MEM_PREFETCH, &n->pmrdev->mr);
+
+    memory_region_set_enabled(&n->pmrdev->mr, false);
 }
 
 static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
-- 
2.30.0

[PATCH v2 09/12] hw/block/nvme: add PMR RDS/WDS support

[Klaus Jensen]

From: Naveen Nagar <naveen.n1@samsung.com>

Add support for the PMRMSCL and PMRMSCU MMIO registers. This allows
adding RDS/WDS support for PMR as well.

Signed-off-by: Naveen Nagar <naveen.n1@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.h      |  1 +
 include/block/nvme.h |  1 +
 hw/block/nvme.c      | 89 ++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 2a25bc84f3f9..5988d9b36e12 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -152,6 +152,7 @@ typedef struct NvmeCtrl {
     uint16_t    temperature;
 
     HostMemoryBackend *pmrdev;
+    bool              pmr_cmse;
 
     uint8_t     aer_mask;
     NvmeRequest **aer_reqs;
diff --git a/include/block/nvme.h b/include/block/nvme.h
index f3cbe17d0971..183dc5c0ecf6 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -62,6 +62,7 @@ enum NvmeCapMask {
 #define NVME_CAP_CSS(cap)   (((cap) >> CAP_CSS_SHIFT)    & CAP_CSS_MASK)
 #define NVME_CAP_MPSMIN(cap)(((cap) >> CAP_MPSMIN_SHIFT) & CAP_MPSMIN_MASK)
 #define NVME_CAP_MPSMAX(cap)(((cap) >> CAP_MPSMAX_SHIFT) & CAP_MPSMAX_MASK)
+#define NVME_CAP_PMRS(cap)  (((cap) >> CAP_PMRS_SHIFT)   & CAP_PMRS_MASK)
 
 #define NVME_CAP_SET_MQES(cap, val)   (cap |= (uint64_t)(val & CAP_MQES_MASK)  \
                                                            << CAP_MQES_SHIFT)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 937a6ed0a70d..cbc2b32f7c87 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -273,6 +273,26 @@ static inline void *nvme_addr_to_cmb(NvmeCtrl *n, hwaddr addr)
     return &n->cmbuf[addr - n->ctrl_mem.addr];
 }
 
+static bool nvme_addr_is_pmr(NvmeCtrl *n, hwaddr addr)
+{
+    hwaddr hi, low;
+
+    if (!n->pmr_cmse) {
+        return false;
+    }
+
+    low = NVME_PMRMSC_CBA(n->bar.pmrmsc) << PMRMSC_CBA_SHIFT;
+    hi  = low + int128_get64(n->pmrdev->mr.size);
+
+    return addr >= low && addr < hi;
+}
+
+static inline void *nvme_addr_to_pmr(NvmeCtrl *n, hwaddr addr)
+{
+    hwaddr cba = NVME_PMRMSC_CBA(n->bar.pmrmsc) << PMRMSC_CBA_SHIFT;
+    return memory_region_get_ram_ptr(&n->pmrdev->mr) + (addr - cba);
+}
+
 static int nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
 {
     hwaddr hi = addr + size - 1;
@@ -285,6 +305,11 @@ static int nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
         return 0;
     }
 
+    if (nvme_addr_is_pmr(n, addr) && nvme_addr_is_pmr(n, hi)) {
+        memcpy(buf, nvme_addr_to_pmr(n, addr), size);
+        return 0;
+    }
+
     return pci_dma_read(&n->parent_obj, addr, buf, size);
 }
 
@@ -406,9 +431,27 @@ static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr addr,
     return NVME_SUCCESS;
 }
 
+static uint16_t nvme_map_addr_pmr(NvmeCtrl *n, QEMUIOVector *iov, hwaddr addr,
+    size_t len)
+{
+    if (!len) {
+        return NVME_SUCCESS;
+    }
+
+    if (!nvme_addr_is_pmr(n, addr) || !nvme_addr_is_pmr(n, addr + len - 1)) {
+        return NVME_DATA_TRAS_ERROR;
+    }
+
+    qemu_iovec_add(iov, nvme_addr_to_pmr(n, addr), len);
+
+    return NVME_SUCCESS;
+}
+
 static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector *iov,
                               hwaddr addr, size_t len)
 {
+    bool cmb = false, pmr = false;
+
     if (!len) {
         return NVME_SUCCESS;
     }
@@ -416,6 +459,12 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector *iov,
     trace_pci_nvme_map_addr(addr, len);
 
     if (nvme_addr_is_cmb(n, addr)) {
+        cmb = true;
+    } else if (nvme_addr_is_pmr(n, addr)) {
+        pmr = true;
+    }
+
+    if (cmb || pmr) {
         if (qsg && qsg->sg) {
             return NVME_INVALID_USE_OF_CMB | NVME_DNR;
         }
@@ -426,7 +475,11 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector *iov,
             qemu_iovec_init(iov, 1);
         }
 
-        return nvme_map_addr_cmb(n, iov, addr, len);
+        if (cmb) {
+            return nvme_map_addr_cmb(n, iov, addr, len);
+        } else {
+            return nvme_map_addr_pmr(n, iov, addr, len);
+        }
     }
 
     if (iov && iov->iov) {
@@ -459,7 +512,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
 
     trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
 
-    if (nvme_addr_is_cmb(n, prp1)) {
+    if (nvme_addr_is_cmb(n, prp1) || (nvme_addr_is_pmr(n, prp1))) {
         qemu_iovec_init(iov, num_prps);
     } else {
         pci_dma_sglist_init(qsg, &n->parent_obj, num_prps);
@@ -3818,6 +3871,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         } else {
             memory_region_set_enabled(&n->pmrdev->mr, false);
             NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 1);
+            n->pmr_cmse = false;
         }
         return;
     case 0xE08: /* PMRSTS */
@@ -3832,8 +3886,32 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
                        "invalid write to PMRSWTP register, ignored");
         return;
-    case 0xE14: /* TODO PMRMSC */
-        break;
+    case 0xE14: /* PMRMSCL */
+        if (!NVME_CAP_PMRS(n->bar.cap)) {
+            return;
+        }
+
+        n->bar.pmrmsc = (n->bar.pmrmsc & ~0xffffffff) | (data & 0xffffffff);
+        n->pmr_cmse = false;
+
+        if (NVME_PMRMSC_CMSE(n->bar.pmrmsc)) {
+            hwaddr cba = NVME_PMRMSC_CBA(n->bar.pmrmsc) << PMRMSC_CBA_SHIFT;
+            if (cba + int128_get64(n->pmrdev->mr.size) < cba) {
+                NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
+                return;
+            }
+
+            n->pmr_cmse = true;
+        }
+
+        return;
+    case 0xE18: /* PMRMSCU */
+        if (!NVME_CAP_PMRS(n->bar.cap)) {
+            return;
+        }
+
+        n->bar.pmrmsc = (n->bar.pmrmsc & 0xffffffff) | (data << 32);
+        return;
     default:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
                        "invalid MMIO write,"
@@ -4187,8 +4265,11 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
 
 static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
 {
+    NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 1);
+    NVME_PMRCAP_SET_WDS(n->bar.pmrcap, 1);
     NVME_PMRCAP_SET_BIR(n->bar.pmrcap, NVME_PMR_BIR);
     NVME_PMRCAP_SET_PMRWBM(n->bar.pmrcap, 0x02);
+    NVME_PMRCAP_SET_CMSS(n->bar.pmrcap, 1);
 
     pci_register_bar(pci_dev, NVME_PMRCAP_BIR(n->bar.pmrcap),
                      PCI_BASE_ADDRESS_SPACE_MEMORY |
-- 
2.30.0

[PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Klaus Jensen]

From: Padmakar Kalghatgi <p.kalghatgi@samsung.com>

Implement v1.4 logic for configuring the Controller Memory Buffer. This
is not backward compatible with v1.3, so drivers that only support v1.3
will not be able to use the CMB anymore.

Signed-off-by: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.h       |   1 +
 include/block/nvme.h  | 107 +++++++++++++++++++++++++++++++++++++-----
 hw/block/nvme.c       |  78 +++++++++++++++++++++++-------
 hw/block/trace-events |   2 +
 4 files changed, 159 insertions(+), 29 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 5988d9b36e12..de9164dd52e6 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -145,6 +145,7 @@ typedef struct NvmeCtrl {
     uint32_t    max_q_ents;
     uint8_t     outstanding_aers;
     uint8_t     *cmbuf;
+    bool        cmb_cmse;
     uint32_t    irq_status;
     uint64_t    host_timestamp;                 /* Timestamp sent by the host */
     uint64_t    timestamp_set_qemu_clock_ms;    /* QEMU clock time */
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 183dc5c0ecf6..7dcd8f9b4e78 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -15,14 +15,19 @@ typedef struct QEMU_PACKED NvmeBar {
     uint64_t    acq;
     uint32_t    cmbloc;
     uint32_t    cmbsz;
-    uint8_t     padding[3520]; /* not used by QEMU */
+    uint32_t    bpinfo;
+    uint32_t    bprsel;
+    uint64_t    bpmbl;
+    uint64_t    cmbmsc;
+    uint32_t    cmbsts;
+    uint8_t     rsvd92[3492];
     uint32_t    pmrcap;
     uint32_t    pmrctl;
     uint32_t    pmrsts;
     uint32_t    pmrebs;
     uint32_t    pmrswtp;
     uint64_t    pmrmsc;
-    uint8_t     reserved[484];
+    uint8_t     css[484];
 } NvmeBar;
 
 enum NvmeCapShift {
@@ -63,6 +68,7 @@ enum NvmeCapMask {
 #define NVME_CAP_MPSMIN(cap)(((cap) >> CAP_MPSMIN_SHIFT) & CAP_MPSMIN_MASK)
 #define NVME_CAP_MPSMAX(cap)(((cap) >> CAP_MPSMAX_SHIFT) & CAP_MPSMAX_MASK)
 #define NVME_CAP_PMRS(cap)  (((cap) >> CAP_PMRS_SHIFT)   & CAP_PMRS_MASK)
+#define NVME_CAP_CMBS(cap)  (((cap) >> CAP_CMBS_SHIFT)   & CAP_CMBS_MASK)
 
 #define NVME_CAP_SET_MQES(cap, val)   (cap |= (uint64_t)(val & CAP_MQES_MASK)  \
                                                            << CAP_MQES_SHIFT)
@@ -184,25 +190,64 @@ enum NvmeAqaMask {
 #define NVME_AQA_ACQS(aqa) ((aqa >> AQA_ACQS_SHIFT) & AQA_ACQS_MASK)
 
 enum NvmeCmblocShift {
-    CMBLOC_BIR_SHIFT  = 0,
-    CMBLOC_OFST_SHIFT = 12,
+    CMBLOC_BIR_SHIFT     = 0,
+    CMBLOC_CQMMS_SHIFT   = 3,
+    CMBLOC_CQPDS_SHIFT   = 4,
+    CMBLOC_CDPMLS_SHIFT  = 5,
+    CMBLOC_CDPCILS_SHIFT = 6,
+    CMBLOC_CDMMMS_SHIFT  = 7,
+    CMBLOC_CQDA_SHIFT    = 8,
+    CMBLOC_OFST_SHIFT    = 12,
 };
 
 enum NvmeCmblocMask {
-    CMBLOC_BIR_MASK  = 0x7,
-    CMBLOC_OFST_MASK = 0xfffff,
+    CMBLOC_BIR_MASK     = 0x7,
+    CMBLOC_CQMMS_MASK   = 0x1,
+    CMBLOC_CQPDS_MASK   = 0x1,
+    CMBLOC_CDPMLS_MASK  = 0x1,
+    CMBLOC_CDPCILS_MASK = 0x1,
+    CMBLOC_CDMMMS_MASK  = 0x1,
+    CMBLOC_CQDA_MASK    = 0x1,
+    CMBLOC_OFST_MASK    = 0xfffff,
 };
 
-#define NVME_CMBLOC_BIR(cmbloc) ((cmbloc >> CMBLOC_BIR_SHIFT)  & \
-                                 CMBLOC_BIR_MASK)
-#define NVME_CMBLOC_OFST(cmbloc)((cmbloc >> CMBLOC_OFST_SHIFT) & \
-                                 CMBLOC_OFST_MASK)
+#define NVME_CMBLOC_BIR(cmbloc) \
+    ((cmbloc >> CMBLOC_BIR_SHIFT) & CMBLOC_BIR_MASK)
+#define NVME_CMBLOC_CQMMS(cmbloc) \
+    ((cmbloc >> CMBLOC_CQMMS_SHIFT) & CMBLOC_CQMMS_MASK)
+#define NVME_CMBLOC_CQPDS(cmbloc) \
+    ((cmbloc >> CMBLOC_CQPDS_SHIFT) & CMBLOC_CQPDS_MASK)
+#define NVME_CMBLOC_CDPMLS(cmbloc) \
+    ((cmbloc >> CMBLOC_CDPMLS_SHIFT) & CMBLOC_CDPMLS_MASK)
+#define NVME_CMBLOC_CDPCILS(cmbloc) \
+    ((cmbloc >> CMBLOC_CDPCILS_SHIFT) & CMBLOC_CDPCILS_MASK)
+#define NVME_CMBLOC_CDMMMS(cmbloc) \
+    ((cmbloc >> CMBLOC_CDMMMS_SHIFT) & CMBLOC_CDMMMS_MASK)
+#define NVME_CMBLOC_CQDA(cmbloc) \
+    ((cmbloc >> CMBLOC_CQDA_SHIFT) & CMBLOC_CQDA_MASK)
+#define NVME_CMBLOC_OFST(cmbloc) \
+    ((cmbloc >> CMBLOC_OFST_SHIFT) & CMBLOC_OFST_MASK)
 
-#define NVME_CMBLOC_SET_BIR(cmbloc, val)  \
+#define NVME_CMBLOC_SET_BIR(cmbloc, val) \
     (cmbloc |= (uint64_t)(val & CMBLOC_BIR_MASK) << CMBLOC_BIR_SHIFT)
+#define NVME_CMBLOC_SET_CQMMS(cmbloc, val) \
+    (cmbloc |= (uint64_t)(val & CMBLOC_CQMMS_MASK) << CMBLOC_CQMMS_SHIFT)
+#define NVME_CMBLOC_SET_CQPDS(cmbloc, val) \
+    (cmbloc |= (uint64_t)(val & CMBLOC_CQPDS_MASK) << CMBLOC_CQPDS_SHIFT)
+#define NVME_CMBLOC_SET_CDPMLS(cmbloc, val) \
+    (cmbloc |= (uint64_t)(val & CMBLOC_CDPMLS_MASK) << CMBLOC_CDPMLS_SHIFT)
+#define NVME_CMBLOC_SET_CDPCILS(cmbloc, val) \
+    (cmbloc |= (uint64_t)(val & CMBLOC_CDPCILS_MASK) << CMBLOC_CDPCILS_SHIFT)
+#define NVME_CMBLOC_SET_CDMMMS(cmbloc, val) \
+    (cmbloc |= (uint64_t)(val & CMBLOC_CDMMMS_MASK) << CMBLOC_CDMMMS_SHIFT)
+#define NVME_CMBLOC_SET_CQDA(cmbloc, val) \
+    (cmbloc |= (uint64_t)(val & CMBLOC_CQDA_MASK) << CMBLOC_CQDA_SHIFT)
 #define NVME_CMBLOC_SET_OFST(cmbloc, val) \
     (cmbloc |= (uint64_t)(val & CMBLOC_OFST_MASK) << CMBLOC_OFST_SHIFT)
 
+#define NVME_CMBMSMC_SET_CRE (cmbmsc, val) \
+    (cmbmsc |= (uint64_t)(val & CMBLOC_OFST_MASK) << CMBMSC_CRE_SHIFT)
+
 enum NvmeCmbszShift {
     CMBSZ_SQS_SHIFT   = 0,
     CMBSZ_CQS_SHIFT   = 1,
@@ -249,6 +294,46 @@ enum NvmeCmbszMask {
 #define NVME_CMBSZ_GETSIZE(cmbsz) \
     (NVME_CMBSZ_SZ(cmbsz) * (1 << (12 + 4 * NVME_CMBSZ_SZU(cmbsz))))
 
+enum NvmeCmbmscShift {
+    CMBMSC_CRE_SHIFT  = 0,
+    CMBMSC_CMSE_SHIFT = 1,
+    CMBMSC_CBA_SHIFT  = 12,
+};
+
+enum NvmeCmbmscMask {
+    CMBMSC_CRE_MASK  = 0x1,
+    CMBMSC_CMSE_MASK = 0x1,
+    CMBMSC_CBA_MASK   = ((1L << 52) - 1),
+};
+
+#define NVME_CMBMSC_CRE(cmbmsc) \
+    ((cmbmsc >> CMBMSC_CRE_SHIFT)  & CMBMSC_CRE_MASK)
+#define NVME_CMBMSC_CMSE(cmbmsc) \
+    ((cmbmsc >> CMBMSC_CMSE_SHIFT) & CMBMSC_CMSE_MASK)
+#define NVME_CMBMSC_CBA(cmbmsc) \
+    ((cmbmsc >> CMBMSC_CBA_SHIFT) & CMBMSC_CBA_MASK)
+
+
+#define NVME_CMBMSC_SET_CRE(cmbmsc, val)  \
+    (cmbmsc |= (uint64_t)(val & CMBMSC_CRE_MASK) << CMBMSC_CRE_SHIFT)
+#define NVME_CMBMSC_SET_CMSE(cmbmsc, val) \
+    (cmbmsc |= (uint64_t)(val & CMBMSC_CMSE_MASK) << CMBMSC_CMSE_SHIFT)
+#define NVME_CMBMSC_SET_CBA(cmbmsc, val) \
+    (cmbmsc |= (uint64_t)(val & CMBMSC_CBA_MASK) << CMBMSC_CBA_SHIFT)
+
+enum NvmeCmbstsShift {
+    CMBSTS_CBAI_SHIFT = 0,
+};
+enum NvmeCmbstsMask {
+    CMBSTS_CBAI_MASK = 0x1,
+};
+
+#define NVME_CMBSTS_CBAI(cmbsts) \
+    ((cmbsts >> CMBSTS_CBAI_SHIFT) & CMBSTS_CBAI_MASK)
+
+#define NVME_CMBSTS_SET_CBAI(cmbsts, val)  \
+    (cmbsts |= (uint64_t)(val & CMBSTS_CBAI_MASK) << CMBSTS_CBAI_SHIFT)
+
 enum NvmePmrcapShift {
     PMRCAP_RDS_SHIFT      = 3,
     PMRCAP_WDS_SHIFT      = 4,
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cbc2b32f7c87..a9b11a193c59 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -260,17 +260,22 @@ static int nvme_aor_check(NvmeNamespace *ns, uint32_t act, uint32_t opn)
 
 static bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
 {
-    hwaddr low = n->ctrl_mem.addr;
-    hwaddr hi  = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
+    hwaddr hi, low;
+
+    if (!n->cmb_cmse) {
+        return false;
+    }
+
+    low = NVME_CMBMSC_CBA(n->bar.cmbmsc) << CMBMSC_CBA_SHIFT;
+    hi = low + int128_get64(n->ctrl_mem.size);
 
     return addr >= low && addr < hi;
 }
 
 static inline void *nvme_addr_to_cmb(NvmeCtrl *n, hwaddr addr)
 {
-    assert(nvme_addr_is_cmb(n, addr));
-
-    return &n->cmbuf[addr - n->ctrl_mem.addr];
+    hwaddr cba = NVME_CMBMSC_CBA(n->bar.cmbmsc) << CMBMSC_CBA_SHIFT;
+    return &n->cmbuf[addr - cba];
 }
 
 static bool nvme_addr_is_pmr(NvmeCtrl *n, hwaddr addr)
@@ -3732,6 +3737,19 @@ static int nvme_start_ctrl(NvmeCtrl *n)
     return 0;
 }
 
+static void nvme_cmb_enable_regs(NvmeCtrl *n)
+{
+    NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
+
+    NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
+    NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
+    NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 1);
+    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);
+}
+
 static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                            unsigned size)
 {
@@ -3859,6 +3877,37 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbsz_readonly,
                        "invalid write to read only CMBSZ, ignored");
         return;
+    case 0x50:  /* CMBMSC */
+        if (!NVME_CAP_CMBS(n->bar.cap)) {
+            return;
+        }
+
+        n->bar.cmbmsc = size == 8 ? data :
+            (n->bar.cmbmsc & ~0xffffffff) | (data & 0xffffffff);
+        n->cmb_cmse = false;
+
+        if (NVME_CMBMSC_CRE(data)) {
+            nvme_cmb_enable_regs(n);
+
+            if (NVME_CMBMSC_CMSE(data)) {
+                hwaddr cba = NVME_CMBMSC_CBA(data) << CMBMSC_CBA_SHIFT;
+                if (cba + int128_get64(n->ctrl_mem.size) < cba) {
+                    NVME_CMBSTS_SET_CBAI(n->bar.cmbsts, 1);
+                    return;
+                }
+
+                n->cmb_cmse = true;
+            }
+        } else {
+            n->bar.cmbsz = 0;
+            n->bar.cmbloc = 0;
+        }
+
+        return;
+    case 0x54:  /* CMBMSC hi */
+        n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
+        return;
+
     case 0xE00: /* PMRCAP */
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
                        "invalid write to PMRCAP register, ignored");
@@ -4243,24 +4292,17 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
 
 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);
+    uint64_t cmb_size = n->params.cmb_size_mb * MiB;
 
-    NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
-    NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
-    NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 1);
-    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);
-
-    n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
+    n->cmbuf = g_malloc0(cmb_size);
     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),
+                          "nvme-cmb", cmb_size);
+    pci_register_bar(pci_dev, NVME_CMB_BIR,
                      PCI_BASE_ADDRESS_SPACE_MEMORY |
                      PCI_BASE_ADDRESS_MEM_TYPE_64 |
                      PCI_BASE_ADDRESS_MEM_PREFETCH, &n->ctrl_mem);
+
+    NVME_CAP_SET_CMBS(n->bar.cap, 1);
 }
 
 static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
diff --git a/hw/block/trace-events b/hw/block/trace-events
index a104d7f4da80..8e249ea910aa 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -123,6 +123,8 @@ 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_log_page_offset(uint64_t ofs, uint64_t size) "must be <= %"PRIu64", got %"PRIu64""
+pci_nvme_err_cmb_invalid_cba(uint64_t cmbmsc) "cmbmsc 0x%"PRIx64""
+pci_nvme_err_cmb_not_enabled(uint64_t cmbmsc) "cmbmsc 0x%"PRIx64""
 pci_nvme_err_unaligned_zone_cmd(uint8_t action, uint64_t slba, uint64_t zslba) "unaligned zone op 0x%"PRIx32", got slba=%"PRIu64", zslba=%"PRIu64""
 pci_nvme_err_invalid_zone_state_transition(uint8_t action, uint64_t slba, uint8_t attrs) "action=0x%"PRIx8", slba=%"PRIu64", attrs=0x%"PRIx32""
 pci_nvme_err_write_not_at_wp(uint64_t slba, uint64_t zone, uint64_t wp) "writing at slba=%"PRIu64", zone=%"PRIu64", but wp=%"PRIu64""
-- 
2.30.0

[PATCH v2 11/12] hw/block/nvme: bump to v1.4

[Klaus Jensen]

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

With the new CMB logic in place, bump the implemented specification
version to v1.4.

This requires adding the setting the CNTRLTYPE field and modifying the
VWC field since 0x00 is no longer a valid value for bits 2:1.

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

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 7dcd8f9b4e78..c34343c13a3c 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -949,7 +949,8 @@ typedef struct QEMU_PACKED NvmeIdCtrl {
     uint32_t    rtd3e;
     uint32_t    oaes;
     uint32_t    ctratt;
-    uint8_t     rsvd100[12];
+    uint8_t     rsvd100[11];
+    uint8_t     cntrltype;
     uint8_t     fguid[16];
     uint8_t     rsvd128[128];
     uint16_t    oacs;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index a9b11a193c59..992665376a71 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -106,7 +106,7 @@
 
 #define NVME_MAX_IOQPAIRS 0xffff
 #define NVME_DB_SIZE  4
-#define NVME_SPEC_VER 0x00010300
+#define NVME_SPEC_VER 0x00010400
 #define NVME_CMB_BIR 2
 #define NVME_PMR_BIR 4
 #define NVME_TEMPERATURE 0x143
@@ -4404,6 +4404,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     id->mdts = n->params.mdts;
     id->ver = cpu_to_le32(NVME_SPEC_VER);
     id->oacs = cpu_to_le16(0);
+    id->cntrltype = 0x1;
 
     /*
      * Because the controller always completes the Abort command immediately,
@@ -4432,7 +4433,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
                            NVME_ONCS_FEATURES | NVME_ONCS_DSM |
                            NVME_ONCS_COMPARE);
 
-    id->vwc = 0x1;
+    id->vwc = (0x2 << 1) | 0x1;
     id->sgls = cpu_to_le32(NVME_CTRL_SGLS_SUPPORT_NO_ALIGN |
                            NVME_CTRL_SGLS_BITBUCKET);
 
-- 
2.30.0

[PATCH v2 12/12] hw/block/nvme: lift cmb restrictions

[Klaus Jensen]

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

The controller now implements v1.4 and we can lift the restrictions on
CMB Data Pointer and Command Independent Locations Support (CDPCILS) and
CMB Data Pointer Mixed Locations Support (CDPMLS) since the device
really does not care about mixed host/cmb pointers in those cases.

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 992665376a71..23a836a343e2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -509,7 +509,6 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
     trans_len = MIN(len, trans_len);
     int num_prps = (len >> n->page_bits) + 1;
     uint16_t status;
-    bool prp_list_in_cmb = false;
     int ret;
 
     QEMUSGList *qsg = &req->qsg;
@@ -535,10 +534,6 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
             uint32_t nents, prp_trans;
             int i = 0;
 
-            if (nvme_addr_is_cmb(n, prp2)) {
-                prp_list_in_cmb = true;
-            }
-
             nents = (len + n->page_size - 1) >> n->page_bits;
             prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
             ret = nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
@@ -555,10 +550,6 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
                         return NVME_INVALID_PRP_OFFSET | NVME_DNR;
                     }
 
-                    if (prp_list_in_cmb != nvme_addr_is_cmb(n, prp_ent)) {
-                        return NVME_INVALID_USE_OF_CMB | NVME_DNR;
-                    }
-
                     i = 0;
                     nents = (len + n->page_size - 1) >> n->page_bits;
                     prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
@@ -692,7 +683,6 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector *iov,
     uint64_t nsgld;
     uint32_t seg_len;
     uint16_t status;
-    bool sgl_in_cmb = false;
     hwaddr addr;
     int ret;
 
@@ -714,18 +704,6 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector *iov,
         goto out;
     }
 
-    /*
-     * If the segment is located in the CMB, the submission queue of the
-     * request must also reside there.
-     */
-    if (nvme_addr_is_cmb(n, addr)) {
-        if (!nvme_addr_is_cmb(n, req->sq->dma_addr)) {
-            return NVME_INVALID_USE_OF_CMB | NVME_DNR;
-        }
-
-        sgl_in_cmb = true;
-    }
-
     for (;;) {
         switch (NVME_SGL_TYPE(sgld->type)) {
         case NVME_SGL_DESCR_TYPE_SEGMENT:
@@ -814,15 +792,6 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector *iov,
         if (status) {
             goto unmap;
         }
-
-        /*
-         * If the next segment is in the CMB, make sure that the sgl was
-         * already located there.
-         */
-        if (sgl_in_cmb != nvme_addr_is_cmb(n, addr)) {
-            status = NVME_INVALID_USE_OF_CMB | NVME_DNR;
-            goto unmap;
-        }
     }
 
 out:
@@ -3739,6 +3708,8 @@ static int nvme_start_ctrl(NvmeCtrl *n)
 
 static void nvme_cmb_enable_regs(NvmeCtrl *n)
 {
+    NVME_CMBLOC_SET_CDPCILS(n->bar.cmbloc, 1);
+    NVME_CMBLOC_SET_CDPMLS(n->bar.cmbloc, 1);
     NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
 
     NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
-- 
2.30.0

Re: [PATCH v2 01/12] hw/block/nvme: add size to mmio read/write trace events

[Minwoo Im]

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Minwoo Im]

On 21-01-18 10:46:55, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> 64 bit registers like ASQ and ACQ should be writable by both a hi/lo 32
> bit write combination as well as a plain 64 bit write. The spec does not
> define ordering on the hi/lo split, but the code currently assumes that
> the low order bits are written first. Additionally, the code does not
> consider that another address might already have been written into the
> register, causing the OR'ing to result in a bad address.
> 
> Fix this by explicitly overwriting only the low or high order bits for
> 32 bit writes.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index bd7e258c3c26..40b9f8ccfc0e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3781,19 +3781,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>          trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
>          break;
>      case 0x28:  /* ASQ */
> -        n->bar.asq = data;
> +        n->bar.asq = size == 8 ? data :
> +            (n->bar.asq & ~0xffffffff) | (data & 0xffffffff);
                             ^^^^^^^^^^^
If this one is to unmask lower 32bits other than higher 32bits, then
it should be:

	(n->bar.asq & ~0xffffffffULL)

Also, maybe we should take care of lower than 4bytes as:

	.min_access_size = 2,
	.max_access_size = 8,

Even we have some error messages up there with:

	if (unlikely(size < sizeof(uint32_t))) {
	    NVME_GUEST_ERR(pci_nvme_ub_mmiowr_toosmall,
			   "MMIO write smaller than 32-bits,"
			   " offset=0x%"PRIx64", size=%u",
			   offset, size);
	    /* should be ignored, fall through for now */
	}

It's a fall-thru error, and that's it.  I would prefer not to have this
error and support for 2bytes access also, OR do not support for 2bytes
access for this MMIO area.

Thanks!

Re: [PATCH v2 03/12] hw/block/nvme: indicate CMB support through controller capabilities register

[Minwoo Im]

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH v2 04/12] hw/block/nvme: move msix table and pba to BAR 0

[Minwoo Im]

On 21-01-18 10:46:57, Klaus Jensen wrote:
> 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 array out of BAR 4 and into
> BAR 0.

Nice!

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
Tested-by: Minwoo Im <minwoo.im.dev@gmail.com>

Thanks,

Re: [PATCH v2 05/12] hw/block/nvme: allow cmb and pmr to coexist

[Minwoo Im]

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH v2 06/12] hw/block/nvme: rename PMR/CMB shift/mask fields

[Minwoo Im]

On 21-01-18 10:46:59, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Use the correct field names.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  include/block/nvme.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 86d7fc2f905c..f3cbe17d0971 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -35,8 +35,8 @@ enum NvmeCapShift {
>      CAP_CSS_SHIFT      = 37,
>      CAP_MPSMIN_SHIFT   = 48,
>      CAP_MPSMAX_SHIFT   = 52,
> -    CAP_PMR_SHIFT      = 56,
> -    CAP_CMB_SHIFT      = 57,
> +    CAP_PMRS_SHIFT     = 56,
> +    CAP_CMBS_SHIFT     = 57,
>  };
>  
>  enum NvmeCapMask {
> @@ -49,8 +49,8 @@ enum NvmeCapMask {
>      CAP_CSS_MASK       = 0xff,
>      CAP_MPSMIN_MASK    = 0xf,
>      CAP_MPSMAX_MASK    = 0xf,
> -    CAP_PMR_MASK       = 0x1,
> -    CAP_CMB_MASK       = 0x1,
> +    CAP_PMRS_MASK      = 0x1,
> +    CAP_CMBS_MASK      = 0x1,
>  };
>  
>  #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
> @@ -81,10 +81,10 @@ enum NvmeCapMask {
>                                                             << 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)
> -#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMB_MASK)   \
> -                                                           << CAP_CMB_SHIFT)
> +#define NVME_CAP_SET_PMRS(cap, val)   (cap |= (uint64_t)(val & CAP_PMRS_MASK)  \
> +                                                           << CAP_PMRS_SHIFT)
> +#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMBS_MASK)  \
> +                                                           << CAP_CMBS_SHIFT)

Oh, it would have been better folded into [3/12] patch, though.

Changes are looking good to me to represent "Supported".

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

>  
>  enum NvmeCapCss {
>      NVME_CAP_CSS_NVM        = 1 << 0,
> -- 
> 2.30.0
> 
> 

Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Klaus Jensen]

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

On Jan 18 21:41, Minwoo Im wrote:
> On 21-01-18 10:46:55, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > 64 bit registers like ASQ and ACQ should be writable by both a hi/lo 32
> > bit write combination as well as a plain 64 bit write. The spec does not
> > define ordering on the hi/lo split, but the code currently assumes that
> > the low order bits are written first. Additionally, the code does not
> > consider that another address might already have been written into the
> > register, causing the OR'ing to result in a bad address.
> > 
> > Fix this by explicitly overwriting only the low or high order bits for
> > 32 bit writes.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index bd7e258c3c26..40b9f8ccfc0e 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -3781,19 +3781,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> >          trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
> >          break;
> >      case 0x28:  /* ASQ */
> > -        n->bar.asq = data;
> > +        n->bar.asq = size == 8 ? data :
> > +            (n->bar.asq & ~0xffffffff) | (data & 0xffffffff);
>                              ^^^^^^^^^^^
> If this one is to unmask lower 32bits other than higher 32bits, then
> it should be:
> 
> 	(n->bar.asq & ~0xffffffffULL)
> 

Ouch. Yes, thanks!

> Also, maybe we should take care of lower than 4bytes as:
> 
> 	.min_access_size = 2,
> 	.max_access_size = 8,
> 
> Even we have some error messages up there with:
> 
> 	if (unlikely(size < sizeof(uint32_t))) {
> 	    NVME_GUEST_ERR(pci_nvme_ub_mmiowr_toosmall,
> 			   "MMIO write smaller than 32-bits,"
> 			   " offset=0x%"PRIx64", size=%u",
> 			   offset, size);
> 	    /* should be ignored, fall through for now */
> 	}
> 
> It's a fall-thru error, and that's it.  I would prefer not to have this
> error and support for 2bytes access also, OR do not support for 2bytes
> access for this MMIO area.
> 

The spec requires aligned 32 bit accesses (or the size of the register),
so maybe it's about time we actually ignore instead of falling through.

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

Re: [PATCH v2 07/12] hw/block/nvme: remove redundant zeroing of PMR registers

[Minwoo Im]

On 21-01-18 10:47:00, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The controller registers are initially zero. Remove the redundant
> zeroing.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 35 -----------------------------------
>  1 file changed, 35 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f3bea582b3c0..9ee9570bb65c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -4179,43 +4179,8 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
>  
>  static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
>  {
> -    /* PMR Capabities register */
> -    n->bar.pmrcap = 0;
> -    NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 0);
> -    NVME_PMRCAP_SET_WDS(n->bar.pmrcap, 0);
>      NVME_PMRCAP_SET_BIR(n->bar.pmrcap, NVME_PMR_BIR);
> -    NVME_PMRCAP_SET_PMRTU(n->bar.pmrcap, 0);
> -    /* Turn on bit 1 support */

This comment says that PMRWBM [1]th bit is set to PMRCAP below :).

>      NVME_PMRCAP_SET_PMRWBM(n->bar.pmrcap, 0x02);
> -    NVME_PMRCAP_SET_PMRTO(n->bar.pmrcap, 0);
> -    NVME_PMRCAP_SET_CMSS(n->bar.pmrcap, 0);
> -
> -    /* PMR Control register */
> -    n->bar.pmrctl = 0;
> -    NVME_PMRCTL_SET_EN(n->bar.pmrctl, 0);
> -
> -    /* PMR Status register */
> -    n->bar.pmrsts = 0;
> -    NVME_PMRSTS_SET_ERR(n->bar.pmrsts, 0);
> -    NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 0);
> -    NVME_PMRSTS_SET_HSTS(n->bar.pmrsts, 0);
> -    NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 0);
> -
> -    /* PMR Elasticity Buffer Size register */
> -    n->bar.pmrebs = 0;
> -    NVME_PMREBS_SET_PMRSZU(n->bar.pmrebs, 0);
> -    NVME_PMREBS_SET_RBB(n->bar.pmrebs, 0);
> -    NVME_PMREBS_SET_PMRWBZ(n->bar.pmrebs, 0);
> -
> -    /* PMR Sustained Write Throughput register */
> -    n->bar.pmrswtp = 0;
> -    NVME_PMRSWTP_SET_PMRSWTU(n->bar.pmrswtp, 0);
> -    NVME_PMRSWTP_SET_PMRSWTV(n->bar.pmrswtp, 0);
> -
> -    /* PMR Memory Space Control register */
> -    n->bar.pmrmsc = 0;
> -    NVME_PMRMSC_SET_CMSE(n->bar.pmrmsc, 0);
> -    NVME_PMRMSC_SET_CBA(n->bar.pmrmsc, 0);
>  
>      pci_register_bar(pci_dev, NVME_PMRCAP_BIR(n->bar.pmrcap),
>                       PCI_BASE_ADDRESS_SPACE_MEMORY |
> -- 
> 2.30.0
> 
> 

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Minwoo Im]

Hello,

On 21-01-18 10:47:03, Klaus Jensen wrote:
> From: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
> 
> Implement v1.4 logic for configuring the Controller Memory Buffer. This
> is not backward compatible with v1.3, so drivers that only support v1.3
> will not be able to use the CMB anymore.
> 
> Signed-off-by: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
model?

Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Minwoo Im]

> The spec requires aligned 32 bit accesses (or the size of the register),
> so maybe it's about time we actually ignore instead of falling through.

Agreed.

Re: [PATCH v2 07/12] hw/block/nvme: remove redundant zeroing of PMR registers

[Klaus Jensen]

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

On Jan 18 21:55, Minwoo Im wrote:
> On 21-01-18 10:47:00, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > The controller registers are initially zero. Remove the redundant
> > zeroing.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 35 -----------------------------------
> >  1 file changed, 35 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index f3bea582b3c0..9ee9570bb65c 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -4179,43 +4179,8 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
> >  
> >  static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
> >  {
> > -    /* PMR Capabities register */
> > -    n->bar.pmrcap = 0;
> > -    NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 0);
> > -    NVME_PMRCAP_SET_WDS(n->bar.pmrcap, 0);
> >      NVME_PMRCAP_SET_BIR(n->bar.pmrcap, NVME_PMR_BIR);
> > -    NVME_PMRCAP_SET_PMRTU(n->bar.pmrcap, 0);
> > -    /* Turn on bit 1 support */
> 
> This comment says that PMRWBM [1]th bit is set to PMRCAP below :).
> 

Thanks!


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

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Klaus Jensen]

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

On Jan 18 21:58, Minwoo Im wrote:
> Hello,
> 
> On 21-01-18 10:47:03, Klaus Jensen wrote:
> > From: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
> > 
> > Implement v1.4 logic for configuring the Controller Memory Buffer. This
> > is not backward compatible with v1.3, so drivers that only support v1.3
> > will not be able to use the CMB anymore.
> > 
> > Signed-off-by: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> 
> Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
> the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
> model?

Next patch moves to v1.4. I wanted to split it because the "bump" patch
also adds a couple of other v1.4 requires fields. But I understand that
this is slightly wrong.

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

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Minwoo Im]

> > Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
> > the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
> > model?
> 
> Next patch moves to v1.4. I wanted to split it because the "bump" patch
> also adds a couple of other v1.4 requires fields. But I understand that
> this is slightly wrong.

Sorry, I meant I'd like to have CMB for v1.3 support along with the v1.4
support in this device model separately.  Maybe I missed the linux-nvme
mailing list for CMB v1.4, but is there no plan to keep supportin CMB
v1.3 in NVMe driver?

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Klaus Jensen]

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

On Jan 18 22:09, Minwoo Im wrote:
> > > Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
> > > the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
> > > model?
> > 
> > Next patch moves to v1.4. I wanted to split it because the "bump" patch
> > also adds a couple of other v1.4 requires fields. But I understand that
> > this is slightly wrong.
> 
> Sorry, I meant I'd like to have CMB for v1.3 support along with the v1.4
> support in this device model separately.  Maybe I missed the linux-nvme
> mailing list for CMB v1.4, but is there no plan to keep supportin CMB
> v1.3 in NVMe driver?

I posted a patch on linux-nvme for v1.4 support in the kernel. It will
support both the v1.3 and v1.4 behavior :)

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

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Minwoo Im]

On 21-01-18 14:10:50, Klaus Jensen wrote:
> On Jan 18 22:09, Minwoo Im wrote:
> > > > Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
> > > > the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
> > > > model?
> > > 
> > > Next patch moves to v1.4. I wanted to split it because the "bump" patch
> > > also adds a couple of other v1.4 requires fields. But I understand that
> > > this is slightly wrong.
> > 
> > Sorry, I meant I'd like to have CMB for v1.3 support along with the v1.4
> > support in this device model separately.  Maybe I missed the linux-nvme
> > mailing list for CMB v1.4, but is there no plan to keep supportin CMB
> > v1.3 in NVMe driver?
> 
> I posted a patch on linux-nvme for v1.4 support in the kernel. It will
> support both the v1.3 and v1.4 behavior :)

Then, can we maintain CMB v1.3 also in QEMU also along with v1.4 ? :)

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Klaus Jensen]

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

On Jan 18 22:12, Minwoo Im wrote:
> On 21-01-18 14:10:50, Klaus Jensen wrote:
> > On Jan 18 22:09, Minwoo Im wrote:
> > > > > Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
> > > > > the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
> > > > > model?
> > > > 
> > > > Next patch moves to v1.4. I wanted to split it because the "bump" patch
> > > > also adds a couple of other v1.4 requires fields. But I understand that
> > > > this is slightly wrong.
> > > 
> > > Sorry, I meant I'd like to have CMB for v1.3 support along with the v1.4
> > > support in this device model separately.  Maybe I missed the linux-nvme
> > > mailing list for CMB v1.4, but is there no plan to keep supportin CMB
> > > v1.3 in NVMe driver?
> > 
> > I posted a patch on linux-nvme for v1.4 support in the kernel. It will
> > support both the v1.3 and v1.4 behavior :)
> 
> Then, can we maintain CMB v1.3 also in QEMU also along with v1.4 ? :)

My first version of this patch actually included compatibility support
for v1.3 ("legacy cmb"), but Keith suggested we just drop that and keep
this device compliant.

What we could do is allow the spec version to be chosen with a
parameter?

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

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Klaus Jensen]

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

On Jan 18 14:22, Klaus Jensen wrote:
> On Jan 18 22:12, Minwoo Im wrote:
> > On 21-01-18 14:10:50, Klaus Jensen wrote:
> > > On Jan 18 22:09, Minwoo Im wrote:
> > > > > > Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
> > > > > > the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
> > > > > > model?
> > > > > 
> > > > > Next patch moves to v1.4. I wanted to split it because the "bump" patch
> > > > > also adds a couple of other v1.4 requires fields. But I understand that
> > > > > this is slightly wrong.
> > > > 
> > > > Sorry, I meant I'd like to have CMB for v1.3 support along with the v1.4
> > > > support in this device model separately.  Maybe I missed the linux-nvme
> > > > mailing list for CMB v1.4, but is there no plan to keep supportin CMB
> > > > v1.3 in NVMe driver?
> > > 
> > > I posted a patch on linux-nvme for v1.4 support in the kernel. It will
> > > support both the v1.3 and v1.4 behavior :)
> > 
> > Then, can we maintain CMB v1.3 also in QEMU also along with v1.4 ? :)
> 
> My first version of this patch actually included compatibility support
> for v1.3 ("legacy cmb"), but Keith suggested we just drop that and keep
> this device compliant.
> 
> What we could do is allow the spec version to be chosen with a
> parameter?

Uhm. Maybe not. I gave this some more thought.

Adding a device parameter to choose the specification version requires
us to maintain QEMU "compat" properties across different QEMU version.
I'm not sure we want that for something like this.

Maybe the best course of action actually *is* an 'x-legacy-cmb'
parameter.

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

Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Klaus Jensen]

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

On Jan 18 21:59, Minwoo Im wrote:
> > The spec requires aligned 32 bit accesses (or the size of the register),
> > so maybe it's about time we actually ignore instead of falling through.
> 
> Agreed.
> 

On the other hand.

The spec just allows undefined behavior if the alignment or size
requirement is violated. So falling through is not wrong.

Keith, any thoughts on this?

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

Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Minwoo Im]

On 21-01-18 20:53:24, Klaus Jensen wrote:
> On Jan 18 21:59, Minwoo Im wrote:
> > > The spec requires aligned 32 bit accesses (or the size of the register),
> > > so maybe it's about time we actually ignore instead of falling through.
> > 
> > Agreed.
> > 
> 
> On the other hand.
> 
> The spec just allows undefined behavior if the alignment or size
> requirement is violated. So falling through is not wrong.

If so, maybe we just can make this MMIO region support under 4bytes
access with error messaging.  I don't think we should not support them
on purpose because spec says it just results in undefined behaviors
which is just undefined how to handle them.

Re: [PATCH v2 10/12] hw/block/nvme: move cmb logic to v1.4

[Minwoo Im]

On 21-01-18 20:23:30, Klaus Jensen wrote:
> On Jan 18 14:22, Klaus Jensen wrote:
> > On Jan 18 22:12, Minwoo Im wrote:
> > > On 21-01-18 14:10:50, Klaus Jensen wrote:
> > > > On Jan 18 22:09, Minwoo Im wrote:
> > > > > > > Yes, CMB in v1.4 is not backward-compatible, but is it okay to move onto
> > > > > > > the CMB v1.4 from v1.3 without supporting the v1.3 usage on this device
> > > > > > > model?
> > > > > > 
> > > > > > Next patch moves to v1.4. I wanted to split it because the "bump" patch
> > > > > > also adds a couple of other v1.4 requires fields. But I understand that
> > > > > > this is slightly wrong.
> > > > > 
> > > > > Sorry, I meant I'd like to have CMB for v1.3 support along with the v1.4
> > > > > support in this device model separately.  Maybe I missed the linux-nvme
> > > > > mailing list for CMB v1.4, but is there no plan to keep supportin CMB
> > > > > v1.3 in NVMe driver?
> > > > 
> > > > I posted a patch on linux-nvme for v1.4 support in the kernel. It will
> > > > support both the v1.3 and v1.4 behavior :)
> > > 
> > > Then, can we maintain CMB v1.3 also in QEMU also along with v1.4 ? :)
> > 
> > My first version of this patch actually included compatibility support
> > for v1.3 ("legacy cmb"), but Keith suggested we just drop that and keep
> > this device compliant.
> > 
> > What we could do is allow the spec version to be chosen with a
> > parameter?
> 
> Uhm. Maybe not. I gave this some more thought.
> 
> Adding a device parameter to choose the specification version requires
> us to maintain QEMU "compat" properties across different QEMU version.
> I'm not sure we want that for something like this.

Agreed.  The default should head for latest one.

> 
> Maybe the best course of action actually *is* an 'x-legacy-cmb'
> parameter.

This looks fine.

As kernel driver does not remove the v1.3 CMB support, then it would be
great if QEMU suports that also.

Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Keith Busch]

On Mon, Jan 18, 2021 at 08:53:24PM +0100, Klaus Jensen wrote:
> On Jan 18 21:59, Minwoo Im wrote:
> > > The spec requires aligned 32 bit accesses (or the size of the register),
> > > so maybe it's about time we actually ignore instead of falling through.
> > 
> > Agreed.
> > 
> 
> On the other hand.
> 
> The spec just allows undefined behavior if the alignment or size
> requirement is violated. So falling through is not wrong.
> 
> Keith, any thoughts on this?

If the host's access is unaligned and breaks something, the host gets
to keep both pieces.