| Message ID | 1472819456-12995-1-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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
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
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
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
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
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,
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(-)