[PATCH v2] xen/pciback: support driver_override

[PATCH v2] xen/pciback: support driver_override

[Juergen Gross]

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@suse.com>
---
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;
+	}
 
-out:
 	return err;
 }
 
@@ -945,7 +949,7 @@ static const struct pci_error_handlers xen_pcibk_error_handler = {
 static struct pci_driver xen_pcibk_pci_driver = {
 	/* The name should be xen_pciback, but until the tools are updated
 	 * we will keep it as pciback. */
-	.name = "pciback",
+	.name = PCISTUB_DRIVER_NAME,
 	.id_table = pcistub_ids,
 	.probe = pcistub_probe,
 	.remove = pcistub_remove,
-- 
2.6.6

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

[Boris Ostrovsky]

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@suse.com>
> ---
> 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.

Also, do you think checking override might better be done first, before
testing for ID match?

-boris


>  
> -out:
>  	return err;
>  }
>  
> @@ -945,7 +949,7 @@ static const struct pci_error_handlers xen_pcibk_error_handler = {
>  static struct pci_driver xen_pcibk_pci_driver = {
>  	/* The name should be xen_pciback, but until the tools are updated
>  	 * we will keep it as pciback. */
> -	.name = "pciback",
> +	.name = PCISTUB_DRIVER_NAME,
>  	.id_table = pcistub_ids,
>  	.probe = pcistub_probe,
>  	.remove = pcistub_remove,

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

[Boris Ostrovsky]

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@suse.com>
> ---
> 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.

Also, do you think checking override might better be done first, before
testing for ID match?

-boris


>  
> -out:
>  	return err;
>  }
>  
> @@ -945,7 +949,7 @@ static const struct pci_error_handlers xen_pcibk_error_handler = {
>  static struct pci_driver xen_pcibk_pci_driver = {
>  	/* The name should be xen_pciback, but until the tools are updated
>  	 * we will keep it as pciback. */
> -	.name = "pciback",
> +	.name = PCISTUB_DRIVER_NAME,
>  	.id_table = pcistub_ids,
>  	.probe = pcistub_probe,
>  	.remove = pcistub_remove,



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Juergen Gross]

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@suse.com>
>> ---
>> 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.

> Also, do you think checking override might better be done first, before
> testing for ID match?

Why? I don't think this really matters.


Juergen

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

[Boris Ostrovsky]

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@suse.com>
>>> ---
>>> 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).


>
>> 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.

-boris

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

[Boris Ostrovsky]

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@suse.com>
>>> ---
>>> 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).


>
>> 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.

-boris



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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@suse.com>
>>>> ---
>>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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@suse.com>
>>>> ---
>>>> 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

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

[David Vrabel]

On 09/09/16 07:14, 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@suse.com>
>>> ---
>>> 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.

Can you include something like this in the commit message?

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen/pciback: support driver_override

[David Vrabel]

On 09/09/16 07:14, 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@suse.com>
>>> ---
>>> 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.

Can you include something like this in the commit message?

David

Re: [Xen-devel] [PATCH v2] xen/pciback: support driver_override

[Juergen Gross]

On 09/09/16 16:27, David Vrabel wrote:
> On 09/09/16 07:14, 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@suse.com>
>>>> ---
>>>> 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.
> 
> Can you include something like this in the commit message?

Okay.


Juergen

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

[Juergen Gross]

On 09/09/16 16:27, David Vrabel wrote:
> On 09/09/16 07:14, 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@suse.com>
>>>> ---
>>>> 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.
> 
> Can you include something like this in the commit message?

Okay.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Juergen Gross]

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@suse.com>
>> ---
>> 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.

> Also, do you think checking override might better be done first, before
> testing for ID match?

Why? I don't think this really matters.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel