Re: [PATCH v1 05/31] virpcimock: Create driver_override file in device dirs

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 7/16/19 2:13 PM, Peter Krempa wrote:
> On Thu, Jul 11, 2019 at 17:53:52 +0200, Michal Privoznik wrote:
> > Newer kernels (v3.16-rc1~29^2~6^4) have 'driver_override' file
> > which simplifies way of binding a PCI device to desired driver.
> > Libvirt has support for this for some time too (v2.3.0-rc1~236),
> > but not our virpcimock. So far we did not care because our code
> > is designed to deal with this situation. Except for one.
> > hypothetical case: binding a device to the vfio-pci driver can be
> > successful only via driver_override. Any attempt to bind a PCI
> > device to vfio-pci driver using old method (new_id + unbind +
> > bind) will fail because of b803b29c1a5. While on vanilla kernel
>
> You've reverted the mentioned commit just before this patch.

Yep, I needed to do that s

> Also I did not understand what this is supposed to do from the commit
> message. Perhaps due to my limited understanding of the pci detachment
> code. So either somebody else reviews this or you'll need to reprhase
> the commit message.
>
> > I'm able to use the old method successfully, it's failing on RHEL
> > kernels (not sure why).
>
> This does not inspire confidence.

I've asked Alex Williamson about this and he confirmed that he can see 
this behaviour but nor he could understand why RHEL-7 kernel behaves 
that way.

> > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > ---
> >   tests/virpcimock.c | 57 +++++++++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 51 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> > index 6865f992dc..18d06d11d4 100644
> > --- a/tests/virpcimock.c
> > +++ b/tests/virpcimock.c
> > @@ -87,6 +87,11 @@ char *fakesysfspcidir;
> >    *   Probe for a driver that handles the specified device.
> >    *   Data in format "DDDD:BB:DD.F" (Domain:Bus:Device.Function).
> >    *
> > + * /sys/bus/pci/devices/<device>/driver_override
> > + *   Name of a driver that overrides preferred driver can be written
> > + *   here. The device will be attached to it on drivers_probe event.
> > + *   Writing an empty string (or "\n") clears the override.
> > + *
> >    * As a little hack, we are not mocking write to these files, but close()
> >    * instead. The advantage is we don't need any self growing array to hold the
> >    * partial writes and construct them back. We can let all the writes finish,
> > @@ -147,6 +152,7 @@ static struct pciDevice *pci_device_find_by_content(const char *path);
> >   static void pci_driver_new(const char *name, int fail, ...);
> >   static struct pciDriver *pci_driver_find_by_dev(struct pciDevice *dev);
> >   static struct pciDriver *pci_driver_find_by_path(const char *path);
> > +static struct pciDriver *pci_driver_find_by_driver_override(struct pciDevice *dev);
> >   static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev);
> >   static int pci_driver_unbind(struct pciDriver *driver, struct pciDevice *dev);
> >   static int pci_driver_handle_change(int fd, const char *path);
> > @@ -202,7 +208,8 @@ make_symlink(const char *path,
> >   static int
> >   pci_read_file(const char *path,
> >                 char *buf,
> > -              size_t buf_size)
> > +              size_t buf_size,
> > +              bool truncate)
> >   {
> >       int ret = -1;
> >       int fd = -1;
> > @@ -224,7 +231,8 @@ pci_read_file(const char *path,
> >           goto cleanup;
> >       }
> >   
> > -    if (ftruncate(fd, 0) < 0)
> > +    if (truncate &&
> > +        ftruncate(fd, 0) < 0)
> >           goto cleanup;
> >   
> >       ret = 0;
> > @@ -398,6 +406,8 @@ pci_device_new_from_stub(const struct pciDevice *data)
> >           ABORT("@tmp overflow");
> >       make_file(devpath, "class", tmp, -1);
> >   
> > +    make_file(devpath, "driver_override", NULL, -1);
> > +
> >       if (snprintf(tmp, sizeof(tmp),
> >                    "%s/../../../kernel/iommu_groups/%d",
> >                    devpath, dev->iommuGroup) < 0) {
> > @@ -441,7 +451,7 @@ pci_device_find_by_content(const char *path)
> >   {
> >       char tmp[32];
> >   
> > -    if (pci_read_file(path, tmp, sizeof(tmp)) < 0)
> > +    if (pci_read_file(path, tmp, sizeof(tmp), true) < 0)
> >           return NULL;
> >   
> >       return pci_device_find_by_id(tmp);
> > @@ -450,7 +460,10 @@ pci_device_find_by_content(const char *path)
> >   static int
> >   pci_device_autobind(struct pciDevice *dev)
> >   {
> > -    struct pciDriver *driver = pci_driver_find_by_dev(dev);
> > +    struct pciDriver *driver = pci_driver_find_by_driver_override(dev);
> > +
> > +    if (!driver)
> > +        driver = pci_driver_find_by_dev(dev);

This could explain why the check from 03/31 can't be here. If it was, 
then nor 'driver_override' would be able to bind a PCI device to the 
vfio-pci driver (which obviously is not the case).

> >   
> >       if (!driver) {
> >           /* No driver found. Nothing to do */
> > @@ -544,6 +557,36 @@ pci_driver_find_by_path(const char *path)
> >       return NULL;
> >   }
> >   
> > +static struct pciDriver *
> > +pci_driver_find_by_driver_override(struct pciDevice *dev)
> > +{
> > +    struct pciDriver *ret = NULL;
> > +    char *path = NULL;
> > +    char tmp[32];
> > +    size_t i;
> > +
> > +    if (virAsprintfQuiet(&path,
> > +                         SYSFS_PCI_PREFIX "devices/%s/driver_override",
> > +                         dev->id) < 0)
> > +        return NULL;
> > +
> > +    if (pci_read_file(path, tmp, sizeof(tmp), false) < 0)
> > +        goto cleanup;
> > +
> > +    for (i = 0; i < nPCIDrivers; i++) {
> > +        struct pciDriver *driver = pciDrivers[i];
> > +
> > +        if (STREQ(tmp, driver->name)) {
> > +            ret = driver;
> > +            break;
> > +        }
> > +    }
> > +
> > + cleanup:
> > +    VIR_FREE(path);
>
> VIR_AUTOFREE should be available.

Ah, good point. Consider fixed.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list