[PATCH v3 0/5] hw/nvme: fix mmio read

[PATCH v3 0/5] hw/nvme: fix mmio read

[Klaus Jensen]

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

Fix mmio read issues on big-endian hosts. The core issue is that values
in the BAR is not stored in little endian as required.

Fix that and add a regression test for this. This required a bit of
cleanup, so it blew up into a series.

v2:

  * "hw/nvme: use symbolic names for registers"
    Use offsetof(NvmeBar, reg) instead of explicit offsets (Philippe)

  * "hw/nvme: fix mmio read"
    Use the st/ld API instead of cpu_to_X (Philippe)

Klaus Jensen (5):
  hw/nvme: split pmrmsc register into upper and lower
  hw/nvme: use symbolic names for registers
  hw/nvme: fix out-of-bounds reads
  hw/nvme: fix mmio read
  tests/qtest/nvme-test: add mmio read test

 include/block/nvme.h    |  60 +++++--
 hw/nvme/ctrl.c          | 362 +++++++++++++++++++++++-----------------
 tests/qtest/nvme-test.c |  26 +++
 3 files changed, 276 insertions(+), 172 deletions(-)

-- 
2.32.0

[PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower

[Klaus Jensen]

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

The specification uses a set of 32 bit PMRMSCL and PMRMSCU registers to
make up the 64 bit logical PMRMSC register.

Make it so.

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

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 527105fafc0b..84053b68b987 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -26,7 +26,8 @@ typedef struct QEMU_PACKED NvmeBar {
     uint32_t    pmrsts;
     uint32_t    pmrebs;
     uint32_t    pmrswtp;
-    uint64_t    pmrmsc;
+    uint32_t    pmrmscl;
+    uint32_t    pmrmscu;
     uint8_t     css[484];
 } NvmeBar;
 
@@ -475,25 +476,25 @@ enum NvmePmrswtpMask {
 #define NVME_PMRSWTP_SET_PMRSWTV(pmrswtp, val)   \
     (pmrswtp |= (uint64_t)(val & PMRSWTP_PMRSWTV_MASK) << PMRSWTP_PMRSWTV_SHIFT)
 
-enum NvmePmrmscShift {
-    PMRMSC_CMSE_SHIFT   = 1,
-    PMRMSC_CBA_SHIFT    = 12,
+enum NvmePmrmsclShift {
+    PMRMSCL_CMSE_SHIFT   = 1,
+    PMRMSCL_CBA_SHIFT    = 12,
 };
 
-enum NvmePmrmscMask {
-    PMRMSC_CMSE_MASK   = 0x1,
-    PMRMSC_CBA_MASK    = 0xfffffffffffff,
+enum NvmePmrmsclMask {
+    PMRMSCL_CMSE_MASK   = 0x1,
+    PMRMSCL_CBA_MASK    = 0xfffff,
 };
 
-#define NVME_PMRMSC_CMSE(pmrmsc)    \
-    ((pmrmsc >> PMRMSC_CMSE_SHIFT)   & PMRMSC_CMSE_MASK)
-#define NVME_PMRMSC_CBA(pmrmsc)     \
-    ((pmrmsc >> PMRMSC_CBA_SHIFT)   & PMRMSC_CBA_MASK)
+#define NVME_PMRMSCL_CMSE(pmrmscl)    \
+    ((pmrmscl >> PMRMSCL_CMSE_SHIFT)   & PMRMSCL_CMSE_MASK)
+#define NVME_PMRMSCL_CBA(pmrmscl)     \
+    ((pmrmscl >> PMRMSCL_CBA_SHIFT)   & PMRMSCL_CBA_MASK)
 
-#define NVME_PMRMSC_SET_CMSE(pmrmsc, val)   \
-    (pmrmsc |= (uint64_t)(val & PMRMSC_CMSE_MASK) << PMRMSC_CMSE_SHIFT)
-#define NVME_PMRMSC_SET_CBA(pmrmsc, val)   \
-    (pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
+#define NVME_PMRMSCL_SET_CMSE(pmrmscl, val)   \
+    (pmrmscl |= (uint32_t)(val & PMRMSCL_CMSE_MASK) << PMRMSCL_CMSE_SHIFT)
+#define NVME_PMRMSCL_SET_CBA(pmrmscl, val)   \
+    (pmrmscl |= (uint32_t)(val & PMRMSCL_CBA_MASK) << PMRMSCL_CBA_SHIFT)
 
 enum NvmeSglDescriptorType {
     NVME_SGL_DESCR_TYPE_DATA_BLOCK          = 0x0,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2f0524e12a36..28299c6f3764 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5916,11 +5916,12 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             return;
         }
 
-        n->bar.pmrmsc = (n->bar.pmrmsc & ~0xffffffff) | (data & 0xffffffff);
+        n->bar.pmrmscl = 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 (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
+            hwaddr cba = n->bar.pmrmscu |
+                (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
             if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
                 NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
                 return;
@@ -5936,7 +5937,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             return;
         }
 
-        n->bar.pmrmsc = (n->bar.pmrmsc & 0xffffffff) | (data << 32);
+        n->bar.pmrmscu = data & 0xffffffff;
         return;
     default:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
-- 
2.32.0

Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower

[Gollu Appalanaidu]

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

On Wed, Jul 14, 2021 at 08:01:21AM +0200, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>The specification uses a set of 32 bit PMRMSCL and PMRMSCU registers to
>make up the 64 bit logical PMRMSC register.
>
>Make it so.
>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> include/block/nvme.h | 31 ++++++++++++++++---------------
> hw/nvme/ctrl.c       |  9 +++++----
> 2 files changed, 21 insertions(+), 19 deletions(-)
>
>diff --git a/include/block/nvme.h b/include/block/nvme.h
>index 527105fafc0b..84053b68b987 100644
>--- a/include/block/nvme.h
>+++ b/include/block/nvme.h
>@@ -26,7 +26,8 @@ typedef struct QEMU_PACKED NvmeBar {
>     uint32_t    pmrsts;
>     uint32_t    pmrebs;
>     uint32_t    pmrswtp;
>-    uint64_t    pmrmsc;
>+    uint32_t    pmrmscl;
>+    uint32_t    pmrmscu;
>     uint8_t     css[484];
> } NvmeBar;
>
>@@ -475,25 +476,25 @@ enum NvmePmrswtpMask {
> #define NVME_PMRSWTP_SET_PMRSWTV(pmrswtp, val)   \
>     (pmrswtp |= (uint64_t)(val & PMRSWTP_PMRSWTV_MASK) << PMRSWTP_PMRSWTV_SHIFT)
>
>-enum NvmePmrmscShift {
>-    PMRMSC_CMSE_SHIFT   = 1,
>-    PMRMSC_CBA_SHIFT    = 12,
>+enum NvmePmrmsclShift {
>+    PMRMSCL_CMSE_SHIFT   = 1,
>+    PMRMSCL_CBA_SHIFT    = 12,
> };
>
>-enum NvmePmrmscMask {
>-    PMRMSC_CMSE_MASK   = 0x1,
>-    PMRMSC_CBA_MASK    = 0xfffffffffffff,
>+enum NvmePmrmsclMask {
>+    PMRMSCL_CMSE_MASK   = 0x1,
>+    PMRMSCL_CBA_MASK    = 0xfffff,
> };
>
>-#define NVME_PMRMSC_CMSE(pmrmsc)    \
>-    ((pmrmsc >> PMRMSC_CMSE_SHIFT)   & PMRMSC_CMSE_MASK)
>-#define NVME_PMRMSC_CBA(pmrmsc)     \
>-    ((pmrmsc >> PMRMSC_CBA_SHIFT)   & PMRMSC_CBA_MASK)
>+#define NVME_PMRMSCL_CMSE(pmrmscl)    \
>+    ((pmrmscl >> PMRMSCL_CMSE_SHIFT)   & PMRMSCL_CMSE_MASK)
>+#define NVME_PMRMSCL_CBA(pmrmscl)     \
>+    ((pmrmscl >> PMRMSCL_CBA_SHIFT)   & PMRMSCL_CBA_MASK)
>
>-#define NVME_PMRMSC_SET_CMSE(pmrmsc, val)   \
>-    (pmrmsc |= (uint64_t)(val & PMRMSC_CMSE_MASK) << PMRMSC_CMSE_SHIFT)
>-#define NVME_PMRMSC_SET_CBA(pmrmsc, val)   \
>-    (pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
>+#define NVME_PMRMSCL_SET_CMSE(pmrmscl, val)   \
>+    (pmrmscl |= (uint32_t)(val & PMRMSCL_CMSE_MASK) << PMRMSCL_CMSE_SHIFT)
>+#define NVME_PMRMSCL_SET_CBA(pmrmscl, val)   \
>+    (pmrmscl |= (uint32_t)(val & PMRMSCL_CBA_MASK) << PMRMSCL_CBA_SHIFT)
>
> enum NvmeSglDescriptorType {
>     NVME_SGL_DESCR_TYPE_DATA_BLOCK          = 0x0,
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 2f0524e12a36..28299c6f3764 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -5916,11 +5916,12 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>             return;
>         }
>
>-        n->bar.pmrmsc = (n->bar.pmrmsc & ~0xffffffff) | (data & 0xffffffff);
>+        n->bar.pmrmscl = 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 (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
>+            hwaddr cba = n->bar.pmrmscu |
>+                (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
>             if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
>                 NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
>                 return;
>@@ -5936,7 +5937,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>             return;
>         }
>
>-        n->bar.pmrmsc = (n->bar.pmrmsc & 0xffffffff) | (data << 32);
>+        n->bar.pmrmscu = data & 0xffffffff;
>         return;
>     default:
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
>-- 
>2.32.0
>
>
Changes LGTM.

Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower

[Peter Maydell]

On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The specification uses a set of 32 bit PMRMSCL and PMRMSCU registers to
> make up the 64 bit logical PMRMSC register.
>
> Make it so.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  include/block/nvme.h | 31 ++++++++++++++++---------------
>  hw/nvme/ctrl.c       |  9 +++++----
>  2 files changed, 21 insertions(+), 19 deletions(-)

> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 2f0524e12a36..28299c6f3764 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -5916,11 +5916,12 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>              return;
>          }
>
> -        n->bar.pmrmsc = (n->bar.pmrmsc & ~0xffffffff) | (data & 0xffffffff);
> +        n->bar.pmrmscl = 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 (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
> +            hwaddr cba = n->bar.pmrmscu |
> +                (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);

Don't we need to shift n->bar.pmrmscu left by 32 before we OR it in
with the pmrmscl part?

>              if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
>                  NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
>                  return;
> @@ -5936,7 +5937,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>              return;
>          }
>
> -        n->bar.pmrmsc = (n->bar.pmrmsc & 0xffffffff) | (data << 32);
> +        n->bar.pmrmscu = data & 0xffffffff;
>          return;

Not an issue with this patch, but why don't we need to do the
"update cba" stuff for a write to the U register that we do for
a write to the L register? Does the spec mandate that guests
do the writes in the order H then L and only the L change makes
it take effect?

thanks
-- PMM

Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower

[Klaus Jensen]

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

On Jul 19 10:13, Peter Maydell wrote:
> On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > The specification uses a set of 32 bit PMRMSCL and PMRMSCU registers to
> > make up the 64 bit logical PMRMSC register.
> >
> > Make it so.
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  include/block/nvme.h | 31 ++++++++++++++++---------------
> >  hw/nvme/ctrl.c       |  9 +++++----
> >  2 files changed, 21 insertions(+), 19 deletions(-)
> 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 2f0524e12a36..28299c6f3764 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -5916,11 +5916,12 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> >              return;
> >          }
> >
> > -        n->bar.pmrmsc = (n->bar.pmrmsc & ~0xffffffff) | (data & 0xffffffff);
> > +        n->bar.pmrmscl = 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 (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
> > +            hwaddr cba = n->bar.pmrmscu |
> > +                (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
> 
> Don't we need to shift n->bar.pmrmscu left by 32 before we OR it in
> with the pmrmscl part?
> 

We do indeed - thanks for catching this!

> >              if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
> >                  NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
> >                  return;
> > @@ -5936,7 +5937,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> >              return;
> >          }
> >
> > -        n->bar.pmrmsc = (n->bar.pmrmsc & 0xffffffff) | (data << 32);
> > +        n->bar.pmrmscu = data & 0xffffffff;
> >          return;
> 
> Not an issue with this patch, but why don't we need to do the
> "update cba" stuff for a write to the U register that we do for
> a write to the L register? Does the spec mandate that guests
> do the writes in the order H then L and only the L change makes
> it take effect?
> 

Yeah, bit 1 in PMRMSCL is the "Controller Memory Space Enable" bit
(CMSE) and we only enable the address space when that bit is set. So the
driver will typically write the high register first (if needed) and then
write the lower register with or without CMSE set.

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

[PATCH v3 2/5] hw/nvme: use symbolic names for registers

[Klaus Jensen]

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

Add the NvmeBarRegs enum and use these instead of explicit register
offsets.

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

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 84053b68b987..77aae0117494 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -9,7 +9,7 @@ typedef struct QEMU_PACKED NvmeBar {
     uint32_t    cc;
     uint8_t     rsvd24[4];
     uint32_t    csts;
-    uint32_t    nssrc;
+    uint32_t    nssr;
     uint32_t    aqa;
     uint64_t    asq;
     uint64_t    acq;
@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
     uint8_t     css[484];
 } NvmeBar;
 
+enum NvmeBarRegs {
+    NVME_REG_CAP     = offsetof(NvmeBar, cap),
+    NVME_REG_VS      = offsetof(NvmeBar, vs),
+    NVME_REG_INTMS   = offsetof(NvmeBar, intms),
+    NVME_REG_INTMC   = offsetof(NvmeBar, intmc),
+    NVME_REG_CC      = offsetof(NvmeBar, cc),
+    NVME_REG_CSTS    = offsetof(NvmeBar, csts),
+    NVME_REG_NSSR    = offsetof(NvmeBar, nssr),
+    NVME_REG_AQA     = offsetof(NvmeBar, aqa),
+    NVME_REG_ASQ     = offsetof(NvmeBar, asq),
+    NVME_REG_ACQ     = offsetof(NvmeBar, acq),
+    NVME_REG_CMBLOC  = offsetof(NvmeBar, cmbloc),
+    NVME_REG_CMBSZ   = offsetof(NvmeBar, cmbsz),
+    NVME_REG_BPINFO  = offsetof(NvmeBar, bpinfo),
+    NVME_REG_BPRSEL  = offsetof(NvmeBar, bprsel),
+    NVME_REG_BPMBL   = offsetof(NvmeBar, bpmbl),
+    NVME_REG_CMBMSC  = offsetof(NvmeBar, cmbmsc),
+    NVME_REG_CMBSTS  = offsetof(NvmeBar, cmbsts),
+    NVME_REG_PMRCAP  = offsetof(NvmeBar, pmrcap),
+    NVME_REG_PMRCTL  = offsetof(NvmeBar, pmrctl),
+    NVME_REG_PMRSTS  = offsetof(NvmeBar, pmrsts),
+    NVME_REG_PMREBS  = offsetof(NvmeBar, pmrebs),
+    NVME_REG_PMRSWTP = offsetof(NvmeBar, pmrswtp),
+    NVME_REG_PMRMSCL = offsetof(NvmeBar, pmrmscl),
+    NVME_REG_PMRMSCU = offsetof(NvmeBar, pmrmscu),
+};
+
 enum NvmeCapShift {
     CAP_MQES_SHIFT     = 0,
     CAP_CQR_SHIFT      = 16,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 28299c6f3764..8c305315f41c 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
     }
 
     switch (offset) {
-    case 0xc:   /* INTMS */
+    case NVME_REG_INTMS:
         if (unlikely(msix_enabled(&(n->parent_obj)))) {
             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
                            "undefined access to interrupt mask set"
@@ -5752,7 +5752,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
         nvme_irq_check(n);
         break;
-    case 0x10:  /* INTMC */
+    case NVME_REG_INTMC:
         if (unlikely(msix_enabled(&(n->parent_obj)))) {
             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
                            "undefined access to interrupt mask clr"
@@ -5764,7 +5764,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
         nvme_irq_check(n);
         break;
-    case 0x14:  /* CC */
+    case NVME_REG_CC:
         trace_pci_nvme_mmio_cfg(data & 0xffffffff);
         /* Windows first sends data, then sends enable bit */
         if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
@@ -5798,7 +5798,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             n->bar.cc = data;
         }
         break;
-    case 0x1c:  /* CSTS */
+    case NVME_REG_CSTS:
         if (data & (1 << 4)) {
             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
                            "attempted to W1C CSTS.NSSRO"
@@ -5809,7 +5809,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                            " of controller status");
         }
         break;
-    case 0x20:  /* NSSR */
+    case NVME_REG_NSSR:
         if (data == 0x4e564d65) {
             trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
         } else {
@@ -5817,38 +5817,38 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             return;
         }
         break;
-    case 0x24:  /* AQA */
+    case NVME_REG_AQA:
         n->bar.aqa = data & 0xffffffff;
         trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
         break;
-    case 0x28:  /* ASQ */
+    case NVME_REG_ASQ:
         n->bar.asq = size == 8 ? data :
             (n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff);
         trace_pci_nvme_mmio_asqaddr(data);
         break;
-    case 0x2c:  /* ASQ hi */
+    case NVME_REG_ASQ + 4:
         n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
         trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
         break;
-    case 0x30:  /* ACQ */
+    case NVME_REG_ACQ:
         trace_pci_nvme_mmio_acqaddr(data);
         n->bar.acq = size == 8 ? data :
             (n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff);
         break;
-    case 0x34:  /* ACQ hi */
+    case NVME_REG_ACQ + 4:
         n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
         trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
         break;
-    case 0x38:  /* CMBLOC */
+    case NVME_REG_CMBLOC:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
                        "invalid write to reserved CMBLOC"
                        " when CMBSZ is zero, ignored");
         return;
-    case 0x3C:  /* CMBSZ */
+    case NVME_REG_CMBSZ:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbsz_readonly,
                        "invalid write to read only CMBSZ, ignored");
         return;
-    case 0x50:  /* CMBMSC */
+    case NVME_REG_CMBMSC:
         if (!NVME_CAP_CMBS(n->bar.cap)) {
             return;
         }
@@ -5876,15 +5876,15 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         }
 
         return;
-    case 0x54:  /* CMBMSC hi */
+    case NVME_REG_CMBMSC + 4:
         n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
         return;
 
-    case 0xe00: /* PMRCAP */
+    case NVME_REG_PMRCAP:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
                        "invalid write to PMRCAP register, ignored");
         return;
-    case 0xe04: /* PMRCTL */
+    case NVME_REG_PMRCTL:
         if (!NVME_CAP_PMRS(n->bar.cap)) {
             return;
         }
@@ -5899,19 +5899,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             n->pmr.cmse = false;
         }
         return;
-    case 0xe08: /* PMRSTS */
+    case NVME_REG_PMRSTS:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
                        "invalid write to PMRSTS register, ignored");
         return;
-    case 0xe0C: /* PMREBS */
+    case NVME_REG_PMREBS:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
                        "invalid write to PMREBS register, ignored");
         return;
-    case 0xe10: /* PMRSWTP */
+    case NVME_REG_PMRSWTP:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
                        "invalid write to PMRSWTP register, ignored");
         return;
-    case 0xe14: /* PMRMSCL */
+    case NVME_REG_PMRMSCL:
         if (!NVME_CAP_PMRS(n->bar.cap)) {
             return;
         }
@@ -5932,7 +5932,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         }
 
         return;
-    case 0xe18: /* PMRMSCU */
+    case NVME_REG_PMRMSCU:
         if (!NVME_CAP_PMRS(n->bar.cap)) {
             return;
         }
@@ -5974,7 +5974,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
          * from PMRSTS should ensure prior writes
          * made it to persistent media
          */
-        if (addr == 0xe08 &&
+        if (addr == NVME_REG_PMRSTS &&
             (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
             memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
         }
-- 
2.32.0

Re: [PATCH v3 2/5] hw/nvme: use symbolic names for registers

[Philippe Mathieu-Daudé]

On 7/14/21 8:01 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add the NvmeBarRegs enum and use these instead of explicit register
> offsets.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  include/block/nvme.h | 29 ++++++++++++++++++++++++++++-
>  hw/nvme/ctrl.c       | 44 ++++++++++++++++++++++----------------------
>  2 files changed, 50 insertions(+), 23 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 2/5] hw/nvme: use symbolic names for registers

[Gollu Appalanaidu]

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

On Wed, Jul 14, 2021 at 08:01:22AM +0200, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>Add the NvmeBarRegs enum and use these instead of explicit register
>offsets.
>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> include/block/nvme.h | 29 ++++++++++++++++++++++++++++-
> hw/nvme/ctrl.c       | 44 ++++++++++++++++++++++----------------------
> 2 files changed, 50 insertions(+), 23 deletions(-)
>
>diff --git a/include/block/nvme.h b/include/block/nvme.h
>index 84053b68b987..77aae0117494 100644
>--- a/include/block/nvme.h
>+++ b/include/block/nvme.h
>@@ -9,7 +9,7 @@ typedef struct QEMU_PACKED NvmeBar {
>     uint32_t    cc;
>     uint8_t     rsvd24[4];
>     uint32_t    csts;
>-    uint32_t    nssrc;
>+    uint32_t    nssr;
>     uint32_t    aqa;
>     uint64_t    asq;
>     uint64_t    acq;
>@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
>     uint8_t     css[484];
> } NvmeBar;
>
>+enum NvmeBarRegs {
>+    NVME_REG_CAP     = offsetof(NvmeBar, cap),
>+    NVME_REG_VS      = offsetof(NvmeBar, vs),
>+    NVME_REG_INTMS   = offsetof(NvmeBar, intms),
>+    NVME_REG_INTMC   = offsetof(NvmeBar, intmc),
>+    NVME_REG_CC      = offsetof(NvmeBar, cc),
>+    NVME_REG_CSTS    = offsetof(NvmeBar, csts),
>+    NVME_REG_NSSR    = offsetof(NvmeBar, nssr),
>+    NVME_REG_AQA     = offsetof(NvmeBar, aqa),
>+    NVME_REG_ASQ     = offsetof(NvmeBar, asq),
>+    NVME_REG_ACQ     = offsetof(NvmeBar, acq),
>+    NVME_REG_CMBLOC  = offsetof(NvmeBar, cmbloc),
>+    NVME_REG_CMBSZ   = offsetof(NvmeBar, cmbsz),
>+    NVME_REG_BPINFO  = offsetof(NvmeBar, bpinfo),
>+    NVME_REG_BPRSEL  = offsetof(NvmeBar, bprsel),
>+    NVME_REG_BPMBL   = offsetof(NvmeBar, bpmbl),
>+    NVME_REG_CMBMSC  = offsetof(NvmeBar, cmbmsc),
>+    NVME_REG_CMBSTS  = offsetof(NvmeBar, cmbsts),
>+    NVME_REG_PMRCAP  = offsetof(NvmeBar, pmrcap),
>+    NVME_REG_PMRCTL  = offsetof(NvmeBar, pmrctl),
>+    NVME_REG_PMRSTS  = offsetof(NvmeBar, pmrsts),
>+    NVME_REG_PMREBS  = offsetof(NvmeBar, pmrebs),
>+    NVME_REG_PMRSWTP = offsetof(NvmeBar, pmrswtp),
>+    NVME_REG_PMRMSCL = offsetof(NvmeBar, pmrmscl),
>+    NVME_REG_PMRMSCU = offsetof(NvmeBar, pmrmscu),
>+};
>+
> enum NvmeCapShift {
>     CAP_MQES_SHIFT     = 0,
>     CAP_CQR_SHIFT      = 16,
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 28299c6f3764..8c305315f41c 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>     }
>
>     switch (offset) {
>-    case 0xc:   /* INTMS */
>+    case NVME_REG_INTMS:
>         if (unlikely(msix_enabled(&(n->parent_obj)))) {
>             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
>                            "undefined access to interrupt mask set"
>@@ -5752,7 +5752,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>         trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
>         nvme_irq_check(n);
>         break;
>-    case 0x10:  /* INTMC */
>+    case NVME_REG_INTMC:
>         if (unlikely(msix_enabled(&(n->parent_obj)))) {
>             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
>                            "undefined access to interrupt mask clr"
>@@ -5764,7 +5764,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>         trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
>         nvme_irq_check(n);
>         break;
>-    case 0x14:  /* CC */
>+    case NVME_REG_CC:
>         trace_pci_nvme_mmio_cfg(data & 0xffffffff);
>         /* Windows first sends data, then sends enable bit */
>         if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
>@@ -5798,7 +5798,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>             n->bar.cc = data;
>         }
>         break;
>-    case 0x1c:  /* CSTS */
>+    case NVME_REG_CSTS:
>         if (data & (1 << 4)) {
>             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
>                            "attempted to W1C CSTS.NSSRO"
>@@ -5809,7 +5809,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>                            " of controller status");
>         }
>         break;
>-    case 0x20:  /* NSSR */
>+    case NVME_REG_NSSR:
>         if (data == 0x4e564d65) {
>             trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
>         } else {
>@@ -5817,38 +5817,38 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>             return;
>         }
>         break;
>-    case 0x24:  /* AQA */
>+    case NVME_REG_AQA:
>         n->bar.aqa = data & 0xffffffff;
>         trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
>         break;
>-    case 0x28:  /* ASQ */
>+    case NVME_REG_ASQ:
>         n->bar.asq = size == 8 ? data :
>             (n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff);
>         trace_pci_nvme_mmio_asqaddr(data);
>         break;
>-    case 0x2c:  /* ASQ hi */
>+    case NVME_REG_ASQ + 4:
>         n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
>         trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
>         break;
>-    case 0x30:  /* ACQ */
>+    case NVME_REG_ACQ:
>         trace_pci_nvme_mmio_acqaddr(data);
>         n->bar.acq = size == 8 ? data :
>             (n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff);
>         break;
>-    case 0x34:  /* ACQ hi */
>+    case NVME_REG_ACQ + 4:
>         n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
>         trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
>         break;
>-    case 0x38:  /* CMBLOC */
>+    case NVME_REG_CMBLOC:
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
>                        "invalid write to reserved CMBLOC"
>                        " when CMBSZ is zero, ignored");
>         return;
>-    case 0x3C:  /* CMBSZ */
>+    case NVME_REG_CMBSZ:
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbsz_readonly,
>                        "invalid write to read only CMBSZ, ignored");
>         return;
>-    case 0x50:  /* CMBMSC */
>+    case NVME_REG_CMBMSC:
>         if (!NVME_CAP_CMBS(n->bar.cap)) {
>             return;
>         }
>@@ -5876,15 +5876,15 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>         }
>
>         return;
>-    case 0x54:  /* CMBMSC hi */
>+    case NVME_REG_CMBMSC + 4:
>         n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
>         return;
>
>-    case 0xe00: /* PMRCAP */
>+    case NVME_REG_PMRCAP:
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
>                        "invalid write to PMRCAP register, ignored");
>         return;
>-    case 0xe04: /* PMRCTL */
>+    case NVME_REG_PMRCTL:
>         if (!NVME_CAP_PMRS(n->bar.cap)) {
>             return;
>         }
>@@ -5899,19 +5899,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>             n->pmr.cmse = false;
>         }
>         return;
>-    case 0xe08: /* PMRSTS */
>+    case NVME_REG_PMRSTS:
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
>                        "invalid write to PMRSTS register, ignored");
>         return;
>-    case 0xe0C: /* PMREBS */
>+    case NVME_REG_PMREBS:
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
>                        "invalid write to PMREBS register, ignored");
>         return;
>-    case 0xe10: /* PMRSWTP */
>+    case NVME_REG_PMRSWTP:
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
>                        "invalid write to PMRSWTP register, ignored");
>         return;
>-    case 0xe14: /* PMRMSCL */
>+    case NVME_REG_PMRMSCL:
>         if (!NVME_CAP_PMRS(n->bar.cap)) {
>             return;
>         }
>@@ -5932,7 +5932,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>         }
>
>         return;
>-    case 0xe18: /* PMRMSCU */
>+    case NVME_REG_PMRMSCU:
>         if (!NVME_CAP_PMRS(n->bar.cap)) {
>             return;
>         }
>@@ -5974,7 +5974,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
>          * from PMRSTS should ensure prior writes
>          * made it to persistent media
>          */
>-        if (addr == 0xe08 &&
>+        if (addr == NVME_REG_PMRSTS &&
>             (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
>             memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
>         }
>-- 
>2.32.0
>
>

LGTM.

Reviewed-by : Gollu Appalanaidu <anaidu.gollu@samsung.com>


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v3 3/5] hw/nvme: fix out-of-bounds reads

[Klaus Jensen]

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

Peter noticed that mmio access may read into the NvmeParams member in
the NvmeCtrl struct.

Fix the bounds check.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8c305315f41c..0449cc4dee9b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5968,23 +5968,26 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
         /* should RAZ, fall through for now */
     }
 
-    if (addr < sizeof(n->bar)) {
-        /*
-         * When PMRWBM bit 1 is set then read from
-         * from PMRSTS should ensure prior writes
-         * made it to persistent media
-         */
-        if (addr == NVME_REG_PMRSTS &&
-            (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
-            memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
-        }
-        memcpy(&val, ptr + addr, size);
-    } else {
+    if (addr > sizeof(n->bar) - size) {
         NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
                        "MMIO read beyond last register,"
                        " offset=0x%"PRIx64", returning 0", addr);
+
+        return 0;
     }
 
+    /*
+     * When PMRWBM bit 1 is set then read from
+     * from PMRSTS should ensure prior writes
+     * made it to persistent media
+     */
+    if (addr == NVME_REG_PMRSTS &&
+        (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
+        memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
+    }
+
+    memcpy(&val, ptr + addr, size);
+
     return val;
 }
 
-- 
2.32.0

Re: [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads

[Stefan Hajnoczi]

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

On Wed, Jul 14, 2021 at 08:01:23AM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Peter noticed that mmio access may read into the NvmeParams member in
> the NvmeCtrl struct.
> 
> Fix the bounds check.
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads

[Peter Maydell]

On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Peter noticed that mmio access may read into the NvmeParams member in
> the NvmeCtrl struct.
>
> Fix the bounds check.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v3 4/5] hw/nvme: fix mmio read

[Klaus Jensen]

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

The new PMR test unearthed a long-standing issue with MMIO reads on
big-endian hosts.

Fix this by unconditionally storing all controller registers in little
endian.

Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 300 ++++++++++++++++++++++++++++---------------------
 1 file changed, 173 insertions(+), 127 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0449cc4dee9b..6d06ed990f2d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -439,10 +439,12 @@ static uint8_t nvme_sq_empty(NvmeSQueue *sq)
 
 static void nvme_irq_check(NvmeCtrl *n)
 {
+    uint32_t intms = ldl_le_p(&n->bar.intms);
+
     if (msix_enabled(&(n->parent_obj))) {
         return;
     }
-    if (~n->bar.intms & n->irq_status) {
+    if (~intms & n->irq_status) {
         pci_irq_assert(&n->parent_obj);
     } else {
         pci_irq_deassert(&n->parent_obj);
@@ -1289,7 +1291,7 @@ static void nvme_post_cqes(void *opaque)
         if (ret) {
             trace_pci_nvme_err_addr_write(addr);
             trace_pci_nvme_err_cfs();
-            n->bar.csts = NVME_CSTS_FAILED;
+            stl_le_p(&n->bar.csts, NVME_CSTS_FAILED);
             break;
         }
         QTAILQ_REMOVE(&cq->req_list, req, entry);
@@ -4022,7 +4024,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req)
         trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
         return NVME_INVALID_QID | NVME_DNR;
     }
-    if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
+    if (unlikely(!qsize || qsize > NVME_CAP_MQES(ldq_le_p(&n->bar.cap)))) {
         trace_pci_nvme_err_invalid_create_sq_size(qsize);
         return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
     }
@@ -4208,7 +4210,7 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
-    switch (NVME_CC_CSS(n->bar.cc)) {
+    switch (NVME_CC_CSS(ldl_le_p(&n->bar.cc))) {
     case NVME_CC_CSS_NVM:
         src_iocs = nvme_cse_iocs_nvm;
         /* fall through */
@@ -4370,7 +4372,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
         trace_pci_nvme_err_invalid_create_cq_cqid(cqid);
         return NVME_INVALID_QID | NVME_DNR;
     }
-    if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
+    if (unlikely(!qsize || qsize > NVME_CAP_MQES(ldq_le_p(&n->bar.cap)))) {
         trace_pci_nvme_err_invalid_create_cq_size(qsize);
         return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
     }
@@ -5163,17 +5165,19 @@ static void nvme_update_dmrsl(NvmeCtrl *n)
 
 static void nvme_select_iocs_ns(NvmeCtrl *n, NvmeNamespace *ns)
 {
+    uint32_t cc = ldl_le_p(&n->bar.cc);
+
     ns->iocs = nvme_cse_iocs_none;
     switch (ns->csi) {
     case NVME_CSI_NVM:
-        if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+        if (NVME_CC_CSS(cc) != NVME_CC_CSS_ADMIN_ONLY) {
             ns->iocs = nvme_cse_iocs_nvm;
         }
         break;
     case NVME_CSI_ZONED:
-        if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_CSI) {
+        if (NVME_CC_CSS(cc) == NVME_CC_CSS_CSI) {
             ns->iocs = nvme_cse_iocs_zoned;
-        } else if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_NVM) {
+        } else if (NVME_CC_CSS(cc) == NVME_CC_CSS_NVM) {
             ns->iocs = nvme_cse_iocs_nvm;
         }
         break;
@@ -5510,7 +5514,7 @@ static void nvme_process_sq(void *opaque)
         if (nvme_addr_read(n, addr, (void *)&cmd, sizeof(cmd))) {
             trace_pci_nvme_err_addr_read(addr);
             trace_pci_nvme_err_cfs();
-            n->bar.csts = NVME_CSTS_FAILED;
+            stl_le_p(&n->bar.csts, NVME_CSTS_FAILED);
             break;
         }
         nvme_inc_sq_head(sq);
@@ -5565,8 +5569,6 @@ static void nvme_ctrl_reset(NvmeCtrl *n)
     n->aer_queued = 0;
     n->outstanding_aers = 0;
     n->qs_created = false;
-
-    n->bar.cc = 0;
 }
 
 static void nvme_ctrl_shutdown(NvmeCtrl *n)
@@ -5605,7 +5607,12 @@ static void nvme_select_iocs(NvmeCtrl *n)
 
 static int nvme_start_ctrl(NvmeCtrl *n)
 {
-    uint32_t page_bits = NVME_CC_MPS(n->bar.cc) + 12;
+    uint64_t cap = ldq_le_p(&n->bar.cap);
+    uint32_t cc = ldl_le_p(&n->bar.cc);
+    uint32_t aqa = ldl_le_p(&n->bar.aqa);
+    uint64_t asq = ldq_le_p(&n->bar.asq);
+    uint64_t acq = ldq_le_p(&n->bar.acq);
+    uint32_t page_bits = NVME_CC_MPS(cc) + 12;
     uint32_t page_size = 1 << page_bits;
 
     if (unlikely(n->cq[0])) {
@@ -5616,73 +5623,72 @@ static int nvme_start_ctrl(NvmeCtrl *n)
         trace_pci_nvme_err_startfail_sq();
         return -1;
     }
-    if (unlikely(!n->bar.asq)) {
+    if (unlikely(!asq)) {
         trace_pci_nvme_err_startfail_nbarasq();
         return -1;
     }
-    if (unlikely(!n->bar.acq)) {
+    if (unlikely(!acq)) {
         trace_pci_nvme_err_startfail_nbaracq();
         return -1;
     }
-    if (unlikely(n->bar.asq & (page_size - 1))) {
-        trace_pci_nvme_err_startfail_asq_misaligned(n->bar.asq);
+    if (unlikely(asq & (page_size - 1))) {
+        trace_pci_nvme_err_startfail_asq_misaligned(asq);
         return -1;
     }
-    if (unlikely(n->bar.acq & (page_size - 1))) {
-        trace_pci_nvme_err_startfail_acq_misaligned(n->bar.acq);
+    if (unlikely(acq & (page_size - 1))) {
+        trace_pci_nvme_err_startfail_acq_misaligned(acq);
         return -1;
     }
-    if (unlikely(!(NVME_CAP_CSS(n->bar.cap) & (1 << NVME_CC_CSS(n->bar.cc))))) {
-        trace_pci_nvme_err_startfail_css(NVME_CC_CSS(n->bar.cc));
+    if (unlikely(!(NVME_CAP_CSS(cap) & (1 << NVME_CC_CSS(cc))))) {
+        trace_pci_nvme_err_startfail_css(NVME_CC_CSS(cc));
         return -1;
     }
-    if (unlikely(NVME_CC_MPS(n->bar.cc) <
-                 NVME_CAP_MPSMIN(n->bar.cap))) {
+    if (unlikely(NVME_CC_MPS(cc) < NVME_CAP_MPSMIN(cap))) {
         trace_pci_nvme_err_startfail_page_too_small(
-                    NVME_CC_MPS(n->bar.cc),
-                    NVME_CAP_MPSMIN(n->bar.cap));
+                    NVME_CC_MPS(cc),
+                    NVME_CAP_MPSMIN(cap));
         return -1;
     }
-    if (unlikely(NVME_CC_MPS(n->bar.cc) >
-                 NVME_CAP_MPSMAX(n->bar.cap))) {
+    if (unlikely(NVME_CC_MPS(cc) >
+                 NVME_CAP_MPSMAX(cap))) {
         trace_pci_nvme_err_startfail_page_too_large(
-                    NVME_CC_MPS(n->bar.cc),
-                    NVME_CAP_MPSMAX(n->bar.cap));
+                    NVME_CC_MPS(cc),
+                    NVME_CAP_MPSMAX(cap));
         return -1;
     }
-    if (unlikely(NVME_CC_IOCQES(n->bar.cc) <
+    if (unlikely(NVME_CC_IOCQES(cc) <
                  NVME_CTRL_CQES_MIN(n->id_ctrl.cqes))) {
         trace_pci_nvme_err_startfail_cqent_too_small(
-                    NVME_CC_IOCQES(n->bar.cc),
-                    NVME_CTRL_CQES_MIN(n->bar.cap));
+                    NVME_CC_IOCQES(cc),
+                    NVME_CTRL_CQES_MIN(cap));
         return -1;
     }
-    if (unlikely(NVME_CC_IOCQES(n->bar.cc) >
+    if (unlikely(NVME_CC_IOCQES(cc) >
                  NVME_CTRL_CQES_MAX(n->id_ctrl.cqes))) {
         trace_pci_nvme_err_startfail_cqent_too_large(
-                    NVME_CC_IOCQES(n->bar.cc),
-                    NVME_CTRL_CQES_MAX(n->bar.cap));
+                    NVME_CC_IOCQES(cc),
+                    NVME_CTRL_CQES_MAX(cap));
         return -1;
     }
-    if (unlikely(NVME_CC_IOSQES(n->bar.cc) <
+    if (unlikely(NVME_CC_IOSQES(cc) <
                  NVME_CTRL_SQES_MIN(n->id_ctrl.sqes))) {
         trace_pci_nvme_err_startfail_sqent_too_small(
-                    NVME_CC_IOSQES(n->bar.cc),
-                    NVME_CTRL_SQES_MIN(n->bar.cap));
+                    NVME_CC_IOSQES(cc),
+                    NVME_CTRL_SQES_MIN(cap));
         return -1;
     }
-    if (unlikely(NVME_CC_IOSQES(n->bar.cc) >
+    if (unlikely(NVME_CC_IOSQES(cc) >
                  NVME_CTRL_SQES_MAX(n->id_ctrl.sqes))) {
         trace_pci_nvme_err_startfail_sqent_too_large(
-                    NVME_CC_IOSQES(n->bar.cc),
-                    NVME_CTRL_SQES_MAX(n->bar.cap));
+                    NVME_CC_IOSQES(cc),
+                    NVME_CTRL_SQES_MAX(cap));
         return -1;
     }
-    if (unlikely(!NVME_AQA_ASQS(n->bar.aqa))) {
+    if (unlikely(!NVME_AQA_ASQS(aqa))) {
         trace_pci_nvme_err_startfail_asqent_sz_zero();
         return -1;
     }
-    if (unlikely(!NVME_AQA_ACQS(n->bar.aqa))) {
+    if (unlikely(!NVME_AQA_ACQS(aqa))) {
         trace_pci_nvme_err_startfail_acqent_sz_zero();
         return -1;
     }
@@ -5690,12 +5696,10 @@ static int nvme_start_ctrl(NvmeCtrl *n)
     n->page_bits = page_bits;
     n->page_size = page_size;
     n->max_prp_ents = n->page_size / sizeof(uint64_t);
-    n->cqe_size = 1 << NVME_CC_IOCQES(n->bar.cc);
-    n->sqe_size = 1 << NVME_CC_IOSQES(n->bar.cc);
-    nvme_init_cq(&n->admin_cq, n, n->bar.acq, 0, 0,
-                 NVME_AQA_ACQS(n->bar.aqa) + 1, 1);
-    nvme_init_sq(&n->admin_sq, n, n->bar.asq, 0, 0,
-                 NVME_AQA_ASQS(n->bar.aqa) + 1);
+    n->cqe_size = 1 << NVME_CC_IOCQES(cc);
+    n->sqe_size = 1 << NVME_CC_IOSQES(cc);
+    nvme_init_cq(&n->admin_cq, n, acq, 0, 0, NVME_AQA_ACQS(aqa) + 1, 1);
+    nvme_init_sq(&n->admin_sq, n, asq, 0, 0, NVME_AQA_ASQS(aqa) + 1);
 
     nvme_set_timestamp(n, 0ULL);
 
@@ -5708,22 +5712,38 @@ 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);
+    uint32_t cmbloc = 0, cmbsz = 0;
 
-    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);
+    NVME_CMBLOC_SET_CDPCILS(cmbloc, 1);
+    NVME_CMBLOC_SET_CDPMLS(cmbloc, 1);
+    NVME_CMBLOC_SET_BIR(cmbloc, NVME_CMB_BIR);
+    stl_le_p(&n->bar.cmbloc, cmbloc);
+
+    NVME_CMBSZ_SET_SQS(cmbsz, 1);
+    NVME_CMBSZ_SET_CQS(cmbsz, 0);
+    NVME_CMBSZ_SET_LISTS(cmbsz, 1);
+    NVME_CMBSZ_SET_RDS(cmbsz, 1);
+    NVME_CMBSZ_SET_WDS(cmbsz, 1);
+    NVME_CMBSZ_SET_SZU(cmbsz, 2); /* MBs */
+    NVME_CMBSZ_SET_SZ(cmbsz, n->params.cmb_size_mb);
+    stl_le_p(&n->bar.cmbsz, cmbsz);
 }
 
 static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                            unsigned size)
 {
+    uint64_t cap = ldq_le_p(&n->bar.cap);
+    uint32_t cc = ldl_le_p(&n->bar.cc);
+    uint32_t intms = ldl_le_p(&n->bar.intms);
+    uint32_t csts = ldl_le_p(&n->bar.csts);
+    uint64_t asq = ldq_le_p(&n->bar.asq);
+    uint64_t acq = ldq_le_p(&n->bar.acq);
+    uint64_t cmbmsc = ldq_le_p(&n->bar.cmbmsc);
+    uint32_t cmbsts = ldl_le_p(&n->bar.cmbsts);
+    uint32_t pmrsts = ldl_le_p(&n->bar.pmrsts);
+    uint32_t pmrmscl = ldl_le_p(&n->bar.pmrmscl);
+    uint32_t pmrmscu = ldl_le_p(&n->bar.pmrmscu);
+
     if (unlikely(offset & (sizeof(uint32_t) - 1))) {
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_misaligned32,
                        "MMIO write not 32-bit aligned,"
@@ -5747,9 +5767,10 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                            " when MSI-X is enabled");
             /* should be ignored, fall through for now */
         }
-        n->bar.intms |= data & 0xffffffff;
+        intms |= data & 0xffffffff;
+        stl_le_p(&n->bar.intms, intms);
         n->bar.intmc = n->bar.intms;
-        trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
+        trace_pci_nvme_mmio_intm_set(data & 0xffffffff, intms);
         nvme_irq_check(n);
         break;
     case NVME_REG_INTMC:
@@ -5759,44 +5780,55 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                            " when MSI-X is enabled");
             /* should be ignored, fall through for now */
         }
-        n->bar.intms &= ~(data & 0xffffffff);
+        intms &= ~(data & 0xffffffff);
+        stl_le_p(&n->bar.intms, intms);
         n->bar.intmc = n->bar.intms;
-        trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
+        trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, intms);
         nvme_irq_check(n);
         break;
     case NVME_REG_CC:
         trace_pci_nvme_mmio_cfg(data & 0xffffffff);
+
         /* Windows first sends data, then sends enable bit */
-        if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
-            !NVME_CC_SHN(data) && !NVME_CC_SHN(n->bar.cc))
+        if (!NVME_CC_EN(data) && !NVME_CC_EN(cc) &&
+            !NVME_CC_SHN(data) && !NVME_CC_SHN(cc))
         {
-            n->bar.cc = data;
+            cc = data;
         }
 
-        if (NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc)) {
-            n->bar.cc = data;
+        if (NVME_CC_EN(data) && !NVME_CC_EN(cc)) {
+            cc = data;
+
+            /* flush CC since nvme_start_ctrl() needs the value */
+            stl_le_p(&n->bar.cc, cc);
             if (unlikely(nvme_start_ctrl(n))) {
                 trace_pci_nvme_err_startfail();
-                n->bar.csts = NVME_CSTS_FAILED;
+                csts = NVME_CSTS_FAILED;
             } else {
                 trace_pci_nvme_mmio_start_success();
-                n->bar.csts = NVME_CSTS_READY;
+                csts = NVME_CSTS_READY;
             }
-        } else if (!NVME_CC_EN(data) && NVME_CC_EN(n->bar.cc)) {
+        } else if (!NVME_CC_EN(data) && NVME_CC_EN(cc)) {
             trace_pci_nvme_mmio_stopped();
             nvme_ctrl_reset(n);
-            n->bar.csts &= ~NVME_CSTS_READY;
+            cc = 0;
+            csts &= ~NVME_CSTS_READY;
         }
-        if (NVME_CC_SHN(data) && !(NVME_CC_SHN(n->bar.cc))) {
+
+        if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) {
             trace_pci_nvme_mmio_shutdown_set();
             nvme_ctrl_shutdown(n);
-            n->bar.cc = data;
-            n->bar.csts |= NVME_CSTS_SHST_COMPLETE;
-        } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(n->bar.cc)) {
+            cc = data;
+            csts |= NVME_CSTS_SHST_COMPLETE;
+        } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) {
             trace_pci_nvme_mmio_shutdown_cleared();
-            n->bar.csts &= ~NVME_CSTS_SHST_COMPLETE;
-            n->bar.cc = data;
+            csts &= ~NVME_CSTS_SHST_COMPLETE;
+            cc = data;
         }
+
+        stl_le_p(&n->bar.cc, cc);
+        stl_le_p(&n->bar.csts, csts);
+
         break;
     case NVME_REG_CSTS:
         if (data & (1 << 4)) {
@@ -5818,26 +5850,30 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         }
         break;
     case NVME_REG_AQA:
-        n->bar.aqa = data & 0xffffffff;
+        stl_le_p(&n->bar.aqa, data & 0xffffffff);
         trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
         break;
     case NVME_REG_ASQ:
-        n->bar.asq = size == 8 ? data :
-            (n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff);
+        asq = size == 8 ? data :
+            (asq & ~0xffffffffULL) | (data & 0xffffffff);
+        stq_le_p(&n->bar.asq, asq);
         trace_pci_nvme_mmio_asqaddr(data);
         break;
     case NVME_REG_ASQ + 4:
-        n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
-        trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
+        asq = (asq & 0xffffffff) | (data << 32);
+        stq_le_p(&n->bar.asq, asq);
+        trace_pci_nvme_mmio_asqaddr_hi(data, asq);
         break;
     case NVME_REG_ACQ:
         trace_pci_nvme_mmio_acqaddr(data);
-        n->bar.acq = size == 8 ? data :
-            (n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff);
+        acq = size == 8 ? data :
+            (acq & ~0xffffffffULL) | (data & 0xffffffff);
+        stq_le_p(&n->bar.acq, acq);
         break;
     case NVME_REG_ACQ + 4:
-        n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
-        trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
+        acq = (acq & 0xffffffff) | (data << 32);
+        stq_le_p(&n->bar.acq, acq);
+        trace_pci_nvme_mmio_acqaddr_hi(data, acq);
         break;
     case NVME_REG_CMBLOC:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
@@ -5849,12 +5885,13 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                        "invalid write to read only CMBSZ, ignored");
         return;
     case NVME_REG_CMBMSC:
-        if (!NVME_CAP_CMBS(n->bar.cap)) {
+        if (!NVME_CAP_CMBS(cap)) {
             return;
         }
 
-        n->bar.cmbmsc = size == 8 ? data :
-            (n->bar.cmbmsc & ~0xffffffff) | (data & 0xffffffff);
+        cmbmsc = size == 8 ? data :
+            (cmbmsc & ~0xffffffff) | (data & 0xffffffff);
+        stq_le_p(&n->bar.cmbmsc, cmbmsc);
         n->cmb.cmse = false;
 
         if (NVME_CMBMSC_CRE(data)) {
@@ -5863,7 +5900,8 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             if (NVME_CMBMSC_CMSE(data)) {
                 hwaddr cba = NVME_CMBMSC_CBA(data) << CMBMSC_CBA_SHIFT;
                 if (cba + int128_get64(n->cmb.mem.size) < cba) {
-                    NVME_CMBSTS_SET_CBAI(n->bar.cmbsts, 1);
+                    NVME_CMBSTS_SET_CBAI(cmbsts, 1);
+                    stl_le_p(&n->bar.cmbsts, cmbsts);
                     return;
                 }
 
@@ -5877,7 +5915,8 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
 
         return;
     case NVME_REG_CMBMSC + 4:
-        n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
+        cmbmsc = (cmbmsc & 0xffffffff) | (data << 32);
+        stq_le_p(&n->bar.cmbmsc, cmbmsc);
         return;
 
     case NVME_REG_PMRCAP:
@@ -5885,19 +5924,20 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                        "invalid write to PMRCAP register, ignored");
         return;
     case NVME_REG_PMRCTL:
-        if (!NVME_CAP_PMRS(n->bar.cap)) {
+        if (!NVME_CAP_PMRS(cap)) {
             return;
         }
 
-        n->bar.pmrctl = data;
+        stl_le_p(&n->bar.pmrctl, data & 0xffffffff);
         if (NVME_PMRCTL_EN(data)) {
             memory_region_set_enabled(&n->pmr.dev->mr, true);
-            n->bar.pmrsts = 0;
+            pmrsts = 0;
         } else {
             memory_region_set_enabled(&n->pmr.dev->mr, false);
-            NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 1);
+            NVME_PMRSTS_SET_NRDY(pmrsts, 1);
             n->pmr.cmse = false;
         }
+        stl_le_p(&n->bar.pmrsts, pmrsts);
         return;
     case NVME_REG_PMRSTS:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
@@ -5912,18 +5952,20 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
                        "invalid write to PMRSWTP register, ignored");
         return;
     case NVME_REG_PMRMSCL:
-        if (!NVME_CAP_PMRS(n->bar.cap)) {
+        if (!NVME_CAP_PMRS(cap)) {
             return;
         }
 
-        n->bar.pmrmscl = data & 0xffffffff;
+        pmrmscl = data & 0xffffffff;
+        stl_le_p(&n->bar.pmrmscl, pmrmscl);
         n->pmr.cmse = false;
 
-        if (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
-            hwaddr cba = n->bar.pmrmscu |
-                (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
+        if (NVME_PMRMSCL_CMSE(data)) {
+            hwaddr cba = pmrmscu |
+                (NVME_PMRMSCL_CBA(pmrmscl) << PMRMSCL_CBA_SHIFT);
             if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
-                NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
+                NVME_PMRSTS_SET_CBAI(pmrsts, 1);
+                stl_le_p(&n->bar.pmrsts, pmrsts);
                 return;
             }
 
@@ -5933,11 +5975,11 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
 
         return;
     case NVME_REG_PMRMSCU:
-        if (!NVME_CAP_PMRS(n->bar.cap)) {
+        if (!NVME_CAP_PMRS(cap)) {
             return;
         }
 
-        n->bar.pmrmscu = data & 0xffffffff;
+        stl_le_p(&n->bar.pmrmscu, data & 0xffffffff);
         return;
     default:
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
@@ -5952,7 +5994,6 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
     uint8_t *ptr = (uint8_t *)&n->bar;
-    uint64_t val = 0;
 
     trace_pci_nvme_mmio_read(addr, size);
 
@@ -5982,13 +6023,11 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
      * made it to persistent media
      */
     if (addr == NVME_REG_PMRSTS &&
-        (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
+        (NVME_PMRCAP_PMRWBM(ldl_le_p(&n->bar.pmrcap)) & 0x02)) {
         memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
     }
 
-    memcpy(&val, ptr + addr, size);
-
-    return val;
+    return ldn_le_p(ptr + addr, size);
 }
 
 static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
@@ -6246,6 +6285,7 @@ static void nvme_init_state(NvmeCtrl *n)
 static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
 {
     uint64_t cmb_size = n->params.cmb_size_mb * MiB;
+    uint64_t cap = ldq_le_p(&n->bar.cap);
 
     n->cmb.buf = g_malloc0(cmb_size);
     memory_region_init_io(&n->cmb.mem, OBJECT(n), &nvme_cmb_ops, n,
@@ -6255,7 +6295,8 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
                      PCI_BASE_ADDRESS_MEM_TYPE_64 |
                      PCI_BASE_ADDRESS_MEM_PREFETCH, &n->cmb.mem);
 
-    NVME_CAP_SET_CMBS(n->bar.cap, 1);
+    NVME_CAP_SET_CMBS(cap, 1);
+    stq_le_p(&n->bar.cap, cap);
 
     if (n->params.legacy_cmb) {
         nvme_cmb_enable_regs(n);
@@ -6265,14 +6306,17 @@ 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);
-    /* Turn on bit 1 support */
-    NVME_PMRCAP_SET_PMRWBM(n->bar.pmrcap, 0x02);
-    NVME_PMRCAP_SET_CMSS(n->bar.pmrcap, 1);
+    uint32_t pmrcap = 0;
 
-    pci_register_bar(pci_dev, NVME_PMRCAP_BIR(n->bar.pmrcap),
+    NVME_PMRCAP_SET_RDS(pmrcap, 1);
+    NVME_PMRCAP_SET_WDS(pmrcap, 1);
+    NVME_PMRCAP_SET_BIR(pmrcap, NVME_PMR_BIR);
+    /* Turn on bit 1 support */
+    NVME_PMRCAP_SET_PMRWBM(pmrcap, 0x02);
+    NVME_PMRCAP_SET_CMSS(pmrcap, 1);
+    stl_le_p(&n->bar.pmrcap, pmrcap);
+
+    pci_register_bar(pci_dev, NVME_PMR_BIR,
                      PCI_BASE_ADDRESS_SPACE_MEMORY |
                      PCI_BASE_ADDRESS_MEM_TYPE_64 |
                      PCI_BASE_ADDRESS_MEM_PREFETCH, &n->pmr.dev->mr);
@@ -6362,6 +6406,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
 {
     NvmeIdCtrl *id = &n->id_ctrl;
     uint8_t *pci_conf = pci_dev->config;
+    uint64_t cap = 0;
 
     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));
@@ -6440,17 +6485,18 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
         id->cmic |= NVME_CMIC_MULTI_CTRL;
     }
 
-    NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
-    NVME_CAP_SET_CQR(n->bar.cap, 1);
-    NVME_CAP_SET_TO(n->bar.cap, 0xf);
-    NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_NVM);
-    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);
-    NVME_CAP_SET_PMRS(n->bar.cap, n->pmr.dev ? 1 : 0);
+    NVME_CAP_SET_MQES(cap, 0x7ff);
+    NVME_CAP_SET_CQR(cap, 1);
+    NVME_CAP_SET_TO(cap, 0xf);
+    NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_NVM);
+    NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_CSI_SUPP);
+    NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_ADMIN_ONLY);
+    NVME_CAP_SET_MPSMAX(cap, 4);
+    NVME_CAP_SET_CMBS(cap, n->params.cmb_size_mb ? 1 : 0);
+    NVME_CAP_SET_PMRS(cap, n->pmr.dev ? 1 : 0);
+    stq_le_p(&n->bar.cap, cap);
 
-    n->bar.vs = NVME_SPEC_VER;
+    stl_le_p(&n->bar.vs, NVME_SPEC_VER);
     n->bar.intmc = n->bar.intms = 0;
 }
 
@@ -6601,7 +6647,7 @@ static void nvme_set_smart_warning(Object *obj, Visitor *v, const char *name,
 
     cap = NVME_SMART_SPARE | NVME_SMART_TEMPERATURE | NVME_SMART_RELIABILITY
           | NVME_SMART_MEDIA_READ_ONLY | NVME_SMART_FAILED_VOLATILE_MEDIA;
-    if (NVME_CAP_PMRS(n->bar.cap)) {
+    if (NVME_CAP_PMRS(ldq_le_p(&n->bar.cap))) {
         cap |= NVME_SMART_PMR_UNRELIABLE;
     }
 
-- 
2.32.0

Re: [PATCH v3 4/5] hw/nvme: fix mmio read

[Peter Maydell]

On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The new PMR test unearthed a long-standing issue with MMIO reads on
> big-endian hosts.
>
> Fix this by unconditionally storing all controller registers in little
> endian.
>
> Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 300 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 173 insertions(+), 127 deletions(-)

> @@ -5708,22 +5712,38 @@ 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);
> +    uint32_t cmbloc = 0, cmbsz = 0;
>
> -    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);
> +    NVME_CMBLOC_SET_CDPCILS(cmbloc, 1);
> +    NVME_CMBLOC_SET_CDPMLS(cmbloc, 1);
> +    NVME_CMBLOC_SET_BIR(cmbloc, NVME_CMB_BIR);
> +    stl_le_p(&n->bar.cmbloc, cmbloc);
> +
> +    NVME_CMBSZ_SET_SQS(cmbsz, 1);
> +    NVME_CMBSZ_SET_CQS(cmbsz, 0);
> +    NVME_CMBSZ_SET_LISTS(cmbsz, 1);
> +    NVME_CMBSZ_SET_RDS(cmbsz, 1);
> +    NVME_CMBSZ_SET_WDS(cmbsz, 1);
> +    NVME_CMBSZ_SET_SZU(cmbsz, 2); /* MBs */
> +    NVME_CMBSZ_SET_SZ(cmbsz, n->params.cmb_size_mb);
> +    stl_le_p(&n->bar.cmbsz, cmbsz);

This used to only set the listed fields and left the
rest of the registers untouched. Now it zeroes the other
fields in the registers. If this is an intentional change it
should be separate from this patch, I think.

> @@ -5747,9 +5767,10 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>                             " when MSI-X is enabled");
>              /* should be ignored, fall through for now */
>          }
> -        n->bar.intms |= data & 0xffffffff;
> +        intms |= data & 0xffffffff;

intms is a uint32_t so the & here is unnecessary; you can just
say "intms |= data;".

> +        stl_le_p(&n->bar.intms, intms);
>          n->bar.intmc = n->bar.intms;
> -        trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
> +        trace_pci_nvme_mmio_intm_set(data & 0xffffffff, intms);
>          nvme_irq_check(n);
>          break;
>      case NVME_REG_INTMC:
> @@ -5759,44 +5780,55 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>                             " when MSI-X is enabled");
>              /* should be ignored, fall through for now */
>          }
> -        n->bar.intms &= ~(data & 0xffffffff);
> +        intms &= ~(data & 0xffffffff);

Similarly here just "intms &= ~data;" is enough.

> +        stl_le_p(&n->bar.intms, intms);
>          n->bar.intmc = n->bar.intms;
> -        trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
> +        trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, intms);
>          nvme_irq_check(n);
>          break;

> @@ -5818,26 +5850,30 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>          }
>          break;
>      case NVME_REG_AQA:
> -        n->bar.aqa = data & 0xffffffff;
> +        stl_le_p(&n->bar.aqa, data & 0xffffffff);

You don't need to mask here, stl_le_p() takes a uint32_t argument and
will only write 4 bytes.

>          trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
>          break;
>      case NVME_REG_ASQ:
> -        n->bar.asq = size == 8 ? data :
> -            (n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff);
> +        asq = size == 8 ? data :
> +            (asq & ~0xffffffffULL) | (data & 0xffffffff);
> +        stq_le_p(&n->bar.asq, asq);

You could save doing the manual assembly of the 64-byte value and just write
the data you have:

           stn_le_p(&n->bar.asq, size, data);

>          trace_pci_nvme_mmio_asqaddr(data);
>          break;
>      case NVME_REG_ASQ + 4:
> -        n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
> -        trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
> +        asq = (asq & 0xffffffff) | (data << 32);
> +        stq_le_p(&n->bar.asq, asq);
> +        trace_pci_nvme_mmio_asqaddr_hi(data, asq);

