[PATCH 0/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[PATCH 0/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Stephen Bates]

We have had a few attempts at adding CMB properties and control to
sysfs [1][2]. This patch is based on Jon Derrick's [1] to some exent but
takes a much more conservative approach by only displaying CMB
properties and not attmpting to control them. Assuming this gets
accepted I do have a follow-on that adds more information to the cmb
attribute file.

Rather nicely we contain these changes solely to pci.c.

Applies to commit 1a6fe74dfd1b ("nvme: Pass pointers, not dma
addresses, to nvme_get/set_features()") in Jens' for-4.9/block branch.

Tested on both QEMU and on some real hardware I have access too that can
advertise and expose a CMB.

[1] http://www.spinics.net/lists/linux-block/msg01560.html
[2] http://www.spinics.net/lists/linux-rdma/msg38752.html

Stephen Bates (1):
  nvme : Add sysfs entry for NVMe CMBs when appropriate

 drivers/nvme/host/pci.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

--
2.1.4

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Stephen Bates]

Add a sysfs attribute that contains salient information about the NVMe
Controller Memory Buffer when one is present. For now, just display the
information about the CMB available from the control registers. We attach
the CMB attribute file to the existing nvme_ctrl sysfs group so it can
handle the sysfs teardown.

Signed-off-by: Stephen Bates <sbates at raithlin.com>
---
 drivers/nvme/host/pci.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8dcf5a9..c7ab07f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -99,6 +99,7 @@ struct nvme_dev {
 	dma_addr_t cmb_dma_addr;
 	u64 cmb_size;
 	u32 cmbsz;
+	u32 cmbloc;
 	struct nvme_ctrl ctrl;
 	struct completion ioq_wait;
 };
@@ -1321,28 +1322,37 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
 	return ret >= 0 ? 0 : ret;
 }

+static ssize_t nvme_cmb_show(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct nvme_dev *ndev = to_nvme_dev(dev_get_drvdata(dev));
+
+	return sprintf(buf, "cmbloc : x%08x\ncmbsz  : x%08x\n",
+		       ndev->cmbloc, ndev->cmbsz);
+}
+static DEVICE_ATTR(cmb, S_IRUGO, nvme_cmb_show, NULL);
+
 static void __iomem *nvme_map_cmb(struct nvme_dev *dev)
 {
 	u64 szu, size, offset;
-	u32 cmbloc;
 	resource_size_t bar_size;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	void __iomem *cmb;
 	dma_addr_t dma_addr;

-	if (!use_cmb_sqes)
-		return NULL;
-
 	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
 	if (!(NVME_CMB_SZ(dev->cmbsz)))
 		return NULL;
+	dev->cmbloc = readl(dev->bar + NVME_REG_CMBLOC);

-	cmbloc = readl(dev->bar + NVME_REG_CMBLOC);
+	if (!use_cmb_sqes)
+		return NULL;

 	szu = (u64)1 << (12 + 4 * NVME_CMB_SZU(dev->cmbsz));
 	size = szu * NVME_CMB_SZ(dev->cmbsz);
-	offset = szu * NVME_CMB_OFST(cmbloc);
-	bar_size = pci_resource_len(pdev, NVME_CMB_BIR(cmbloc));
+	offset = szu * NVME_CMB_OFST(dev->cmbloc);
+	bar_size = pci_resource_len(pdev, NVME_CMB_BIR(dev->cmbloc));

 	if (offset > bar_size)
 		return NULL;
@@ -1355,7 +1365,7 @@ static void __iomem *nvme_map_cmb(struct nvme_dev *dev)
 	if (size > bar_size - offset)
 		size = bar_size - offset;

-	dma_addr = pci_resource_start(pdev, NVME_CMB_BIR(cmbloc)) + offset;
+	dma_addr = pci_resource_start(pdev, NVME_CMB_BIR(dev->cmbloc)) + offset;
 	cmb = ioremap_wc(dma_addr, size);
 	if (!cmb)
 		return NULL;
@@ -1642,9 +1652,25 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 			dev->q_depth);
 	}

