nvme : Add sysfs entry for NVMe CMBs when appropriate
cover nvme : Add sysfs entry for NVMe CMBs when appropriate
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
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.
--- drivers/nvme/host/pci.c | 46 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-)
drivers/nvme/host/pci.c +35 -9
@@ -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
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.
Quoted 5 lines
drivers/nvme/host/pci.c +2
-1} -if (readl (dev->bar +NVME_REG_VS ) >=NVME_VS (1 ,2 )) +/* +* CMBs can currently only exist on >=1.2 PCIe devices. We only
drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 1666
Quoted 10 lines
drivers/nvme/host/pci.c +8
-0+* 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
Quoted 8 lines · 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.
Great.
Quoted 15 lines
Quoted 15 lines
drivers/nvme/host/pci.c +9
-1} -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);
Quoted 3 lines
drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 1666
Quoted 1 line
drivers/nvme/host/pci.c +1
-0+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
Sagi Grimberg
maintainerPATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate
In reply to Stephen BatesQuoted 6 lines
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_ctrlsysfs 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
Quoted 13 lines · Sagi Grimberg
Quoted 6 lines
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_ctrlsysfs 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).
Quoted 5 lines
Perhaps cmbloc/cmbsize should exist in ctrl and be queried by ->reg_read32() callout. In addition, we would need a new
map_cmb()callout inctrl->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
Sagi Grimberg
maintainerPATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate
Quoted 3 lines
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,
Quoted 3 lines
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
Quoted 9 lines · Sagi Grimberg
Quoted 3 lines
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...
Quoted 6 lines
Quoted 3 lines
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
Quoted 10 lines · Stephen Bates
Quoted 9 lines · Sagi Grimberg
Quoted 5 lines
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,
Quoted 7 lines
Quoted 2 lines
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:
Quoted 24 lines
Quoted 11 lines
Quoted 8 lines
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