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

[Klaus Jensen]

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.

signature.asc
Description: PGP signature