[Skiboot] [PATCH] nvlink: Set a bit in config space to indicate a real PCI device was bound
Alistair Popple
alistair at popple.id.au
Thu Jan 7 11:28:15 AEDT 2016
Thanks Stewart/Russell.
Looks pretty good to me, sorry for the slow review.
Acked-By: Alistair Popple <alistair at popple.id.au>
On Wed, 6 Jan 2016 15:07:44 Stewart Smith wrote:
> 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)
> >
> >
> >> 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
>
>
More information about the Skiboot
mailing list