[PATCHv1 0/2] xen-pciback: allow device reset to work more often

[PATCHv1 0/2] xen-pciback: allow device reset to work more often

[David Vrabel]

When a PCI device is passed through to a different domain, it must be
reset to ensure it works correctly in the new domain.  Usually the
toolstack will request a device reset via the "reset" sysfs file.  But
this file is not present if a function reset is not available.

A common use case is a GPU device and its associated audio function.
In the case where these two devices are simultaneously co-assigned to
the same domain, it is safe to reset them with an SBR.

Patch #1 export pci_probe_reset_function() so pciback can know if an
alternate reset mechanism needs to be proved.

Patch #2 adds a "reset" file to devices bound to pciback that performs
the SBR if it is safe to do so.

David

[PATCH 1/2] pci: export pci_probe_reset_function()

[David Vrabel]

The xen-pciback needs to test if a per-function reset is available so
it can provide an interface for a safe bus reset instead.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/pci/pci.c   |    1 +
 drivers/pci/pci.h   |    1 -
 include/linux/pci.h |    1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 63a54a3..950e517 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3459,6 +3459,7 @@ int pci_probe_reset_function(struct pci_dev *dev)
 {
 	return pci_dev_reset(dev, 1);
 }
+EXPORT_SYMBOL_GPL(pci_probe_reset_function);
 
 /**
  * pci_reset_function - quiesce and reset a PCI device function
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..7be87d4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -28,7 +28,6 @@ enum pci_mmap_api {
 int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
 		  enum pci_mmap_api mmap_api);
 #endif
-int pci_probe_reset_function(struct pci_dev *dev);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 466bcd1..a2d9871f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -968,6 +968,7 @@ int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
 			  enum pcie_link_width *width);
+int pci_probe_reset_function(struct pci_dev *slot);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
-- 
1.7.10.4

[PATCH 1/2] pci: export pci_probe_reset_function()

[David Vrabel]

The xen-pciback needs to test if a per-function reset is available so
it can provide an interface for a safe bus reset instead.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/pci/pci.c   |    1 +
 drivers/pci/pci.h   |    1 -
 include/linux/pci.h |    1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 63a54a3..950e517 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3459,6 +3459,7 @@ int pci_probe_reset_function(struct pci_dev *dev)
 {
 	return pci_dev_reset(dev, 1);
 }
+EXPORT_SYMBOL_GPL(pci_probe_reset_function);
 
 /**
  * pci_reset_function - quiesce and reset a PCI device function
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..7be87d4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -28,7 +28,6 @@ enum pci_mmap_api {
 int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
 		  enum pci_mmap_api mmap_api);
 #endif
-int pci_probe_reset_function(struct pci_dev *dev);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 466bcd1..a2d9871f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -968,6 +968,7 @@ int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
 			  enum pcie_link_width *width);
+int pci_probe_reset_function(struct pci_dev *slot);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
-- 
1.7.10.4

[PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[David Vrabel]

The standard implementation of pci_reset_function() does not try an
SBR if there are other sibling devices.  This is a common
configuration for multi-function devices (e.g., GPUs with a HDMI audio
device function).

If all the functions are co-assigned to the same domain at the same
time, then it is safe to perform an SBR to reset all functions at the
same time.

Add a "reset" sysfs file with the same interface as the standard one
(i.e., write 1 to reset) that will try an SBR if all sibling devices
are unbound or bound to pciback.

Note that this is weaker than the requirement for functions to be
simultaneously co-assigned, but the toolstack is expected to ensure
this.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
Changes in v3:
- probe for function reset
- use pci_reset_bus().

Changes in v2:
- defer creating sysfs node to late init.
---
 drivers/xen/xen-pciback/pci_stub.c |  107 +++++++++++++++++++++++++++++++++++-
 drivers/xen/xen-pciback/pciback.h  |    1 +
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index d57a173..c7deceb 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -63,6 +63,100 @@ static LIST_HEAD(pcistub_devices);
 static int initialize_devices;
 static LIST_HEAD(seized_devices);
 
+/*
+ * pci_reset_function() will only work if there is a mechanism to
+ * reset that single function (e.g., FLR or a D-state transition).
+ * For PCI hardware that has two or more functions but no per-function
+ * reset, we can do a bus reset iff all the functions are co-assigned
+ * to the same domain.
+ *
+ * If a function has no per-function reset mechanism the 'reset' sysfs
+ * file that the toolstack uses to reset a function prior to assigning
+ * the device will be missing.  In this case, pciback adds its own
+ * which will try a bus reset.
+ *
+ * Note: pciback does not check for co-assigment before doing a bus
+ * reset, only that the devices are bound to pciback.  The toolstack
+ * is assumed to have done the right thing.
+ */
+static int __pcistub_reset_function(struct pci_dev *dev)
+{
+	struct pci_dev *pdev;
+	int ret;
+
+	ret = __pci_reset_function_locked(dev);
+	if (ret == 0)
+		return 0;
+
+	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
+		return -ENOTTY;
+
+	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
+		if (pdev != dev && (!pdev->driver
+				    || strcmp(pdev->driver->name, "pciback")))
+			return -ENOTTY;
+	}
+
+	return pci_reset_bus(dev->bus);
+}
+
+static int pcistub_reset_function(struct pci_dev *dev)
+{
+	int ret;
+
+	device_lock(&dev->dev);
+	ret = __pcistub_reset_function(dev);
+	device_unlock(&dev->dev);
+
+	return ret;
+}
+
+static ssize_t pcistub_reset_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long val;
+	ssize_t result = strict_strtoul(buf, 0, &val);
+
+	if (result < 0)
+		return result;
+
+	if (val != 1)
+		return -EINVAL;
+
+	result = pcistub_reset_function(pdev);
+	if (result < 0)
+		return result;
+	return count;
+}
+static DEVICE_ATTR(reset, 0200, NULL, pcistub_reset_store);
+
+static int pcistub_try_create_reset_file(struct pci_dev *pci)
+{
+	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
+	struct device *dev = &pci->dev;
+	int ret;
+
+	/* Already have a per-function reset? */
+	if (pci_probe_reset_function(pci) == 0)
+		return 0;
+
+	ret = device_create_file(dev, &dev_attr_reset);
+	if (ret < 0)
+		return ret;
+	dev_data->created_reset_file = true;
+	return 0;
+}
+
+static void pcistub_remove_reset_file(struct pci_dev *pci)
+{
+	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
+
+	if (dev_data && dev_data->created_reset_file)
+		device_remove_file(&pci->dev, &dev_attr_reset);
+}
+
 static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev;
@@ -103,7 +197,8 @@ static void pcistub_device_release(struct kref *kref)
 	/* Call the reset function which does not take lock as this
 	 * is called from "unbind" which takes a device_lock mutex.
 	 */
-	__pci_reset_function_locked(dev);
+	__pcistub_reset_function(psdev->dev);
+
 	if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
 		dev_dbg(&dev->dev, "Could not reload PCI state\n");
 	else
@@ -280,7 +375,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	/* This is OK - we are running from workqueue context
 	 * and want to inhibit the user from fiddling with 'reset'
 	 */
-	pci_reset_function(dev);
+	pcistub_reset_function(dev);
 	pci_restore_state(dev);
 
 	/* This disables the device. */
@@ -404,7 +499,7 @@ static int pcistub_init_device(struct pci_dev *dev)
 		dev_err(&dev->dev, "Could not store PCI conf saved state!\n");
 	else {
 		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
-		__pci_reset_function_locked(dev);
+		__pcistub_reset_function(dev);
 		pci_restore_state(dev);
 	}
 	/* Now disable the device (this also ensures some private device
@@ -413,6 +508,10 @@ static int pcistub_init_device(struct pci_dev *dev)
 	dev_dbg(&dev->dev, "reset device\n");
 	xen_pcibk_reset_device(dev);
 
+	err = pcistub_try_create_reset_file(dev);
+	if (err < 0)
+		goto config_release;
+
 	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
 	return 0;
 
@@ -540,6 +639,8 @@ static void pcistub_remove(struct pci_dev *dev)
 
 	dev_dbg(&dev->dev, "removing\n");
 
+	pcistub_remove_reset_file(dev);
+
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
 	xen_pcibk_config_quirk_release(dev);
diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
index f72af87..708ade9 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -42,6 +42,7 @@ struct xen_pcibk_device {
 struct xen_pcibk_dev_data {
 	struct list_head config_fields;
 	struct pci_saved_state *pci_saved_state;
+	bool created_reset_file;
 	unsigned int permissive:1;
 	unsigned int warned_on_write:1;
 	unsigned int enable_intx:1;
-- 
1.7.10.4

[PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[David Vrabel]

The standard implementation of pci_reset_function() does not try an
SBR if there are other sibling devices.  This is a common
configuration for multi-function devices (e.g., GPUs with a HDMI audio
device function).

If all the functions are co-assigned to the same domain at the same
time, then it is safe to perform an SBR to reset all functions at the
same time.

Add a "reset" sysfs file with the same interface as the standard one
(i.e., write 1 to reset) that will try an SBR if all sibling devices
are unbound or bound to pciback.

Note that this is weaker than the requirement for functions to be
simultaneously co-assigned, but the toolstack is expected to ensure
this.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
Changes in v3:
- probe for function reset
- use pci_reset_bus().

Changes in v2:
- defer creating sysfs node to late init.
---
 drivers/xen/xen-pciback/pci_stub.c |  107 +++++++++++++++++++++++++++++++++++-
 drivers/xen/xen-pciback/pciback.h  |    1 +
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index d57a173..c7deceb 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -63,6 +63,100 @@ static LIST_HEAD(pcistub_devices);
 static int initialize_devices;
 static LIST_HEAD(seized_devices);
 
+/*
+ * pci_reset_function() will only work if there is a mechanism to
+ * reset that single function (e.g., FLR or a D-state transition).
+ * For PCI hardware that has two or more functions but no per-function
+ * reset, we can do a bus reset iff all the functions are co-assigned
+ * to the same domain.
+ *
+ * If a function has no per-function reset mechanism the 'reset' sysfs
+ * file that the toolstack uses to reset a function prior to assigning
+ * the device will be missing.  In this case, pciback adds its own
+ * which will try a bus reset.
+ *
+ * Note: pciback does not check for co-assigment before doing a bus
+ * reset, only that the devices are bound to pciback.  The toolstack
+ * is assumed to have done the right thing.
+ */
+static int __pcistub_reset_function(struct pci_dev *dev)
+{
+	struct pci_dev *pdev;
+	int ret;
+
+	ret = __pci_reset_function_locked(dev);
+	if (ret == 0)
+		return 0;
+
+	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
+		return -ENOTTY;
+
+	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
+		if (pdev != dev && (!pdev->driver
+				    || strcmp(pdev->driver->name, "pciback")))
+			return -ENOTTY;
+	}
+
+	return pci_reset_bus(dev->bus);
+}
+
+static int pcistub_reset_function(struct pci_dev *dev)
+{
+	int ret;
+
+	device_lock(&dev->dev);
+	ret = __pcistub_reset_function(dev);
+	device_unlock(&dev->dev);
+
+	return ret;
+}
+
+static ssize_t pcistub_reset_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long val;
+	ssize_t result = strict_strtoul(buf, 0, &val);
+
+	if (result < 0)
+		return result;
+
+	if (val != 1)
+		return -EINVAL;
+
+	result = pcistub_reset_function(pdev);
+	if (result < 0)
+		return result;
+	return count;
+}
+static DEVICE_ATTR(reset, 0200, NULL, pcistub_reset_store);
+
+static int pcistub_try_create_reset_file(struct pci_dev *pci)
+{
+	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
+	struct device *dev = &pci->dev;
+	int ret;
+
+	/* Already have a per-function reset? */
+	if (pci_probe_reset_function(pci) == 0)
+		return 0;
+
+	ret = device_create_file(dev, &dev_attr_reset);
+	if (ret < 0)
+		return ret;
+	dev_data->created_reset_file = true;
+	return 0;
+}
+
+static void pcistub_remove_reset_file(struct pci_dev *pci)
+{
+	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
+
+	if (dev_data && dev_data->created_reset_file)
+		device_remove_file(&pci->dev, &dev_attr_reset);
+}
+
 static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev;
@@ -103,7 +197,8 @@ static void pcistub_device_release(struct kref *kref)
 	/* Call the reset function which does not take lock as this
 	 * is called from "unbind" which takes a device_lock mutex.
 	 */
-	__pci_reset_function_locked(dev);
+	__pcistub_reset_function(psdev->dev);
+
 	if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
 		dev_dbg(&dev->dev, "Could not reload PCI state\n");
 	else
@@ -280,7 +375,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	/* This is OK - we are running from workqueue context
 	 * and want to inhibit the user from fiddling with 'reset'
 	 */
-	pci_reset_function(dev);
+	pcistub_reset_function(dev);
 	pci_restore_state(dev);
 
 	/* This disables the device. */
@@ -404,7 +499,7 @@ static int pcistub_init_device(struct pci_dev *dev)
 		dev_err(&dev->dev, "Could not store PCI conf saved state!\n");
 	else {
 		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
-		__pci_reset_function_locked(dev);
+		__pcistub_reset_function(dev);
 		pci_restore_state(dev);
 	}
 	/* Now disable the device (this also ensures some private device
@@ -413,6 +508,10 @@ static int pcistub_init_device(struct pci_dev *dev)
 	dev_dbg(&dev->dev, "reset device\n");
 	xen_pcibk_reset_device(dev);
 
+	err = pcistub_try_create_reset_file(dev);
+	if (err < 0)
+		goto config_release;
+
 	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
 	return 0;
 
@@ -540,6 +639,8 @@ static void pcistub_remove(struct pci_dev *dev)
 
 	dev_dbg(&dev->dev, "removing\n");
 
+	pcistub_remove_reset_file(dev);
+
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
 	xen_pcibk_config_quirk_release(dev);
diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
index f72af87..708ade9 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -42,6 +42,7 @@ struct xen_pcibk_device {
 struct xen_pcibk_dev_data {
 	struct list_head config_fields;
 	struct pci_saved_state *pci_saved_state;
+	bool created_reset_file;
 	unsigned int permissive:1;
 	unsigned int warned_on_write:1;
 	unsigned int enable_intx:1;
-- 
1.7.10.4

Re: [PATCHv1 0/2] xen-pciback: allow device reset to work more often

[Bjorn Helgaas]

[+cc Alex]

On Thu, Jul 10, 2014 at 7:03 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> When a PCI device is passed through to a different domain, it must be
> reset to ensure it works correctly in the new domain.  Usually the
> toolstack will request a device reset via the "reset" sysfs file.  But
> this file is not present if a function reset is not available.
>
> A common use case is a GPU device and its associated audio function.
> In the case where these two devices are simultaneously co-assigned to
> the same domain, it is safe to reset them with an SBR.
>
> Patch #1 export pci_probe_reset_function() so pciback can know if an
> alternate reset mechanism needs to be proved.
>
> Patch #2 adds a "reset" file to devices bound to pciback that performs
> the SBR if it is safe to do so.

I haven't looked at your patches yet, but I added Alex because he did
a lot of similar work for vfio.

Bjorn

Re: [PATCHv1 0/2] xen-pciback: allow device reset to work more often

[Bjorn Helgaas]

[+cc Alex]

On Thu, Jul 10, 2014 at 7:03 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> When a PCI device is passed through to a different domain, it must be
> reset to ensure it works correctly in the new domain.  Usually the
> toolstack will request a device reset via the "reset" sysfs file.  But
> this file is not present if a function reset is not available.
>
> A common use case is a GPU device and its associated audio function.
> In the case where these two devices are simultaneously co-assigned to
> the same domain, it is safe to reset them with an SBR.
>
> Patch #1 export pci_probe_reset_function() so pciback can know if an
> alternate reset mechanism needs to be proved.
>
> Patch #2 adds a "reset" file to devices bound to pciback that performs
> the SBR if it is safe to do so.

I haven't looked at your patches yet, but I added Alex because he did
a lot of similar work for vfio.

Bjorn

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[Alex Williamson]

On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
> The standard implementation of pci_reset_function() does not try an
> SBR if there are other sibling devices.  This is a common
> configuration for multi-function devices (e.g., GPUs with a HDMI audio
> device function).
> 
> If all the functions are co-assigned to the same domain at the same
> time, then it is safe to perform an SBR to reset all functions at the
> same time.
> 
> Add a "reset" sysfs file with the same interface as the standard one
> (i.e., write 1 to reset) that will try an SBR if all sibling devices
> are unbound or bound to pciback.
> 
> Note that this is weaker than the requirement for functions to be
> simultaneously co-assigned, but the toolstack is expected to ensure
> this.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> Changes in v3:
> - probe for function reset
> - use pci_reset_bus().
> 
> Changes in v2:
> - defer creating sysfs node to late init.
> ---
>  drivers/xen/xen-pciback/pci_stub.c |  107 +++++++++++++++++++++++++++++++++++-
>  drivers/xen/xen-pciback/pciback.h  |    1 +
>  2 files changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index d57a173..c7deceb 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -63,6 +63,100 @@ static LIST_HEAD(pcistub_devices);
>  static int initialize_devices;
>  static LIST_HEAD(seized_devices);
>  
> +/*
> + * pci_reset_function() will only work if there is a mechanism to
> + * reset that single function (e.g., FLR or a D-state transition).
> + * For PCI hardware that has two or more functions but no per-function
> + * reset, we can do a bus reset iff all the functions are co-assigned
> + * to the same domain.
> + *
> + * If a function has no per-function reset mechanism the 'reset' sysfs
> + * file that the toolstack uses to reset a function prior to assigning
> + * the device will be missing.  In this case, pciback adds its own
> + * which will try a bus reset.
> + *
> + * Note: pciback does not check for co-assigment before doing a bus
> + * reset, only that the devices are bound to pciback.  The toolstack
> + * is assumed to have done the right thing.
> + */
> +static int __pcistub_reset_function(struct pci_dev *dev)
> +{
> +	struct pci_dev *pdev;
> +	int ret;
> +
> +	ret = __pci_reset_function_locked(dev);
> +	if (ret == 0)
> +		return 0;
> +
> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> +		return -ENOTTY;
> +
> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {

What if there are buses below this one?

> +		if (pdev != dev && (!pdev->driver
> +				    || strcmp(pdev->driver->name, "pciback")))
> +			return -ENOTTY;
> +	}
> +
> +	return pci_reset_bus(dev->bus);
> +}
> +
> +static int pcistub_reset_function(struct pci_dev *dev)
> +{
> +	int ret;
> +
> +	device_lock(&dev->dev);
> +	ret = __pcistub_reset_function(dev);
> +	device_unlock(&dev->dev);
> +
> +	return ret;
> +}
> +
> +static ssize_t pcistub_reset_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	unsigned long val;
> +	ssize_t result = strict_strtoul(buf, 0, &val);
> +
> +	if (result < 0)
> +		return result;
> +
> +	if (val != 1)
> +		return -EINVAL;
> +
> +	result = pcistub_reset_function(pdev);
> +	if (result < 0)
> +		return result;
> +	return count;
> +}
> +static DEVICE_ATTR(reset, 0200, NULL, pcistub_reset_store);
> +
> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
> +{
> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> +	struct device *dev = &pci->dev;
> +	int ret;
> +
> +	/* Already have a per-function reset? */
> +	if (pci_probe_reset_function(pci) == 0)
> +		return 0;
> +
> +	ret = device_create_file(dev, &dev_attr_reset);
> +	if (ret < 0)
> +		return ret;
> +	dev_data->created_reset_file = true;
> +	return 0;
> +}

So the idea here is that if pci-sysfs did not create a sysfs reset file,
create one when it's bound to pciback that does a secondary bus reset
instead of a reset isolated to the PCI function, right?  It seems like a
lot to ask of userspace to know that the extent of the reset depends on
the driver that it's bound to.  How does userspace figure this out when
the device is bound to pciback and _does_ support a function level
reset?  Overloading "reset" seems like a bad idea.

> +
> +static void pcistub_remove_reset_file(struct pci_dev *pci)
> +{
> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> +
> +	if (dev_data && dev_data->created_reset_file)
> +		device_remove_file(&pci->dev, &dev_attr_reset);
> +}
> +
>  static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
>  {
>  	struct pcistub_device *psdev;
> @@ -103,7 +197,8 @@ static void pcistub_device_release(struct kref *kref)
>  	/* Call the reset function which does not take lock as this
>  	 * is called from "unbind" which takes a device_lock mutex.
>  	 */
> -	__pci_reset_function_locked(dev);
> +	__pcistub_reset_function(psdev->dev);
> +
>  	if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
>  		dev_dbg(&dev->dev, "Could not reload PCI state\n");
>  	else
> @@ -280,7 +375,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>  	/* This is OK - we are running from workqueue context
>  	 * and want to inhibit the user from fiddling with 'reset'
>  	 */
> -	pci_reset_function(dev);
> +	pcistub_reset_function(dev);
>  	pci_restore_state(dev);
>  
>  	/* This disables the device. */
> @@ -404,7 +499,7 @@ static int pcistub_init_device(struct pci_dev *dev)
>  		dev_err(&dev->dev, "Could not store PCI conf saved state!\n");
>  	else {
>  		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
> -		__pci_reset_function_locked(dev);
> +		__pcistub_reset_function(dev);
>  		pci_restore_state(dev);
>  	}
>  	/* Now disable the device (this also ensures some private device
> @@ -413,6 +508,10 @@ static int pcistub_init_device(struct pci_dev *dev)
>  	dev_dbg(&dev->dev, "reset device\n");
>  	xen_pcibk_reset_device(dev);
>  
> +	err = pcistub_try_create_reset_file(dev);
> +	if (err < 0)
> +		goto config_release;
> +
>  	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>  	return 0;
>  
> @@ -540,6 +639,8 @@ static void pcistub_remove(struct pci_dev *dev)
>  
>  	dev_dbg(&dev->dev, "removing\n");
>  
> +	pcistub_remove_reset_file(dev);
> +
>  	spin_lock_irqsave(&pcistub_devices_lock, flags);
>  
>  	xen_pcibk_config_quirk_release(dev);
> diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
> index f72af87..708ade9 100644
> --- a/drivers/xen/xen-pciback/pciback.h
> +++ b/drivers/xen/xen-pciback/pciback.h
> @@ -42,6 +42,7 @@ struct xen_pcibk_device {
>  struct xen_pcibk_dev_data {
>  	struct list_head config_fields;
>  	struct pci_saved_state *pci_saved_state;
> +	bool created_reset_file;
>  	unsigned int permissive:1;
>  	unsigned int warned_on_write:1;
>  	unsigned int enable_intx:1;

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[Alex Williamson]

On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
> The standard implementation of pci_reset_function() does not try an
> SBR if there are other sibling devices.  This is a common
> configuration for multi-function devices (e.g., GPUs with a HDMI audio
> device function).
> 
> If all the functions are co-assigned to the same domain at the same
> time, then it is safe to perform an SBR to reset all functions at the
> same time.
> 
> Add a "reset" sysfs file with the same interface as the standard one
> (i.e., write 1 to reset) that will try an SBR if all sibling devices
> are unbound or bound to pciback.
> 
> Note that this is weaker than the requirement for functions to be
> simultaneously co-assigned, but the toolstack is expected to ensure
> this.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> Changes in v3:
> - probe for function reset
> - use pci_reset_bus().
> 
> Changes in v2:
> - defer creating sysfs node to late init.
> ---
>  drivers/xen/xen-pciback/pci_stub.c |  107 +++++++++++++++++++++++++++++++++++-
>  drivers/xen/xen-pciback/pciback.h  |    1 +
>  2 files changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index d57a173..c7deceb 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -63,6 +63,100 @@ static LIST_HEAD(pcistub_devices);
>  static int initialize_devices;
>  static LIST_HEAD(seized_devices);
>  
> +/*
> + * pci_reset_function() will only work if there is a mechanism to
> + * reset that single function (e.g., FLR or a D-state transition).
> + * For PCI hardware that has two or more functions but no per-function
> + * reset, we can do a bus reset iff all the functions are co-assigned
> + * to the same domain.
> + *
> + * If a function has no per-function reset mechanism the 'reset' sysfs
> + * file that the toolstack uses to reset a function prior to assigning
> + * the device will be missing.  In this case, pciback adds its own
> + * which will try a bus reset.
> + *
> + * Note: pciback does not check for co-assigment before doing a bus
> + * reset, only that the devices are bound to pciback.  The toolstack
> + * is assumed to have done the right thing.
> + */
> +static int __pcistub_reset_function(struct pci_dev *dev)
> +{
> +	struct pci_dev *pdev;
> +	int ret;
> +
> +	ret = __pci_reset_function_locked(dev);
> +	if (ret == 0)
> +		return 0;
> +
> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> +		return -ENOTTY;
> +
> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {

What if there are buses below this one?

> +		if (pdev != dev && (!pdev->driver
> +				    || strcmp(pdev->driver->name, "pciback")))
> +			return -ENOTTY;
> +	}
> +
> +	return pci_reset_bus(dev->bus);
> +}
> +
> +static int pcistub_reset_function(struct pci_dev *dev)
> +{
> +	int ret;
> +
> +	device_lock(&dev->dev);
> +	ret = __pcistub_reset_function(dev);
> +	device_unlock(&dev->dev);
> +
> +	return ret;
> +}
> +
> +static ssize_t pcistub_reset_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	unsigned long val;
> +	ssize_t result = strict_strtoul(buf, 0, &val);
> +
> +	if (result < 0)
> +		return result;
> +
> +	if (val != 1)
> +		return -EINVAL;
> +
> +	result = pcistub_reset_function(pdev);
> +	if (result < 0)
> +		return result;
> +	return count;
> +}
> +static DEVICE_ATTR(reset, 0200, NULL, pcistub_reset_store);
> +
> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
> +{
> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> +	struct device *dev = &pci->dev;
> +	int ret;
> +
> +	/* Already have a per-function reset? */
> +	if (pci_probe_reset_function(pci) == 0)
> +		return 0;
> +
> +	ret = device_create_file(dev, &dev_attr_reset);
> +	if (ret < 0)
> +		return ret;
> +	dev_data->created_reset_file = true;
> +	return 0;
> +}

So the idea here is that if pci-sysfs did not create a sysfs reset file,
create one when it's bound to pciback that does a secondary bus reset
instead of a reset isolated to the PCI function, right?  It seems like a
lot to ask of userspace to know that the extent of the reset depends on
the driver that it's bound to.  How does userspace figure this out when
the device is bound to pciback and _does_ support a function level
reset?  Overloading "reset" seems like a bad idea.

> +
> +static void pcistub_remove_reset_file(struct pci_dev *pci)
> +{
> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> +
> +	if (dev_data && dev_data->created_reset_file)
> +		device_remove_file(&pci->dev, &dev_attr_reset);
> +}
> +
>  static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
>  {
>  	struct pcistub_device *psdev;
> @@ -103,7 +197,8 @@ static void pcistub_device_release(struct kref *kref)
>  	/* Call the reset function which does not take lock as this
>  	 * is called from "unbind" which takes a device_lock mutex.
>  	 */
> -	__pci_reset_function_locked(dev);
> +	__pcistub_reset_function(psdev->dev);
> +
>  	if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
>  		dev_dbg(&dev->dev, "Could not reload PCI state\n");
>  	else
> @@ -280,7 +375,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>  	/* This is OK - we are running from workqueue context
>  	 * and want to inhibit the user from fiddling with 'reset'
>  	 */
> -	pci_reset_function(dev);
> +	pcistub_reset_function(dev);
>  	pci_restore_state(dev);
>  
>  	/* This disables the device. */
> @@ -404,7 +499,7 @@ static int pcistub_init_device(struct pci_dev *dev)
>  		dev_err(&dev->dev, "Could not store PCI conf saved state!\n");
>  	else {
>  		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
> -		__pci_reset_function_locked(dev);
> +		__pcistub_reset_function(dev);
>  		pci_restore_state(dev);
>  	}
>  	/* Now disable the device (this also ensures some private device
> @@ -413,6 +508,10 @@ static int pcistub_init_device(struct pci_dev *dev)
>  	dev_dbg(&dev->dev, "reset device\n");
>  	xen_pcibk_reset_device(dev);
>  
> +	err = pcistub_try_create_reset_file(dev);
> +	if (err < 0)
> +		goto config_release;
> +
>  	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>  	return 0;
>  
> @@ -540,6 +639,8 @@ static void pcistub_remove(struct pci_dev *dev)
>  
>  	dev_dbg(&dev->dev, "removing\n");
>  
> +	pcistub_remove_reset_file(dev);
> +
>  	spin_lock_irqsave(&pcistub_devices_lock, flags);
>  
>  	xen_pcibk_config_quirk_release(dev);
> diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
> index f72af87..708ade9 100644
> --- a/drivers/xen/xen-pciback/pciback.h
> +++ b/drivers/xen/xen-pciback/pciback.h
> @@ -42,6 +42,7 @@ struct xen_pcibk_device {
>  struct xen_pcibk_dev_data {
>  	struct list_head config_fields;
>  	struct pci_saved_state *pci_saved_state;
> +	bool created_reset_file;
>  	unsigned int permissive:1;
>  	unsigned int warned_on_write:1;
>  	unsigned int enable_intx:1;

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[David Vrabel]

On 11/07/14 00:14, Alex Williamson wrote:
> On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
>> The standard implementation of pci_reset_function() does not try an
>> SBR if there are other sibling devices.  This is a common
>> configuration for multi-function devices (e.g., GPUs with a HDMI audio
>> device function).
>>
>> If all the functions are co-assigned to the same domain at the same
>> time, then it is safe to perform an SBR to reset all functions at the
>> same time.
>>
>> Add a "reset" sysfs file with the same interface as the standard one
>> (i.e., write 1 to reset) that will try an SBR if all sibling devices
>> are unbound or bound to pciback.
>>
>> Note that this is weaker than the requirement for functions to be
>> simultaneously co-assigned, but the toolstack is expected to ensure
>> this.
[...]
>> +/*
>> + * pci_reset_function() will only work if there is a mechanism to
>> + * reset that single function (e.g., FLR or a D-state transition).
>> + * For PCI hardware that has two or more functions but no per-function
>> + * reset, we can do a bus reset iff all the functions are co-assigned
>> + * to the same domain.
>> + *
>> + * If a function has no per-function reset mechanism the 'reset' sysfs
>> + * file that the toolstack uses to reset a function prior to assigning
>> + * the device will be missing.  In this case, pciback adds its own
>> + * which will try a bus reset.
>> + *
>> + * Note: pciback does not check for co-assigment before doing a bus
>> + * reset, only that the devices are bound to pciback.  The toolstack
>> + * is assumed to have done the right thing.
>> + */
>> +static int __pcistub_reset_function(struct pci_dev *dev)
>> +{
>> +	struct pci_dev *pdev;
>> +	int ret;
>> +
>> +	ret = __pci_reset_function_locked(dev);
>> +	if (ret == 0)
>> +		return 0;
>> +
>> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
>> +		return -ENOTTY;
>> +
>> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> 
> What if there are buses below this one?

Good point.

>> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
>> +{
>> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
>> +	struct device *dev = &pci->dev;
>> +	int ret;
>> +
>> +	/* Already have a per-function reset? */
>> +	if (pci_probe_reset_function(pci) == 0)
>> +		return 0;
>> +
>> +	ret = device_create_file(dev, &dev_attr_reset);
>> +	if (ret < 0)
>> +		return ret;
>> +	dev_data->created_reset_file = true;
>> +	return 0;
>> +}
> 
> So the idea here is that if pci-sysfs did not create a sysfs reset file,
> create one when it's bound to pciback that does a secondary bus reset
> instead of a reset isolated to the PCI function, right?  It seems like a
> lot to ask of userspace to know that the extent of the reset depends on
> the driver that it's bound to.  How does userspace figure this out when
> the device is bound to pciback and _does_ support a function level
> reset?  Overloading "reset" seems like a bad idea.

The idea is that this "reset" file will only do an SBR if the
side-effect of resetting siblings is harmless.

An alternate interface would be to provide "bus_reset" knobs and have
the toolstack understand the bus topology and issue the appropriate bus
reset if it determines it is safe and per-function reset isn't
available.  This seems like considerably more work both kernel and
toolstack side.

David

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[David Vrabel]

On 11/07/14 00:14, Alex Williamson wrote:
> On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
>> The standard implementation of pci_reset_function() does not try an
>> SBR if there are other sibling devices.  This is a common
>> configuration for multi-function devices (e.g., GPUs with a HDMI audio
>> device function).
>>
>> If all the functions are co-assigned to the same domain at the same
>> time, then it is safe to perform an SBR to reset all functions at the
>> same time.
>>
>> Add a "reset" sysfs file with the same interface as the standard one
>> (i.e., write 1 to reset) that will try an SBR if all sibling devices
>> are unbound or bound to pciback.
>>
>> Note that this is weaker than the requirement for functions to be
>> simultaneously co-assigned, but the toolstack is expected to ensure
>> this.
[...]
>> +/*
>> + * pci_reset_function() will only work if there is a mechanism to
>> + * reset that single function (e.g., FLR or a D-state transition).
>> + * For PCI hardware that has two or more functions but no per-function
>> + * reset, we can do a bus reset iff all the functions are co-assigned
>> + * to the same domain.
>> + *
>> + * If a function has no per-function reset mechanism the 'reset' sysfs
>> + * file that the toolstack uses to reset a function prior to assigning
>> + * the device will be missing.  In this case, pciback adds its own
>> + * which will try a bus reset.
>> + *
>> + * Note: pciback does not check for co-assigment before doing a bus
>> + * reset, only that the devices are bound to pciback.  The toolstack
>> + * is assumed to have done the right thing.
>> + */
>> +static int __pcistub_reset_function(struct pci_dev *dev)
>> +{
>> +	struct pci_dev *pdev;
>> +	int ret;
>> +
>> +	ret = __pci_reset_function_locked(dev);
>> +	if (ret == 0)
>> +		return 0;
>> +
>> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
>> +		return -ENOTTY;
>> +
>> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> 
> What if there are buses below this one?

Good point.

>> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
>> +{
>> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
>> +	struct device *dev = &pci->dev;
>> +	int ret;
>> +
>> +	/* Already have a per-function reset? */
>> +	if (pci_probe_reset_function(pci) == 0)
>> +		return 0;
>> +
>> +	ret = device_create_file(dev, &dev_attr_reset);
>> +	if (ret < 0)
>> +		return ret;
>> +	dev_data->created_reset_file = true;
>> +	return 0;
>> +}
> 
> So the idea here is that if pci-sysfs did not create a sysfs reset file,
> create one when it's bound to pciback that does a secondary bus reset
> instead of a reset isolated to the PCI function, right?  It seems like a
> lot to ask of userspace to know that the extent of the reset depends on
> the driver that it's bound to.  How does userspace figure this out when
> the device is bound to pciback and _does_ support a function level
> reset?  Overloading "reset" seems like a bad idea.

The idea is that this "reset" file will only do an SBR if the
side-effect of resetting siblings is harmless.

An alternate interface would be to provide "bus_reset" knobs and have
the toolstack understand the bus topology and issue the appropriate bus
reset if it determines it is safe and per-function reset isn't
available.  This seems like considerably more work both kernel and
toolstack side.

David

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[Konrad Rzeszutek Wilk]

On Fri, Jul 11, 2014 at 10:53:32AM +0100, David Vrabel wrote:
> On 11/07/14 00:14, Alex Williamson wrote:
> > On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
> >> The standard implementation of pci_reset_function() does not try an
> >> SBR if there are other sibling devices.  This is a common
> >> configuration for multi-function devices (e.g., GPUs with a HDMI audio
> >> device function).
> >>
> >> If all the functions are co-assigned to the same domain at the same
> >> time, then it is safe to perform an SBR to reset all functions at the
> >> same time.
> >>
> >> Add a "reset" sysfs file with the same interface as the standard one
> >> (i.e., write 1 to reset) that will try an SBR if all sibling devices
> >> are unbound or bound to pciback.
> >>
> >> Note that this is weaker than the requirement for functions to be
> >> simultaneously co-assigned, but the toolstack is expected to ensure
> >> this.
> [...]
> >> +/*
> >> + * pci_reset_function() will only work if there is a mechanism to
> >> + * reset that single function (e.g., FLR or a D-state transition).
> >> + * For PCI hardware that has two or more functions but no per-function
> >> + * reset, we can do a bus reset iff all the functions are co-assigned
> >> + * to the same domain.
> >> + *
> >> + * If a function has no per-function reset mechanism the 'reset' sysfs
> >> + * file that the toolstack uses to reset a function prior to assigning
> >> + * the device will be missing.  In this case, pciback adds its own
> >> + * which will try a bus reset.
> >> + *
> >> + * Note: pciback does not check for co-assigment before doing a bus
> >> + * reset, only that the devices are bound to pciback.  The toolstack
> >> + * is assumed to have done the right thing.
> >> + */
> >> +static int __pcistub_reset_function(struct pci_dev *dev)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +	int ret;
> >> +
> >> +	ret = __pci_reset_function_locked(dev);
> >> +	if (ret == 0)
> >> +		return 0;
> >> +
> >> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> >> +		return -ENOTTY;
> >> +
> >> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> > 
> > What if there are buses below this one?
> 
> Good point.

You could use an

pci_walk_bus(dev->bus, pcistub_pci_walk_wrapper, &arg);

to check that all of them belong to pciback (see 
https://lkml.org/lkml/2014/7/8/782)


> 
> >> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
> >> +{
> >> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> >> +	struct device *dev = &pci->dev;
> >> +	int ret;
> >> +
> >> +	/* Already have a per-function reset? */
> >> +	if (pci_probe_reset_function(pci) == 0)
> >> +		return 0;
> >> +
> >> +	ret = device_create_file(dev, &dev_attr_reset);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	dev_data->created_reset_file = true;
> >> +	return 0;
> >> +}
> > 
> > So the idea here is that if pci-sysfs did not create a sysfs reset file,
> > create one when it's bound to pciback that does a secondary bus reset
> > instead of a reset isolated to the PCI function, right?  It seems like a
> > lot to ask of userspace to know that the extent of the reset depends on
> > the driver that it's bound to.  How does userspace figure this out when
> > the device is bound to pciback and _does_ support a function level
> > reset?  Overloading "reset" seems like a bad idea.
> 
> The idea is that this "reset" file will only do an SBR if the
> side-effect of resetting siblings is harmless.
> 
> An alternate interface would be to provide "bus_reset" knobs and have
> the toolstack understand the bus topology and issue the appropriate bus
> reset if it determines it is safe and per-function reset isn't
> available.  This seems like considerably more work both kernel and
> toolstack side.

In regards to alternative interface, I presume you are referring
to the 'do_flr' one I posted?


In that I thought it would be easier to implement this logic in the kernel
since it has a pretty good idea of the bus->device topology is and
better yet there is ton of good code in PCI API to use it. And it can
figure these sort of things out pretty easily - like do not do the bus reset
if say the devices are not all owned by pciback).

See https://lkml.org/lkml/2014/7/8/782

Now the one part I hadn't thought about is 'safe'?
The 'per-function reset isn't available' is certainy part of the patch.

FYI, I had a different piece of code (not posted) which would add per
device the 'do_flr' (so similar to 'reset') and would do the appropiate
checks and do the bus reset if all the conditions were meet. As in
instead of 'do_flr' in /sys/bus/pci/drivers/pciback/do_flr it would
be in /sys/bus/pci/devices/0000:04:00.0/do_flr (if 04:00.0 was owned
by pciback).

is ce


> 
> David

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[Konrad Rzeszutek Wilk]

On Fri, Jul 11, 2014 at 10:53:32AM +0100, David Vrabel wrote:
> On 11/07/14 00:14, Alex Williamson wrote:
> > On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
> >> The standard implementation of pci_reset_function() does not try an
> >> SBR if there are other sibling devices.  This is a common
> >> configuration for multi-function devices (e.g., GPUs with a HDMI audio
> >> device function).
> >>
> >> If all the functions are co-assigned to the same domain at the same
> >> time, then it is safe to perform an SBR to reset all functions at the
> >> same time.
> >>
> >> Add a "reset" sysfs file with the same interface as the standard one
> >> (i.e., write 1 to reset) that will try an SBR if all sibling devices
> >> are unbound or bound to pciback.
> >>
> >> Note that this is weaker than the requirement for functions to be
> >> simultaneously co-assigned, but the toolstack is expected to ensure
> >> this.
> [...]
> >> +/*
> >> + * pci_reset_function() will only work if there is a mechanism to
> >> + * reset that single function (e.g., FLR or a D-state transition).
> >> + * For PCI hardware that has two or more functions but no per-function
> >> + * reset, we can do a bus reset iff all the functions are co-assigned
> >> + * to the same domain.
> >> + *
> >> + * If a function has no per-function reset mechanism the 'reset' sysfs
> >> + * file that the toolstack uses to reset a function prior to assigning
> >> + * the device will be missing.  In this case, pciback adds its own
> >> + * which will try a bus reset.
> >> + *
> >> + * Note: pciback does not check for co-assigment before doing a bus
> >> + * reset, only that the devices are bound to pciback.  The toolstack
> >> + * is assumed to have done the right thing.
> >> + */
> >> +static int __pcistub_reset_function(struct pci_dev *dev)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +	int ret;
> >> +
> >> +	ret = __pci_reset_function_locked(dev);
> >> +	if (ret == 0)
> >> +		return 0;
> >> +
> >> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> >> +		return -ENOTTY;
> >> +
> >> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> > 
> > What if there are buses below this one?
> 
> Good point.

You could use an

pci_walk_bus(dev->bus, pcistub_pci_walk_wrapper, &arg);

to check that all of them belong to pciback (see 
https://lkml.org/lkml/2014/7/8/782)


> 
> >> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
> >> +{
> >> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> >> +	struct device *dev = &pci->dev;
> >> +	int ret;
> >> +
> >> +	/* Already have a per-function reset? */
> >> +	if (pci_probe_reset_function(pci) == 0)
> >> +		return 0;
> >> +
> >> +	ret = device_create_file(dev, &dev_attr_reset);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	dev_data->created_reset_file = true;
> >> +	return 0;
> >> +}
> > 
> > So the idea here is that if pci-sysfs did not create a sysfs reset file,
> > create one when it's bound to pciback that does a secondary bus reset
> > instead of a reset isolated to the PCI function, right?  It seems like a
> > lot to ask of userspace to know that the extent of the reset depends on
> > the driver that it's bound to.  How does userspace figure this out when
> > the device is bound to pciback and _does_ support a function level
> > reset?  Overloading "reset" seems like a bad idea.
> 
> The idea is that this "reset" file will only do an SBR if the
> side-effect of resetting siblings is harmless.
> 
> An alternate interface would be to provide "bus_reset" knobs and have
> the toolstack understand the bus topology and issue the appropriate bus
> reset if it determines it is safe and per-function reset isn't
> available.  This seems like considerably more work both kernel and
> toolstack side.

In regards to alternative interface, I presume you are referring
to the 'do_flr' one I posted?


In that I thought it would be easier to implement this logic in the kernel
since it has a pretty good idea of the bus->device topology is and
better yet there is ton of good code in PCI API to use it. And it can
figure these sort of things out pretty easily - like do not do the bus reset
if say the devices are not all owned by pciback).

See https://lkml.org/lkml/2014/7/8/782

Now the one part I hadn't thought about is 'safe'?
The 'per-function reset isn't available' is certainy part of the patch.

FYI, I had a different piece of code (not posted) which would add per
device the 'do_flr' (so similar to 'reset') and would do the appropiate
checks and do the bus reset if all the conditions were meet. As in
instead of 'do_flr' in /sys/bus/pci/drivers/pciback/do_flr it would
be in /sys/bus/pci/devices/0000:04:00.0/do_flr (if 04:00.0 was owned
by pciback).

is ce


> 
> David

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[Alex Williamson]

On Fri, 2014-07-11 at 10:53 +0100, David Vrabel wrote:
> On 11/07/14 00:14, Alex Williamson wrote:
> > On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
> >> The standard implementation of pci_reset_function() does not try an
> >> SBR if there are other sibling devices.  This is a common
> >> configuration for multi-function devices (e.g., GPUs with a HDMI audio
> >> device function).
> >>
> >> If all the functions are co-assigned to the same domain at the same
> >> time, then it is safe to perform an SBR to reset all functions at the
> >> same time.
> >>
> >> Add a "reset" sysfs file with the same interface as the standard one
> >> (i.e., write 1 to reset) that will try an SBR if all sibling devices
> >> are unbound or bound to pciback.
> >>
> >> Note that this is weaker than the requirement for functions to be
> >> simultaneously co-assigned, but the toolstack is expected to ensure
> >> this.
> [...]
> >> +/*
> >> + * pci_reset_function() will only work if there is a mechanism to
> >> + * reset that single function (e.g., FLR or a D-state transition).
> >> + * For PCI hardware that has two or more functions but no per-function
> >> + * reset, we can do a bus reset iff all the functions are co-assigned
> >> + * to the same domain.
> >> + *
> >> + * If a function has no per-function reset mechanism the 'reset' sysfs
> >> + * file that the toolstack uses to reset a function prior to assigning
> >> + * the device will be missing.  In this case, pciback adds its own
> >> + * which will try a bus reset.
> >> + *
> >> + * Note: pciback does not check for co-assigment before doing a bus
> >> + * reset, only that the devices are bound to pciback.  The toolstack
> >> + * is assumed to have done the right thing.
> >> + */
> >> +static int __pcistub_reset_function(struct pci_dev *dev)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +	int ret;
> >> +
> >> +	ret = __pci_reset_function_locked(dev);
> >> +	if (ret == 0)
> >> +		return 0;
> >> +
> >> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> >> +		return -ENOTTY;
> >> +
> >> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> > 
> > What if there are buses below this one?
> 
> Good point.
> 
> >> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
> >> +{
> >> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> >> +	struct device *dev = &pci->dev;
> >> +	int ret;
> >> +
> >> +	/* Already have a per-function reset? */
> >> +	if (pci_probe_reset_function(pci) == 0)
> >> +		return 0;
> >> +
> >> +	ret = device_create_file(dev, &dev_attr_reset);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	dev_data->created_reset_file = true;
> >> +	return 0;
> >> +}
> > 
> > So the idea here is that if pci-sysfs did not create a sysfs reset file,
> > create one when it's bound to pciback that does a secondary bus reset
> > instead of a reset isolated to the PCI function, right?  It seems like a
> > lot to ask of userspace to know that the extent of the reset depends on
> > the driver that it's bound to.  How does userspace figure this out when
> > the device is bound to pciback and _does_ support a function level
> > reset?  Overloading "reset" seems like a bad idea.
> 
> The idea is that this "reset" file will only do an SBR if the
> side-effect of resetting siblings is harmless.

Let's say you have a graphics card with the GPU function assigned.  The
VM is running and now you want to hot-add the audio function.  Is it
harmless to do an SBR to get the audio function into a clean state prior
to hotplug?  Of course not.  How does userspace know that the reset file
under a device is going to do a SBR vs a function-local reset?  It
doesn't.  This is exactly why VFIO has separate interfaces for these.
An SBR is only useful on create or reset of the entire VM while a
function-local reset can be used elsewhere.  Thanks,

Alex

> An alternate interface would be to provide "bus_reset" knobs and have
> the toolstack understand the bus topology and issue the appropriate bus
> reset if it determines it is safe and per-function reset isn't
> available.  This seems like considerably more work both kernel and
> toolstack side.
> 
> David

Re: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR

[Alex Williamson]

On Fri, 2014-07-11 at 10:53 +0100, David Vrabel wrote:
> On 11/07/14 00:14, Alex Williamson wrote:
> > On Thu, 2014-07-10 at 14:03 +0100, David Vrabel wrote:
> >> The standard implementation of pci_reset_function() does not try an
> >> SBR if there are other sibling devices.  This is a common
> >> configuration for multi-function devices (e.g., GPUs with a HDMI audio
> >> device function).
> >>
> >> If all the functions are co-assigned to the same domain at the same
> >> time, then it is safe to perform an SBR to reset all functions at the
> >> same time.
> >>
> >> Add a "reset" sysfs file with the same interface as the standard one
> >> (i.e., write 1 to reset) that will try an SBR if all sibling devices
> >> are unbound or bound to pciback.
> >>
> >> Note that this is weaker than the requirement for functions to be
> >> simultaneously co-assigned, but the toolstack is expected to ensure
> >> this.
> [...]
> >> +/*
> >> + * pci_reset_function() will only work if there is a mechanism to
> >> + * reset that single function (e.g., FLR or a D-state transition).
> >> + * For PCI hardware that has two or more functions but no per-function
> >> + * reset, we can do a bus reset iff all the functions are co-assigned
> >> + * to the same domain.
> >> + *
> >> + * If a function has no per-function reset mechanism the 'reset' sysfs
> >> + * file that the toolstack uses to reset a function prior to assigning
> >> + * the device will be missing.  In this case, pciback adds its own
> >> + * which will try a bus reset.
> >> + *
> >> + * Note: pciback does not check for co-assigment before doing a bus
> >> + * reset, only that the devices are bound to pciback.  The toolstack
> >> + * is assumed to have done the right thing.
> >> + */
> >> +static int __pcistub_reset_function(struct pci_dev *dev)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +	int ret;
> >> +
> >> +	ret = __pci_reset_function_locked(dev);
> >> +	if (ret == 0)
> >> +		return 0;
> >> +
> >> +	if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> >> +		return -ENOTTY;
> >> +
> >> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> > 
> > What if there are buses below this one?
> 
> Good point.
> 
> >> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
> >> +{
> >> +	struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> >> +	struct device *dev = &pci->dev;
> >> +	int ret;
> >> +
> >> +	/* Already have a per-function reset? */
> >> +	if (pci_probe_reset_function(pci) == 0)
> >> +		return 0;
> >> +
> >> +	ret = device_create_file(dev, &dev_attr_reset);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	dev_data->created_reset_file = true;
> >> +	return 0;
> >> +}
> > 
> > So the idea here is that if pci-sysfs did not create a sysfs reset file,
> > create one when it's bound to pciback that does a secondary bus reset
> > instead of a reset isolated to the PCI function, right?  It seems like a
> > lot to ask of userspace to know that the extent of the reset depends on
> > the driver that it's bound to.  How does userspace figure this out when
> > the device is bound to pciback and _does_ support a function level
> > reset?  Overloading "reset" seems like a bad idea.
> 
> The idea is that this "reset" file will only do an SBR if the
> side-effect of resetting siblings is harmless.

Let's say you have a graphics card with the GPU function assigned.  The
VM is running and now you want to hot-add the audio function.  Is it
harmless to do an SBR to get the audio function into a clean state prior
to hotplug?  Of course not.  How does userspace know that the reset file
under a device is going to do a SBR vs a function-local reset?  It
doesn't.  This is exactly why VFIO has separate interfaces for these.
An SBR is only useful on create or reset of the entire VM while a
function-local reset can be used elsewhere.  Thanks,

Alex

> An alternate interface would be to provide "bus_reset" knobs and have
> the toolstack understand the bus topology and issue the appropriate bus
> reset if it determines it is safe and per-function reset isn't
> available.  This seems like considerably more work both kernel and
> toolstack side.
> 
> David