Similarly here
           stn_le_p(&n->bar.asq + 4, size, data);
           trace_pci_nvme_mmio_asqaddr_hi(data, ldq_le_p(&n->bar.asq));

(and then you don't need 'asq' as a local variable in this function.)

>          break;
>      case NVME_REG_ACQ:
>          trace_pci_nvme_mmio_acqaddr(data);
> -        n->bar.acq = size == 8 ? data :
> -            (n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff);
> +        acq = size == 8 ? data :
> +            (acq & ~0xffffffffULL) | (data & 0xffffffff);
> +        stq_le_p(&n->bar.acq, acq);
>          break;
>      case NVME_REG_ACQ + 4:
> -        n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
> -        trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
> +        acq = (acq & 0xffffffff) | (data << 32);
> +        stq_le_p(&n->bar.acq, acq);
> +        trace_pci_nvme_mmio_acqaddr_hi(data, acq);
>          break;

Same remarks as for ASQ apply here for ACQ.

>      case NVME_REG_CMBLOC:
>          NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
> @@ -5849,12 +5885,13 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>                         "invalid write to read only CMBSZ, ignored");
>          return;
>      case NVME_REG_CMBMSC:
> -        if (!NVME_CAP_CMBS(n->bar.cap)) {
> +        if (!NVME_CAP_CMBS(cap)) {
>              return;
>          }
>
> -        n->bar.cmbmsc = size == 8 ? data :
> -            (n->bar.cmbmsc & ~0xffffffff) | (data & 0xffffffff);
> +        cmbmsc = size == 8 ? data :
> +            (cmbmsc & ~0xffffffff) | (data & 0xffffffff);
> +        stq_le_p(&n->bar.cmbmsc, cmbmsc);
>          n->cmb.cmse = false;

and for CMBMSC

>
>          if (NVME_CMBMSC_CRE(data)) {
> @@ -5863,7 +5900,8 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>              if (NVME_CMBMSC_CMSE(data)) {
>                  hwaddr cba = NVME_CMBMSC_CBA(data) << CMBMSC_CBA_SHIFT;
>                  if (cba + int128_get64(n->cmb.mem.size) < cba) {
> -                    NVME_CMBSTS_SET_CBAI(n->bar.cmbsts, 1);
> +                    NVME_CMBSTS_SET_CBAI(cmbsts, 1);
> +                    stl_le_p(&n->bar.cmbsts, cmbsts);
>                      return;
>                  }
>
> @@ -5877,7 +5915,8 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>
>          return;
>      case NVME_REG_CMBMSC + 4:
> -        n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
> +        cmbmsc = (cmbmsc & 0xffffffff) | (data << 32);
> +        stq_le_p(&n->bar.cmbmsc, cmbmsc);

>          return;
>
>      case NVME_REG_PMRCAP:
> @@ -5885,19 +5924,20 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>                         "invalid write to PMRCAP register, ignored");
>          return;
>      case NVME_REG_PMRCTL:
> -        if (!NVME_CAP_PMRS(n->bar.cap)) {
> +        if (!NVME_CAP_PMRS(cap)) {
>              return;
>          }
>
> -        n->bar.pmrctl = data;
> +        stl_le_p(&n->bar.pmrctl, data & 0xffffffff);

More unnecessary masking

>          if (NVME_PMRCTL_EN(data)) {
>              memory_region_set_enabled(&n->pmr.dev->mr, true);
> -            n->bar.pmrsts = 0;
> +            pmrsts = 0;
>          } else {
>              memory_region_set_enabled(&n->pmr.dev->mr, false);
> -            NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 1);
> +            NVME_PMRSTS_SET_NRDY(pmrsts, 1);
>              n->pmr.cmse = false;
>          }
> +        stl_le_p(&n->bar.pmrsts, pmrsts);
>          return;
>      case NVME_REG_PMRSTS:
>          NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
> @@ -5912,18 +5952,20 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>                         "invalid write to PMRSWTP register, ignored");
>          return;
>      case NVME_REG_PMRMSCL:
> -        if (!NVME_CAP_PMRS(n->bar.cap)) {
> +        if (!NVME_CAP_PMRS(cap)) {
>              return;
>          }
>
> -        n->bar.pmrmscl = data & 0xffffffff;
> +        pmrmscl = data & 0xffffffff;

pmrmscl is a uint32_t so the mask is unneeded

> +        stl_le_p(&n->bar.pmrmscl, pmrmscl);
>          n->pmr.cmse = false;
>
> -        if (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
> -            hwaddr cba = n->bar.pmrmscu |
> -                (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
> +        if (NVME_PMRMSCL_CMSE(data)) {
> +            hwaddr cba = pmrmscu |
> +                (NVME_PMRMSCL_CBA(pmrmscl) << PMRMSCL_CBA_SHIFT);
>              if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
> -                NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
> +                NVME_PMRSTS_SET_CBAI(pmrsts, 1);
> +                stl_le_p(&n->bar.pmrsts, pmrsts);
>                  return;
>              }
>
> @@ -5933,11 +5975,11 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>
>          return;
>      case NVME_REG_PMRMSCU:
> -        if (!NVME_CAP_PMRS(n->bar.cap)) {
> +        if (!NVME_CAP_PMRS(cap)) {
>              return;
>          }
>
> -        n->bar.pmrmscu = data & 0xffffffff;
> +        stl_le_p(&n->bar.pmrmscu, data & 0xffffffff);

Unneeded mask

>          return;
>      default:
>          NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,

> @@ -6265,14 +6306,17 @@ 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);
> -    /* Turn on bit 1 support */
> -    NVME_PMRCAP_SET_PMRWBM(n->bar.pmrcap, 0x02);
> -    NVME_PMRCAP_SET_CMSS(n->bar.pmrcap, 1);
> +    uint32_t pmrcap = 0;
>
> -    pci_register_bar(pci_dev, NVME_PMRCAP_BIR(n->bar.pmrcap),
> +    NVME_PMRCAP_SET_RDS(pmrcap, 1);
> +    NVME_PMRCAP_SET_WDS(pmrcap, 1);
> +    NVME_PMRCAP_SET_BIR(pmrcap, NVME_PMR_BIR);
> +    /* Turn on bit 1 support */
> +    NVME_PMRCAP_SET_PMRWBM(pmrcap, 0x02);
> +    NVME_PMRCAP_SET_CMSS(pmrcap, 1);
> +    stl_le_p(&n->bar.pmrcap, pmrcap);
> +
> +    pci_register_bar(pci_dev, NVME_PMR_BIR,
>                       PCI_BASE_ADDRESS_SPACE_MEMORY |
>                       PCI_BASE_ADDRESS_MEM_TYPE_64 |
>                       PCI_BASE_ADDRESS_MEM_PREFETCH, &n->pmr.dev->mr);

Again, this function is changing from "set these fields" to
"set these fields and zero the rest".

> @@ -6362,6 +6406,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>  {
>      NvmeIdCtrl *id = &n->id_ctrl;
>      uint8_t *pci_conf = pci_dev->config;
> +    uint64_t cap = 0;
>
>      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));
> @@ -6440,17 +6485,18 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>          id->cmic |= NVME_CMIC_MULTI_CTRL;
>      }
>
> -    NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
> -    NVME_CAP_SET_CQR(n->bar.cap, 1);
> -    NVME_CAP_SET_TO(n->bar.cap, 0xf);
> -    NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_NVM);
> -    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);
> -    NVME_CAP_SET_PMRS(n->bar.cap, n->pmr.dev ? 1 : 0);
> +    NVME_CAP_SET_MQES(cap, 0x7ff);
> +    NVME_CAP_SET_CQR(cap, 1);
> +    NVME_CAP_SET_TO(cap, 0xf);
> +    NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_NVM);
> +    NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_CSI_SUPP);
> +    NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_ADMIN_ONLY);
> +    NVME_CAP_SET_MPSMAX(cap, 4);
> +    NVME_CAP_SET_CMBS(cap, n->params.cmb_size_mb ? 1 : 0);
> +    NVME_CAP_SET_PMRS(cap, n->pmr.dev ? 1 : 0);
> +    stq_le_p(&n->bar.cap, cap);

