Re: [PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register
On Tue, 2020-07-07 at 21:15 +0200, Klaus Jensen wrote:
> 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
> > > Reviewed-by: Klaus Jensen
> > > ---
> > > 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, _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
> >
>
> 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.
>
Ah, makes sense!
I by now have specs for each NVME revision, but I am getting lazy sometimes to
cross-check
them.
Best regards,
Maxim Levitsky
Re: [PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register
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
> > Reviewed-by: Klaus Jensen
> > ---
> > 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, _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
>
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.
Re: [PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register
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
> Reviewed-by: Klaus Jensen
> ---
> 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, _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
Best regards,
Maxim Levitsky
[PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register
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
Reviewed-by: Klaus Jensen
---
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, _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,
--
2.21.1