-	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2))
+	/*
+	 * CMBs can currently only exist on >=1.2 PCIe devices. We only
+	 * populate sysfs if a CMB is implemented. Note that we add the
+	 * CMB attribute to the nvme_ctrl kobj which removes the need to remove
+	 * it on exit. Since nvme_dev_attrs_group has no name we can pass
+	 * NULL as final argument to sysfs_add_file_to_group.
+	 */
+
+	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2)) {
 		dev->cmb = nvme_map_cmb(dev);

+		if (readl(dev->bar + NVME_REG_CMBSZ)) {
+			if (sysfs_add_file_to_group(&dev->ctrl.device->kobj,
+						    &dev_attr_cmb.attr, NULL))
+				dev_warn(dev->dev,
+					 "failed to add sysfs attribute for CMB\n");
+		}
+	}
+
 	pci_enable_pcie_error_reporting(pdev);
 	pci_save_state(pdev);
 	return 0;
--
2.1.4

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Jon Derrick]

Hi Stephen,

Thanks for reviving the CMB discussions. Everything seems reasonable to
me, but I must question if we want a 'show' without a 'control'.
Consider if the 'control' ends up requiring an API that supercedes the
'show' in a more natural way.

Otherwise I'm for this.

>  	}
> 
> -	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2))
> +	/*
> +	 * CMBs can currently only exist on >=1.2 PCIe devices. We only
> +	 * populate sysfs if a CMB is implemented. Note that we add the
> +	 * CMB attribute to the nvme_ctrl kobj which removes the need to remove
> +	 * it on exit. Since nvme_dev_attrs_group has no name we can pass
> +	 * NULL as final argument to sysfs_add_file_to_group.
> +	 */
> +
> +	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2)) {
>  		dev->cmb = nvme_map_cmb(dev);
> 
> +		if (readl(dev->bar + NVME_REG_CMBSZ)) {
Small nit: we have this value cached in dev->cmbsz. So unless something
has changed in the configuration, we could use that instead.

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Stephen Bates]

On Tue, Sep 27, 2016@05:17:04PM -0600, Jon Derrick wrote:
> Hi Stephen,
>
> Thanks for reviving the CMB discussions. Everything seems reasonable to
> me, but I must question if we want a 'show' without a 'control'.
> Consider if the 'control' ends up requiring an API that supercedes the
> 'show' in a more natural way.
>
> Otherwise I'm for this.

Great.

>
> >  	}
> >
> > -	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2))
> > +	/*
> > +	 * CMBs can currently only exist on >=1.2 PCIe devices. We only
> > +	 * populate sysfs if a CMB is implemented. Note that we add the
> > +	 * CMB attribute to the nvme_ctrl kobj which removes the need to remove
> > +	 * it on exit. Since nvme_dev_attrs_group has no name we can pass
> > +	 * NULL as final argument to sysfs_add_file_to_group.
> > +	 */
> > +
> > +	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2)) {
> >  		dev->cmb = nvme_map_cmb(dev);
> >
> > +		if (readl(dev->bar + NVME_REG_CMBSZ)) {
> Small nit: we have this value cached in dev->cmbsz. So unless something
> has changed in the configuration, we could use that instead.

Good point. I will await other feedback and then issue a v1 with this
change. Can I add a Reviewed-by tag from you?

Cheers

Stephen

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Sagi Grimberg]

> Add a sysfs attribute that contains salient information about the NVMe
> Controller Memory Buffer when one is present. For now, just display the
> information about the CMB available from the control registers. We attach
> the CMB attribute file to the existing nvme_ctrl sysfs group so it can
> handle the sysfs teardown.

Hey Stephen,

I'm wandering if placing this only for pci is correct. In theory, we
could have fabrics expose a cmb (while its usage is still somewhat
unknown at the moment).

Perhaps cmbloc/cmbsize should exist in ctrl and be queried by
->reg_read32() callout. In addition, we would need a new map_cmb()
callout in ctrl->ops.

Thoughts?

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Stephen Bates]

