linux-kernel - Re: [PATCH 8/9] vfio/pci: export nvlink2 support into vendor vfio

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: <20210319200749.GB2356281@nvidia.com>
Date:   Fri, 19 Mar 2021 17:07:49 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Christoph Hellwig <hch@....de>,
        Max Gurtovoy <mgurtovoy@...dia.com>,
        Alexey Kardashevskiy <aik@...abs.ru>, cohuck@...hat.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        liranl@...dia.com, oren@...dia.com, tzahio@...dia.com,
        leonro@...dia.com, yarong@...dia.com, aviadye@...dia.com,
        shahafs@...dia.com, artemp@...dia.com, kwankhede@...dia.com,
        ACurrid@...dia.com, cjia@...dia.com, yishaih@...dia.com,
        mjrosato@...ux.ibm.com
Subject: Re: [PATCH 8/9] vfio/pci: export nvlink2 support into vendor
 vfio_pci drivers

On Fri, Mar 19, 2021 at 11:36:42AM -0600, Alex Williamson wrote:
> On Fri, 19 Mar 2021 17:34:49 +0100
> Christoph Hellwig <hch@....de> wrote:
> 
> > On Fri, Mar 19, 2021 at 01:28:48PM -0300, Jason Gunthorpe wrote:
> > > The wrinkle I don't yet have an easy answer to is how to load vfio_pci
> > > as a universal "default" within the driver core lazy bind scheme and
> > > still have working module autoloading... I'm hoping to get some
> > > research into this..  
> 
> What about using MODULE_SOFTDEP("pre: ...") in the vfio-pci base
> driver, which would load all the known variants in order to influence
> the match, and therefore probe ordering?

The way the driver core works is to first match against the already
loaded driver list, then trigger an event for module loading and when
new drivers are registered they bind to unbound devices.

So, the trouble is the event through userspace because the kernel
can't just go on to use vfio_pci until it knows userspace has failed
to satisfy the load request.

One answer is to have userspace udev have the "hook" here and when a
vfio flavour mod alias is requested on a PCI device it swaps in
vfio_pci if it can't find an alternative.

The dream would be a system with no vfio modules loaded could do some

 echo "vfio" > /sys/bus/pci/xxx/driver_flavour

And a module would be loaded and a struct vfio_device is created for
that device. Very easy for the user.

> If we coupled that with wildcard support in driver_override, ex.
> "vfio_pci*", and used consistent module naming, I think we'd only need
> to teach userspace about this wildcard and binding to a specific module
> would come for free.

What would the wildcard do?

> This assumes we drop the per-variant id_table and use the probe
> function to skip devices without the necessary requirements, either
> wrong device or missing the tables we expect to expose.

Without a module table how do we know which driver is which? 

Open coding a match table in probe() and returning failure feels hacky
to me.

> > Should we even load it by default?  One answer would be that the sysfs
> > file to switch to vfio mode goes into the core PCI layer, and that core
> > PCI code would contain a hack^H^H^H^Hhook to first load and bind vfio_pci
> > for that device.
> 
> Generally we don't want to be the default driver for anything (I think
> mdev devices are the exception).  Assignment to userspace or VM is a
> niche use case.  Thanks,

By "default" I mean if the user says device A is in "vfio" mode then
the kernel should
 - Search for a specific driver for this device and autoload it
 - If no specific driver is found then attach a default "universal"
   driver for it. vfio_pci is a universal driver.

vfio_platform is also a "universal" driver when in ACPI mode, in some
cases.

For OF cases platform it builts its own little subsystem complete with
autoloading:

                request_module("vfio-reset:%s", vdev->compat);
                vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
                                                        &vdev->reset_module);

And it is a good example of why I don't like this subsystem design
because vfio_platform doesn't do the driver loading for OF entirely
right, vdev->compat is a single string derived from the compatible
property:

        ret = device_property_read_string(dev, "compatible",
                                          &vdev->compat);
        if (ret)
                dev_err(dev, "Cannot retrieve compat for %s\n", vdev->name);

Unfortunately OF requires that compatible is a *list* of strings and a
correct driver is supposed to evaluate all of them. The driver core
does this all correctly, and this was lost when it was open coded
here.

We should NOT be avoiding the standard infrastructure for matching
drivers to devices by re-implementing it poorly.

Jason