[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