Same here.

> -    n->bar.vs = NVME_SPEC_VER;
> +    stl_le_p(&n->bar.vs, NVME_SPEC_VER);
>      n->bar.intmc = n->bar.intms = 0;
>  }

thanks
-- PMM

[PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test

[Klaus Jensen]

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

Add a regression test for mmio read on big-endian hosts.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 tests/qtest/nvme-test.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tests/qtest/nvme-test.c b/tests/qtest/nvme-test.c
index 47e757d7e2af..f8bafb5d70fb 100644
--- a/tests/qtest/nvme-test.c
+++ b/tests/qtest/nvme-test.c
@@ -67,6 +67,30 @@ static void nvmetest_oob_cmb_test(void *obj, void *data, QGuestAllocator *alloc)
     g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 0x44332211);
 }
 
+static void nvmetest_reg_read_test(void *obj, void *data, QGuestAllocator *alloc)
+{
+    QNvme *nvme = obj;
+    QPCIDevice *pdev = &nvme->dev;
+    QPCIBar bar;
+    uint32_t cap_lo, cap_hi;
+    uint64_t cap;
+
+    qpci_device_enable(pdev);
+    bar = qpci_iomap(pdev, 0, NULL);
+
+    cap_lo = qpci_io_readl(pdev, bar, 0x0);
+    g_assert_cmpint(NVME_CAP_MQES(cap_lo), ==, 0x7ff);
+
+    cap_hi = qpci_io_readl(pdev, bar, 0x4);
+    g_assert_cmpint(NVME_CAP_MPSMAX((uint64_t)cap_hi << 32), ==, 0x4);
+
+    cap = qpci_io_readq(pdev, bar, 0x0);
+    g_assert_cmpint(NVME_CAP_MQES(cap), ==, 0x7ff);
+    g_assert_cmpint(NVME_CAP_MPSMAX(cap), ==, 0x4);
+
+    qpci_iounmap(pdev, bar);
+}
+
 static void nvmetest_pmr_reg_test(void *obj, void *data, QGuestAllocator *alloc)
 {
     QNvme *nvme = obj;
@@ -142,6 +166,8 @@ static void nvme_register_nodes(void)
                  &(QOSGraphTestOptions) {
         .edge.extra_device_opts = "pmrdev=pmr0"
     });
+
+    qos_add_test("reg-read", "nvme", nvmetest_reg_read_test, NULL);
 }
 
 libqos_init(nvme_register_nodes);
-- 
2.32.0

Re: [PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test

[Gollu Appalanaidu]

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

On Wed, Jul 14, 2021 at 08:01:25AM +0200, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>Add a regression test for mmio read on big-endian hosts.
>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> tests/qtest/nvme-test.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
>diff --git a/tests/qtest/nvme-test.c b/tests/qtest/nvme-test.c
>index 47e757d7e2af..f8bafb5d70fb 100644
>--- a/tests/qtest/nvme-test.c
>+++ b/tests/qtest/nvme-test.c
>@@ -67,6 +67,30 @@ static void nvmetest_oob_cmb_test(void *obj, void *data, QGuestAllocator *alloc)
>     g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 0x44332211);
> }
>
>+static void nvmetest_reg_read_test(void *obj, void *data, QGuestAllocator *alloc)
>+{
>+    QNvme *nvme = obj;
>+    QPCIDevice *pdev = &nvme->dev;
>+    QPCIBar bar;
>+    uint32_t cap_lo, cap_hi;
>+    uint64_t cap;
>+
>+    qpci_device_enable(pdev);
>+    bar = qpci_iomap(pdev, 0, NULL);
>+
>+    cap_lo = qpci_io_readl(pdev, bar, 0x0);
>+    g_assert_cmpint(NVME_CAP_MQES(cap_lo), ==, 0x7ff);
>+
>+    cap_hi = qpci_io_readl(pdev, bar, 0x4);
>+    g_assert_cmpint(NVME_CAP_MPSMAX((uint64_t)cap_hi << 32), ==, 0x4);
>+
>+    cap = qpci_io_readq(pdev, bar, 0x0);
>+    g_assert_cmpint(NVME_CAP_MQES(cap), ==, 0x7ff);
>+    g_assert_cmpint(NVME_CAP_MPSMAX(cap), ==, 0x4);
>+
>+    qpci_iounmap(pdev, bar);
>+}
>+
> static void nvmetest_pmr_reg_test(void *obj, void *data, QGuestAllocator *alloc)
> {
>     QNvme *nvme = obj;
>@@ -142,6 +166,8 @@ static void nvme_register_nodes(void)
>                  &(QOSGraphTestOptions) {
>         .edge.extra_device_opts = "pmrdev=pmr0"
>     });
>+
>+    qos_add_test("reg-read", "nvme", nvmetest_reg_read_test, NULL);
> }
>
> libqos_init(nvme_register_nodes);
>-- 
>2.32.0
>
>

Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 0/5] hw/nvme: fix mmio read

[Klaus Jensen]

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

On Jul 14 08:01, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Fix mmio read issues on big-endian hosts. The core issue is that values
> in the BAR is not stored in little endian as required.
> 
> Fix that and add a regression test for this. This required a bit of
> cleanup, so it blew up into a series.
> 
> v2:
> 
>   * "hw/nvme: use symbolic names for registers"
>     Use offsetof(NvmeBar, reg) instead of explicit offsets (Philippe)
> 
>   * "hw/nvme: fix mmio read"
>     Use the st/ld API instead of cpu_to_X (Philippe)
> 
> Klaus Jensen (5):
>   hw/nvme: split pmrmsc register into upper and lower
>   hw/nvme: use symbolic names for registers
>   hw/nvme: fix out-of-bounds reads
>   hw/nvme: fix mmio read
>   tests/qtest/nvme-test: add mmio read test
> 
>  include/block/nvme.h    |  60 +++++--
>  hw/nvme/ctrl.c          | 362 +++++++++++++++++++++++-----------------
>  tests/qtest/nvme-test.c |  26 +++
>  3 files changed, 276 insertions(+), 172 deletions(-)
> 

Oi,

A review on patch 3 and 4 would be appreciated so this has a chance of
reaching Peter for -rc0 :)

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

Re: [PATCH v3 0/5] hw/nvme: fix mmio read

