Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230801121824.174556-1-alistair.francis@wdc.com>
 <2023080152-disobey-widen-65a4@gregkh> <CAKmqyKMEqrfP8BrXd9pVd4a5Aodipty-8bAkxK5xcGSewsC9JA@mail.gmail.com>
 <20230801170739.000048cb@Huawei.com>
In-Reply-To: <20230801170739.000048cb@Huawei.com>
From:   Alistair Francis <alistair23@gmail.com>
Date:   Tue, 1 Aug 2023 14:24:24 -0400
Message-ID: <CAKmqyKND01=xaiB-VFVsi3+KRbxu4dBKfh_RhCN-jric5VzNpA@mail.gmail.com>
Subject: Re: [PATCH v2] PCI/DOE: Expose the DOE protocols via sysfs
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>, bhelgaas@google.com,
        linux-pci@vger.kernel.org, lukas@wunner.de,
        alex.williamson@redhat.com, christian.koenig@amd.com,
        kch@nvidia.com, logang@deltatee.com, linux-kernel@vger.kernel.org,
        Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 1, 2023 at 12:07=E2=80=AFPM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 1 Aug 2023 09:48:13 -0400
> Alistair Francis <alistair23@gmail.com> wrote:
>
> > On Tue, Aug 1, 2023 at 9:28=E2=80=AFAM Greg KH <gregkh@linuxfoundation.=
org> 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 cach=
eing
> > > > the values. This patch exposes the values via sysfs. This will allo=
w
> > > > userspace to determine which DOE protocols are supported by the PCI=
e
> > > > 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@wdc.com>
> > > > ---
> > > >  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/Documentatio=
n/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-sy=
sfs
> > > >               resourceN attributes must be terminated prior to resi=
zing.
> > > >               Success of the resizing operation is not guaranteed.
> > > > +
> > > > +What:                /sys/bus/pci/devices/.../doe_proto
> > > > +Date:                July 2023
> > > > +Contact:     Linux PCI developers <linux-pci@vger.kernel.org>
> > > > +Description:
> > > > +             This file contains a list of the supported Data Objec=
t Exchange (DOE)
> > > > +             protocols. The protocols are seperated by newlines.
> > > > +             The value comes from the device and specifies the ven=
dor and
> > > > +             protocol supported. The lower byte is the protocol an=
d 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 "li=
st
> > > 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_d=
oe_mb *doe_mb, u16 vid, u8 type)
> > > >       return false;
> > > >  }
> > > >
> > > > +#ifdef CONFIG_SYSFS
> > > > +/**
> > > > + * pci_doe_sysfs_proto_supports() - Write the supported DOE protoc=
ols
> > > > + *                        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_m=
b *doe_mb,
> > > > +                                               char *buf, ssize_t =
offset)
> > > > +{
> > > > +     unsigned long index;
> > > > +     ssize_t ret =3D offset;
> > > > +     ssize_t r;
> > > > +     void *entry;
> > > > +
> > > > +     xa_for_each(&doe_mb->prots, index, entry) {
> > > > +             r =3D sysfs_emit_at(buf, ret, "0x%08lX\n", xa_to_valu=
e(entry));
> > > > +
> > >
> > > No need for a blank line.
> > >
> > > > +             if (r =3D=3D 0)
> > > > +                     return ret;
> > >
> > >
> > >
> > > > +
> > > > +             ret +=3D r;
> > > > +     }
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +ssize_t doe_proto_show(struct device *dev, struct device_attribute=
 *attr,
> > > > +                    char *buf)
> > > > +{
> > > > +     struct pci_dev *pci_dev =3D to_pci_dev(dev);
> > > > +     unsigned long index;
> > > > +     ssize_t ret =3D 0;
> > > > +     ssize_t r;
> > > > +     struct pci_doe_mb *doe_mb;
> > > > +
> > > > +     xa_for_each(&pci_dev->doe_mbs, index, doe_mb) {
> > > > +             r =3D pci_doe_sysfs_proto_supports(doe_mb, buf, ret);
> > > > +
> > > > +             if (r =3D=3D 0)
> > > > +                     return ret;
> > > > +
> > > > +             ret +=3D 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
>