diff mbox

nvlink: Set a bit in config space to indicate a real PCI device was bound

Message ID 1446179790-11505-1-git-send-email-ruscur@russell.cc
State Superseded
Headers show

Commit Message

Russell Currey Oct. 30, 2015, 4:36 a.m. UTC
Previously, it was difficult to determine from userspace whether a NVLink
emulated PCI device was associated with a real PCI device. A bit in
the vendor capabilities section of the config space is now set if a
device was bound.  The documentation has been updated to reflect this.

In addition, the config space offsets are refactored into #defines.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 doc/nvlink.txt |  7 ++++++-
 hw/npu.c       | 16 ++++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

Comments

Alistair Popple Nov. 9, 2015, 3:12 a.m. UTC | #1
Russell,

A few minor comments below. Mainly we need to add a version field to the 
vendor specific capability so that Nvidia can detect that this bit is meant to 
be present. I would suggest using the upper byte of the vendor specific 
capability like so:

+-----------------+----------------+----------------+----------------+
|  Version (0x1)  |   Cap Length   |  Next Cap Ptr  |  Cap ID (0x09) |
+-----------------+----------------+----------------+----------------+


On Fri, 30 Oct 2015 15:36:30 Russell Currey wrote:
> Previously, it was difficult to determine from userspace whether a NVLink
> emulated PCI device was associated with a real PCI device. A bit in
> the vendor capabilities section of the config space is now set if a
> device was bound.  The documentation has been updated to reflect this.
> 
> In addition, the config space offsets are refactored into #defines.
> 
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>  doc/nvlink.txt |  7 ++++++-
>  hw/npu.c       | 16 ++++++++++++++--
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/nvlink.txt b/doc/nvlink.txt
> index 5673de4..d5f96c1 100644
> --- a/doc/nvlink.txt
> +++ b/doc/nvlink.txt
> @@ -66,7 +66,7 @@ Vendor Specific Capabilities
>  +--------------------------------------------------------------------+
>  |                      Procedure Control Register                    |
>  +---------------------------------------------------+----------------+
> -|                      Reserved                     |   Link Number  |
> +|             Reserved             |  PCI Dev Flag  |   Link Number  |
>  +---------------------------------------------------+----------------+
>  
>  Procedure Control Register
> @@ -121,6 +121,11 @@ Procedure Status Register
>         3 - Procedure aborted.
>         4 - Unsupported procedure.
>  
> +PCI Device Flag
> +
> +   This bit is set only if an actual PCI device was bound to this
> +   emulated device.

It would be good if we could call out the actual bit here (ie. bit 0) - we may 
define other PCI device flag bits in future.

Thanks.

- Alistair

>  Link Number
>  
>     Physical link number this emulated PCI device is assoicated
> diff --git a/hw/npu.c b/hw/npu.c
> index c9bc12b..daf3d1f 100644
> --- a/hw/npu.c
> +++ b/hw/npu.c
> @@ -111,6 +111,13 @@
>  static struct npu_dev_cap *npu_dev_find_capability(struct npu_dev *dev,
>  						   uint16_t id);
>  
> +#define PCIE_CAP_START	          0x40
> +#define PCIE_CAP_END	          0x80
> +#define VENDOR_CAP_START          0x80
> +#define VENDOR_CAP_END	          0x90
> +
> +#define VENDOR_CAP_PCI_DEV_OFFSET 0x0d
> +
>  /* PCI config raw accessors */
>  #define NPU_DEV_CFG_NORMAL_RD(d, o, s, v)	\
>  	npu_dev_cfg_read_raw(d, NPU_DEV_CFG_NORMAL, o, s, v)
> @@ -546,6 +553,9 @@ static void npu_dev_bind_pci_dev(struct npu_dev *dev)
>  		dev->pd = pci_walk_dev(phb, __npu_dev_bind_pci_dev, dev);
>  		if (dev->pd) {
>  			dev->phb = phb;
> +			/* Found the device, set the bit in config space */
> +			NPU_DEV_CFG_INIT_RO(dev, VENDOR_CAP_START +
> +				VENDOR_CAP_PCI_DEV_OFFSET, 1, 0x01);
>  			return;
>  		}
>  	}
> @@ -1352,11 +1362,13 @@ static void npu_dev_create_capabilities(struct 
npu_dev *dev)
>  
>  	/* PCI express capability */
>  	npu_dev_create_capability(dev, npu_dev_populate_pcie_cap,
> -				  PCI_CFG_CAP_ID_EXP, 0x40, 0x80);
> +				  PCI_CFG_CAP_ID_EXP, PCIE_CAP_START,
> +				  PCIE_CAP_END);
>  
>  	/* Vendor specific capability */
>  	npu_dev_create_capability(dev, npu_dev_populate_vendor_cap,
> -				  PCI_CFG_CAP_ID_VENDOR, 0x80, 0x90);
> +				  PCI_CFG_CAP_ID_VENDOR, VENDOR_CAP_START,
> +				  VENDOR_CAP_END);
>  }
>  
>  static void npu_dev_create_cfg(struct npu_dev *dev)
>
Stewart Smith Jan. 12, 2016, 4:45 a.m. UTC | #2
Russell Currey <ruscur@russell.cc> writes:
> Previously, it was difficult to determine from userspace whether a NVLink
> emulated PCI device was associated with a real PCI device. A bit in
> the vendor capabilities section of the config space is now set if a
> device was bound.  The documentation has been updated to reflect this.
>
> In addition, the config space offsets are refactored into #defines.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>  doc/nvlink.txt |  7 ++++++-
>  hw/npu.c       | 16 ++++++++++++++--
>  2 files changed, 20 insertions(+), 3 deletions(-)

Thanks! Merged to master as of d67d79a.
diff mbox

Patch

diff --git a/doc/nvlink.txt b/doc/nvlink.txt
index 5673de4..d5f96c1 100644
--- a/doc/nvlink.txt
+++ b/doc/nvlink.txt
@@ -66,7 +66,7 @@  Vendor Specific Capabilities
 +--------------------------------------------------------------------+
 |                      Procedure Control Register                    |
 +---------------------------------------------------+----------------+
-|                      Reserved                     |   Link Number  |
+|             Reserved             |  PCI Dev Flag  |   Link Number  |
 +---------------------------------------------------+----------------+
 
 Procedure Control Register
@@ -121,6 +121,11 @@  Procedure Status Register
        3 - Procedure aborted.
        4 - Unsupported procedure.
 
+PCI Device Flag
+
+   This bit is set only if an actual PCI device was bound to this
+   emulated device.
+
 Link Number
 
    Physical link number this emulated PCI device is assoicated
diff --git a/hw/npu.c b/hw/npu.c
index c9bc12b..daf3d1f 100644
--- a/hw/npu.c
+++ b/hw/npu.c
@@ -111,6 +111,13 @@ 
 static struct npu_dev_cap *npu_dev_find_capability(struct npu_dev *dev,
 						   uint16_t id);
 
+#define PCIE_CAP_START	          0x40
+#define PCIE_CAP_END	          0x80
+#define VENDOR_CAP_START          0x80
+#define VENDOR_CAP_END	          0x90
+
+#define VENDOR_CAP_PCI_DEV_OFFSET 0x0d
+
 /* PCI config raw accessors */
 #define NPU_DEV_CFG_NORMAL_RD(d, o, s, v)	\
 	npu_dev_cfg_read_raw(d, NPU_DEV_CFG_NORMAL, o, s, v)
@@ -546,6 +553,9 @@  static void npu_dev_bind_pci_dev(struct npu_dev *dev)
 		dev->pd = pci_walk_dev(phb, __npu_dev_bind_pci_dev, dev);
 		if (dev->pd) {
 			dev->phb = phb;
+			/* Found the device, set the bit in config space */
+			NPU_DEV_CFG_INIT_RO(dev, VENDOR_CAP_START +
+				VENDOR_CAP_PCI_DEV_OFFSET, 1, 0x01);
 			return;
 		}
 	}
@@ -1352,11 +1362,13 @@  static void npu_dev_create_capabilities(struct npu_dev *dev)
 
 	/* PCI express capability */
 	npu_dev_create_capability(dev, npu_dev_populate_pcie_cap,
-				  PCI_CFG_CAP_ID_EXP, 0x40, 0x80);
+				  PCI_CFG_CAP_ID_EXP, PCIE_CAP_START,
+				  PCIE_CAP_END);
 
 	/* Vendor specific capability */
 	npu_dev_create_capability(dev, npu_dev_populate_vendor_cap,
-				  PCI_CFG_CAP_ID_VENDOR, 0x80, 0x90);
+				  PCI_CFG_CAP_ID_VENDOR, VENDOR_CAP_START,
+				  VENDOR_CAP_END);
 }
 
 static void npu_dev_create_cfg(struct npu_dev *dev)