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

[Alistair Francis]

On Wed, Aug 2, 2023 at 6:52 PM Lukas Wunner <lukas@xxxxxxxxx> wrote:
>
> On Tue, Aug 01, 2023 at 02:24:24PM -0400, Alistair Francis wrote:
> > 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:
> > > > > > +What:                /sys/bus/pci/devices/.../doe_proto
>
> The PCISIG published the DOE r1.1 ECN in September 2022.
>
> It replaced all occurrences of the term "protocol" with either "feature"
> or "data object type".  Please adhere to the terms used by the spec so
> that it is easy for an uninitiated reader to make the connection between
> the spec and the implementation.
>
> DOE r1.1 was merged into the PCIe Base Spec r6.1.  It wasn't merged into
> r6.0.1 yet.
>
>
> > > > > > +             This file contains a list of the supported Data Object Exchange (DOE)
> > > > > > +             protocols. The protocols are seperated by newlines.
>                                                      ^^^^^^^^^
> s/seperated/separated/
>
>
> > > > > > +             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.
>
> I kind of like the approach of exposing a list which can be grep'ed,
> even though it may go against the rule of having just one datum per
> attribute.  I'd prefer a representation that's human-readable though,
> e.g. "0001:01" for CMA-SPDM.

Yeah, it's my preferred method as well, but it's not going to be
accepted upstream

>
>
> > > > > 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.
>
> For all practical purposes, the maximum size which can be returned
> by a sysfs attribute (PAGE_SIZE, i.e. 4 kByte on x86) ought to be
> sufficient.  I'd say a mailbox typically doesn't support more than,
> say, 10 protocols.
>
>
> > > > 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.
>
> Right, just expose raw numbers and let lspci print them in beautified
> (parsed) form.
>
>
> > 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
>
> Since you intend to expose an interface for interacting with mailboxes,
> on top of just exposing a list of supported data types (protocols),
> I think you should first come up with a plan how to do that instead
> of kicking the can down the road.  The sysfs ABI is sort of set in
> stone, you can't easily change it if you realize after the fact
> that it has deficiencies for your use case.

So I think no matter what we want the DOE protocols exposed via sysfs.
That will allow tools like lspci to report the DOE protocols
supported.

Any other features aren't going to use sysfs. The future question of a
DOE mailbox or exposing SPDM bits does seem to be already determined
anyway.

>
> sysfs is not suitable for interaction with DOE mailboxes because the
> filesystem imposes a size restriction of PAGE_SIZE per read.  DOE
> allows up to 1 MByte per request or response, so way bigger than the
> puny 4 kByte PAGE_SIZE on x86.  Splitting response reception into
> multiple reads of the same attribute would be an awful kludge.
> So I think you need to resort to devfs or procfs for mailbox interaction,
> instead of sysfs.

Agreed

>
> Question is, if you use devfs/procfs for mailbox interaction, maybe it
> makes sense to expose the list of supported data types there as well,
> instead of in sysfs?

I do think that listing the protocols in sysfs makes sense, even with
a mailbox somewhere else makes sense. In saying that I don't think we
will end up adding mailbox support anyway.

>
> If you do expose a list of supported protocols, you should definitely
> have one sysfs attribute per mailbox, e.g. "doe_123" or "doe@123" if
> the mailbox is located at offset 123 in config space.
>
>
> > 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
>
> I envision that we'll provide a higher-level ABI for things like
> measurement retrieval, either through IMA or maybe sysfs, but not
> low-level access to the SPDM session.

That seems like the best approach to me as well.

>
> In fact, I think if you do implement mailbox interaction, you may
> want to blacklist certain data types that are handled in-kernel,
> such as CMA-SPDM.
>
> And you should constrain the whole thing to
> !security_locked_down(LOCKDOWN_PCI_ACCESS).
>
> FWIW, an experimental in-kernel implementation of SPDM measurement
> retrieval already exists (it goes on top of my doe branch that I
> linked to previously):
>
> https://github.com/debox1/spdm/commits/measurement

Awesome! Thank for that

Alistair

>
> Thanks,
>
> Lukas