On Wed, Oct 05, 2016@11:22:51AM +0300, Sagi Grimberg wrote:
>
> >Add a sysfs attribute that contains salient information about the NVMe
> >Controller Memory Buffer when one is present. For now, just display the
> >information about the CMB available from the control registers. We attach
> >the CMB attribute file to the existing nvme_ctrl sysfs group so it can
> >handle the sysfs teardown.
>
> Hey Stephen,
>
> I'm wandering if placing this only for pci is correct. In theory, we
> could have fabrics expose a cmb (while its usage is still somewhat
> unknown at the moment).
>
> Perhaps cmbloc/cmbsize should exist in ctrl and be queried by
> ->reg_read32() callout. In addition, we would need a new map_cmb()
> callout in ctrl->ops.
>
> Thoughts?

Sagi

The fabrics specification currently denotes the CMBLOC and CMBSZ
properties as reserved [1]. So it is not currently possible to
advertise a CMB in a fabrics controller. While that might change down
the road we can't assume that is going to happen.

I am inclined to keep thingsin pci.c for now and then address this
again when (if) the fabrics specification gets updated.

Also I am wondering where the physical memory backing a fabrics CMB
would reside. Would this be memory in the fabric NIC on the host, in
the NIC on the target or somewhere else?

1. See Figure 27 of http://www.nvmexpress.org/wp-content/uploads/\
   NVMe_over_Fabrics_1_0_Gold_20160605.pdf

Cheers

Stephen

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Sagi Grimberg]

> The fabrics specification currently denotes the CMBLOC and CMBSZ
> properties as reserved [1].

Managed to forget that... So I guess I'm fine with the patch as
it is,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

> Also I am wondering where the physical memory backing a fabrics CMB
> would reside. Would this be memory in the fabric NIC on the host, in
> the NIC on the target or somewhere else?

This was discussed in the WG a few times before, not sure if we
can discuss this before its in the spec though...

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[Stephen Bates]

On Wed, Oct 05, 2016@07:50:57PM +0300, Sagi Grimberg wrote:
>
> >The fabrics specification currently denotes the CMBLOC and CMBSZ
> >properties as reserved [1].
>
> Managed to forget that... So I guess I'm fine with the patch as
> it is,
>
> Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
>

Sagi

Thanks! I will work on a v1 to address Jonathan's issue and add your
tag to that. Coming soon...

> >Also I am wondering where the physical memory backing a fabrics CMB
> >would reside. Would this be memory in the fabric NIC on the host, in
> >the NIC on the target or somewhere else?
>
> This was discussed in the WG a few times before, not sure if we
> can discuss this before its in the spec though...

Ooops, you are correct. Us lowely kernel developers should not be
discussing things related to changes to the NVMe specification ;-). I
will take this discussion inside the NVMe technical working group.

Cheers

Stephen

[PATCH 1/1] nvme : Add sysfs entry for NVMe CMBs when appropriate

[J Freyensee]

On Wed, 2016-10-05@11:48 -0600, Stephen Bates wrote:
> On Wed, Oct 05, 2016@07:50:57PM +0300, Sagi Grimberg wrote:
> > 
> > 
> > > 
> > > The fabrics specification currently denotes the CMBLOC and CMBSZ
> > > properties as reserved [1].
> > 
> > Managed to forget that... So I guess I'm fine with the patch as
> > it is,
> > 
> > Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
> > 
> 
> Sagi
> 
> Thanks! I will work on a v1 to address Jonathan's issue and add your
> tag to that. Coming soon...

You can add my tag too if you like as well, as I've chatted my thoughts
to you via phone/email:

Reviewed-by: Jay Freyensee <james_p_freyensee at linux.intel.com>


> 
> > 
> > > 
> > > Also I am wondering where the physical memory backing a fabrics
> > > CMB
> > > would reside. Would this be memory in the fabric NIC on the host,
> > > in
> > > the NIC on the target or somewhere else?
> > 
> > This was discussed in the WG a few times before, not sure if we
> > can discuss this before its in the spec though...
> 
> Ooops, you are correct. Us lowely kernel developers should not be
> discussing things related to changes to the NVMe specification ;-). I
> will take this discussion inside the NVMe technical working group.
> 
> Cheers
> 
> Stephen
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme