[Skiboot] [PATCH] nvlink: Set a bit in config space to indicate a real PCI device was bound

Stewart Smith stewart at linux.vnet.ibm.com
Wed Jan 6 15:07:44 AEDT 2016


Alistair - if you want to ack this I'm okay to merge it.

Russell Currey <ruscur at russell.cc> writes:
> The version was already set (somewhat obscurely), that has been refactored and the version incremented as per the following patch (which supercedes the previous):
>
> Signed-off-by: Russell Currey <ruscur at russell.cc>
> ---
>  doc/nvlink.txt | 15 +++++++++++++--
>  hw/npu.c       | 31 +++++++++++++++++++++++--------
>  2 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/doc/nvlink.txt b/doc/nvlink.txt
> index 5673de4..d871d20 100644
> --- a/doc/nvlink.txt
> +++ b/doc/nvlink.txt
> @@ -60,15 +60,21 @@ Vendor Specific Capabilities
>  ----------------------------
>  
>  +-----------------+----------------+----------------+----------------+
> -|     Reserved    |   Cap Length   |  Next Cap Ptr  |  Cap ID (0x09) |
> +|  Version (0x02) |   Cap Length   |  Next Cap Ptr  |  Cap ID (0x09) |
>  +-----------------+----------------+----------------+----------------+
>  |                      Procedure Status Register                     |
>  +--------------------------------------------------------------------+
>  |                      Procedure Control Register                    |
>  +---------------------------------------------------+----------------+
> -|                      Reserved                     |   Link Number  |
> +|             Reserved            |   PCI Dev Flag  |   Link Number  |
>  +---------------------------------------------------+----------------+
>  
> +Version
> +
> +   This refers to the version of the NPU config space.  Used by device
> +   drivers to determine which fields of the config space they can
> +   expect to be available.
> +
>  Procedure Control Register
>  
>     Used to start hardware procedures.
> @@ -121,6 +127,11 @@ Procedure Status Register
>         3 - Procedure aborted.
>         4 - Unsupported procedure.
>  
> +PCI Device Flag
> +
> +   Bit 0 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..85edabb 100644
> --- a/hw/npu.c
> +++ b/hw/npu.c
> @@ -111,6 +111,15 @@
>  static struct npu_dev_cap *npu_dev_find_capability(struct npu_dev *dev,
>  						   uint16_t id);
>  
> +#define OPAL_NPU_VERSION          0x02
> +
> +#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 +555,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;
>  		}
>  	}
> @@ -1194,11 +1206,12 @@ static void npu_dev_populate_vendor_cap(struct npu_dev_cap *cap)
>  {
>  	struct npu_dev *dev = cap->dev;
>  	uint32_t offset = cap->start;
> -	uint32_t val;
> +	uint8_t val;
>  
> -	/* Add version and length information */
> -	val = (cap->end - cap->start) | 0x1 << 8;
> -	NPU_DEV_CFG_INIT_RO(dev, offset + 2, 4, val);
> +	/* Add length and version information */
> +	val = cap->end - cap->start;
> +	NPU_DEV_CFG_INIT_RO(dev, offset + 2, 1, val);
> +	NPU_DEV_CFG_INIT_RO(dev, offset + 3, 1, OPAL_NPU_VERSION);
>  	offset += 4;
>  
>  	/* Defaults when the trap can't handle the read/write (eg. due
> @@ -1212,7 +1225,7 @@ static void npu_dev_populate_vendor_cap(struct npu_dev_cap *cap)
>  			     npu_dev_procedure_write);
>  	offset += 8;
>  
> -	NPU_DEV_CFG_INIT_RO(dev, offset, 4, dev->index);
> +	NPU_DEV_CFG_INIT_RO(dev, offset, 1, dev->index);
>  }
>  
>  static void npu_dev_populate_pcie_cap(struct npu_dev_cap *cap)
> @@ -1229,7 +1242,7 @@ static void npu_dev_populate_pcie_cap(struct npu_dev_cap *cap)
>  	}
>  
>  	/* Sanity check on spanned registers */
> -	if ((cap->end - cap->start) < 0x40) {
> +	if ((cap->end - cap->start) < PCIE_CAP_START) {
>  		prlog(PR_NOTICE, "%s: Invalid reg region [%x, %x] for cap %d\n",
>  		      __func__, cap->start, cap->end, cap->id);
>  		return;
> @@ -1352,11 +1365,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)
> -- 
> 2.6.2
>
>
>> 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 at 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)
>>>
>> 
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

-- 
Stewart Smith
OPAL Architect, IBM.



More information about the Skiboot mailing list