[PATCH v3 0/3] xen/pciback: support driver_override

[PATCH v3 0/3] 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")

Today you'd 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

Changes in V3:
- Added new patches 1 and 2 to do some cleanup and preparatory work.
- patch 3: Add an assigned device to the slot list
- patch 3: Move override check up in order not to miss the PCI_HEADER_TYPE
  check.

Changes in V2:
- Removed now unused label.

*** BLURB HERE ***

Juergen Gross (3):
  xen/pciback: simplify pcistub device handling
  xen/pciback: avoid multiple entries in slot list
  xen/pciback: support driver_override

 drivers/xen/xen-pciback/pci_stub.c | 125 +++++++++++++++++++++++++------------
 1 file changed, 85 insertions(+), 40 deletions(-)

-- 
2.6.6

[PATCH v3 1/3] xen/pciback: simplify pcistub device handling

[Juergen Gross]

The Xen pciback driver maintains a list of all its seized devices.
There are two functions searching the list for a specific device with
basically the same semantics just returning different structures in
case of a match.

Split out the search function.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 50 +++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 258b7c3..79a9e4d 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -149,29 +149,36 @@ static inline void pcistub_device_put(struct pcistub_device *psdev)
 	kref_put(&psdev->kref, pcistub_device_release);
 }
 
+static struct pcistub_device *pcistub_device_find_locked(int domain, int bus,
+							 int slot, int func)
+{
+	struct pcistub_device *psdev;
+
+	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
+		if (psdev->dev != NULL
+		    && domain == pci_domain_nr(psdev->dev->bus)
+		    && bus == psdev->dev->bus->number
+		    && slot == PCI_SLOT(psdev->dev->devfn)
+		    && func == PCI_FUNC(psdev->dev->devfn)) {
+			return psdev;
+		}
+	}
+
+	return NULL;
+}
+
 static struct pcistub_device *pcistub_device_find(int domain, int bus,
 						  int slot, int func)
 {
-	struct pcistub_device *psdev = NULL;
+	struct pcistub_device *psdev;
 	unsigned long flags;
 
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
-	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
-		if (psdev->dev != NULL
-		    && domain == pci_domain_nr(psdev->dev->bus)
-		    && bus == psdev->dev->bus->number
-		    && slot == PCI_SLOT(psdev->dev->devfn)
-		    && func == PCI_FUNC(psdev->dev->devfn)) {
-			pcistub_device_get(psdev);
-			goto out;
-		}
-	}
+	psdev = pcistub_device_find_locked(domain, bus, slot, func);
+	if (psdev)
+		pcistub_device_get(psdev);
 
-	/* didn't find it */
-	psdev = NULL;
-
-out:
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 	return psdev;
 }
@@ -207,16 +214,9 @@ struct pci_dev *pcistub_get_pci_dev_by_slot(struct xen_pcibk_device *pdev,
 
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
-	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
-		if (psdev->dev != NULL
-		    && domain == pci_domain_nr(psdev->dev->bus)
-		    && bus == psdev->dev->bus->number
-		    && slot == PCI_SLOT(psdev->dev->devfn)
-		    && func == PCI_FUNC(psdev->dev->devfn)) {
-			found_dev = pcistub_device_get_pci_dev(pdev, psdev);
-			break;
-		}
-	}
+	psdev = pcistub_device_find_locked(domain, bus, slot, func);
+	if (psdev)
+		found_dev = pcistub_device_get_pci_dev(pdev, psdev);
 
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 	return found_dev;
-- 
2.6.6

[PATCH v3 1/3] xen/pciback: simplify pcistub device handling

[Juergen Gross]

The Xen pciback driver maintains a list of all its seized devices.
There are two functions searching the list for a specific device with
basically the same semantics just returning different structures in
case of a match.

Split out the search function.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 50 +++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 258b7c3..79a9e4d 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -149,29 +149,36 @@ static inline void pcistub_device_put(struct pcistub_device *psdev)
 	kref_put(&psdev->kref, pcistub_device_release);
 }
 
+static struct pcistub_device *pcistub_device_find_locked(int domain, int bus,
+							 int slot, int func)
+{
+	struct pcistub_device *psdev;
+
+	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
+		if (psdev->dev != NULL
+		    && domain == pci_domain_nr(psdev->dev->bus)
+		    && bus == psdev->dev->bus->number
+		    && slot == PCI_SLOT(psdev->dev->devfn)
+		    && func == PCI_FUNC(psdev->dev->devfn)) {
+			return psdev;
+		}
+	}
+
+	return NULL;
+}
+
 static struct pcistub_device *pcistub_device_find(int domain, int bus,
 						  int slot, int func)
 {
-	struct pcistub_device *psdev = NULL;
+	struct pcistub_device *psdev;
 	unsigned long flags;
 
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
-	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
-		if (psdev->dev != NULL
-		    && domain == pci_domain_nr(psdev->dev->bus)
-		    && bus == psdev->dev->bus->number
-		    && slot == PCI_SLOT(psdev->dev->devfn)
-		    && func == PCI_FUNC(psdev->dev->devfn)) {
-			pcistub_device_get(psdev);
-			goto out;
-		}
-	}
+	psdev = pcistub_device_find_locked(domain, bus, slot, func);
+	if (psdev)
+		pcistub_device_get(psdev);
 
-	/* didn't find it */
-	psdev = NULL;
-
-out:
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 	return psdev;
 }
