| Message ID | 20230801121824.174556-1-alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI/DOE: Expose the DOE protocols via sysfs | expand |
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@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(+) > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
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@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/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@vger.kernel.org> > +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. What userspace tool wants this information? thanks, greg k-h
On Tue, Aug 1, 2023 at 9:28 AM 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 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@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/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@vger.kernel.org> > > +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? > > 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). 1: https://github.com/pciutils/pciutils/pull/152 > > thanks, > > greg k-h
On Tue, 1 Aug 2023 09:48:13 -0400 Alistair Francis <alistair23@gmail.com> wrote: > On Tue, Aug 1, 2023 at 9:28 AM 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 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@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/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@vger.kernel.org> > > > +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? Files are cheap(ish) :) + expectation is probably a few protocols at most per DOE and a few DOEs per device. 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. 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 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
On Tue, Aug 1, 2023 at 12:07 PM 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 AM 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 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@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/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@vger.kernel.org> > > > > +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 >
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@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???AM Greg KH <gregkh@linuxfoundation.org> 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
On Wed, Aug 2, 2023 at 6:52 PM Lukas Wunner <lukas@wunner.de> 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@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???AM Greg KH <gregkh@linuxfoundation.org> 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
On Fri, Aug 04, 2023 at 11:17:59AM -0400, Alistair Francis wrote: > On Wed, Aug 2, 2023 at 6:52???PM Lukas Wunner <lukas@wunner.de> wrote: > > 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 How about procfs instead of sysfs? No "single datum per file" rule over there. PCI content goes into /proc/bus/pci/. Already used by lspci to access config space. Thanks, Lukas
On Fri, Aug 04, 2023 at 06:05:42PM +0200, Lukas Wunner wrote: > On Fri, Aug 04, 2023 at 11:17:59AM -0400, Alistair Francis wrote: > > On Wed, Aug 2, 2023 at 6:52???PM Lukas Wunner <lukas@wunner.de> wrote: > > > 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 > > How about procfs instead of sysfs? No, procfs is for "processes", not devices. > No "single datum per file" rule over there. > PCI content goes into /proc/bus/pci/. > Already used by lspci to access config space. And that is old legacy stuff, please, let us learn from our past mistakes in api creation, sysfs was created this way for a reason (i.e. large files in procfs do not work well over time.) thanks, greg k-h
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@vger.kernel.org> +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. 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)); + + 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; + } + + return ret; +} +#endif + /** * pci_doe_submit_task() - Submit a task to be processed by the state machine * diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index ab32a91f287b..cdb6aea2d263 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/sched.h> #include <linux/pci.h> +#include <linux/pci-doe.h> #include <linux/stat.h> #include <linux/export.h> #include <linux/topology.h> @@ -290,6 +291,10 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, } static DEVICE_ATTR_RO(modalias); +#ifdef CONFIG_PCI_DOE +static DEVICE_ATTR_RO(doe_proto); +#endif + static ssize_t enable_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { @@ -603,6 +608,9 @@ static struct attribute *pci_dev_attrs[] = { &dev_attr_local_cpus.attr, &dev_attr_local_cpulist.attr, &dev_attr_modalias.attr, +#ifdef CONFIG_PCI_DOE + &dev_attr_doe_proto.attr, +#endif #ifdef CONFIG_NUMA &dev_attr_numa_node.attr, #endif diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h index 1f14aed4354b..3033eb4bd17d 100644 --- a/include/linux/pci-doe.h +++ b/include/linux/pci-doe.h @@ -22,4 +22,6 @@ int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type, const void *request, size_t request_sz, void *response, size_t response_sz); +ssize_t doe_proto_show(struct device *dev, struct device_attribute *attr, + char *buf); #endif
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@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(+)