Re: [PATCH v2] xen/pciback: support driver_override

[Juergen Gross]

On 09/09/16 16:20, Boris Ostrovsky wrote:
> On 09/09/2016 02:14 AM, Juergen Gross wrote:
>> On 08/09/16 16:10, Boris Ostrovsky wrote:
>>> On 09/02/2016 08:30 AM, Juergen Gross wrote:
>>>> Support the driver_override scheme introduced with commit 782a985d7af2
>>>> ("PCI: Introduce new device binding path using pci_dev.driver_override")
>>>>
>>>> As pcistub_probe() is called for all devices (it has to check for a
>>>> match based on the slot address rather than device type) it has to
>>>> check for driver_override set to "pciback" itself.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>> ---
>>>> V2: removed now unused label
>>>> ---
>>>>  drivers/xen/xen-pciback/pci_stub.c | 16 ++++++++++------
>>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>>>> index 258b7c3..85c28f7 100644
>>>> --- a/drivers/xen/xen-pciback/pci_stub.c
>>>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>>>> @@ -25,6 +25,8 @@
>>>>  #include "conf_space.h"
>>>>  #include "conf_space_quirks.h"
>>>>  
>>>> +#define PCISTUB_DRIVER_NAME "pciback"
>>>> +
>>>>  static char *pci_devs_to_hide;
>>>>  wait_queue_head_t xen_pcibk_aer_wait_queue;
>>>>  /*Add sem for sync AER handling and xen_pcibk remove/reconfigue ops,
>>>> @@ -529,16 +531,18 @@ static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>  				"don't have a normal (0) or bridge (1) "
>>>>  				"header type!\n");
>>>>  			err = -ENODEV;
>>>> -			goto out;
>>>>  		}
>>>>  
>>>> +	} else if (!dev->driver_override ||
>>>> +		   strcmp(dev->driver_override, PCISTUB_DRIVER_NAME))
>>>> +		/* Didn't find the device */
>>>> +		err = -ENODEV;
>>>> +
>>>> +	if (!err) {
>>>>  		dev_info(&dev->dev, "seizing device\n");
>>>>  		err = pcistub_seize(dev);
>>>> -	} else
>>>> -		/* Didn't find the device */
>>>> -		err = -ENODEV;
>>>> +	}
>>> Should devices with pciback override be displayed in
>>> /sys/bus/pci/drivers/pciback/slots? If they should then they need to be
>>> either added to pcistub_device_ids or kept on some other list.
>> No, I don't think so. The patch is just needed to _avoid_ having to use
>> the slots stuff: without the patch you need something like:
>>
>> echo 0000:07:10.0 > /sys/bus/pci/devices/0000\:07\:10.0/driver/unbind
>> echo 0000:07:10.0 > /sys/bus/pci/drivers/pciback/new_slot
>> echo 0000:07:10.0 > /sys/bus/pci/drivers_probe
>>
>> while with the patch you can use the same mechanism as for similar
>> drivers like pci-stub and vfio-pci:
>>
>> echo pciback > /sys/bus/pci/devices/0000\:07\:10.0/driver_override
>> echo 0000:07:10.0 > /sys/bus/pci/devices/0000\:07\:10.0/driver/unbind
>> echo 0000:07:10.0 > /sys/bus/pci/drivers_probe
>>
>> So e.g. libvirt doesn't need special handling for pciback. The slot list
>> is necessary for assigning devices to pciback on boot, but I think the
>> override mechanism is better for runtime assignment.
> 
> I am not arguing against override mechanism.
> 
> My point is that people/tools may rely on the fact devices are always
> listed in slots file. For example, libxl_pci.c parses at this file (I
> haven't look at this code in details so perhaps it's only when checking
> for devices assigned at boot time).

Looking at libxl_pci.c I'm even more convinced we should _not_ add a
device added via the override mechanism to the list of slots: the
parsing of the file is done in case a device is to be added via xl
to pciback. This happens via the new_slot mechanism, while libvirt
wants to use the override mechanism. This is just one more case where
using both libvirt and xl for the same thing is a bad idea.

In case someone wants to remove a pci device which was added with
libvirt by means of xl it is still possible to add the device to slots
by echoing its pci address to new_slot.

>>> Also, do you think checking override might better be done first, before
>>> testing for ID match?
>> Why? I don't think this really matters.
> 
> It may provide (probably very slight) performance improvement when you
> have lots of assigned devices.

Really? A system where the override is being used should have only
those pci devices in the ID list which have been assigned at boot time
to pciback. How many are those? Even on a really huge machine less
than 1000. This would save us a few microseconds.

OTOH I don't mind changing the sequence. I've just realized that I've
missed the PCI_HEADER_TYPE test in the override path, so I should
send a V2 in any case.


Juergen