Re: [PATCH v2] PCI/DOE: Expose the DOE protocols via sysfs

[Alistair Francis]

On Tue, Aug 1, 2023 at 12:07 PM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Tue, 1 Aug 2023 09:48:13 -0400
> Alistair Francis <alistair23@xxxxxxxxx> wrote:
>
> > On Tue, Aug 1, 2023 at 9:28 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Aug 01, 2023 at 08:18:24AM -0400, Alistair Francis wrote:
> > > > The PCIe 6 specification added support for the Data Object Exchange (DOE).
> > > > When DOE is supported the Discovery Data Object Protocol must be
> > > > implemented. The protocol allows a requester to obtain information about
> > > > the other DOE protocols supported by the device.
> > > >
> > > > The kernel is already querying the DOE protocols supported and cacheing
> > > > the values. This patch exposes the values via sysfs. This will allow
> > > > userspace to determine which DOE protocols are supported by the PCIe
> > > > device.
> > > >
> > > > By exposing the information to userspace tools like lspci can relay the
> > > > information to users. By listing all of the supported protocols we can
> > > > allow userspace to parse and support the list, which might include
> > > > vendor specific protocols as well as yet to be supported protocols.
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@xxxxxxx>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-pci | 11 ++++++
> > > >  drivers/pci/doe.c                       | 52 +++++++++++++++++++++++++
> > > >  drivers/pci/pci-sysfs.c                 |  8 ++++
> > > >  include/linux/pci-doe.h                 |  2 +
> > > >  4 files changed, 73 insertions(+)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> > > > index ecf47559f495..ae969bbfa631 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > > > @@ -500,3 +500,14 @@ Description:
> > > >               console drivers from the device.  Raw users of pci-sysfs
> > > >               resourceN attributes must be terminated prior to resizing.
> > > >               Success of the resizing operation is not guaranteed.
> > > > +
> > > > +What:                /sys/bus/pci/devices/.../doe_proto
> > > > +Date:                July 2023
> > > > +Contact:     Linux PCI developers <linux-pci@xxxxxxxxxxxxxxx>
> > > > +Description:
> > > > +             This file contains a list of the supported Data Object Exchange (DOE)
> > > > +             protocols. The protocols are seperated by newlines.
> > > > +             The value comes from the device and specifies the vendor and
> > > > +             protocol supported. The lower byte is the protocol and the next
> > > > +             two bytes are the vendor ID.
> > > > +             The file is read only.
> > >
> > > Sorry, but sysfs files are "one value per file", you can't have a "list
> > > of protocols with new lines" in a one value-per-file rule.
> > >
> > >
> > > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > > > index 1b97a5ab71a9..70900b79b239 100644
> > > > --- a/drivers/pci/doe.c
> > > > +++ b/drivers/pci/doe.c
> > > > @@ -563,6 +563,58 @@ static bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
> > > >       return false;
> > > >  }
> > > >
> > > > +#ifdef CONFIG_SYSFS
> > > > +/**
> > > > + * pci_doe_sysfs_proto_supports() - Write the supported DOE protocols
> > > > + *                        to a sysfs buffer
> > > > + * @doe_mb: DOE mailbox capability to query
> > > > + * @buf: buffer to store the sysfs strings
> > > > + * @offset: offset in buffer to store the sysfs strings
> > > > + *
> > > > + * RETURNS: The number of bytes written, 0 means an error occured
> > > > + */
> > > > +static unsigned long pci_doe_sysfs_proto_supports(struct pci_doe_mb *doe_mb,
> > > > +                                               char *buf, ssize_t offset)
> > > > +{
> > > > +     unsigned long index;
> > > > +     ssize_t ret = offset;
> > > > +     ssize_t r;
> > > > +     void *entry;
> > > > +
> > > > +     xa_for_each(&doe_mb->prots, index, entry) {
> > > > +             r = sysfs_emit_at(buf, ret, "0x%08lX\n", xa_to_value(entry));
> > > > +
> > >
> > > No need for a blank line.
> > >
> > > > +             if (r == 0)
> > > > +                     return ret;
> > >
> > >
> > >
> > > > +
> > > > +             ret += r;
> > > > +     }
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +ssize_t doe_proto_show(struct device *dev, struct device_attribute *attr,
> > > > +                    char *buf)
> > > > +{
> > > > +     struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > +     unsigned long index;
> > > > +     ssize_t ret = 0;
> > > > +     ssize_t r;
> > > > +     struct pci_doe_mb *doe_mb;
> > > > +
> > > > +     xa_for_each(&pci_dev->doe_mbs, index, doe_mb) {
> > > > +             r = pci_doe_sysfs_proto_supports(doe_mb, buf, ret);
> > > > +
> > > > +             if (r == 0)
> > > > +                     return ret;
> > > > +
> > > > +             ret += r;
> > > > +     }
> > >
> > > So this is going to be a lot of data, what is ensuring that you didn't
> > > truncate it?  Which again, is the reason why this is not a good idea for
> > > sysfs, sorry.
> >
> > Hmm... That's a pain.
> >
> > I was hoping to avoid the kernel needing to know the protocols. This
> > list can include vendor specific protocols, as well as future
> > protocols that the running kernel doesn't yet support, so I wanted to
> > directly pass it to userspace without having to parse it in the
> > kernel.
> >
> > Does anyone have any thoughts on a better way to expose the information?
>
> File per protocol or better yet a directory per protocol vid and prot as files?

A directory per vid and files for each protocol sounds good to me.
I'll update the patch to do that. If anyone doesn't like that idea let
me know

> Files are cheap(ish) :) + expectation is probably a few protocols at
> most per DOE and a few DOEs per device.

I agree that's my expectation as well.

>
> Bit similar to listing out end points for USB devices.
>
> >
> > >
> > > What userspace tool wants this information?
> >
> > pciutils (lspci) is the first user [1], but I suspect more userspace
> > tools will want to query the DOE protocols as SPDM catches on more.
> > Eventually I would like to expose the DOE mailboxes to userspace (but
> > that's a separate issue).
>
> You may find it tricky to get anyone to move on that as I think we
> had a fairly strong consensus behind a per protocol interface only.
> One of the early DOE patch sets had a generic interface but we ripped
> it out based on discussions at the time.

Fair enough, thanks for letting me know

>
> One avenue discussed (after SPDM lands in kernel) was to provide some
> hooks for some parts of the exchange to be pushed to userspace, but
> it was never totally clear to me which bits make sense.  If this

I think we will need to at least expose a few parts of SPDM to
userspace. It could either be the kernel passing data back (like the
measurements for example) or userspace orchestrating the
communication. That's a future problem at the moment though

Alistair

> happens it will probably be the AMD SEV-SNP or similar usecase that
> drives it with a tightly defined interface for this purpose (not
> a generic DOE one).
>
> >
> > 1: https://github.com/pciutils/pciutils/pull/152
> >
> > >
> > > thanks,
> > >
> > > greg k-h
>