Re: [PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register

From: Klaus Jensen <its@irrelevant.dk>
To: Maxim Levitsky <mlevitsk@redhat.com>
Cc: kwolf@redhat.com, qemu-block@nongnu.org,
	Klaus Jensen <k.jensen@samsung.com>,
	qemu-devel@nongnu.org, mreitz@redhat.com,
	Andrzej Jakowski <andrzej.jakowski@linux.intel.com>,
	kbusch@kernel.org
Subject: Re: [PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register
Date: Tue, 7 Jul 2020 21:15:59 +0200	[thread overview]
Message-ID: <20200707191559.GA669353@apples.localdomain> (raw)
In-Reply-To: <a483e0a935a9c2d47232a9f7e653e950de525e68.camel@redhat.com>

On Jul  7 19:27, Maxim Levitsky wrote:
> On Wed, 2020-07-01 at 14:48 -0700, Andrzej Jakowski wrote:
> > This patch sets CMBS bit in controller capabilities register when user
> > configures NVMe driver with CMB support, so capabilites are correctly
> > reported to guest OS.
> > 
> > Signed-off-by: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c      | 2 +-
> >  include/block/nvme.h | 6 +++++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 1aee042d4c..9f11f3e9da 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1582,6 +1582,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> >      NVME_CAP_SET_TO(n->bar.cap, 0xf);
> >      NVME_CAP_SET_CSS(n->bar.cap, 1);
> >      NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
> > +    NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
> >  
> >      n->bar.vs = 0x00010200;
> >      n->bar.intmc = n->bar.intms = 0;
> > @@ -1591,7 +1592,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >  {
> >      NvmeCtrl *n = NVME(pci_dev);
> >      Error *local_err = NULL;
> > -
> >      int i;
> >  
> >      nvme_check_constraints(n, &local_err);
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index 1720ee1d51..14cf398dfa 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -35,6 +35,7 @@ enum NvmeCapShift {
> >      CAP_MPSMIN_SHIFT   = 48,
> >      CAP_MPSMAX_SHIFT   = 52,
> >      CAP_PMR_SHIFT      = 56,
> > +    CAP_CMB_SHIFT      = 57,
> >  };
> >  
> >  enum NvmeCapMask {
> > @@ -48,6 +49,7 @@ enum NvmeCapMask {
> >      CAP_MPSMIN_MASK    = 0xf,
> >      CAP_MPSMAX_MASK    = 0xf,
> >      CAP_PMR_MASK       = 0x1,
> > +    CAP_CMB_MASK       = 0x1,
> >  };
> >  
> >  #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
> > @@ -78,8 +80,10 @@ enum NvmeCapMask {
> >                                                             << CAP_MPSMIN_SHIFT)
> >  #define NVME_CAP_SET_MPSMAX(cap, val) (cap |= (uint64_t)(val & CAP_MPSMAX_MASK)\
> >                                                              << CAP_MPSMAX_SHIFT)
> > -#define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)\
> > +#define NVME_CAP_SET_PMRS(cap, val)   (cap |= (uint64_t)(val & CAP_PMR_MASK)   \
> >                                                              << CAP_PMR_SHIFT)
> > +#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMB_MASK)   \
> > +                                                           << CAP_CMB_SHIFT)
> >  
> >  enum NvmeCcShift {
> >      CC_EN_SHIFT     = 0,
> 
> 
> I wonder how this could have beeing forgotten. Hmm.
> I see that Linux kernel uses CMBSZ != for that.
> I guess this explains it.
> 
> Reviewed-by: Maxim Levitsky <mlevitsky@gmail.com>
> 

It is a v1.4 field. The CMB support was added when NVMe was at v1.2.
And the Linux kernel is also basically adhering to v1.3 wrt. CMB
support. In v1.4 the host actually needs to specifically enable the CMB
- and that is not something the kernel does currently IIRC.