[Stefan Hajnoczi]

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

On Mon, Jul 19, 2021 at 08:43:33AM +0200, Klaus Jensen wrote:
> On Jul 14 08:01, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Fix mmio read issues on big-endian hosts. The core issue is that values
> > in the BAR is not stored in little endian as required.
> > 
> > Fix that and add a regression test for this. This required a bit of
> > cleanup, so it blew up into a series.
> > 
> > v2:
> > 
> >   * "hw/nvme: use symbolic names for registers"
> >     Use offsetof(NvmeBar, reg) instead of explicit offsets (Philippe)
> > 
> >   * "hw/nvme: fix mmio read"
> >     Use the st/ld API instead of cpu_to_X (Philippe)
> > 
> > Klaus Jensen (5):
> >   hw/nvme: split pmrmsc register into upper and lower
> >   hw/nvme: use symbolic names for registers
> >   hw/nvme: fix out-of-bounds reads
> >   hw/nvme: fix mmio read
> >   tests/qtest/nvme-test: add mmio read test
> > 
> >  include/block/nvme.h    |  60 +++++--
> >  hw/nvme/ctrl.c          | 362 +++++++++++++++++++++++-----------------
> >  tests/qtest/nvme-test.c |  26 +++
> >  3 files changed, 276 insertions(+), 172 deletions(-)
> > 
> 
> Oi,
> 
> A review on patch 3 and 4 would be appreciated so this has a chance of
> reaching Peter for -rc0 :)

I have reviewed Patch 3. Unfortunately I don't have time to review the
rest right now but maybe you can ask a specific person on the CC list to
review other patches. A Reply-All to multiple people might not receive
any attention :).

Stefan

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