Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Ruediger Otte]

Hi,

this patch actually solves the problem I experienced when trying to
passthrough a Radeon 7750 to a windows guest with a custom build of
Xen 4.4.0 on Debian Wheezy.

While I was always getting dom0 lockups at the first reboot of the
guest, I'm now able to do several reboots without sending the host to
standby in between. I haven't done extensive testing yet, but gpu
passthrough seems to just work now.

However I'm now seeing a different issue at dom0 boot when the
devices are assigned to xen-pciback (in kernel, no module). After the
message "xen_pciback: backend is passthrough" the host hangs for 2-3
seconds, then reboots. Strangely sometimes the host just boots ok,
but then again I get three or more failed boots in a row before it
finally works.

Before I applied your patch I have already followed every hint and
tried every available workaround with no success so far, so I would
be glad if this code could be further improved. If there's the need I
can of course provide debug output and configuration details from my
setup.

Kind regards,
Ruediger Otte

> Hey,
> 
> While I was trying to narrow down the state of GPU passthrough
> (still not finished) and figuring what needs to be done I realized
> that Xen PCIback did not reset my GPU properly (when I crashed the
> Windows guest by mistake). It does an FLR reset or Power one - if
> the device supports it. But it seems that some of these GPUs
> are liars and actually don't do the power part properly.
> 
> One way to fix this is by doing a slot (aka device) and bus reset.
> Of course to do that - you need to make sure that all of the
> functions of a device are under the ownership of xen-pciback.
> Otherwise you might accidently reset your sound card while it is
> being used.
> 
> These RFC patches cleanup pci back a bit and also make it possible
> for Xen pciback to do the whole gamma of 'reset' for PCI devices:
> FLR, power management, AER, slot and bus reset if neccessary.
> 
> Thanks go to Gordan Bobic for educating me on how to "reprogram"
> and GFX460 in a Quardro 4000M and also reporting oddities when
> a PCI device was reset but it looked like it was not reset.
> 
> 
>  drivers/xen/xen-pciback/pci_stub.c | 142
> +++++++++++++++++++++++++++++++------
> drivers/xen/xen-pciback/xenbus.c   |   5 +- 2 files changed, 124
> insertions(+), 23 deletions(-)
> 
> 
> Konrad Rzeszutek Wilk (5):
>       xen-pciback: Cleanup up pcistub_put_pci_dev
>       xen-pciback: First reset, then free.
>       xen-pciback: Document when we FLR an PCI device.
>       xen/pciback: Move the FLR code to a function.
>       xen/pciback: PCI reset slot or bus

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Gordan Bobic]

On 12/13/2013 04:09 PM, Konrad Rzeszutek Wilk wrote:
> Hey,
>
> While I was trying to narrow down the state of GPU passthrough
> (still not finished) and figuring what needs to be done I realized
> that Xen PCIback did not reset my GPU properly (when I crashed the
> Windows guest by mistake). It does an FLR reset or Power one - if
> the device supports it. But it seems that some of these GPUs
> are liars and actually don't do the power part properly.

Some cards (IIRC Fermi based Nvidia cards) support neither the required 
power states nor FLR, although you can use setpci to force a powered off 
state on the card without an error being triggered. Unfortunately, it 
seems to be a null operation, and the card doesn't end up getting reset.

> One way to fix this is by doing a slot (aka device) and bus reset.
> Of course to do that - you need to make sure that all of the
> functions of a device are under the ownership of xen-pciback.
> Otherwise you might accidently reset your sound card while it is
> being used.
 >
> These RFC patches cleanup pci back a bit and also make it possible
> for Xen pciback to do the whole gamma of 'reset' for PCI devices:
> FLR, power management, AER, slot and bus reset if neccessary.
>
> Thanks go to Gordan Bobic for educating me on how to "reprogram"
> and GFX460 in a Quardro 4000M and also reporting oddities when
> a PCI device was reset but it looked like it was not reset.

You're welcome. :)

Gordan

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[David Vrabel]

On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
> Hey,
> 
> While I was trying to narrow down the state of GPU passthrough
> (still not finished) and figuring what needs to be done I realized
> that Xen PCIback did not reset my GPU properly (when I crashed the
> Windows guest by mistake). It does an FLR reset or Power one - if
> the device supports it. But it seems that some of these GPUs
> are liars and actually don't do the power part properly.

In my experience the devices do not lie.  They correctly report that
they do not perform a reset in D3hot.

Here's the patch I'm using to solve this.  It does something similar.
i.e., a SBR if all devices on that bus are safe to be reset.

I prefer it because it provides the standard 'reset' sysfs file that the
toolstack/userspace can use.

It does have some limitations:  a) It does not check whether a device is
in use (only if it is bound to pciback); and b) it hand rolls
pci_slot_reset() (because it didn't exist at the time).

diff --git a/drivers/xen/xen-pciback/pci_stub.c
b/drivers/xen/xen-pciback/pci_stub.c
index 4e8ba38..5a03e63 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -14,6 +14,7 @@
 #include <linux/wait.h>
 #include <linux/sched.h>
 #include <linux/atomic.h>
+#include <linux/delay.h>
 #include <xen/events.h>
 #include <asm/xen/pci.h>
 #include <asm/xen/hypervisor.h>
@@ -43,6 +44,7 @@ struct pcistub_device {
 	struct kref kref;
 	struct list_head dev_list;
 	spinlock_t lock;
+	bool created_reset_file;

 	struct pci_dev *dev;
 	struct xen_pcibk_device *pdev;/* non-NULL if struct pci_dev is in use */
@@ -60,6 +62,114 @@ 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;
+	u16 ctrl;
+	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;
+		pci_save_state(pdev);
+	}
+
+	pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
+	ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
+	pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
+	msleep(200);
+
+	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
+	pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
+	msleep(200);
+
+	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
+		pci_restore_state(pdev);
+
+	return 0;
+}
+
+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 pcistub_device *psdev)
+{
+	struct device *dev = &psdev->dev->dev;
+	struct sysfs_dirent *reset_dirent;
+	int ret;
+
+	reset_dirent = sysfs_get_dirent(dev->kobj.sd, NULL, "reset");
+	if (reset_dirent) {
+		sysfs_put(reset_dirent);
+		return 0;
+	}
+
+	ret = device_create_file(dev, &dev_attr_reset);
+	if (ret < 0)
+		return ret;
+	psdev->created_reset_file = true;
+	return 0;
+}
+
+static void pcistub_remove_reset_file(struct pcistub_device *psdev)
+{
+	if (psdev && psdev->created_reset_file)
+		device_remove_file(&psdev->dev->dev, &dev_attr_reset);
+}
+
 static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev;
@@ -95,12 +205,15 @@ static void pcistub_device_release(struct kref *kref)

 	dev_dbg(&dev->dev, "pcistub_device_release\n");

+	pcistub_remove_reset_file(psdev);
+
 	xen_unregister_device_domain_owner(dev);

 	/* 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
@@ -268,7 +381,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(psdev->dev);
 	pci_restore_state(psdev->dev);

 	/* This disables the device. */
@@ -392,7 +505,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
@@ -467,6 +580,10 @@ static int pcistub_seize(struct pci_dev *dev)
 	if (!psdev)
 		return -ENOMEM;

+	err = pcistub_try_create_reset_file(psdev);
+	if (err < 0)
+		goto out;
+
 	spin_lock_irqsave(&pcistub_devices_lock, flags);

 	if (initialize_devices) {
@@ -485,10 +602,9 @@ static int pcistub_seize(struct pci_dev *dev)
 	}

 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
-
+out:
 	if (err)
 		pcistub_device_put(psdev);
-
 	return err;
 }

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Konrad Rzeszutek Wilk]

On Mon, Dec 16, 2013 at 10:59:01AM +0000, David Vrabel wrote:
> On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
> > Hey,
> > 
> > While I was trying to narrow down the state of GPU passthrough
> > (still not finished) and figuring what needs to be done I realized
> > that Xen PCIback did not reset my GPU properly (when I crashed the
> > Windows guest by mistake). It does an FLR reset or Power one - if
> > the device supports it. But it seems that some of these GPUs
> > are liars and actually don't do the power part properly.
> 
> In my experience the devices do not lie.  They correctly report that
> they do not perform a reset in D3hot.
> 
> Here's the patch I'm using to solve this.  It does something similar.
> i.e., a SBR if all devices on that bus are safe to be reset.
> 
> I prefer it because it provides the standard 'reset' sysfs file that the
> toolstack/userspace can use.

We can still add the 'reset' to SysFS
> 
> It does have some limitations:  a) It does not check whether a device is
> in use (only if it is bound to pciback); and b) it hand rolls
> pci_slot_reset() (because it didn't exist at the time).

.. which can have those limiations removed and be based on this patchset.
Meaning it won't do a bus-reset or device reset if the rest of the devices
are _not_ assigned to pciback.

> 
> diff --git a/drivers/xen/xen-pciback/pci_stub.c
> b/drivers/xen/xen-pciback/pci_stub.c
> index 4e8ba38..5a03e63 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -14,6 +14,7 @@
>  #include <linux/wait.h>
>  #include <linux/sched.h>
>  #include <linux/atomic.h>
> +#include <linux/delay.h>
>  #include <xen/events.h>
>  #include <asm/xen/pci.h>
>  #include <asm/xen/hypervisor.h>
> @@ -43,6 +44,7 @@ struct pcistub_device {
>  	struct kref kref;
>  	struct list_head dev_list;
>  	spinlock_t lock;
> +	bool created_reset_file;
> 
>  	struct pci_dev *dev;
>  	struct xen_pcibk_device *pdev;/* non-NULL if struct pci_dev is in use */
> @@ -60,6 +62,114 @@ 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;
> +	u16 ctrl;
> +	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;
> +		pci_save_state(pdev);
> +	}
> +
> +	pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
> +	ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> +	pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> +	msleep(200);
> +
> +	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> +	pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> +	msleep(200);
> +
> +	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
> +		pci_restore_state(pdev);
> +
> +	return 0;
> +}
> +
> +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 pcistub_device *psdev)
> +{
> +	struct device *dev = &psdev->dev->dev;
> +	struct sysfs_dirent *reset_dirent;
> +	int ret;
> +
> +	reset_dirent = sysfs_get_dirent(dev->kobj.sd, NULL, "reset");
> +	if (reset_dirent) {
> +		sysfs_put(reset_dirent);
> +		return 0;
> +	}
> +
> +	ret = device_create_file(dev, &dev_attr_reset);
> +	if (ret < 0)
> +		return ret;
> +	psdev->created_reset_file = true;
> +	return 0;
> +}
> +
> +static void pcistub_remove_reset_file(struct pcistub_device *psdev)
> +{
> +	if (psdev && psdev->created_reset_file)
> +		device_remove_file(&psdev->dev->dev, &dev_attr_reset);
> +}
> +
>  static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
>  {
>  	struct pcistub_device *psdev;
> @@ -95,12 +205,15 @@ static void pcistub_device_release(struct kref *kref)
> 
>  	dev_dbg(&dev->dev, "pcistub_device_release\n");
> 
> +	pcistub_remove_reset_file(psdev);
> +
>  	xen_unregister_device_domain_owner(dev);
> 
>  	/* 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
> @@ -268,7 +381,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(psdev->dev);
>  	pci_restore_state(psdev->dev);
> 
>  	/* This disables the device. */
> @@ -392,7 +505,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
> @@ -467,6 +580,10 @@ static int pcistub_seize(struct pci_dev *dev)
>  	if (!psdev)
>  		return -ENOMEM;
> 
> +	err = pcistub_try_create_reset_file(psdev);
> +	if (err < 0)
> +		goto out;
> +
>  	spin_lock_irqsave(&pcistub_devices_lock, flags);
> 
>  	if (initialize_devices) {
> @@ -485,10 +602,9 @@ static int pcistub_seize(struct pci_dev *dev)
>  	}
> 
>  	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
> -
> +out:
>  	if (err)
>  		pcistub_device_put(psdev);
> -
>  	return err;
>  }
> 

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Sander Eikelenboom]

Monday, December 16, 2013, 3:35:15 PM, you wrote:

> On Mon, Dec 16, 2013 at 10:59:01AM +0000, David Vrabel wrote:
>> On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
>> > Hey,
>> > 
>> > While I was trying to narrow down the state of GPU passthrough
>> > (still not finished) and figuring what needs to be done I realized
>> > that Xen PCIback did not reset my GPU properly (when I crashed the
>> > Windows guest by mistake). It does an FLR reset or Power one - if
>> > the device supports it. But it seems that some of these GPUs
>> > are liars and actually don't do the power part properly.
>> 
>> In my experience the devices do not lie.  They correctly report that
>> they do not perform a reset in D3hot.
>> 
>> Here's the patch I'm using to solve this.  It does something similar.
>> i.e., a SBR if all devices on that bus are safe to be reset.
>> 
>> I prefer it because it provides the standard 'reset' sysfs file that the
>> toolstack/userspace can use.

> We can still add the 'reset' to SysFS
>> 
>> It does have some limitations:  a) It does not check whether a device is
>> in use (only if it is bound to pciback); and b) it hand rolls
>> pci_slot_reset() (because it didn't exist at the time).

> .. which can have those limiations removed and be based on this patchset.
> Meaning it won't do a bus-reset or device reset if the rest of the devices
> are _not_ assigned to pciback.

Perhaps there is something to learn from the steps vfio-pci takes to do this ?
(they sorted out quite some stuff around pci/vga passtrough)

>> 
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c
>> b/drivers/xen/xen-pciback/pci_stub.c
>> index 4e8ba38..5a03e63 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/wait.h>
>>  #include <linux/sched.h>
>>  #include <linux/atomic.h>
>> +#include <linux/delay.h>
>>  #include <xen/events.h>
>>  #include <asm/xen/pci.h>
>>  #include <asm/xen/hypervisor.h>
>> @@ -43,6 +44,7 @@ struct pcistub_device {
>>       struct kref kref;
>>       struct list_head dev_list;
>>       spinlock_t lock;
>> +     bool created_reset_file;
>> 
>>       struct pci_dev *dev;
>>       struct xen_pcibk_device *pdev;/* non-NULL if struct pci_dev is in use */
>> @@ -60,6 +62,114 @@ 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;
>> +     u16 ctrl;
>> +     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;
>> +             pci_save_state(pdev);
>> +     }
>> +
>> +     pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
>> +     ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
>> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> +     msleep(200);
>> +
>> +     ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
>> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> +     msleep(200);
>> +
>> +     list_for_each_entry(pdev, &dev->bus->devices, bus_list)
>> +             pci_restore_state(pdev);
>> +
>> +     return 0;
>> +}
>> +
>> +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 pcistub_device *psdev)
>> +{
>> +     struct device *dev = &psdev->dev->dev;
>> +     struct sysfs_dirent *reset_dirent;
>> +     int ret;
>> +
>> +     reset_dirent = sysfs_get_dirent(dev->kobj.sd, NULL, "reset");
>> +     if (reset_dirent) {
>> +             sysfs_put(reset_dirent);
>> +             return 0;
>> +     }
>> +
>> +     ret = device_create_file(dev, &dev_attr_reset);
>> +     if (ret < 0)
>> +             return ret;
>> +     psdev->created_reset_file = true;
>> +     return 0;
>> +}
>> +
>> +static void pcistub_remove_reset_file(struct pcistub_device *psdev)
>> +{
>> +     if (psdev && psdev->created_reset_file)
>> +             device_remove_file(&psdev->dev->dev, &dev_attr_reset);
>> +}
>> +
>>  static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
>>  {
>>       struct pcistub_device *psdev;
>> @@ -95,12 +205,15 @@ static void pcistub_device_release(struct kref *kref)
>> 
>>       dev_dbg(&dev->dev, "pcistub_device_release\n");
>> 
>> +     pcistub_remove_reset_file(psdev);
>> +
>>       xen_unregister_device_domain_owner(dev);
>> 
>>       /* 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
>> @@ -268,7 +381,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(psdev->dev);
>>       pci_restore_state(psdev->dev);
>> 
>>       /* This disables the device. */
>> @@ -392,7 +505,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
>> @@ -467,6 +580,10 @@ static int pcistub_seize(struct pci_dev *dev)
>>       if (!psdev)
>>               return -ENOMEM;
>> 
>> +     err = pcistub_try_create_reset_file(psdev);
>> +     if (err < 0)
>> +             goto out;
>> +
>>       spin_lock_irqsave(&pcistub_devices_lock, flags);
>> 
>>       if (initialize_devices) {
>> @@ -485,10 +602,9 @@ static int pcistub_seize(struct pci_dev *dev)
>>       }
>> 
>>       spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>> -
>> +out:
>>       if (err)
>>               pcistub_device_put(psdev);
>> -
>>       return err;
>>  }
>> 

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Konrad Rzeszutek Wilk]

On Mon, Dec 16, 2013 at 04:23:53PM +0100, Sander Eikelenboom wrote:
> 
> Monday, December 16, 2013, 3:35:15 PM, you wrote:
> 
> > On Mon, Dec 16, 2013 at 10:59:01AM +0000, David Vrabel wrote:
> >> On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
> >> > Hey,
> >> > 
> >> > While I was trying to narrow down the state of GPU passthrough
> >> > (still not finished) and figuring what needs to be done I realized
> >> > that Xen PCIback did not reset my GPU properly (when I crashed the
> >> > Windows guest by mistake). It does an FLR reset or Power one - if
> >> > the device supports it. But it seems that some of these GPUs
> >> > are liars and actually don't do the power part properly.
> >> 
> >> In my experience the devices do not lie.  They correctly report that
> >> they do not perform a reset in D3hot.
> >> 
> >> Here's the patch I'm using to solve this.  It does something similar.
> >> i.e., a SBR if all devices on that bus are safe to be reset.
> >> 
> >> I prefer it because it provides the standard 'reset' sysfs file that the
> >> toolstack/userspace can use.
> 
> > We can still add the 'reset' to SysFS
> >> 
> >> It does have some limitations:  a) It does not check whether a device is
> >> in use (only if it is bound to pciback); and b) it hand rolls
> >> pci_slot_reset() (because it didn't exist at the time).
> 
> > .. which can have those limiations removed and be based on this patchset.
> > Meaning it won't do a bus-reset or device reset if the rest of the devices
> > are _not_ assigned to pciback.
> 
> Perhaps there is something to learn from the steps vfio-pci takes to do this ?
> (they sorted out quite some stuff around pci/vga passtrough)

That is actually what I based it on :-)

> 
> >> 
> >> diff --git a/drivers/xen/xen-pciback/pci_stub.c
> >> b/drivers/xen/xen-pciback/pci_stub.c
> >> index 4e8ba38..5a03e63 100644
> >> --- a/drivers/xen/xen-pciback/pci_stub.c
> >> +++ b/drivers/xen/xen-pciback/pci_stub.c
> >> @@ -14,6 +14,7 @@
> >>  #include <linux/wait.h>
> >>  #include <linux/sched.h>
> >>  #include <linux/atomic.h>
> >> +#include <linux/delay.h>
> >>  #include <xen/events.h>
> >>  #include <asm/xen/pci.h>
> >>  #include <asm/xen/hypervisor.h>
> >> @@ -43,6 +44,7 @@ struct pcistub_device {
> >>       struct kref kref;
> >>       struct list_head dev_list;
> >>       spinlock_t lock;
> >> +     bool created_reset_file;
> >> 
> >>       struct pci_dev *dev;
> >>       struct xen_pcibk_device *pdev;/* non-NULL if struct pci_dev is in use */
> >> @@ -60,6 +62,114 @@ 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;
> >> +     u16 ctrl;
> >> +     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;
> >> +             pci_save_state(pdev);
> >> +     }
> >> +
> >> +     pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
> >> +     ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> >> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> >> +     msleep(200);
> >> +
> >> +     ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> >> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> >> +     msleep(200);
> >> +
> >> +     list_for_each_entry(pdev, &dev->bus->devices, bus_list)
> >> +             pci_restore_state(pdev);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +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 pcistub_device *psdev)
> >> +{
> >> +     struct device *dev = &psdev->dev->dev;
> >> +     struct sysfs_dirent *reset_dirent;
> >> +     int ret;
> >> +
> >> +     reset_dirent = sysfs_get_dirent(dev->kobj.sd, NULL, "reset");
> >> +     if (reset_dirent) {
> >> +             sysfs_put(reset_dirent);
> >> +             return 0;
> >> +     }
> >> +
> >> +     ret = device_create_file(dev, &dev_attr_reset);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +     psdev->created_reset_file = true;
> >> +     return 0;
> >> +}
> >> +
> >> +static void pcistub_remove_reset_file(struct pcistub_device *psdev)
> >> +{
> >> +     if (psdev && psdev->created_reset_file)
> >> +             device_remove_file(&psdev->dev->dev, &dev_attr_reset);
> >> +}
> >> +
> >>  static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
> >>  {
> >>       struct pcistub_device *psdev;
> >> @@ -95,12 +205,15 @@ static void pcistub_device_release(struct kref *kref)
> >> 
> >>       dev_dbg(&dev->dev, "pcistub_device_release\n");
> >> 
> >> +     pcistub_remove_reset_file(psdev);
> >> +
> >>       xen_unregister_device_domain_owner(dev);
> >> 
> >>       /* 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
> >> @@ -268,7 +381,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(psdev->dev);
> >>       pci_restore_state(psdev->dev);
> >> 
> >>       /* This disables the device. */
> >> @@ -392,7 +505,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
> >> @@ -467,6 +580,10 @@ static int pcistub_seize(struct pci_dev *dev)
> >>       if (!psdev)
> >>               return -ENOMEM;
> >> 
> >> +     err = pcistub_try_create_reset_file(psdev);
> >> +     if (err < 0)
> >> +             goto out;
> >> +
> >>       spin_lock_irqsave(&pcistub_devices_lock, flags);
> >> 
> >>       if (initialize_devices) {
> >> @@ -485,10 +602,9 @@ static int pcistub_seize(struct pci_dev *dev)
> >>       }
> >> 
> >>       spin_unlock_irqrestore(&pcistub_devices_lock, flags);
> >> -
> >> +out:
> >>       if (err)
> >>               pcistub_device_put(psdev);
> >> -
> >>       return err;
> >>  }
> >> 
> 
> 
> 

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Sander Eikelenboom]

Monday, December 16, 2013, 4:36:12 PM, you wrote:

> On Mon, Dec 16, 2013 at 04:23:53PM +0100, Sander Eikelenboom wrote:
>> 
>> Monday, December 16, 2013, 3:35:15 PM, you wrote:
>> 
>> > On Mon, Dec 16, 2013 at 10:59:01AM +0000, David Vrabel wrote:
>> >> On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
>> >> > Hey,
>> >> > 
>> >> > While I was trying to narrow down the state of GPU passthrough
>> >> > (still not finished) and figuring what needs to be done I realized
>> >> > that Xen PCIback did not reset my GPU properly (when I crashed the
>> >> > Windows guest by mistake). It does an FLR reset or Power one - if
>> >> > the device supports it. But it seems that some of these GPUs
>> >> > are liars and actually don't do the power part properly.
>> >> 
>> >> In my experience the devices do not lie.  They correctly report that
>> >> they do not perform a reset in D3hot.
>> >> 
>> >> Here's the patch I'm using to solve this.  It does something similar.
>> >> i.e., a SBR if all devices on that bus are safe to be reset.
>> >> 
>> >> I prefer it because it provides the standard 'reset' sysfs file that the
>> >> toolstack/userspace can use.
>> 
>> > We can still add the 'reset' to SysFS
>> >> 
>> >> It does have some limitations:  a) It does not check whether a device is
>> >> in use (only if it is bound to pciback); and b) it hand rolls
>> >> pci_slot_reset() (because it didn't exist at the time).
>> 
>> > .. which can have those limiations removed and be based on this patchset.
>> > Meaning it won't do a bus-reset or device reset if the rest of the devices
>> > are _not_ assigned to pciback.
>> 
>> Perhaps there is something to learn from the steps vfio-pci takes to do this ?
>> (they sorted out quite some stuff around pci/vga passtrough)

> That is actually what I based it on :-)

OK was already suspecting it somehow :-)

Reminds me to see if the Radeon maintainer knows a way to hookup a sysfs reset entry for
Radeon cards, that completly cycles the card (including potential bios voodoo). Since AMD is supporting the development
of the opensource driver now, there should be chance and it would be welcome for both Xen and KVM
since those cards don't support FLR.

>> 
>> >> 
>> >> diff --git a/drivers/xen/xen-pciback/pci_stub.c
>> >> b/drivers/xen/xen-pciback/pci_stub.c
>> >> index 4e8ba38..5a03e63 100644
>> >> --- a/drivers/xen/xen-pciback/pci_stub.c
>> >> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> >> @@ -14,6 +14,7 @@
>> >>  #include <linux/wait.h>
>> >>  #include <linux/sched.h>
>> >>  #include <linux/atomic.h>
>> >> +#include <linux/delay.h>
>> >>  #include <xen/events.h>
>> >>  #include <asm/xen/pci.h>
>> >>  #include <asm/xen/hypervisor.h>
>> >> @@ -43,6 +44,7 @@ struct pcistub_device {
>> >>       struct kref kref;
>> >>       struct list_head dev_list;
>> >>       spinlock_t lock;
>> >> +     bool created_reset_file;
>> >> 
>> >>       struct pci_dev *dev;
>> >>       struct xen_pcibk_device *pdev;/* non-NULL if struct pci_dev is in use */
>> >> @@ -60,6 +62,114 @@ 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;
>> >> +     u16 ctrl;
>> >> +     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;
>> >> +             pci_save_state(pdev);
>> >> +     }
>> >> +
>> >> +     pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
>> >> +     ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
>> >> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> >> +     msleep(200);
>> >> +
>> >> +     ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
>> >> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> >> +     msleep(200);
>> >> +
>> >> +     list_for_each_entry(pdev, &dev->bus->devices, bus_list)
>> >> +             pci_restore_state(pdev);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +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 pcistub_device *psdev)
>> >> +{
>> >> +     struct device *dev = &psdev->dev->dev;
>> >> +     struct sysfs_dirent *reset_dirent;
>> >> +     int ret;
>> >> +
>> >> +     reset_dirent = sysfs_get_dirent(dev->kobj.sd, NULL, "reset");
>> >> +     if (reset_dirent) {
>> >> +             sysfs_put(reset_dirent);
>> >> +             return 0;
>> >> +     }
>> >> +
>> >> +     ret = device_create_file(dev, &dev_attr_reset);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >> +     psdev->created_reset_file = true;
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +static void pcistub_remove_reset_file(struct pcistub_device *psdev)
>> >> +{
>> >> +     if (psdev && psdev->created_reset_file)
>> >> +             device_remove_file(&psdev->dev->dev, &dev_attr_reset);
>> >> +}
>> >> +
>> >>  static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
>> >>  {
>> >>       struct pcistub_device *psdev;
>> >> @@ -95,12 +205,15 @@ static void pcistub_device_release(struct kref *kref)
>> >> 
>> >>       dev_dbg(&dev->dev, "pcistub_device_release\n");
>> >> 
>> >> +     pcistub_remove_reset_file(psdev);
>> >> +
>> >>       xen_unregister_device_domain_owner(dev);
>> >> 
>> >>       /* 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
>> >> @@ -268,7 +381,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(psdev->dev);
>> >>       pci_restore_state(psdev->dev);
>> >> 
>> >>       /* This disables the device. */
>> >> @@ -392,7 +505,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
>> >> @@ -467,6 +580,10 @@ static int pcistub_seize(struct pci_dev *dev)
>> >>       if (!psdev)
>> >>               return -ENOMEM;
>> >> 
>> >> +     err = pcistub_try_create_reset_file(psdev);
>> >> +     if (err < 0)
>> >> +             goto out;
>> >> +
>> >>       spin_lock_irqsave(&pcistub_devices_lock, flags);
>> >> 
>> >>       if (initialize_devices) {
>> >> @@ -485,10 +602,9 @@ static int pcistub_seize(struct pci_dev *dev)
>> >>       }
>> >> 
>> >>       spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>> >> -
>> >> +out:
>> >>       if (err)
>> >>               pcistub_device_put(psdev);
>> >> -
>> >>       return err;
>> >>  }
>> >> 
>> 
>> 
>> 

Re: [RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Sander Eikelenboom]

Monday, December 16, 2013, 4:36:12 PM, you wrote:

> On Mon, Dec 16, 2013 at 04:23:53PM +0100, Sander Eikelenboom wrote:
>> 
>> Monday, December 16, 2013, 3:35:15 PM, you wrote:
>> 
>> > On Mon, Dec 16, 2013 at 10:59:01AM +0000, David Vrabel wrote:
>> >> On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
>> >> > Hey,
>> >> > 
>> >> > While I was trying to narrow down the state of GPU passthrough
>> >> > (still not finished) and figuring what needs to be done I realized
>> >> > that Xen PCIback did not reset my GPU properly (when I crashed the
>> >> > Windows guest by mistake). It does an FLR reset or Power one - if
>> >> > the device supports it. But it seems that some of these GPUs
>> >> > are liars and actually don't do the power part properly.
>> >> 
>> >> In my experience the devices do not lie.  They correctly report that
>> >> they do not perform a reset in D3hot.
>> >> 
>> >> Here's the patch I'm using to solve this.  It does something similar.
>> >> i.e., a SBR if all devices on that bus are safe to be reset.
>> >> 
>> >> I prefer it because it provides the standard 'reset' sysfs file that the
>> >> toolstack/userspace can use.
>> 
>> > We can still add the 'reset' to SysFS
>> >> 
>> >> It does have some limitations:  a) It does not check whether a device is
>> >> in use (only if it is bound to pciback); and b) it hand rolls
>> >> pci_slot_reset() (because it didn't exist at the time).
>> 
>> > .. which can have those limiations removed and be based on this patchset.
>> > Meaning it won't do a bus-reset or device reset if the rest of the devices
>> > are _not_ assigned to pciback.
>> 
>> Perhaps there is something to learn from the steps vfio-pci takes to do this ?
>> (they sorted out quite some stuff around pci/vga passtrough)

> That is actually what I based it on :-)

Perhaps noteworthy then: [PATCH] pci: Add "try" reset interfaces
http://lkml.indiana.edu/hypermail/linux/kernel/1312.2/00577.html

>> 
>> >> 
>> >> diff --git a/drivers/xen/xen-pciback/pci_stub.c
>> >> b/drivers/xen/xen-pciback/pci_stub.c
>> >> index 4e8ba38..5a03e63 100644
>> >> --- a/drivers/xen/xen-pciback/pci_stub.c
>> >> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> >> @@ -14,6 +14,7 @@
>> >>  #include <linux/wait.h>
>> >>  #include <linux/sched.h>
>> >>  #include <linux/atomic.h>
>> >> +#include <linux/delay.h>
>> >>  #include <xen/events.h>
>> >>  #include <asm/xen/pci.h>
>> >>  #include <asm/xen/hypervisor.h>
>> >> @@ -43,6 +44,7 @@ struct pcistub_device {
>> >>       struct kref kref;
>> >>       struct list_head dev_list;
>> >>       spinlock_t lock;
>> >> +     bool created_reset_file;
>> >> 
>> >>       struct pci_dev *dev;
>> >>       struct xen_pcibk_device *pdev;/* non-NULL if struct pci_dev is in use */
>> >> @@ -60,6 +62,114 @@ 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;
>> >> +     u16 ctrl;
>> >> +     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;
>> >> +             pci_save_state(pdev);
>> >> +     }
>> >> +
>> >> +     pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
>> >> +     ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
>> >> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> >> +     msleep(200);
>> >> +
>> >> +     ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
>> >> +     pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> >> +     msleep(200);
>> >> +
>> >> +     list_for_each_entry(pdev, &dev->bus->devices, bus_list)
>> >> +             pci_restore_state(pdev);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +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 pcistub_device *psdev)
>> >> +{
>> >> +     struct device *dev = &psdev->dev->dev;
>> >> +     struct sysfs_dirent *reset_dirent;
>> >> +     int ret;
>> >> +
>> >> +     reset_dirent = sysfs_get_dirent(dev->kobj.sd, NULL, "reset");
>> >> +     if (reset_dirent) {
>> >> +             sysfs_put(reset_dirent);
>> >> +             return 0;
>> >> +     }
>> >> +
>> >> +     ret = device_create_file(dev, &dev_attr_reset);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >> +     psdev->created_reset_file = true;
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +static void pcistub_remove_reset_file(struct pcistub_device *psdev)
>> >> +{
>> >> +     if (psdev && psdev->created_reset_file)
>> >> +             device_remove_file(&psdev->dev->dev, &dev_attr_reset);
>> >> +}
>> >> +
>> >>  static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
>> >>  {
>> >>       struct pcistub_device *psdev;
>> >> @@ -95,12 +205,15 @@ static void pcistub_device_release(struct kref *kref)
>> >> 
>> >>       dev_dbg(&dev->dev, "pcistub_device_release\n");
>> >> 
>> >> +     pcistub_remove_reset_file(psdev);
>> >> +
>> >>       xen_unregister_device_domain_owner(dev);
>> >> 
>> >>       /* 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
>> >> @@ -268,7 +381,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(psdev->dev);
>> >>       pci_restore_state(psdev->dev);
>> >> 
>> >>       /* This disables the device. */
>> >> @@ -392,7 +505,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
>> >> @@ -467,6 +580,10 @@ static int pcistub_seize(struct pci_dev *dev)
>> >>       if (!psdev)
>> >>               return -ENOMEM;
>> >> 
>> >> +     err = pcistub_try_create_reset_file(psdev);
>> >> +     if (err < 0)
>> >> +             goto out;
>> >> +
>> >>       spin_lock_irqsave(&pcistub_devices_lock, flags);
>> >> 
>> >>       if (initialize_devices) {
>> >> @@ -485,10 +602,9 @@ static int pcistub_seize(struct pci_dev *dev)
>> >>       }
>> >> 
>> >>       spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>> >> -
>> >> +out:
>> >>       if (err)
>> >>               pcistub_device_put(psdev);
>> >> -
>> >>       return err;
>> >>  }
>> >> 
>> 
>> 
>> 

[RFC PATCH] Xen PCI back - do slot and bus reset (v0).

[Konrad Rzeszutek Wilk]

Hey,

While I was trying to narrow down the state of GPU passthrough
(still not finished) and figuring what needs to be done I realized
that Xen PCIback did not reset my GPU properly (when I crashed the
Windows guest by mistake). It does an FLR reset or Power one - if
the device supports it. But it seems that some of these GPUs
are liars and actually don't do the power part properly.

One way to fix this is by doing a slot (aka device) and bus reset.
Of course to do that - you need to make sure that all of the
functions of a device are under the ownership of xen-pciback.
Otherwise you might accidently reset your sound card while it is
being used.

These RFC patches cleanup pci back a bit and also make it possible
for Xen pciback to do the whole gamma of 'reset' for PCI devices:
FLR, power management, AER, slot and bus reset if neccessary.

Thanks go to Gordan Bobic for educating me on how to "reprogram"
and GFX460 in a Quardro 4000M and also reporting oddities when
a PCI device was reset but it looked like it was not reset.


 drivers/xen/xen-pciback/pci_stub.c | 142 +++++++++++++++++++++++++++++++------
 drivers/xen/xen-pciback/xenbus.c   |   5 +-
 2 files changed, 124 insertions(+), 23 deletions(-)


Konrad Rzeszutek Wilk (5):
      xen-pciback: Cleanup up pcistub_put_pci_dev
      xen-pciback: First reset, then free.
      xen-pciback: Document when we FLR an PCI device.
      xen/pciback: Move the FLR code to a function.
      xen/pciback: PCI reset slot or bus