@@ -207,16 +214,9 @@ struct pci_dev *pcistub_get_pci_dev_by_slot(struct xen_pcibk_device *pdev,
 
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
-	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
-		if (psdev->dev != NULL
-		    && domain == pci_domain_nr(psdev->dev->bus)
-		    && bus == psdev->dev->bus->number
-		    && slot == PCI_SLOT(psdev->dev->devfn)
-		    && func == PCI_FUNC(psdev->dev->devfn)) {
-			found_dev = pcistub_device_get_pci_dev(pdev, psdev);
-			break;
-		}
-	}
+	psdev = pcistub_device_find_locked(domain, bus, slot, func);
+	if (psdev)
+		found_dev = pcistub_device_get_pci_dev(pdev, psdev);
 
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 	return found_dev;
-- 
2.6.6


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

[PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[Juergen Gross]

The Xen pciback driver has a list of all pci devices it is ready to
seize. There is no check whether a to be added entry already exists.
While this might be no problem in the common case it might confuse
those which consume the list via sysfs.

Modify the handling of this list by not adding an entry which already
exists. As this will be needed later split out the list handling into
a separate function.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 79a9e4d..0179333 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
 	return 0;
 }
 
+static void pcistub_device_id_add_list(struct pcistub_device_id *new,
+				       int domain, int bus, unsigned int devfn)
+{
+	struct pcistub_device_id *pci_dev_id;
+	unsigned long flags;
+	int found = 0;
+
+	spin_lock_irqsave(&device_ids_lock, flags);
+
+	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
+		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
+		    pci_dev_id->devfn == devfn) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (!found) {
+		new->domain = domain;
+		new->bus = bus;
+		new->devfn = devfn;
+		list_add_tail(&new->slot_list, &pcistub_device_ids);
+	}
+
+	spin_unlock_irqrestore(&device_ids_lock, flags);
+
+	if (found)
+		kfree(new);
+}
+
 static int pcistub_seize(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev;
@@ -1012,7 +1042,6 @@ static inline int str_to_quirk(const char *buf, int *domain, int *bus, int
 static int pcistub_device_id_add(int domain, int bus, int slot, int func)
 {
 	struct pcistub_device_id *pci_dev_id;
-	unsigned long flags;
 	int rc = 0, devfn = PCI_DEVFN(slot, func);
 
 	if (slot < 0) {
@@ -1042,16 +1071,10 @@ static int pcistub_device_id_add(int domain, int bus, int slot, int func)
 	if (!pci_dev_id)
 		return -ENOMEM;
 
-	pci_dev_id->domain = domain;
-	pci_dev_id->bus = bus;
-	pci_dev_id->devfn = devfn;
-
 	pr_debug("wants to seize %04x:%02x:%02x.%d\n",
 		 domain, bus, slot, func);
 
-	spin_lock_irqsave(&device_ids_lock, flags);
-	list_add_tail(&pci_dev_id->slot_list, &pcistub_device_ids);
-	spin_unlock_irqrestore(&device_ids_lock, flags);
+	pcistub_device_id_add_list(pci_dev_id, domain, bus, devfn);
 
 	return 0;
 }
-- 
2.6.6

[PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[Juergen Gross]

The Xen pciback driver has a list of all pci devices it is ready to
seize. There is no check whether a to be added entry already exists.
While this might be no problem in the common case it might confuse
those which consume the list via sysfs.

Modify the handling of this list by not adding an entry which already
exists. As this will be needed later split out the list handling into
a separate function.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 79a9e4d..0179333 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
 	return 0;
 }
 
+static void pcistub_device_id_add_list(struct pcistub_device_id *new,
+				       int domain, int bus, unsigned int devfn)
+{
+	struct pcistub_device_id *pci_dev_id;
+	unsigned long flags;
+	int found = 0;
+
+	spin_lock_irqsave(&device_ids_lock, flags);
+
+	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
+		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
+		    pci_dev_id->devfn == devfn) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (!found) {
+		new->domain = domain;
+		new->bus = bus;
+		new->devfn = devfn;
+		list_add_tail(&new->slot_list, &pcistub_device_ids);
+	}
+
+	spin_unlock_irqrestore(&device_ids_lock, flags);
+
+	if (found)
+		kfree(new);
+}
+
 static int pcistub_seize(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev;
@@ -1012,7 +1042,6 @@ static inline int str_to_quirk(const char *buf, int *domain, int *bus, int
 static int pcistub_device_id_add(int domain, int bus, int slot, int func)
 {
 	struct pcistub_device_id *pci_dev_id;
-	unsigned long flags;
 	int rc = 0, devfn = PCI_DEVFN(slot, func);
 
 	if (slot < 0) {
@@ -1042,16 +1071,10 @@ static int pcistub_device_id_add(int domain, int bus, int slot, int func)
 	if (!pci_dev_id)
 		return -ENOMEM;
 
-	pci_dev_id->domain = domain;
-	pci_dev_id->bus = bus;
-	pci_dev_id->devfn = devfn;
-
 	pr_debug("wants to seize %04x:%02x:%02x.%d\n",
 		 domain, bus, slot, func);
 
-	spin_lock_irqsave(&device_ids_lock, flags);
-	list_add_tail(&pci_dev_id->slot_list, &pcistub_device_ids);
-	spin_unlock_irqrestore(&device_ids_lock, flags);
+	pcistub_device_id_add_list(pci_dev_id, domain, bus, devfn);
 
 	return 0;
 }
-- 
2.6.6


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

[PATCH v3 3/3] 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.

Up to now for assigning a pci device to pciback 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.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3: Move override check up in order not to miss the PCI_HEADER_TYPE
    check.
    Add an assigned device to the slot list.

V2: Removed now unused label
---
 drivers/xen/xen-pciback/pci_stub.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 0179333..6331a95 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,
@@ -508,15 +510,18 @@ static void pcistub_device_id_add_list(struct pcistub_device_id *new,
 		kfree(new);
 }
 
-static int pcistub_seize(struct pci_dev *dev)
+static int pcistub_seize(struct pci_dev *dev,
+			 struct pcistub_device_id *pci_dev_id)
 {
 	struct pcistub_device *psdev;
 	unsigned long flags;
 	int err = 0;
 
 	psdev = pcistub_device_alloc(dev);
-	if (!psdev)
+	if (!psdev) {
+		kfree(pci_dev_id);
 		return -ENOMEM;
+	}
 
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
@@ -537,8 +542,12 @@ static int pcistub_seize(struct pci_dev *dev)
 
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 
-	if (err)
+	if (err) {
+		kfree(pci_dev_id);
 		pcistub_device_put(psdev);
+	} else if (pci_dev_id)
+		pcistub_device_id_add_list(pci_dev_id, pci_domain_nr(dev->bus),
+					   dev->bus->number, dev->devfn);
 
 	return err;
 }
@@ -547,11 +556,16 @@ static int pcistub_seize(struct pci_dev *dev)
  * other functions that take the sysfs lock. */
 static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	int err = 0;
+	int err = 0, match;
+	struct pcistub_device_id *pci_dev_id = NULL;
 
 	dev_dbg(&dev->dev, "probing...\n");
 
-	if (pcistub_match(dev)) {
+	match = pcistub_match(dev);
+
+	if ((dev->driver_override &&
+	     !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) ||
+	    match) {
 
 		if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL
 		    && dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
@@ -562,8 +576,16 @@ static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
 			goto out;
 		}
 
+		if (!match) {
+			pci_dev_id = kmalloc(sizeof(*pci_dev_id), GFP_ATOMIC);
+			if (!pci_dev_id) {
+				err = -ENOMEM;
+				goto out;
+			}
+		}
+
 		dev_info(&dev->dev, "seizing device\n");
-		err = pcistub_seize(dev);
+		err = pcistub_seize(dev, pci_dev_id);
 	} else
 		/* Didn't find the device */
 		err = -ENODEV;
@@ -975,7 +997,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

[PATCH v3 3/3] 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.

Up to now for assigning a pci device to pciback 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.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3: Move override check up in order not to miss the PCI_HEADER_TYPE
    check.
    Add an assigned device to the slot list.

V2: Removed now unused label
---
 drivers/xen/xen-pciback/pci_stub.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 0179333..6331a95 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,
@@ -508,15 +510,18 @@ static void pcistub_device_id_add_list(struct pcistub_device_id *new,
 		kfree(new);
 }
 
-static int pcistub_seize(struct pci_dev *dev)
+static int pcistub_seize(struct pci_dev *dev,
+			 struct pcistub_device_id *pci_dev_id)
 {
 	struct pcistub_device *psdev;
 	unsigned long flags;
 	int err = 0;
 
 	psdev = pcistub_device_alloc(dev);
-	if (!psdev)
+	if (!psdev) {
+		kfree(pci_dev_id);
 		return -ENOMEM;
+	}
 
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
@@ -537,8 +542,12 @@ static int pcistub_seize(struct pci_dev *dev)
 
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 
-	if (err)
+	if (err) {
+		kfree(pci_dev_id);
 		pcistub_device_put(psdev);
+	} else if (pci_dev_id)
+		pcistub_device_id_add_list(pci_dev_id, pci_domain_nr(dev->bus),
+					   dev->bus->number, dev->devfn);
 
 	return err;
 }
@@ -547,11 +556,16 @@ static int pcistub_seize(struct pci_dev *dev)
  * other functions that take the sysfs lock. */
 static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	int err = 0;
+	int err = 0, match;
+	struct pcistub_device_id *pci_dev_id = NULL;
 
 	dev_dbg(&dev->dev, "probing...\n");
 
-	if (pcistub_match(dev)) {
+	match = pcistub_match(dev);
+
+	if ((dev->driver_override &&
+	     !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) ||
+	    match) {
 
 		if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL
 		    && dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
@@ -562,8 +576,16 @@ static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
 			goto out;
 		}
 
+		if (!match) {
+			pci_dev_id = kmalloc(sizeof(*pci_dev_id), GFP_ATOMIC);
+			if (!pci_dev_id) {
+				err = -ENOMEM;
+				goto out;
+			}
+		}
+
 		dev_info(&dev->dev, "seizing device\n");
-		err = pcistub_seize(dev);
+		err = pcistub_seize(dev, pci_dev_id);
 	} else
 		/* Didn't find the device */
 		err = -ENODEV;
@@ -975,7 +997,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


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

Re: [PATCH v3 1/3] xen/pciback: simplify pcistub device handling

[Boris Ostrovsky]

On 09/22/2016 04:45 AM, Juergen Gross wrote:
> The Xen pciback driver maintains a list of all its seized devices.
> There are two functions searching the list for a specific device with
> basically the same semantics just returning different structures in
> case of a match.
>
> Split out the search function.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [PATCH v3 1/3] xen/pciback: simplify pcistub device handling

[Boris Ostrovsky]

On 09/22/2016 04:45 AM, Juergen Gross wrote:
> The Xen pciback driver maintains a list of all its seized devices.
> There are two functions searching the list for a specific device with
> basically the same semantics just returning different structures in
> case of a match.
>
> Split out the search function.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



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

Re: [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[Boris Ostrovsky]

On 09/22/2016 04:45 AM, Juergen Gross wrote:
> The Xen pciback driver has a list of all pci devices it is ready to
> seize. There is no check whether a to be added entry already exists.
> While this might be no problem in the common case it might confuse
> those which consume the list via sysfs.
>
> Modify the handling of this list by not adding an entry which already
> exists. As this will be needed later split out the list handling into
> a separate function.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 79a9e4d..0179333 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>  	return 0;
>  }
>  
> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
> +				       int domain, int bus, unsigned int devfn)
> +{
> +	struct pcistub_device_id *pci_dev_id;
> +	unsigned long flags;
> +	int found = 0;
> +
> +	spin_lock_irqsave(&device_ids_lock, flags);
> +
> +	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
> +		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
> +		    pci_dev_id->devfn == devfn) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		new->domain = domain;
> +		new->bus = bus;
> +		new->devfn = devfn;
> +		list_add_tail(&new->slot_list, &pcistub_device_ids);
> +	}
> +
> +	spin_unlock_irqrestore(&device_ids_lock, flags);
> +
> +	if (found)
> +		kfree(new);

I'd rather free 'new' in the caller (who allocated it) and return
something like -EEXIST if device is already on the list.

-boris

> +}
> +
>  static int pcistub_seize(struct pci_dev *dev)
>  {
>  	struct pcistub_device *psdev;
> @@ -1012,7 +1042,6 @@ static inline int str_to_quirk(const char *buf, int *domain, int *bus, int
>  static int pcistub_device_id_add(int domain, int bus, int slot, int func)
>  {
>  	struct pcistub_device_id *pci_dev_id;
> -	unsigned long flags;
>  	int rc = 0, devfn = PCI_DEVFN(slot, func);
>  
>  	if (slot < 0) {
> @@ -1042,16 +1071,10 @@ static int pcistub_device_id_add(int domain, int bus, int slot, int func)
>  	if (!pci_dev_id)
>  		return -ENOMEM;
>  
> -	pci_dev_id->domain = domain;
> -	pci_dev_id->bus = bus;
> -	pci_dev_id->devfn = devfn;
> -
>  	pr_debug("wants to seize %04x:%02x:%02x.%d\n",
>  		 domain, bus, slot, func);
>  
> -	spin_lock_irqsave(&device_ids_lock, flags);
> -	list_add_tail(&pci_dev_id->slot_list, &pcistub_device_ids);
> -	spin_unlock_irqrestore(&device_ids_lock, flags);
> +	pcistub_device_id_add_list(pci_dev_id, domain, bus, devfn);
>  
>  	return 0;
>  }

Re: [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[Boris Ostrovsky]

On 09/22/2016 04:45 AM, Juergen Gross wrote:
> The Xen pciback driver has a list of all pci devices it is ready to
> seize. There is no check whether a to be added entry already exists.
> While this might be no problem in the common case it might confuse
> those which consume the list via sysfs.
>
> Modify the handling of this list by not adding an entry which already
> exists. As this will be needed later split out the list handling into
> a separate function.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 79a9e4d..0179333 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>  	return 0;
>  }
>  
> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
> +				       int domain, int bus, unsigned int devfn)
> +{
> +	struct pcistub_device_id *pci_dev_id;
> +	unsigned long flags;
> +	int found = 0;
> +
> +	spin_lock_irqsave(&device_ids_lock, flags);
> +
> +	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
> +		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
> +		    pci_dev_id->devfn == devfn) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		new->domain = domain;
> +		new->bus = bus;
> +		new->devfn = devfn;
> +		list_add_tail(&new->slot_list, &pcistub_device_ids);
> +	}
> +
> +	spin_unlock_irqrestore(&device_ids_lock, flags);
> +
> +	if (found)
> +		kfree(new);

I'd rather free 'new' in the caller (who allocated it) and return
something like -EEXIST if device is already on the list.

-boris

> +}
> +
>  static int pcistub_seize(struct pci_dev *dev)
>  {
>  	struct pcistub_device *psdev;
> @@ -1012,7 +1042,6 @@ static inline int str_to_quirk(const char *buf, int *domain, int *bus, int
>  static int pcistub_device_id_add(int domain, int bus, int slot, int func)
>  {
>  	struct pcistub_device_id *pci_dev_id;
> -	unsigned long flags;
>  	int rc = 0, devfn = PCI_DEVFN(slot, func);
>  
>  	if (slot < 0) {
> @@ -1042,16 +1071,10 @@ static int pcistub_device_id_add(int domain, int bus, int slot, int func)
>  	if (!pci_dev_id)
>  		return -ENOMEM;
>  
> -	pci_dev_id->domain = domain;
> -	pci_dev_id->bus = bus;
> -	pci_dev_id->devfn = devfn;
> -
>  	pr_debug("wants to seize %04x:%02x:%02x.%d\n",
>  		 domain, bus, slot, func);
>  
> -	spin_lock_irqsave(&device_ids_lock, flags);
> -	list_add_tail(&pci_dev_id->slot_list, &pcistub_device_ids);
> -	spin_unlock_irqrestore(&device_ids_lock, flags);
> +	pcistub_device_id_add_list(pci_dev_id, domain, bus, devfn);
>  
>  	return 0;
>  }


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

Re: [PATCH v3 3/3] xen/pciback: support driver_override

[Boris Ostrovsky]

On 09/22/2016 04:45 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.
>
> Up to now for assigning a pci device to pciback 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.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3: Move override check up in order not to miss the PCI_HEADER_TYPE
>     check.
>     Add an assigned device to the slot list.
>
> V2: Removed now unused label
> ---
>  drivers/xen/xen-pciback/pci_stub.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 0179333..6331a95 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,
> @@ -508,15 +510,18 @@ static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>  		kfree(new);
>  }
>  
> -static int pcistub_seize(struct pci_dev *dev)
> +static int pcistub_seize(struct pci_dev *dev,
> +			 struct pcistub_device_id *pci_dev_id)
>  {
>  	struct pcistub_device *psdev;
>  	unsigned long flags;
>  	int err = 0;
>  
>  	psdev = pcistub_device_alloc(dev);
> -	if (!psdev)
> +	if (!psdev) {
> +		kfree(pci_dev_id);

Here too --- I'd rather have it freed by whoever allocated it (i.e. the
caller). In fact I think you can allocate this here and instead of
passing pci_dev_id pointer pass a flag of some sort.


-boris

>  		return -ENOMEM;
> +	}
>  
>  	spin_lock_irqsave(&pcistub_devices_lock, flags);
>  
> @@ -537,8 +542,12 @@ static int pcistub_seize(struct pci_dev *dev)
>  
>  	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>  
> -	if (err)
> +	if (err) {
> +		kfree(pci_dev_id);
>  		pcistub_device_put(psdev);
> +	} else if (pci_dev_id)
> +		pcistub_device_id_add_list(pci_dev_id, pci_domain_nr(dev->bus),
> +					   dev->bus->number, dev->devfn);
>  
>  	return err;
>  }
> @@ -547,11 +556,16 @@ static int pcistub_seize(struct pci_dev *dev)
>   * other functions that take the sysfs lock. */
>  static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> -	int err = 0;
> +	int err = 0, match;
> +	struct pcistub_device_id *pci_dev_id = NULL;
>  
>  	dev_dbg(&dev->dev, "probing...\n");
>  
> -	if (pcistub_match(dev)) {
> +	match = pcistub_match(dev);
> +
> +	if ((dev->driver_override &&
> +	     !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) ||
> +	    match) {
>  
>  		if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL
>  		    && dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
> @@ -562,8 +576,16 @@ static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  			goto out;
>  		}
>  
> +		if (!match) {
> +			pci_dev_id = kmalloc(sizeof(*pci_dev_id), GFP_ATOMIC);
> +			if (!pci_dev_id) {
> +				err = -ENOMEM;
> +				goto out;
> +			}
> +		}
> +
>  		dev_info(&dev->dev, "seizing device\n");
> -		err = pcistub_seize(dev);
> +		err = pcistub_seize(dev, pci_dev_id);
>  	} else
>  		/* Didn't find the device */
>  		err = -ENODEV;
> @@ -975,7 +997,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 v3 3/3] xen/pciback: support driver_override

[Boris Ostrovsky]

On 09/22/2016 04:45 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.
>
> Up to now for assigning a pci device to pciback 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.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3: Move override check up in order not to miss the PCI_HEADER_TYPE
>     check.
>     Add an assigned device to the slot list.
>
> V2: Removed now unused label
> ---
>  drivers/xen/xen-pciback/pci_stub.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 0179333..6331a95 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,
> @@ -508,15 +510,18 @@ static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>  		kfree(new);
>  }
>  
> -static int pcistub_seize(struct pci_dev *dev)
> +static int pcistub_seize(struct pci_dev *dev,
> +			 struct pcistub_device_id *pci_dev_id)
>  {
>  	struct pcistub_device *psdev;
>  	unsigned long flags;
>  	int err = 0;
>  
>  	psdev = pcistub_device_alloc(dev);
> -	if (!psdev)
> +	if (!psdev) {
> +		kfree(pci_dev_id);

Here too --- I'd rather have it freed by whoever allocated it (i.e. the
caller). In fact I think you can allocate this here and instead of
passing pci_dev_id pointer pass a flag of some sort.


-boris

>  		return -ENOMEM;
> +	}
>  
>  	spin_lock_irqsave(&pcistub_devices_lock, flags);
>  
> @@ -537,8 +542,12 @@ static int pcistub_seize(struct pci_dev *dev)
>  
>  	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>  
> -	if (err)
> +	if (err) {
> +		kfree(pci_dev_id);
>  		pcistub_device_put(psdev);
> +	} else if (pci_dev_id)
> +		pcistub_device_id_add_list(pci_dev_id, pci_domain_nr(dev->bus),
> +					   dev->bus->number, dev->devfn);
>  
>  	return err;
>  }
> @@ -547,11 +556,16 @@ static int pcistub_seize(struct pci_dev *dev)
>   * other functions that take the sysfs lock. */
>  static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> -	int err = 0;
> +	int err = 0, match;
> +	struct pcistub_device_id *pci_dev_id = NULL;
>  
>  	dev_dbg(&dev->dev, "probing...\n");
>  
> -	if (pcistub_match(dev)) {
> +	match = pcistub_match(dev);
> +
> +	if ((dev->driver_override &&
> +	     !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) ||
> +	    match) {
>  
>  		if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL
>  		    && dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
> @@ -562,8 +576,16 @@ static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  			goto out;
>  		}
>  
> +		if (!match) {
> +			pci_dev_id = kmalloc(sizeof(*pci_dev_id), GFP_ATOMIC);
> +			if (!pci_dev_id) {
> +				err = -ENOMEM;
> +				goto out;
> +			}
> +		}
> +
>  		dev_info(&dev->dev, "seizing device\n");
> -		err = pcistub_seize(dev);
> +		err = pcistub_seize(dev, pci_dev_id);
>  	} else
>  		/* Didn't find the device */
>  		err = -ENODEV;
> @@ -975,7 +997,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 v3 2/3] xen/pciback: avoid multiple entries in slot list

[Juergen Gross]

On 22/09/16 23:02, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>> The Xen pciback driver has a list of all pci devices it is ready to
>> seize. There is no check whether a to be added entry already exists.
>> While this might be no problem in the common case it might confuse
>> those which consume the list via sysfs.
>>
>> Modify the handling of this list by not adding an entry which already
>> exists. As this will be needed later split out the list handling into
>> a separate function.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 79a9e4d..0179333 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>  	return 0;
>>  }
>>  
>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>> +				       int domain, int bus, unsigned int devfn)
>> +{
>> +	struct pcistub_device_id *pci_dev_id;
>> +	unsigned long flags;
>> +	int found = 0;
>> +
>> +	spin_lock_irqsave(&device_ids_lock, flags);
>> +
>> +	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
>> +		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
>> +		    pci_dev_id->devfn == devfn) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!found) {
>> +		new->domain = domain;
>> +		new->bus = bus;
>> +		new->devfn = devfn;
>> +		list_add_tail(&new->slot_list, &pcistub_device_ids);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&device_ids_lock, flags);
>> +
>> +	if (found)
>> +		kfree(new);
> 
> I'd rather free 'new' in the caller (who allocated it) and return
> something like -EEXIST if device is already on the list.

Hmm, I thought of this, but with two callers after the following patch
having to deal with the situation I've chosen this way to do it.

The code is smaller this way.


Juergen

Re: [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[Juergen Gross]

On 22/09/16 23:02, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>> The Xen pciback driver has a list of all pci devices it is ready to
>> seize. There is no check whether a to be added entry already exists.
>> While this might be no problem in the common case it might confuse
>> those which consume the list via sysfs.
>>
>> Modify the handling of this list by not adding an entry which already
>> exists. As this will be needed later split out the list handling into
>> a separate function.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 79a9e4d..0179333 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>  	return 0;
>>  }
>>  
>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>> +				       int domain, int bus, unsigned int devfn)
>> +{
>> +	struct pcistub_device_id *pci_dev_id;
>> +	unsigned long flags;
>> +	int found = 0;
>> +
>> +	spin_lock_irqsave(&device_ids_lock, flags);
>> +
>> +	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
>> +		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
>> +		    pci_dev_id->devfn == devfn) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!found) {
>> +		new->domain = domain;
>> +		new->bus = bus;
>> +		new->devfn = devfn;
>> +		list_add_tail(&new->slot_list, &pcistub_device_ids);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&device_ids_lock, flags);
>> +
>> +	if (found)
>> +		kfree(new);
> 
> I'd rather free 'new' in the caller (who allocated it) and return
> something like -EEXIST if device is already on the list.

Hmm, I thought of this, but with two callers after the following patch
having to deal with the situation I've chosen this way to do it.

The code is smaller this way.


Juergen

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

Re: [PATCH v3 3/3] xen/pciback: support driver_override

[Juergen Gross]

On 22/09/16 23:10, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 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.
>>
>> Up to now for assigning a pci device to pciback 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.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3: Move override check up in order not to miss the PCI_HEADER_TYPE
>>     check.
>>     Add an assigned device to the slot list.
>>
>> V2: Removed now unused label
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 36 +++++++++++++++++++++++++++++-------
>>  1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 0179333..6331a95 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,
>> @@ -508,15 +510,18 @@ static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>>  		kfree(new);
>>  }
>>  
>> -static int pcistub_seize(struct pci_dev *dev)
>> +static int pcistub_seize(struct pci_dev *dev,
>> +			 struct pcistub_device_id *pci_dev_id)
>>  {
>>  	struct pcistub_device *psdev;
>>  	unsigned long flags;
>>  	int err = 0;
>>  
>>  	psdev = pcistub_device_alloc(dev);
>> -	if (!psdev)
>> +	if (!psdev) {
>> +		kfree(pci_dev_id);
> 
> Here too --- I'd rather have it freed by whoever allocated it (i.e. the
> caller). In fact I think you can allocate this here and instead of
> passing pci_dev_id pointer pass a flag of some sort.

Allocating it here will become nasty: The allocation flags are
different in the two cases, so I'd have to either pass the flags
or have to use GFP_ATOMIC in both cases.

And dealing with an allocation failure at the very end of
pcistub_seize() would require quite some rollback action.


Juergen

> -boris
> 
>>  		return -ENOMEM;
>> +	}
>>  
>>  	spin_lock_irqsave(&pcistub_devices_lock, flags);
>>  
>> @@ -537,8 +542,12 @@ static int pcistub_seize(struct pci_dev *dev)
>>  
>>  	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>>  
>> -	if (err)
>> +	if (err) {
>> +		kfree(pci_dev_id);
>>  		pcistub_device_put(psdev);
>> +	} else if (pci_dev_id)
>> +		pcistub_device_id_add_list(pci_dev_id, pci_domain_nr(dev->bus),
>> +					   dev->bus->number, dev->devfn);
>>  
>>  	return err;
>>  }
>> @@ -547,11 +556,16 @@ static int pcistub_seize(struct pci_dev *dev)
>>   * other functions that take the sysfs lock. */
>>  static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  {
>> -	int err = 0;
>> +	int err = 0, match;
>> +	struct pcistub_device_id *pci_dev_id = NULL;
>>  
>>  	dev_dbg(&dev->dev, "probing...\n");
>>  
>> -	if (pcistub_match(dev)) {
>> +	match = pcistub_match(dev);
>> +
>> +	if ((dev->driver_override &&
>> +	     !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) ||
>> +	    match) {
>>  
>>  		if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL
>>  		    && dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
>> @@ -562,8 +576,16 @@ static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  			goto out;
>>  		}
>>  
>> +		if (!match) {
>> +			pci_dev_id = kmalloc(sizeof(*pci_dev_id), GFP_ATOMIC);
>> +			if (!pci_dev_id) {
>> +				err = -ENOMEM;
>> +				goto out;
>> +			}
>> +		}
>> +
>>  		dev_info(&dev->dev, "seizing device\n");
>> -		err = pcistub_seize(dev);
>> +		err = pcistub_seize(dev, pci_dev_id);
>>  	} else
>>  		/* Didn't find the device */
>>  		err = -ENODEV;
>> @@ -975,7 +997,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 v3 3/3] xen/pciback: support driver_override

[Juergen Gross]

On 22/09/16 23:10, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 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.
>>
>> Up to now for assigning a pci device to pciback 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.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3: Move override check up in order not to miss the PCI_HEADER_TYPE
>>     check.
>>     Add an assigned device to the slot list.
>>
>> V2: Removed now unused label
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 36 +++++++++++++++++++++++++++++-------
>>  1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 0179333..6331a95 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,
>> @@ -508,15 +510,18 @@ static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>>  		kfree(new);
>>  }
>>  
>> -static int pcistub_seize(struct pci_dev *dev)
>> +static int pcistub_seize(struct pci_dev *dev,
>> +			 struct pcistub_device_id *pci_dev_id)
>>  {
>>  	struct pcistub_device *psdev;
>>  	unsigned long flags;
>>  	int err = 0;
>>  
>>  	psdev = pcistub_device_alloc(dev);
>> -	if (!psdev)
>> +	if (!psdev) {
>> +		kfree(pci_dev_id);
> 
> Here too --- I'd rather have it freed by whoever allocated it (i.e. the
> caller). In fact I think you can allocate this here and instead of
> passing pci_dev_id pointer pass a flag of some sort.

Allocating it here will become nasty: The allocation flags are
different in the two cases, so I'd have to either pass the flags
or have to use GFP_ATOMIC in both cases.

And dealing with an allocation failure at the very end of
pcistub_seize() would require quite some rollback action.


Juergen

> -boris
> 
>>  		return -ENOMEM;
>> +	}
>>  
>>  	spin_lock_irqsave(&pcistub_devices_lock, flags);
>>  
>> @@ -537,8 +542,12 @@ static int pcistub_seize(struct pci_dev *dev)
>>  
>>  	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>>  
>> -	if (err)
>> +	if (err) {
>> +		kfree(pci_dev_id);
>>  		pcistub_device_put(psdev);
>> +	} else if (pci_dev_id)
>> +		pcistub_device_id_add_list(pci_dev_id, pci_domain_nr(dev->bus),
>> +					   dev->bus->number, dev->devfn);
>>  
>>  	return err;
>>  }
>> @@ -547,11 +556,16 @@ static int pcistub_seize(struct pci_dev *dev)
>>   * other functions that take the sysfs lock. */
>>  static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  {
>> -	int err = 0;
>> +	int err = 0, match;
>> +	struct pcistub_device_id *pci_dev_id = NULL;
>>  
>>  	dev_dbg(&dev->dev, "probing...\n");
>>  
>> -	if (pcistub_match(dev)) {
>> +	match = pcistub_match(dev);
>> +
>> +	if ((dev->driver_override &&
>> +	     !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) ||
>> +	    match) {
>>  
>>  		if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL
>>  		    && dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
>> @@ -562,8 +576,16 @@ static int pcistub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  			goto out;
>>  		}
>>  
>> +		if (!match) {
>> +			pci_dev_id = kmalloc(sizeof(*pci_dev_id), GFP_ATOMIC);
>> +			if (!pci_dev_id) {
>> +				err = -ENOMEM;
>> +				goto out;
>> +			}
>> +		}
>> +
>>  		dev_info(&dev->dev, "seizing device\n");
>> -		err = pcistub_seize(dev);
>> +		err = pcistub_seize(dev, pci_dev_id);
>>  	} else
>>  		/* Didn't find the device */
>>  		err = -ENODEV;
>> @@ -975,7 +997,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: [Xen-devel] [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[David Vrabel]

On 22/09/16 22:02, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>> The Xen pciback driver has a list of all pci devices it is ready to
>> seize. There is no check whether a to be added entry already exists.
>> While this might be no problem in the common case it might confuse
>> those which consume the list via sysfs.
>>
>> Modify the handling of this list by not adding an entry which already
>> exists. As this will be needed later split out the list handling into
>> a separate function.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 79a9e4d..0179333 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>  	return 0;
>>  }
>>  
>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>> +				       int domain, int bus, unsigned int devfn)

