[PATCH] nvme: allow use of cmb on v1.4 controllers

[PATCH] nvme: allow use of cmb on v1.4 controllers

[Klaus Jensen]

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

Since NVMe v1.4 the Controller Memory Buffer must be explicitly enabled
by the host.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 drivers/nvme/host/pci.c | 25 ++++++++++++++++++++++++-
 include/linux/nvme.h    |  6 ++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 50d9a20568a2..9c2bbb242994 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1787,7 +1787,8 @@ static u32 nvme_cmb_size(struct nvme_dev *dev)
 
 static void nvme_map_cmb(struct nvme_dev *dev)
 {
-	u64 size, offset;
+	u32 vs;
+	u64 size, offset, cmbmsc;
 	resource_size_t bar_size;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	int bar;
@@ -1795,6 +1796,15 @@ static void nvme_map_cmb(struct nvme_dev *dev)
 	if (dev->cmb_size)
 		return;
 
+	vs = readl(dev->bar + NVME_REG_VS);
+
+	if (vs >= NVME_VS(1, 4, 0)) {
+		if (!NVME_CAP_CMBS(dev->ctrl.cap))
+			return;
+
+		writel(NVME_CMBMSC_CRE, dev->bar + NVME_REG_CMBMSC);
+	}
+
 	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
 	if (!dev->cmbsz)
 		return;
@@ -1805,6 +1815,19 @@ static void nvme_map_cmb(struct nvme_dev *dev)
 	bar = NVME_CMB_BIR(dev->cmbloc);
 	bar_size = pci_resource_len(pdev, bar);
 
+	if (vs >= NVME_VS(1, 4, 0)) {
+		cmbmsc = pci_resource_start(pdev, bar);
+		cmbmsc |= (NVME_CMBMSC_CRE | NVME_CMBMSC_CMSE);
+
+		/*
+		 * Writing the low bits may cause the controller to check the
+		 * validity of the 52 bit controller base address in the CMBMSC
+		 * register, so write the high bits first.
+		 */
+		writel(cmbmsc >> 32, dev->bar + NVME_REG_CMBMSC + 4);
+		writel(cmbmsc, dev->bar + NVME_REG_CMBMSC);
+	}
+
 	if (offset > bar_size)
 		return;
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d92535997687..bfed36e342cc 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -116,6 +116,9 @@ enum {
 	NVME_REG_BPMBL	= 0x0048,	/* Boot Partition Memory Buffer
 					 * Location
 					 */
+	NVME_REG_CMBMSC = 0x0050,	/* Controller Memory Buffer Memory
+					 * Space Control
+					 */
 	NVME_REG_PMRCAP	= 0x0e00,	/* Persistent Memory Capabilities */
 	NVME_REG_PMRCTL	= 0x0e04,	/* Persistent Memory Region Control */
 	NVME_REG_PMRSTS	= 0x0e08,	/* Persistent Memory Region Status */
@@ -135,6 +138,7 @@ enum {
 #define NVME_CAP_CSS(cap)	(((cap) >> 37) & 0xff)
 #define NVME_CAP_MPSMIN(cap)	(((cap) >> 48) & 0xf)
 #define NVME_CAP_MPSMAX(cap)	(((cap) >> 52) & 0xf)
+#define NVME_CAP_CMBS(cap)	(((cap) >> 57) & 0x1)
 
 #define NVME_CMB_BIR(cmbloc)	((cmbloc) & 0x7)
 #define NVME_CMB_OFST(cmbloc)	(((cmbloc) >> 12) & 0xfffff)
@@ -192,6 +196,8 @@ enum {
 	NVME_CSTS_SHST_OCCUR	= 1 << 2,
 	NVME_CSTS_SHST_CMPLT	= 2 << 2,
 	NVME_CSTS_SHST_MASK	= 3 << 2,
+	NVME_CMBMSC_CRE		= 1 << 0,
+	NVME_CMBMSC_CMSE	= 1 << 1,
 };
 
 struct nvme_id_power_state {
-- 
2.30.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: allow use of cmb on v1.4 controllers

[Christoph Hellwig]

On Thu, Jan 14, 2021 at 02:24:41PM +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Since NVMe v1.4 the Controller Memory Buffer must be explicitly enabled
> by the host.

Thanks, this has been on my TODO list for a while.

> +	vs = readl(dev->bar + NVME_REG_VS);
> +
> +	if (vs >= NVME_VS(1, 4, 0)) {
> +		if (!NVME_CAP_CMBS(dev->ctrl.cap))
> +			return;

We should not check the version here, but the presence of the new
register so that controllers claiming an older version complicance
that implement the new scheme are supported as well.

Given that the old scheme is highly dangerous in virtualized enviroments
we should also warn about it and eventually required an explicit opt-in
to use it.

> +	if (vs >= NVME_VS(1, 4, 0)) {
> +		cmbmsc = pci_resource_start(pdev, bar);
> +		cmbmsc |= (NVME_CMBMSC_CRE | NVME_CMBMSC_CMSE);
> +
> +		/*
> +		 * Writing the low bits may cause the controller to check the
> +		 * validity of the 52 bit controller base address in the CMBMSC
> +		 * register, so write the high bits first.
> +		 */
> +		writel(cmbmsc >> 32, dev->bar + NVME_REG_CMBMSC + 4);
> +		writel(cmbmsc, dev->bar + NVME_REG_CMBMSC);

I think we should just use writeq here, with the hilo wrapper for 32-bit
platforms here.  The reason why NVMe has so avoided the 64-bit access
is for buggy Apple controllers, but they are unlikely to implement CMB.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: allow use of cmb on v1.4 controllers

[Keith Busch]

On Thu, Jan 14, 2021 at 02:24:41PM +0100, Klaus Jensen wrote:
> @@ -1795,6 +1796,15 @@ static void nvme_map_cmb(struct nvme_dev *dev)
>  	if (dev->cmb_size)
>  		return;
>  
> +	vs = readl(dev->bar + NVME_REG_VS);
> +
> +	if (vs >= NVME_VS(1, 4, 0)) {
> +		if (!NVME_CAP_CMBS(dev->ctrl.cap))
> +			return;
> +
> +		writel(NVME_CMBMSC_CRE, dev->bar + NVME_REG_CMBMSC);
> +	}
> +
>  	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
>  	if (!dev->cmbsz)
>  		return;
> @@ -1805,6 +1815,19 @@ static void nvme_map_cmb(struct nvme_dev *dev)
>  	bar = NVME_CMB_BIR(dev->cmbloc);
>  	bar_size = pci_resource_len(pdev, bar);
>  
> +	if (vs >= NVME_VS(1, 4, 0)) {
> +		cmbmsc = pci_resource_start(pdev, bar);

Shouldn't this have a '+ offset' in case the CMB doesn't start at the
beginning of the bar?

> +		cmbmsc |= (NVME_CMBMSC_CRE | NVME_CMBMSC_CMSE);
> +
> +		/*
> +		 * Writing the low bits may cause the controller to check the
> +		 * validity of the 52 bit controller base address in the CMBMSC
> +		 * register, so write the high bits first.
> +		 */
> +		writel(cmbmsc >> 32, dev->bar + NVME_REG_CMBMSC + 4);
> +		writel(cmbmsc, dev->bar + NVME_REG_CMBMSC);

hi_lo_writeq()?

This should probably come after we've verified the '(offset > bar_size)'
check below so that we're not enabling these capabilities before we know
if they're within the BAR.

> +	}
> +
>  	if (offset > bar_size)
>  		return;

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: allow use of cmb on v1.4 controllers

[Keith Busch]

On Thu, Jan 14, 2021 at 10:06:00AM -0800, Keith Busch wrote:
> On Thu, Jan 14, 2021 at 02:24:41PM +0100, Klaus Jensen wrote:
> > @@ -1795,6 +1796,15 @@ static void nvme_map_cmb(struct nvme_dev *dev)
> >  	if (dev->cmb_size)
> >  		return;
> >  
> > +	vs = readl(dev->bar + NVME_REG_VS);
> > +
> > +	if (vs >= NVME_VS(1, 4, 0)) {
> > +		if (!NVME_CAP_CMBS(dev->ctrl.cap))
> > +			return;
> > +
> > +		writel(NVME_CMBMSC_CRE, dev->bar + NVME_REG_CMBMSC);
> > +	}
> > +
> >  	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
> >  	if (!dev->cmbsz)
> >  		return;
> > @@ -1805,6 +1815,19 @@ static void nvme_map_cmb(struct nvme_dev *dev)
> >  	bar = NVME_CMB_BIR(dev->cmbloc);
> >  	bar_size = pci_resource_len(pdev, bar);
> >  
> > +	if (vs >= NVME_VS(1, 4, 0)) {
> > +		cmbmsc = pci_resource_start(pdev, bar);
> 
> Shouldn't this have a '+ offset' in case the CMB doesn't start at the
> beginning of the bar?

One other thing, I believe pci_bus_address() is required instead of
pci_resource_start().

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: allow use of cmb on v1.4 controllers

[Klaus Jensen]

[-- Attachment #1.1: Type: text/plain, Size: 1789 bytes --]

On Jan 14 18:33, Christoph Hellwig wrote:
> On Thu, Jan 14, 2021 at 02:24:41PM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Since NVMe v1.4 the Controller Memory Buffer must be explicitly enabled
> > by the host.
> 
> Thanks, this has been on my TODO list for a while.
> 
> > +	vs = readl(dev->bar + NVME_REG_VS);
> > +
> > +	if (vs >= NVME_VS(1, 4, 0)) {
> > +		if (!NVME_CAP_CMBS(dev->ctrl.cap))
> > +			return;
> 
> We should not check the version here, but the presence of the new
> register so that controllers claiming an older version complicance
> that implement the new scheme are supported as well.
> 

Understood!

> Given that the old scheme is highly dangerous in virtualized enviroments
> we should also warn about it and eventually required an explicit opt-in
> to use it.
> 

Not completely sure what warning you'd want, so I'll leave it to you to
add an 'else dev_warn(...)'. :)

> > +	if (vs >= NVME_VS(1, 4, 0)) {
> > +		cmbmsc = pci_resource_start(pdev, bar);
> > +		cmbmsc |= (NVME_CMBMSC_CRE | NVME_CMBMSC_CMSE);
> > +
> > +		/*
> > +		 * Writing the low bits may cause the controller to check the
> > +		 * validity of the 52 bit controller base address in the CMBMSC
> > +		 * register, so write the high bits first.
> > +		 */
> > +		writel(cmbmsc >> 32, dev->bar + NVME_REG_CMBMSC + 4);
> > +		writel(cmbmsc, dev->bar + NVME_REG_CMBMSC);
> 
> I think we should just use writeq here, with the hilo wrapper for 32-bit
> platforms here.  The reason why NVMe has so avoided the 64-bit access
> is for buggy Apple controllers, but they are unlikely to implement CMB.

Right, I tried writeq, but on QEMU I observed two 4 byte writes in wrong
order anyway.

hi_lo_writeq looks good.

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

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: allow use of cmb on v1.4 controllers

[Klaus Jensen]

[-- Attachment #1.1: Type: text/plain, Size: 1656 bytes --]

On Jan 14 10:06, Keith Busch wrote:
> On Thu, Jan 14, 2021 at 02:24:41PM +0100, Klaus Jensen wrote:
> > @@ -1795,6 +1796,15 @@ static void nvme_map_cmb(struct nvme_dev *dev)
> >  	if (dev->cmb_size)
> >  		return;
> >  
> > +	vs = readl(dev->bar + NVME_REG_VS);
> > +
> > +	if (vs >= NVME_VS(1, 4, 0)) {
> > +		if (!NVME_CAP_CMBS(dev->ctrl.cap))
> > +			return;
> > +
> > +		writel(NVME_CMBMSC_CRE, dev->bar + NVME_REG_CMBMSC);
> > +	}
> > +
> >  	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
> >  	if (!dev->cmbsz)
> >  		return;
> > @@ -1805,6 +1815,19 @@ static void nvme_map_cmb(struct nvme_dev *dev)
> >  	bar = NVME_CMB_BIR(dev->cmbloc);
> >  	bar_size = pci_resource_len(pdev, bar);
> >  
> > +	if (vs >= NVME_VS(1, 4, 0)) {
> > +		cmbmsc = pci_resource_start(pdev, bar);
> 
> Shouldn't this have a '+ offset' in case the CMB doesn't start at the
> beginning of the bar?
> 


Of course, good catch!

> > +		cmbmsc |= (NVME_CMBMSC_CRE | NVME_CMBMSC_CMSE);
> > +
> > +		/*
> > +		 * Writing the low bits may cause the controller to check the
> > +		 * validity of the 52 bit controller base address in the CMBMSC
> > +		 * register, so write the high bits first.
> > +		 */
> > +		writel(cmbmsc >> 32, dev->bar + NVME_REG_CMBMSC + 4);
> > +		writel(cmbmsc, dev->bar + NVME_REG_CMBMSC);
> 
> hi_lo_writeq()?
> 

Ah, was looking for that...

> This should probably come after we've verified the '(offset > bar_size)'
> check below so that we're not enabling these capabilities before we know
> if they're within the BAR.
> 

Right.

> > +	}
> > +
> >  	if (offset > bar_size)
> >  		return;


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

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme