lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240705112953.00007303@Huawei.com>
Date: Fri, 5 Jul 2024 11:29:53 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Alistair Francis <alistair23@...il.com>
CC: <bhelgaas@...gle.com>, <linux-pci@...r.kernel.org>, <lukas@...ner.de>,
	<alex.williamson@...hat.com>, <christian.koenig@....com>, <kch@...dia.com>,
	<gregkh@...uxfoundation.org>, <logang@...tatee.com>,
	<linux-kernel@...r.kernel.org>, <chaitanyak@...dia.com>,
	<rdunlap@...radead.org>, Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH v13 3/4] PCI/DOE: Expose the DOE features via sysfs

On Fri, 5 Jul 2024 11:24:25 +1000
Alistair Francis <alistair23@...il.com> wrote:

> On Tue, Jul 2, 2024 at 11:58 PM Jonathan Cameron
> <Jonathan.Cameron@...wei.com> wrote:
> >
> > On Tue,  2 Jul 2024 16:04:17 +1000
> > Alistair Francis <alistair23@...il.com> wrote:
> >  
> > > The PCIe 6 specification added support for the Data Object
> > > Exchange (DOE).
> > > When DOE is supported the DOE Discovery Feature must be implemented per
> > > PCIe r6.1 sec 6.30.1.1. The protocol allows a requester to obtain
> > > information about the other DOE features supported by the device.
> > >
> > > The kernel is already querying the DOE features supported and cacheing
> > > the values. Expose the values in sysfs to allow user space to
> > > determine which DOE features 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 features we can
> > > allow userspace to parse the list, which might include
> > > vendor specific features as well as yet to be supported features.
> > >
> > > After this patch is supported you can see something like this when
> > > attaching a DOE device
> > >
> > > $ ls /sys/devices/pci0000:00/0000:00:02.0//doe*
> > > 0001:00        0001:01        0001:02
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@....com>
> > > ---
> > > v13:
> > >  - Drop pci_doe_sysfs_init() and use pci_doe_sysfs_group
> > >      - As discussed in https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/
> > >        we can just modify pci_doe_sysfs_group at the DOE init and let  
> >
> > Can't do that as it is global so you expose the same DOE features for
> > all DOEs.
> >
> > Also, I think that this is only processing features on last doe_mb found
> > for a given device. Fix that and the duplicates problem resurfaces.
> >
> >  
> > >        device_add() handle the sysfs attributes.  
> >
> >  
> > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > > index defc4be81bd4..e7b702afce88 100644
> > > --- a/drivers/pci/doe.c
> > > +++ b/drivers/pci/doe.c  
> >  
> > > +
> > >  static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> > >  {
> > >       if (wait_event_timeout(doe_mb->wq,
> > > @@ -687,6 +747,12 @@ void pci_doe_init(struct pci_dev *pdev)
> > >  {
> > >       struct pci_doe_mb *doe_mb;
> > >       u16 offset = 0;
> > > +     struct attribute **sysfs_attrs;
> > > +     struct device_attribute *attrs;
> > > +     unsigned long num_features = 0;
> > > +     unsigned long i;
> > > +     unsigned long vid, type;
> > > +     void *entry;
> > >       int rc;
> > >
> > >       xa_init(&pdev->doe_mbs);
> > > @@ -707,6 +773,45 @@ void pci_doe_init(struct pci_dev *pdev)
> > >                       pci_doe_destroy_mb(doe_mb);
> > >               }
> > >       }  
> >
> > The above is looping over multiple DOEs but this just considers last one.
> > That doesn't look right...  
> 
> Yeah... That isn't
> 
> >
> > I think this needs to be in the loop and having done that
> > the duplicate handing may be an issue.  I'm not sure what happens
> > in that path with a presupplied set of attributes.
> >  
> > > +
> > > +     if (doe_mb) {
> > > +             xa_for_each(&doe_mb->feats, i, entry)
> > > +                     num_features++;
> > > +
> > > +             sysfs_attrs = kcalloc(num_features + 1, sizeof(*sysfs_attrs), GFP_KERNEL);
> > > +             if (!sysfs_attrs)
> > > +                     return;
> > > +
> > > +             attrs = kcalloc(num_features, sizeof(*attrs), GFP_KERNEL);
> > > +             if (!attrs) {
> > > +                     kfree(sysfs_attrs);
> > > +                     return;
> > > +             }
> > > +
> > > +             doe_mb->device_attrs = attrs;
> > > +             doe_mb->sysfs_attrs = sysfs_attrs;
> > > +
> > > +             xa_for_each(&doe_mb->feats, i, entry) {
> > > +                     sysfs_attr_init(&attrs[i].attr);
> > > +
> > > +                     vid = xa_to_value(entry) >> 8;
> > > +                     type = xa_to_value(entry) & 0xFF;
> > > +
> > > +                     attrs[i].attr.name = kasprintf(GFP_KERNEL, "%04lx:%02lx", vid, type);
> > > +                     if (!attrs[i].attr.name) {
> > > +                             pci_doe_sysfs_feature_remove(pdev, doe_mb);
> > > +                             return;
> > > +                     }
> > > +                     attrs[i].attr.mode = 0444;
> > > +                     attrs[i].show = pci_doe_sysfs_feature_show;
> > > +
> > > +                     sysfs_attrs[i] = &attrs[i].attr;
> > > +             }
> > > +
> > > +             sysfs_attrs[num_features] = NULL;
> > > +
> > > +             pci_doe_sysfs_group.attrs = sysfs_attrs;  
> > Hmm. Isn't this global?  What if you have multiple devices.  
> 
> Any input from a PCI maintainer here?
> 
> There are basically two approaches.
> 
>  1. We can have a pci_doe_sysfs_init() function that is called where
> we dynamically add the entries, like in v12
>  2. We can go down the dev->groups and device_add() path, like this
> patch and discussed at
> https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/
> 
> For the second we will have to create a global pci_doe_sysfs_group
> that contains all possible DOE entries on the system and then have the
> show functions determine if they should be displayed for that device.
> 
> Everytime we call pci_doe_init() we can check for any missing entries
> in pci_doe_sysfs_group.attrs and then realloc
> pci_doe_sysfs_group.attrs to add them. 
> Untested, but that should work
> even for hot-plugged devices. pci_doe_sysfs_group.attrs would just
> grow forever though as I don't think we have an easy way to deallocate
> anything as we aren't sure if we are the only entry.

I think this needs to be per device, not global and you'll have to manually
do the group visibility magic rather than using the macros.

> 
> Thoughts?
> 
> Alistair


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