I think this should allocate the new pcistub_device_id if needed.  You
can pass in GFP flags if needed.

Then it can return the newly allocated one, or the existing one.

static struct pcistub_device_id *pcistub_device_id_add_list(
    int domain, int bus, unsigned int devfn)

David

Re: [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[David Vrabel]

On 22/09/16 22:02, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>> The Xen pciback driver has a list of all pci devices it is ready to
>> seize. There is no check whether a to be added entry already exists.
>> While this might be no problem in the common case it might confuse
>> those which consume the list via sysfs.
>>
>> Modify the handling of this list by not adding an entry which already
>> exists. As this will be needed later split out the list handling into
>> a separate function.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 79a9e4d..0179333 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>  	return 0;
>>  }
>>  
>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>> +				       int domain, int bus, unsigned int devfn)

I think this should allocate the new pcistub_device_id if needed.  You
can pass in GFP flags if needed.

Then it can return the newly allocated one, or the existing one.

static struct pcistub_device_id *pcistub_device_id_add_list(
    int domain, int bus, unsigned int devfn)

David

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

Re: [Xen-devel] [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[Juergen Gross]

On 23/09/16 12:15, David Vrabel wrote:
> On 22/09/16 22:02, Boris Ostrovsky wrote:
>> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>>> The Xen pciback driver has a list of all pci devices it is ready to
>>> seize. There is no check whether a to be added entry already exists.
>>> While this might be no problem in the common case it might confuse
>>> those which consume the list via sysfs.
>>>
>>> Modify the handling of this list by not adding an entry which already
>>> exists. As this will be needed later split out the list handling into
>>> a separate function.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>>> index 79a9e4d..0179333 100644
>>> --- a/drivers/xen/xen-pciback/pci_stub.c
>>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>>  	return 0;
>>>  }
>>>  
>>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>>> +				       int domain, int bus, unsigned int devfn)
> 
> I think this should allocate the new pcistub_device_id if needed.  You
> can pass in GFP flags if needed.
> 
> Then it can return the newly allocated one, or the existing one.
> 
> static struct pcistub_device_id *pcistub_device_id_add_list(
>     int domain, int bus, unsigned int devfn)

