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

[Lukas Wunner]

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.


> > > > 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.

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.

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?

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.

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

Thanks,

Lukas