diff mbox series

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

Message ID 20210714060125.994882-2-its@irrelevant.dk
State New
Headers show
Series hw/nvme: fix mmio read | expand

Commit Message

Klaus Jensen July 14, 2021, 6:01 a.m. UTC 
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(-)

Comments

Gollu Appalanaidu July 14, 2021, 11:35 a.m. UTC | #1 
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>
Peter Maydell July 19, 2021, 9:13 a.m. UTC | #2 
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
Klaus Jensen July 19, 2021, 9:32 a.m. UTC | #3 
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.
diff mbox series

Patch

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,