* [PATCH] nvme: allow use of cmb on v1.4 controllers
@ 2021-01-14 13:24 Klaus Jensen
2021-01-14 17:33 ` Christoph Hellwig
2021-01-14 18:06 ` Keith Busch
0 siblings, 2 replies; 6+ messages in thread
From: Klaus Jensen @ 2021-01-14 13:24 UTC (permalink / raw)
To: linux-nvme
Cc: Keith Busch, Jens Axboe, Klaus Jensen, Christoph Hellwig, Sagi Grimberg
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] nvme: allow use of cmb on v1.4 controllers
2021-01-14 13:24 [PATCH] nvme: allow use of cmb on v1.4 controllers Klaus Jensen
@ 2021-01-14 17:33 ` Christoph Hellwig
2021-01-15 6:12 ` Klaus Jensen
2021-01-14 18:06 ` Keith Busch
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2021-01-14 17:33 UTC (permalink / raw)
To: Klaus Jensen
Cc: Sagi Grimberg, Klaus Jensen, linux-nvme, Jens Axboe, Keith Busch,
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme: allow use of cmb on v1.4 controllers
2021-01-14 17:33 ` Christoph Hellwig
@ 2021-01-15 6:12 ` Klaus Jensen
0 siblings, 0 replies; 6+ messages in thread
From: Klaus Jensen @ 2021-01-15 6:12 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, Jens Axboe, Sagi Grimberg, linux-nvme, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme: allow use of cmb on v1.4 controllers
2021-01-14 13:24 [PATCH] nvme: allow use of cmb on v1.4 controllers Klaus Jensen
2021-01-14 17:33 ` Christoph Hellwig
@ 2021-01-14 18:06 ` Keith Busch
2021-01-14 18:15 ` Keith Busch
2021-01-15 6:13 ` Klaus Jensen
1 sibling, 2 replies; 6+ messages in thread
From: Keith Busch @ 2021-01-14 18:06 UTC (permalink / raw)
To: Klaus Jensen
Cc: Jens Axboe, Klaus Jensen, Christoph Hellwig, linux-nvme, Sagi Grimberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme: allow use of cmb on v1.4 controllers
2021-01-14 18:06 ` Keith Busch
@ 2021-01-14 18:15 ` Keith Busch
2021-01-15 6:13 ` Klaus Jensen
1 sibling, 0 replies; 6+ messages in thread
From: Keith Busch @ 2021-01-14 18:15 UTC (permalink / raw)
To: Klaus Jensen
Cc: Jens Axboe, Klaus Jensen, Christoph Hellwig, linux-nvme, Sagi Grimberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme: allow use of cmb on v1.4 controllers
2021-01-14 18:06 ` Keith Busch
2021-01-14 18:15 ` Keith Busch
@ 2021-01-15 6:13 ` Klaus Jensen
1 sibling, 0 replies; 6+ messages in thread
From: Klaus Jensen @ 2021-01-15 6:13 UTC (permalink / raw)
To: Keith Busch
Cc: Jens Axboe, Klaus Jensen, Christoph Hellwig, linux-nvme, Sagi Grimberg
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-15 6:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-14 13:24 [PATCH] nvme: allow use of cmb on v1.4 controllers Klaus Jensen
2021-01-14 17:33 ` Christoph Hellwig
2021-01-15 6:12 ` Klaus Jensen
2021-01-14 18:06 ` Keith Busch
2021-01-14 18:15 ` Keith Busch
2021-01-15 6:13 ` Klaus Jensen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.