Patch 3 will be very nasty then: in case of an allocation failure
all of the actions done in pcistub_seize() will have to be undone
again. I'd really like to avoid that.


Juergen

Re: [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

[Juergen Gross]

On 23/09/16 12:15, David Vrabel wrote:
> On 22/09/16 22:02, Boris Ostrovsky wrote:
>> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>>> The Xen pciback driver has a list of all pci devices it is ready to
>>> seize. There is no check whether a to be added entry already exists.
>>> While this might be no problem in the common case it might confuse
>>> those which consume the list via sysfs.
>>>
>>> Modify the handling of this list by not adding an entry which already
>>> exists. As this will be needed later split out the list handling into
>>> a separate function.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>>> index 79a9e4d..0179333 100644
>>> --- a/drivers/xen/xen-pciback/pci_stub.c
>>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>>  	return 0;
>>>  }
>>>  
>>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>>> +				       int domain, int bus, unsigned int devfn)
> 
> I think this should allocate the new pcistub_device_id if needed.  You
> can pass in GFP flags if needed.
> 
> Then it can return the newly allocated one, or the existing one.
> 
> static struct pcistub_device_id *pcistub_device_id_add_list(
>     int domain, int bus, unsigned int devfn)

Patch 3 will be very nasty then: in case of an allocation failure
all of the actions done in pcistub_seize() will have to be undone
again. I'd really like to avoid that.


Juergen

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

Re: [Xen-devel] [PATCH v3 0/3] xen/pciback: support driver_override

[David Vrabel]

On 22/09/16 09:45, Juergen Gross wrote:
> Support the driver_override scheme introduced with commit 782a985d7af2
> ("PCI: Introduce new device binding path using pci_dev.driver_override")

Applied to for-linus-4.9, thanks.

David

Re: [PATCH v3 0/3] xen/pciback: support driver_override

[David Vrabel]

On 22/09/16 09:45, Juergen Gross wrote:
> Support the driver_override scheme introduced with commit 782a985d7af2
> ("PCI: Introduce new device binding path using pci_dev.driver_override")

Applied to for-linus-4.9, thanks.

David

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

[PATCH v3 0/3] 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")

Today you'd 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

Changes in V3:
- Added new patches 1 and 2 to do some cleanup and preparatory work.
- patch 3: Add an assigned device to the slot list
- patch 3: Move override check up in order not to miss the PCI_HEADER_TYPE
  check.

Changes in V2:
- Removed now unused label.

*** BLURB HERE ***

Juergen Gross (3):
  xen/pciback: simplify pcistub device handling
  xen/pciback: avoid multiple entries in slot list
  xen/pciback: support driver_override

 drivers/xen/xen-pciback/pci_stub.c | 125 +++++++++++++++++++++++++------------
 1 file changed, 85 insertions(+), 40 deletions(-)

-- 
2.6.6


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