[PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

The life-cycle of a PCI device in Xen pciback is complex and is constrained
by the generic PCI locking mechanism.

- It starts with the device being bound to us, for which we do a function
  reset (done via SysFS so the PCI lock is held).
- If the device is unbound from us, we also do a function reset
  (done via SysFS so the PCI lock is held).
- If the device is un-assigned from a guest - we do a function reset
  (no PCI lock is held).

All reset operations are done on the individual PCI function level
(so bus:device:function).

The reset for an individual PCI function means device must support FLR
(PCIe or AF), PM reset on D3hot->D0 device specific reset, or a secondary
bus reset for a singleton device on a bus but FLR does not have widespread
support or it is not reliable in some cases. So, we need to provide an
alternate mechanism to users to perform a slot or bus level reset.

Currently, a slot or bus reset is not exposed in SysFS as there is no good
way of exposing a bus topology there. This is due to the complexity -
we MUST know that the different functions of a PCIe device are not in use
by other drivers, or if they are in use (say one of them is assigned to a
guest and the other is  idle) - it is still OK to reset the slot (assuming
both of them are owned by Xen pciback).

This patch does that by doing a slot or bus reset (if slot not supported)
if all of the functions of a PCIe device belong to Xen PCIback.

Due to the complexity with the PCI lock we cannot do the reset when a
device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind')
as the pci_[slot|bus]_reset also takes the same lock resulting in a
dead-lock.

Putting the reset function in a work-queue or thread won't work either -
as we have to do the reset function outside the 'unbind' context (it holds
the PCI lock). But once you 'unbind' a device the device is no longer under
the ownership of Xen pciback and the pci_set_drvdata has been reset, so
we cannot use a thread for this.

Instead of doing all this complex dance, we depend on the tool-stack doing
the right thing. As such, we implement the 'do_flr' SysFS attribute which
'xl' uses when a device is detached or attached from/to a guest. It
bypasses the need to worry about the PCI lock.

To not inadvertently do a bus reset that would affect devices that are in
use by other drivers (other than Xen pciback) prior to the reset, we check
that all of the devices under the bridge are owned by Xen pciback. If they
are not, we refrain from executing the bus (or slot) reset.

Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 Documentation/ABI/testing/sysfs-driver-pciback |  12 +++
 drivers/xen/xen-pciback/pci_stub.c             | 125 +++++++++++++++++++++++++
 2 files changed, 137 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
index 6a733bf..ccf7dc0 100644
--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -11,3 +11,15 @@ Description:
                 #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
                 will allow the guest to read and write to the configuration
                 register 0x0E.
+
+What:           /sys/bus/pci/drivers/pciback/do_flr
+Date:           Nov 2017
+KernelVersion:  4.15
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                An option to perform a slot or bus reset when a PCI device
+		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
+		will cause the pciback driver to perform a slot or bus reset
+		if the device supports it. It also checks to make sure that
+		all of the devices under the bridge are owned by Xen PCI
+		backend.
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 6331a95..2b2c269 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -244,6 +244,96 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
 	return found_dev;
 }
 
+struct pcistub_args {
+	struct pci_dev *dev;
+	int dcount;
+};
+
+static int pcistub_search_dev(struct pci_dev *dev, void *data)
+{
+	struct pcistub_device *psdev;
+	struct pcistub_args *arg = data;
+	bool found_dev = false;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pcistub_devices_lock, flags);
+
+	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
+		if (psdev->dev == dev) {
+			found_dev = true;
+			arg->dcount++;
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
+
+	/* Device not owned by pcistub, someone owns it. Abort the walk */
+	if (!found_dev)
+		arg->dev = dev;
+
+	return found_dev ? 0 : 1;
+}
+
+static int pcistub_reset_dev(struct pci_dev *dev)
+{
+	struct xen_pcibk_dev_data *dev_data;
+	bool slot = false, bus = false;
+
+	if (!dev)
+		return -EINVAL;
+
+	dev_dbg(&dev->dev, "[%s]\n", __func__);
+
+	if (!pci_probe_reset_slot(dev->slot)) {
+		slot = true;
+	} else if (!pci_probe_reset_bus(dev->bus)) {
+		/* We won't attempt to reset a root bridge. */
+		if (!pci_is_root_bus(dev->bus))
+			bus = true;
+	}
+
+	if (!bus && !slot)
+		return -EOPNOTSUPP;
+
+	if (!slot) {
+		struct pcistub_args arg = { .dev = NULL, .dcount = 0 };
+
+		/*
+		 * Make sure all devices on this bus are owned by the
+		 * PCI backend so that we can safely reset the whole bus.
+		 */
+		pci_walk_bus(dev->bus, pcistub_search_dev, &arg);
+
+		/* All devices under the bus should be part of pcistub! */
+		if (arg.dev) {
+			dev_err(&dev->dev, "%s device on the bus is not owned by pcistub\n",
+				pci_name(arg.dev));
+
+			return -EBUSY;
+		}
+
+		dev_dbg(&dev->dev, "pcistub owns %d devices on the bus\n",
+			arg.dcount);
+	}
+
+	dev_data = pci_get_drvdata(dev);
+	if (!pci_load_saved_state(dev, dev_data->pci_saved_state))
+		pci_restore_state(dev);
+
+	/* This disables the device. */
+	xen_pcibk_reset_device(dev);
+
+	/* Cleanup up any emulated fields */
+	xen_pcibk_config_reset_dev(dev);
+
+	dev_dbg(&dev->dev, "resetting %s device using %s reset\n",
+		pci_name(dev), slot ? "slot" : "bus");
+
+	return slot ? pci_try_reset_slot(dev->slot) :
+			pci_try_reset_bus(dev->bus);
+}
+
 /*
  * Called when:
  *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
@@ -1434,6 +1524,34 @@ static ssize_t permissive_show(struct device_driver *drv, char *buf)
 static DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show,
 		   permissive_add);
 
+static ssize_t pcistub_do_flr(struct device_driver *drv, const char *buf,
+			      size_t count)
+{
+	struct pcistub_device *psdev;
+	int domain, bus, slot, func;
+	int err;
+
+	err = str_to_slot(buf, &domain, &bus, &slot, &func);
+	if (err)
+		goto out;
+
+	psdev = pcistub_device_find(domain, bus, slot, func);
+	if (psdev) {
+		err = pcistub_reset_dev(psdev->dev);
+		pcistub_device_put(psdev);
+	} else {
+		err = -ENODEV;
+	}
+
+out:
+	if (!err)
+		err = count;
+
+	return err;
+}
+
+static DRIVER_ATTR(do_flr, S_IWUSR, NULL, pcistub_do_flr);
+
 static void pcistub_exit(void)
 {
 	driver_remove_file(&xen_pcibk_pci_driver.driver, &driver_attr_new_slot);
@@ -1447,6 +1565,8 @@ static void pcistub_exit(void)
 			   &driver_attr_irq_handlers);
 	driver_remove_file(&xen_pcibk_pci_driver.driver,
 			   &driver_attr_irq_handler_state);
+	driver_remove_file(&xen_pcibk_pci_driver.driver,
+			   &driver_attr_do_flr);
 	pci_unregister_driver(&xen_pcibk_pci_driver);
 }
 
@@ -1540,6 +1660,11 @@ static int __init pcistub_init(void)
 	if (!err)
 		err = driver_create_file(&xen_pcibk_pci_driver.driver,
 					&driver_attr_irq_handler_state);
+
+	if (!err)
+		err = driver_create_file(&xen_pcibk_pci_driver.driver,
+					 &driver_attr_do_flr);
+
 	if (err)
 		pcistub_exit();
 
-- 
2.9.5

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Roger Pau Monné]

On Mon, Nov 06, 2017 at 12:48:42PM -0500, Govinda Tatti wrote:
> The life-cycle of a PCI device in Xen pciback is complex and is constrained
> by the generic PCI locking mechanism.
> 
> - It starts with the device being bound to us, for which we do a function
>   reset (done via SysFS so the PCI lock is held).
> - If the device is unbound from us, we also do a function reset
>   (done via SysFS so the PCI lock is held).
> - If the device is un-assigned from a guest - we do a function reset
>   (no PCI lock is held).
> 
> All reset operations are done on the individual PCI function level
> (so bus:device:function).
> 
> The reset for an individual PCI function means device must support FLR
> (PCIe or AF), PM reset on D3hot->D0 device specific reset, or a secondary
> bus reset for a singleton device on a bus but FLR does not have widespread
> support or it is not reliable in some cases. So, we need to provide an
> alternate mechanism to users to perform a slot or bus level reset.
> 
> Currently, a slot or bus reset is not exposed in SysFS as there is no good
> way of exposing a bus topology there. This is due to the complexity -
> we MUST know that the different functions of a PCIe device are not in use
> by other drivers, or if they are in use (say one of them is assigned to a
> guest and the other is  idle) - it is still OK to reset the slot (assuming
> both of them are owned by Xen pciback).
> 
> This patch does that by doing a slot or bus reset (if slot not supported)
> if all of the functions of a PCIe device belong to Xen PCIback.
> 
> Due to the complexity with the PCI lock we cannot do the reset when a
> device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind')
> as the pci_[slot|bus]_reset also takes the same lock resulting in a
> dead-lock.
> 
> Putting the reset function in a work-queue or thread won't work either -
> as we have to do the reset function outside the 'unbind' context (it holds
> the PCI lock). But once you 'unbind' a device the device is no longer under
> the ownership of Xen pciback and the pci_set_drvdata has been reset, so
> we cannot use a thread for this.
> 
> Instead of doing all this complex dance, we depend on the tool-stack doing
> the right thing. As such, we implement the 'do_flr' SysFS attribute which
> 'xl' uses when a device is detached or attached from/to a guest. It
> bypasses the need to worry about the PCI lock.
> 
> To not inadvertently do a bus reset that would affect devices that are in
> use by other drivers (other than Xen pciback) prior to the reset, we check
> that all of the devices under the bridge are owned by Xen pciback. If they
> are not, we refrain from executing the bus (or slot) reset.
> 
> Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-pciback |  12 +++
>  drivers/xen/xen-pciback/pci_stub.c             | 125 +++++++++++++++++++++++++
>  2 files changed, 137 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
> index 6a733bf..ccf7dc0 100644
> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,15 @@ Description:
>                  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>                  will allow the guest to read and write to the configuration
>                  register 0x0E.
> +
> +What:           /sys/bus/pci/drivers/pciback/do_flr
> +Date:           Nov 2017
> +KernelVersion:  4.15
> +Contact:        xen-devel@lists.xenproject.org
> +Description:
> +                An option to perform a slot or bus reset when a PCI device
> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
> +		will cause the pciback driver to perform a slot or bus reset
> +		if the device supports it. It also checks to make sure that
> +		all of the devices under the bridge are owned by Xen PCI
> +		backend.
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 6331a95..2b2c269 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -244,6 +244,96 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
>  	return found_dev;
>  }
>  
> +struct pcistub_args {
> +	struct pci_dev *dev;

const?

> +	int dcount;

unsigned int.

> +};
> +
> +static int pcistub_search_dev(struct pci_dev *dev, void *data)

Seems like this function would better return a boolean rather than an
int.

> +{
> +	struct pcistub_device *psdev;
> +	struct pcistub_args *arg = data;
> +	bool found_dev = false;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pcistub_devices_lock, flags);
> +
> +	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
> +		if (psdev->dev == dev) {
> +			found_dev = true;
> +			arg->dcount++;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
> +
> +	/* Device not owned by pcistub, someone owns it. Abort the walk */
> +	if (!found_dev)
> +		arg->dev = dev;
> +
> +	return found_dev ? 0 : 1;
> +}
> +
> +static int pcistub_reset_dev(struct pci_dev *dev)
> +{
> +	struct xen_pcibk_dev_data *dev_data;
> +	bool slot = false, bus = false;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	dev_dbg(&dev->dev, "[%s]\n", __func__);
> +
> +	if (!pci_probe_reset_slot(dev->slot)) {
> +		slot = true;
> +	} else if (!pci_probe_reset_bus(dev->bus)) {
> +		/* We won't attempt to reset a root bridge. */
> +		if (!pci_is_root_bus(dev->bus))
> +			bus = true;

Con't you join the two if, ie:

} else if (!pci_probe_reset_bus(dev->bus) && !pci_is_root_bus(dev->bus)) {

> +	}
> +
> +	if (!bus && !slot)
> +		return -EOPNOTSUPP;
> +
> +	if (!slot) {
> +		struct pcistub_args arg = { .dev = NULL, .dcount = 0 };
> +
> +		/*
> +		 * Make sure all devices on this bus are owned by the
> +		 * PCI backend so that we can safely reset the whole bus.
> +		 */
> +		pci_walk_bus(dev->bus, pcistub_search_dev, &arg);
> +
> +		/* All devices under the bus should be part of pcistub! */
> +		if (arg.dev) {
> +			dev_err(&dev->dev, "%s device on the bus is not owned by pcistub\n",
> +				pci_name(arg.dev));
> +
> +			return -EBUSY;

Not sure EBUSY is the best return code here, EINVAL?

> +		}
> +
> +		dev_dbg(&dev->dev, "pcistub owns %d devices on the bus\n",
> +			arg.dcount);
> +	}
> +
> +	dev_data = pci_get_drvdata(dev);
> +	if (!pci_load_saved_state(dev, dev_data->pci_saved_state))
> +		pci_restore_state(dev);
> +
> +	/* This disables the device. */
> +	xen_pcibk_reset_device(dev);
> +
> +	/* Cleanup up any emulated fields */
> +	xen_pcibk_config_reset_dev(dev);
> +
> +	dev_dbg(&dev->dev, "resetting %s device using %s reset\n",
> +		pci_name(dev), slot ? "slot" : "bus");
> +
> +	return slot ? pci_try_reset_slot(dev->slot) :
> +			pci_try_reset_bus(dev->bus);
> +}
> +
>  /*
>   * Called when:
>   *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
> @@ -1434,6 +1524,34 @@ static ssize_t permissive_show(struct device_driver *drv, char *buf)
>  static DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show,
>  		   permissive_add);
>  
> +static ssize_t pcistub_do_flr(struct device_driver *drv, const char *buf,
> +			      size_t count)
> +{
> +	struct pcistub_device *psdev;
> +	int domain, bus, slot, func;
> +	int err;
> +
> +	err = str_to_slot(buf, &domain, &bus, &slot, &func);
> +	if (err)
> +		goto out;
		return err;

> +
> +	psdev = pcistub_device_find(domain, bus, slot, func);
	if (!psdev)
		return -ENODEV;

> +	if (psdev) {
> +		err = pcistub_reset_dev(psdev->dev);
> +		pcistub_device_put(psdev);
> +	} else {
> +		err = -ENODEV;
> +	}
> +
> +out:
> +	if (!err)
> +		err = count;
> +
> +	return err;

What's the purpose of returning count here?

Roger.

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Jan Beulich]

>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,15 @@ Description:
>                  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>                  will allow the guest to read and write to the configuration
>                  register 0x0E.
> +
> +What:           /sys/bus/pci/drivers/pciback/do_flr
> +Date:           Nov 2017
> +KernelVersion:  4.15
> +Contact:        xen-devel@lists.xenproject.org 
> +Description:
> +                An option to perform a slot or bus reset when a PCI device
> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
> +		will cause the pciback driver to perform a slot or bus reset
> +		if the device supports it. It also checks to make sure that
> +		all of the devices under the bridge are owned by Xen PCI
> +		backend.

Why do you name this "do_flr" when you don't even try FLR, but
go to slot or then bus reset right away.

> +static int pcistub_reset_dev(struct pci_dev *dev)
> +{
> +	struct xen_pcibk_dev_data *dev_data;
> +	bool slot = false, bus = false;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	dev_dbg(&dev->dev, "[%s]\n", __func__);
> +
> +	if (!pci_probe_reset_slot(dev->slot)) {
> +		slot = true;
> +	} else if (!pci_probe_reset_bus(dev->bus)) {
> +		/* We won't attempt to reset a root bridge. */
> +		if (!pci_is_root_bus(dev->bus))
> +			bus = true;
> +	}
> +
> +	if (!bus && !slot)
> +		return -EOPNOTSUPP;
> +
> +	if (!slot) {
> +		struct pcistub_args arg = { .dev = NULL, .dcount = 0 };

Neither of the two initializers is really needed - just {} will do.

> +		/*
> +		 * Make sure all devices on this bus are owned by the
> +		 * PCI backend so that we can safely reset the whole bus.
> +		 */
> +		pci_walk_bus(dev->bus, pcistub_search_dev, &arg);
> +
> +		/* All devices under the bus should be part of pcistub! */
> +		if (arg.dev) {
> +			dev_err(&dev->dev, "%s device on the bus is not owned by pcistub\n",
> +				pci_name(arg.dev));

I think "device" is superfluous here, while "the bus" could do with
replacing by something actually identifying the bus.

> +			return -EBUSY;
> +		}
> +
> +		dev_dbg(&dev->dev, "pcistub owns %d devices on the bus\n",
> +			arg.dcount);

Same here for "the bus", provided this log message is useful in the
first place.

> +	}

Aren't you missing an "else" here? Aiui in the "slot" case it may
still be multiple devices/functions which are affected.

Jan

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

Thanks Roger for your review comments. Please see below for my comments.

On 11/7/2017 5:21 AM, Roger Pau Monné wrote:
> On Mon, Nov 06, 2017 at 12:48:42PM -0500, Govinda Tatti wrote:
>> The life-cycle of a PCI device in Xen pciback is complex and is constrained
>> by the generic PCI locking mechanism.
>>
>> - It starts with the device being bound to us, for which we do a function
>>    reset (done via SysFS so the PCI lock is held).
>> - If the device is unbound from us, we also do a function reset
>>    (done via SysFS so the PCI lock is held).
>> - If the device is un-assigned from a guest - we do a function reset
>>    (no PCI lock is held).
>>
>> All reset operations are done on the individual PCI function level
>> (so bus:device:function).
>>
>> The reset for an individual PCI function means device must support FLR
>> (PCIe or AF), PM reset on D3hot->D0 device specific reset, or a secondary
>> bus reset for a singleton device on a bus but FLR does not have widespread
>> support or it is not reliable in some cases. So, we need to provide an
>> alternate mechanism to users to perform a slot or bus level reset.
>>
>> Currently, a slot or bus reset is not exposed in SysFS as there is no good
>> way of exposing a bus topology there. This is due to the complexity -
>> we MUST know that the different functions of a PCIe device are not in use
>> by other drivers, or if they are in use (say one of them is assigned to a
>> guest and the other is  idle) - it is still OK to reset the slot (assuming
>> both of them are owned by Xen pciback).
>>
>> This patch does that by doing a slot or bus reset (if slot not supported)
>> if all of the functions of a PCIe device belong to Xen PCIback.
>>
>> Due to the complexity with the PCI lock we cannot do the reset when a
>> device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind')
>> as the pci_[slot|bus]_reset also takes the same lock resulting in a
>> dead-lock.
>>
>> Putting the reset function in a work-queue or thread won't work either -
>> as we have to do the reset function outside the 'unbind' context (it holds
>> the PCI lock). But once you 'unbind' a device the device is no longer under
>> the ownership of Xen pciback and the pci_set_drvdata has been reset, so
>> we cannot use a thread for this.
>>
>> Instead of doing all this complex dance, we depend on the tool-stack doing
>> the right thing. As such, we implement the 'do_flr' SysFS attribute which
>> 'xl' uses when a device is detached or attached from/to a guest. It
>> bypasses the need to worry about the PCI lock.
>>
>> To not inadvertently do a bus reset that would affect devices that are in
>> use by other drivers (other than Xen pciback) prior to the reset, we check
>> that all of the devices under the bridge are owned by Xen pciback. If they
>> are not, we refrain from executing the bus (or slot) reset.
>>
>> Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   Documentation/ABI/testing/sysfs-driver-pciback |  12 +++
>>   drivers/xen/xen-pciback/pci_stub.c             | 125 +++++++++++++++++++++++++
>>   2 files changed, 137 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
>> index 6a733bf..ccf7dc0 100644
>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>> @@ -11,3 +11,15 @@ Description:
>>                   #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>>                   will allow the guest to read and write to the configuration
>>                   register 0x0E.
>> +
>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>> +Date:           Nov 2017
>> +KernelVersion:  4.15
>> +Contact:        xen-devel@lists.xenproject.org
>> +Description:
>> +                An option to perform a slot or bus reset when a PCI device
>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
>> +		will cause the pciback driver to perform a slot or bus reset
>> +		if the device supports it. It also checks to make sure that
>> +		all of the devices under the bridge are owned by Xen PCI
>> +		backend.
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 6331a95..2b2c269 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -244,6 +244,96 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
>>   	return found_dev;
>>   }
>>   
>> +struct pcistub_args {
>> +	struct pci_dev *dev;
> const?
This field will point to first device that is not owned by pcistub.
>
>> +	int dcount;
> unsigned int.
OK.
>
>> +};
>> +
>> +static int pcistub_search_dev(struct pci_dev *dev, void *data)
> Seems like this function would better return a boolean rather than an
> int.
pcistub_search_dev() is invoked through pci_walk_bus() and it expects 
int return code.
>
>> +{
>> +	struct pcistub_device *psdev;
>> +	struct pcistub_args *arg = data;
>> +	bool found_dev = false;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&pcistub_devices_lock, flags);
>> +
>> +	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
>> +		if (psdev->dev == dev) {
>> +			found_dev = true;
>> +			arg->dcount++;
>> +			break;
>> +		}
>> +	}
>> +
>> +	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>> +
>> +	/* Device not owned by pcistub, someone owns it. Abort the walk */
>> +	if (!found_dev)
>> +		arg->dev = dev;
>> +
>> +	return found_dev ? 0 : 1;
>> +}
>> +
>> +static int pcistub_reset_dev(struct pci_dev *dev)
>> +{
>> +	struct xen_pcibk_dev_data *dev_data;
>> +	bool slot = false, bus = false;
>> +
>> +	if (!dev)
>> +		return -EINVAL;
>> +
>> +	dev_dbg(&dev->dev, "[%s]\n", __func__);
>> +
>> +	if (!pci_probe_reset_slot(dev->slot)) {
>> +		slot = true;
>> +	} else if (!pci_probe_reset_bus(dev->bus)) {
>> +		/* We won't attempt to reset a root bridge. */
>> +		if (!pci_is_root_bus(dev->bus))
>> +			bus = true;
> Con't you join the two if, ie:
>
> } else if (!pci_probe_reset_bus(dev->bus) && !pci_is_root_bus(dev->bus)) {
Ok
>
>> +	}
>> +
>> +	if (!bus && !slot)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!slot) {
>> +		struct pcistub_args arg = { .dev = NULL, .dcount = 0 };
>> +
>> +		/*
>> +		 * Make sure all devices on this bus are owned by the
>> +		 * PCI backend so that we can safely reset the whole bus.
>> +		 */
>> +		pci_walk_bus(dev->bus, pcistub_search_dev, &arg);
>> +
>> +		/* All devices under the bus should be part of pcistub! */
>> +		if (arg.dev) {
>> +			dev_err(&dev->dev, "%s device on the bus is not owned by pcistub\n",
>> +				pci_name(arg.dev));
>> +
>> +			return -EBUSY;
> Not sure EBUSY is the best return code here, EINVAL?
I don't think EINVAL is right return code for this case.
>
>> +		}
>> +
>> +		dev_dbg(&dev->dev, "pcistub owns %d devices on the bus\n",
>> +			arg.dcount);
>> +	}
>> +
>> +	dev_data = pci_get_drvdata(dev);
>> +	if (!pci_load_saved_state(dev, dev_data->pci_saved_state))
>> +		pci_restore_state(dev);
>> +
>> +	/* This disables the device. */
>> +	xen_pcibk_reset_device(dev);
>> +
>> +	/* Cleanup up any emulated fields */
>> +	xen_pcibk_config_reset_dev(dev);
>> +
>> +	dev_dbg(&dev->dev, "resetting %s device using %s reset\n",
>> +		pci_name(dev), slot ? "slot" : "bus");
>> +
>> +	return slot ? pci_try_reset_slot(dev->slot) :
>> +			pci_try_reset_bus(dev->bus);
>> +}
>> +
>>   /*
>>    * Called when:
>>    *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
>> @@ -1434,6 +1524,34 @@ static ssize_t permissive_show(struct device_driver *drv, char *buf)
>>   static DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show,
>>   		   permissive_add);
>>   
>> +static ssize_t pcistub_do_flr(struct device_driver *drv, const char *buf,
>> +			      size_t count)
>> +{
>> +	struct pcistub_device *psdev;
>> +	int domain, bus, slot, func;
>> +	int err;
>> +
>> +	err = str_to_slot(buf, &domain, &bus, &slot, &func);
>> +	if (err)
>> +		goto out;
> 		return err;
>
>> +
>> +	psdev = pcistub_device_find(domain, bus, slot, func);
> 	if (!psdev)
> 		return -ENODEV;
>
>> +	if (psdev) {
>> +		err = pcistub_reset_dev(psdev->dev);
>> +		pcistub_device_put(psdev);
>> +	} else {
>> +		err = -ENODEV;
>> +	}
>> +
>> +out:
>> +	if (!err)
>> +		err = count;
>> +
>> +	return err;
> What's the purpose of returning count here?
Not sure. Will check with Konrad and address this comment.

I will post revised patch later this week. Thanks.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Juergen Gross]

On 08/11/17 16:00, Govinda Tatti wrote:
> Thanks Roger for your review comments. Please see below for my comments.
> 
> On 11/7/2017 5:21 AM, Roger Pau Monné wrote:
>> On Mon, Nov 06, 2017 at 12:48:42PM -0500, Govinda Tatti wrote:
>>> +out:
>>> +    if (!err)
>>> +        err = count;
>>> +
>>> +    return err;
>> What's the purpose of returning count here?
> Not sure. Will check with Konrad and address this comment.

You are telling sysfs that you have consumed all input characters.


Juergen

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

On 11/8/2017 9:09 AM, Juergen Gross wrote:
> On 08/11/17 16:00, Govinda Tatti wrote:
>> Thanks Roger for your review comments. Please see below for my comments.
>>
>> On 11/7/2017 5:21 AM, Roger Pau Monné wrote:
>>> On Mon, Nov 06, 2017 at 12:48:42PM -0500, Govinda Tatti wrote:
>>>> +out:
>>>> +    if (!err)
>>>> +        err = count;
>>>> +
>>>> +    return err;
>>> What's the purpose of returning count here?
>> Not sure. Will check with Konrad and address this comment.
> You are telling sysfs that you have consumed all input characters.
>
Thanks Juergen

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

Thanks Jan for your review comments. Please see below for my comments.

On 11/7/2017 8:40 AM, Jan Beulich wrote:
>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>> @@ -11,3 +11,15 @@ Description:
>>                   #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>>                   will allow the guest to read and write to the configuration
>>                   register 0x0E.
>> +
>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>> +Date:           Nov 2017
>> +KernelVersion:  4.15
>> +Contact:        xen-devel@lists.xenproject.org
>> +Description:
>> +                An option to perform a slot or bus reset when a PCI device
>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
>> +		will cause the pciback driver to perform a slot or bus reset
>> +		if the device supports it. It also checks to make sure that
>> +		all of the devices under the bridge are owned by Xen PCI
>> +		backend.
> Why do you name this "do_flr" when you don't even try FLR, but
> go to slot or then bus reset right away.
Yes, I agree but xen toolstack has already been modified to 
consume"do_flr" attribute. Hence, we are using the
function that matches with sysfs attribute.
>
>> +static int pcistub_reset_dev(struct pci_dev *dev)
>> +{
>> +	struct xen_pcibk_dev_data *dev_data;
>> +	bool slot = false, bus = false;
>> +
>> +	if (!dev)
>> +		return -EINVAL;
>> +
>> +	dev_dbg(&dev->dev, "[%s]\n", __func__);
>> +
>> +	if (!pci_probe_reset_slot(dev->slot)) {
>> +		slot = true;
>> +	} else if (!pci_probe_reset_bus(dev->bus)) {
>> +		/* We won't attempt to reset a root bridge. */
>> +		if (!pci_is_root_bus(dev->bus))
>> +			bus = true;
>> +	}
>> +
>> +	if (!bus && !slot)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!slot) {
>> +		struct pcistub_args arg = { .dev = NULL, .dcount = 0 };
> Neither of the two initializers is really needed - just {} will do.
OK.
>
>> +		/*
>> +		 * Make sure all devices on this bus are owned by the
>> +		 * PCI backend so that we can safely reset the whole bus.
>> +		 */
>> +		pci_walk_bus(dev->bus, pcistub_search_dev, &arg);
>> +
>> +		/* All devices under the bus should be part of pcistub! */
>> +		if (arg.dev) {
>> +			dev_err(&dev->dev, "%s device on the bus is not owned by pcistub\n",
>> +				pci_name(arg.dev));
> I think "device" is superfluous here, while "the bus" could do with
> replacing by something actually identifying the bus.
I assume you want "bus" number to be printed in above msg. If yes, will do.
>
>> +			return -EBUSY;
>> +		}
>> +
>> +		dev_dbg(&dev->dev, "pcistub owns %d devices on the bus\n",
>> +			arg.dcount);
> Same here for "the bus", provided this log message is useful in the
> first place.
>
>> +	}
> Aren't you missing an "else" here? Aiui in the "slot" case it may
> still be multiple devices/functions which are affected.
Good catch.  Our original code was performing same check, both for bus 
and slot case
but somehow I removed it during internal review based on some comment.

I will post revised patch later this week. Thanks.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Pasi Kärkkäinen]

Hi,

On Wed, Nov 08, 2017 at 09:44:48AM -0600, Govinda Tatti wrote:
> Thanks Jan for your review comments. Please see below for my comments.
> 
> On 11/7/2017 8:40 AM, Jan Beulich wrote:
> >>>>On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
> >>--- a/Documentation/ABI/testing/sysfs-driver-pciback
> >>+++ b/Documentation/ABI/testing/sysfs-driver-pciback
> >>@@ -11,3 +11,15 @@ Description:
> >>                  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
> >>                  will allow the guest to read and write to the configuration
> >>                  register 0x0E.
> >>+
> >>+What:           /sys/bus/pci/drivers/pciback/do_flr
> >>+Date:           Nov 2017
> >>+KernelVersion:  4.15
> >>+Contact:        xen-devel@lists.xenproject.org
> >>+Description:
> >>+                An option to perform a slot or bus reset when a PCI device
> >>+		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
> >>+		will cause the pciback driver to perform a slot or bus reset
> >>+		if the device supports it. It also checks to make sure that
> >>+		all of the devices under the bridge are owned by Xen PCI
> >>+		backend.
> >Why do you name this "do_flr" when you don't even try FLR, but
> >go to slot or then bus reset right away.
> Yes, I agree but xen toolstack has already been modified to consume"do_flr"
> attribute. Hence, we are using the
> function that matches with sysfs attribute.
>

Hmm.. I remember some discussion from ages ago related to this.

Back then it was suggested to "emulate" the flr capability (by doing slot or bus reset) for devices which don't have *native* flr available? So is this patch perhaps related to that?

If the PCI device in question has native flr capability, then native flr is used, right ? 
I guess I should read the full patch.. 

-- Pasi

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

Thanks Pasi for your response. Please see below for my comments.

On 11/8/2017 11:38 AM, Pasi Kärkkäinen wrote:
> Hi,
>
> On Wed, Nov 08, 2017 at 09:44:48AM -0600, Govinda Tatti wrote:
>> Thanks Jan for your review comments. Please see below for my comments.
>>
>> On 11/7/2017 8:40 AM, Jan Beulich wrote:
>>>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>>>> @@ -11,3 +11,15 @@ Description:
>>>>                   #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>>>>                   will allow the guest to read and write to the configuration
>>>>                   register 0x0E.
>>>> +
>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>>>> +Date:           Nov 2017
>>>> +KernelVersion:  4.15
>>>> +Contact:        xen-devel@lists.xenproject.org
>>>> +Description:
>>>> +                An option to perform a slot or bus reset when a PCI device
>>>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
>>>> +		will cause the pciback driver to perform a slot or bus reset
>>>> +		if the device supports it. It also checks to make sure that
>>>> +		all of the devices under the bridge are owned by Xen PCI
>>>> +		backend.
>>> Why do you name this "do_flr" when you don't even try FLR, but
>>> go to slot or then bus reset right away.
>> Yes, I agree but xen toolstack has already been modified to consume"do_flr"
>> attribute. Hence, we are using the
>> function that matches with sysfs attribute.
>>
> Hmm.. I remember some discussion from ages ago related to this.
>
> Back then it was suggested to "emulate" the flr capability (by doing slot or bus reset) for devices which don't have *native* flr available? So is this patch perhaps related to that?
I don't think so but either Konrad or someone can comment on it.
>
> If the PCI device in question has native flr capability, then native flr is used, right ?
Yes.
> I guess I should read the full patch..
Please check it and let us know your comments.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Jan Beulich]

>>> On 08.11.17 at 16:44, <govinda.tatti@oracle.com> wrote:
> On 11/7/2017 8:40 AM, Jan Beulich wrote:
>>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>>> @@ -11,3 +11,15 @@ Description:
>>>                   #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>>>                   will allow the guest to read and write to the configuration
>>>                   register 0x0E.
>>> +
>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>>> +Date:           Nov 2017
>>> +KernelVersion:  4.15
>>> +Contact:        xen-devel@lists.xenproject.org 
>>> +Description:
>>> +                An option to perform a slot or bus reset when a PCI device
>>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
>>> +		will cause the pciback driver to perform a slot or bus reset
>>> +		if the device supports it. It also checks to make sure that
>>> +		all of the devices under the bridge are owned by Xen PCI
>>> +		backend.
>> Why do you name this "do_flr" when you don't even try FLR, but
>> go to slot or then bus reset right away.
> Yes, I agree but xen toolstack has already been modified to 
> consume"do_flr" attribute. Hence, we are using the
> function that matches with sysfs attribute.

That's not a valid reason imo: Right now the driver doesn't expose
such an attribute, so if the tool stack was trying to use it, it would
not do what is intended at present anyway (i.e. the code could at
best be called dead). Furthermore, contrary to what you claim in
your reply to Pasi, I can't see where you try an actual FLR first -
you go straight to pci_probe_reset_{slot,bus}(). If you actually
tried FLR first, only falling back to the other methods as "emulation",
I could certainly agree with the file name chosen.

And btw - I don't consider it too good an idea to post the next
version of a patch when discussion of the prior one hasn't really
settled yet.

Jan

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

Jan,

Sorry for the late response. Please see below for my comments.

On 11/9/2017 2:28 AM, Jan Beulich wrote:
>>>> On 08.11.17 at 16:44, <govinda.tatti@oracle.com> wrote:
>> On 11/7/2017 8:40 AM, Jan Beulich wrote:
>>>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>>>> @@ -11,3 +11,15 @@ Description:
>>>>                    #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>>>>                    will allow the guest to read and write to the configuration
>>>>                    register 0x0E.
>>>> +
>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>>>> +Date:           Nov 2017
>>>> +KernelVersion:  4.15
>>>> +Contact:        xen-devel@lists.xenproject.org
>>>> +Description:
>>>> +                An option to perform a slot or bus reset when a PCI device
>>>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
>>>> +		will cause the pciback driver to perform a slot or bus reset
>>>> +		if the device supports it. It also checks to make sure that
>>>> +		all of the devices under the bridge are owned by Xen PCI
>>>> +		backend.
>>> Why do you name this "do_flr" when you don't even try FLR, but
>>> go to slot or then bus reset right away.
>> Yes, I agree but xen toolstack has already been modified to
>> consume"do_flr" attribute. Hence, we are using the
>> function that matches with sysfs attribute.
> That's not a valid reason imo: Right now the driver doesn't expose
> such an attribute, so if the tool stack was trying to use it, it would
> not do what is intended at present anyway (i.e. the code could at
> best be called dead).
Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus 
reset".
Please let me knowyour preference.

> Furthermore, contrary to what you claim in
> your reply to Pasi, I can't see where you try an actual FLR first -
> you go straight to pci_probe_reset_{slot,bus}(). If you actually
> tried FLR first, only falling back to the other methods as "emulation",
> I could certainly agree with the file name chosen.
Currently, multiple resets are being invoked (independently) in the context
of "xl attach/detach/shutdown/reboot".

- pci_reset_function_locked (invoked by pcistub_put_pci_dev())
   This function tries various PCI reset methods including FLR.
- pcistub_reset_dev (called by toolsstack based on "do_flr" attribute)

> And btw - I don't consider it too good an idea to post the next
> version of a patch when discussion of the prior one hasn't really
> settled yet.
Sure, In future I will not post next version of the patch until we 
complete agree
on all the review comment/fixes.Thanks.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Jan Beulich]

>>> On 29.11.17 at 16:08, <govinda.tatti@oracle.com> wrote:
> On 11/9/2017 2:28 AM, Jan Beulich wrote:
>>>>> On 08.11.17 at 16:44, <govinda.tatti@oracle.com> wrote:
>>> On 11/7/2017 8:40 AM, Jan Beulich wrote:
>>>>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
>>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>>>>> @@ -11,3 +11,15 @@ Description:
>>>>>                    #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>>>>>                    will allow the guest to read and write to the configuration
>>>>>                    register 0x0E.
>>>>> +
>>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>>>>> +Date:           Nov 2017
>>>>> +KernelVersion:  4.15
>>>>> +Contact:        xen-devel@lists.xenproject.org 
>>>>> +Description:
>>>>> +                An option to perform a slot or bus reset when a PCI device
>>>>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
>>>>> +		will cause the pciback driver to perform a slot or bus reset
>>>>> +		if the device supports it. It also checks to make sure that
>>>>> +		all of the devices under the bridge are owned by Xen PCI
>>>>> +		backend.
>>>> Why do you name this "do_flr" when you don't even try FLR, but
>>>> go to slot or then bus reset right away.
>>> Yes, I agree but xen toolstack has already been modified to
>>> consume"do_flr" attribute. Hence, we are using the
>>> function that matches with sysfs attribute.
>> That's not a valid reason imo: Right now the driver doesn't expose
>> such an attribute, so if the tool stack was trying to use it, it would
>> not do what is intended at present anyway (i.e. the code could at
>> best be called dead).
> Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus 
> reset".
> Please let me knowyour preference.

Well, that's more a question to Konrad as the maintainer.
Personally I'd prefer just "reset", as "pci" is redundant and "bus"
doesn't cover the slot variant.

>> Furthermore, contrary to what you claim in
>> your reply to Pasi, I can't see where you try an actual FLR first -
>> you go straight to pci_probe_reset_{slot,bus}(). If you actually
>> tried FLR first, only falling back to the other methods as "emulation",
>> I could certainly agree with the file name chosen.
> Currently, multiple resets are being invoked (independently) in the context
> of "xl attach/detach/shutdown/reboot".
> 
> - pci_reset_function_locked (invoked by pcistub_put_pci_dev())
>    This function tries various PCI reset methods including FLR.
> - pcistub_reset_dev (called by toolsstack based on "do_flr" attribute)

While related in a certain way, I can't really see how this addresses
the comment.

Jan

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Konrad Rzeszutek Wilk]

On Wed, Nov 29, 2017 at 08:35:33AM -0700, Jan Beulich wrote:
> >>> On 29.11.17 at 16:08, <govinda.tatti@oracle.com> wrote:
> > On 11/9/2017 2:28 AM, Jan Beulich wrote:
> >>>>> On 08.11.17 at 16:44, <govinda.tatti@oracle.com> wrote:
> >>> On 11/7/2017 8:40 AM, Jan Beulich wrote:
> >>>>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
> >>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> >>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> >>>>> @@ -11,3 +11,15 @@ Description:
> >>>>>                    #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
> >>>>>                    will allow the guest to read and write to the configuration
> >>>>>                    register 0x0E.
> >>>>> +
> >>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
> >>>>> +Date:           Nov 2017
> >>>>> +KernelVersion:  4.15
> >>>>> +Contact:        xen-devel@lists.xenproject.org 
> >>>>> +Description:
> >>>>> +                An option to perform a slot or bus reset when a PCI device
> >>>>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
> >>>>> +		will cause the pciback driver to perform a slot or bus reset
> >>>>> +		if the device supports it. It also checks to make sure that
> >>>>> +		all of the devices under the bridge are owned by Xen PCI
> >>>>> +		backend.
> >>>> Why do you name this "do_flr" when you don't even try FLR, but
> >>>> go to slot or then bus reset right away.
> >>> Yes, I agree but xen toolstack has already been modified to
> >>> consume"do_flr" attribute. Hence, we are using the
> >>> function that matches with sysfs attribute.
> >> That's not a valid reason imo: Right now the driver doesn't expose
> >> such an attribute, so if the tool stack was trying to use it, it would
> >> not do what is intended at present anyway (i.e. the code could at
> >> best be called dead).
> > Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus 
> > reset".
> > Please let me knowyour preference.
> 
> Well, that's more a question to Konrad as the maintainer.
> Personally I'd prefer just "reset", as "pci" is redundant and "bus"

Can't do 'reset'.
> doesn't cover the slot variant.

'bus_reset' sounds lovely?
> 
> >> Furthermore, contrary to what you claim in
> >> your reply to Pasi, I can't see where you try an actual FLR first -
> >> you go straight to pci_probe_reset_{slot,bus}(). If you actually
> >> tried FLR first, only falling back to the other methods as "emulation",
> >> I could certainly agree with the file name chosen.
> > Currently, multiple resets are being invoked (independently) in the context
> > of "xl attach/detach/shutdown/reboot".
> > 
> > - pci_reset_function_locked (invoked by pcistub_put_pci_dev())
> >    This function tries various PCI reset methods including FLR.
> > - pcistub_reset_dev (called by toolsstack based on "do_flr" attribute)
> 
> While related in a certain way, I can't really see how this addresses
> the comment.
> 
> Jan
> 
> 

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Jan Beulich]

>>> On 29.11.17 at 17:29, <konrad.wilk@oracle.com> wrote:
> On Wed, Nov 29, 2017 at 08:35:33AM -0700, Jan Beulich wrote:
>> >>> On 29.11.17 at 16:08, <govinda.tatti@oracle.com> wrote:
>> > On 11/9/2017 2:28 AM, Jan Beulich wrote:
>> >>>>> On 08.11.17 at 16:44, <govinda.tatti@oracle.com> wrote:
>> >>> On 11/7/2017 8:40 AM, Jan Beulich wrote:
>> >>>>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
>> >>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>> >>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>> >>>>> @@ -11,3 +11,15 @@ Description:
>> >>>>>                    #echo 00:19.0-E0:2:FF > 
> /sys/bus/pci/drivers/pciback/quirks
>> >>>>>                    will allow the guest to read and write to the 
> configuration
>> >>>>>                    register 0x0E.
>> >>>>> +
>> >>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>> >>>>> +Date:           Nov 2017
>> >>>>> +KernelVersion:  4.15
>> >>>>> +Contact:        xen-devel@lists.xenproject.org 
>> >>>>> +Description:
>> >>>>> +                An option to perform a slot or bus reset when a PCI device
>> >>>>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
>> >>>>> +		will cause the pciback driver to perform a slot or bus reset
>> >>>>> +		if the device supports it. It also checks to make sure that
>> >>>>> +		all of the devices under the bridge are owned by Xen PCI
>> >>>>> +		backend.
>> >>>> Why do you name this "do_flr" when you don't even try FLR, but
>> >>>> go to slot or then bus reset right away.
>> >>> Yes, I agree but xen toolstack has already been modified to
>> >>> consume"do_flr" attribute. Hence, we are using the
>> >>> function that matches with sysfs attribute.
>> >> That's not a valid reason imo: Right now the driver doesn't expose
>> >> such an attribute, so if the tool stack was trying to use it, it would
>> >> not do what is intended at present anyway (i.e. the code could at
>> >> best be called dead).
>> > Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus 
>> > reset".
>> > Please let me knowyour preference.
>> 
>> Well, that's more a question to Konrad as the maintainer.
>> Personally I'd prefer just "reset", as "pci" is redundant and "bus"
> 
> Can't do 'reset'.

Why?

>> doesn't cover the slot variant.
> 
> 'bus_reset' sounds lovely?

Lovely sounding or not, it may end up misleading, and even more so
if - like asked for - FLR would be tried first.

Jan

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

>>> Furthermore, contrary to what you claim in
>>> your reply to Pasi, I can't see where you try an actual FLR first -
>>> you go straight to pci_probe_reset_{slot,bus}(). If you actually
>>> tried FLR first, only falling back to the other methods as "emulation",
>>> I could certainly agree with the file name chosen.
>> Currently, multiple resets are being invoked (independently) in the context
>> of "xl attach/detach/shutdown/reboot".
>>
>> - pci_reset_function_locked (invoked by pcistub_put_pci_dev())
>>     This function tries various PCI reset methods including FLR.
>> - pcistub_reset_dev (called by toolsstack based on "do_flr" attribute)
> While related in a certain way, I can't really see how this addresses
> the comment.
pcistub_reset_dev() just tries slot or bus reset but not FLR since it is 
being
checked and executed by pci_reset_function_locked() if supported. May be 
we can
add FLR reset code to pcistub_reset_dev() and try FLR first before 
fall-back to
slot/bus reset.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Pasi Kärkkäinen]

On Wed, Nov 29, 2017 at 11:25:09AM -0600, Govinda Tatti wrote:
> 
> >>>Furthermore, contrary to what you claim in
> >>>your reply to Pasi, I can't see where you try an actual FLR first -
> >>>you go straight to pci_probe_reset_{slot,bus}(). If you actually
> >>>tried FLR first, only falling back to the other methods as "emulation",
> >>>I could certainly agree with the file name chosen.
> >>Currently, multiple resets are being invoked (independently) in the context
> >>of "xl attach/detach/shutdown/reboot".
> >>
> >>- pci_reset_function_locked (invoked by pcistub_put_pci_dev())
> >>    This function tries various PCI reset methods including FLR.
> >>- pcistub_reset_dev (called by toolsstack based on "do_flr" attribute)
> >While related in a certain way, I can't really see how this addresses
> >the comment.
>
> pcistub_reset_dev() just tries slot or bus reset but not FLR since it is
> being
> checked and executed by pci_reset_function_locked() if supported. May be we
> can
> add FLR reset code to pcistub_reset_dev() and try FLR first before fall-back
> to
> slot/bus reset.
>

Hmm.. is the suitable reset method something that can be figured out automatically?

I mean if FLR is tried first, but it isn't supported by the device, is it OK to go ahead and do a slot/bus reset automatically?
What if there are other devices in the same bus, wouldn't automatic bus reset be a bad thing? 

Or should the reset-method be configured by the user for the VM / PCI device ? 

Just thinking out loud here..


> Cheers
> GOVINDA
> 


Thanks,

-- Pasi

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Konrad Rzeszutek Wilk]

On Wed, Nov 29, 2017 at 09:40:20AM -0700, Jan Beulich wrote:
> >>> On 29.11.17 at 17:29, <konrad.wilk@oracle.com> wrote:
> > On Wed, Nov 29, 2017 at 08:35:33AM -0700, Jan Beulich wrote:
> >> >>> On 29.11.17 at 16:08, <govinda.tatti@oracle.com> wrote:
> >> > On 11/9/2017 2:28 AM, Jan Beulich wrote:
> >> >>>>> On 08.11.17 at 16:44, <govinda.tatti@oracle.com> wrote:
> >> >>> On 11/7/2017 8:40 AM, Jan Beulich wrote:
> >> >>>>>>> On 06.11.17 at 18:48, <Govinda.Tatti@Oracle.COM> wrote:
> >> >>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> >> >>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> >> >>>>> @@ -11,3 +11,15 @@ Description:
> >> >>>>>                    #echo 00:19.0-E0:2:FF > 
> > /sys/bus/pci/drivers/pciback/quirks
> >> >>>>>                    will allow the guest to read and write to the 
> > configuration
> >> >>>>>                    register 0x0E.
> >> >>>>> +
> >> >>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
> >> >>>>> +Date:           Nov 2017
> >> >>>>> +KernelVersion:  4.15
> >> >>>>> +Contact:        xen-devel@lists.xenproject.org 
> >> >>>>> +Description:
> >> >>>>> +                An option to perform a slot or bus reset when a PCI device
> >> >>>>> +		is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F
> >> >>>>> +		will cause the pciback driver to perform a slot or bus reset
> >> >>>>> +		if the device supports it. It also checks to make sure that
> >> >>>>> +		all of the devices under the bridge are owned by Xen PCI
> >> >>>>> +		backend.
> >> >>>> Why do you name this "do_flr" when you don't even try FLR, but
> >> >>>> go to slot or then bus reset right away.
> >> >>> Yes, I agree but xen toolstack has already been modified to
> >> >>> consume"do_flr" attribute. Hence, we are using the
> >> >>> function that matches with sysfs attribute.
> >> >> That's not a valid reason imo: Right now the driver doesn't expose
> >> >> such an attribute, so if the tool stack was trying to use it, it would
> >> >> not do what is intended at present anyway (i.e. the code could at
> >> >> best be called dead).
> >> > Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus 
> >> > reset".
> >> > Please let me knowyour preference.
> >> 
> >> Well, that's more a question to Konrad as the maintainer.
> >> Personally I'd prefer just "reset", as "pci" is redundant and "bus"
> > 
> > Can't do 'reset'.
> 
> Why?

B/c I forgot that this attribute is not per device, but on the module sub-directory:

/sys/bus/pci/drivers/pciback/do_flr

It can be indeed called 'reset'.

> 
> >> doesn't cover the slot variant.
> > 
> > 'bus_reset' sounds lovely?
> 
> Lovely sounding or not, it may end up misleading, and even more so
> if - like asked for - FLR would be tried first.

Fair enough. Reset should work then.

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

Thanks Konrad for your response. Please see below for my comments.

>>>> Well, that's more a question to Konrad as the maintainer.
>>>> Personally I'd prefer just "reset", as "pci" is redundant and "bus"
>>> Can't do 'reset'.
>> Why?
> B/c I forgot that this attribute is not per device, but on the module sub-directory:
>
> /sys/bus/pci/drivers/pciback/do_flr
>
> It can be indeed called 'reset'.

Good.  We will rename sysfs attribute from "do_flr" to "reset"

>>>> doesn't cover the slot variant.
>>> 'bus_reset' sounds lovely?
>> Lovely sounding or not, it may end up misleading, and even more so
>> if - like asked for - FLR would be tried first.
> Fair enough. Reset should work then.
So, we will use the following sequence to reset the requested 
device/function.

- FLR (as first option)
- BUS/SLOT reset (as fall-back option) if FLR is not supported or any 
issue with FLR

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

On 11/29/2017 12:05 PM, Pasi Kärkkäinen wrote:
> On Wed, Nov 29, 2017 at 11:25:09AM -0600, Govinda Tatti wrote:
>>>>> Furthermore, contrary to what you claim in
>>>>> your reply to Pasi, I can't see where you try an actual FLR first -
>>>>> you go straight to pci_probe_reset_{slot,bus}(). If you actually
>>>>> tried FLR first, only falling back to the other methods as "emulation",
>>>>> I could certainly agree with the file name chosen.
>>>> Currently, multiple resets are being invoked (independently) in the context
>>>> of "xl attach/detach/shutdown/reboot".
>>>>
>>>> - pci_reset_function_locked (invoked by pcistub_put_pci_dev())
>>>>     This function tries various PCI reset methods including FLR.
>>>> - pcistub_reset_dev (called by toolsstack based on "do_flr" attribute)
>>> While related in a certain way, I can't really see how this addresses
>>> the comment.
>> pcistub_reset_dev() just tries slot or bus reset but not FLR since it is
>> being
>> checked and executed by pci_reset_function_locked() if supported. May be we
>> can
>> add FLR reset code to pcistub_reset_dev() and try FLR first before fall-back
>> to
>> slot/bus reset.
>>
> Hmm.. is the suitable reset method something that can be figured out automatically?
>
> I mean if FLR is tried first, but it isn't supported by the device, is it OK to go ahead and do a slot/bus reset automatically?
Yes, we are moving in the same direction.

> What if there are other devices in the same bus, wouldn't automatic bus reset be a bad thing?
We will perform BUS or SLOT reset only if all device/functions behind 
the bus/slot are owned by pcistub.
Otherwise, we will skipthis reset.

> Or should the reset-method be configured by the user for the VM / PCI device ?
>
> Just thinking out loud here..
>

Cheers
GOVINDA

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Jan Beulich]

>>> On 29.11.17 at 20:44, <govinda.tatti@oracle.com> wrote:
> So, we will use the following sequence to reset the requested 
> device/function.
> 
> - FLR (as first option)
> - BUS/SLOT reset (as fall-back option) if FLR is not supported or any 
> issue with FLR

It looks to me as if the slot reset could also fail despite the probe
having succeeded. In such a case it might be better to try a bus
reset, i.e. the sequence would become
- FLR
- slot reset if FLR failed
- bus reset if slot reset failed

Jan

Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

On 11/30/2017 2:29 AM, Jan Beulich wrote:
>>>> On 29.11.17 at 20:44, <govinda.tatti@oracle.com> wrote:
>> So, we will use the following sequence to reset the requested
>> device/function.
>>
>> - FLR (as first option)
>> - BUS/SLOT reset (as fall-back option) if FLR is not supported or any
>> issue with FLR
> It looks to me as if the slot reset could also fail despite the probe
> having succeeded. In such a case it might be better to try a bus
> reset, i.e. the sequence would become
> - FLR
> - slot reset if FLR failed
> - bus reset if slot reset failed
Fine with me.

Cheers
GOVINDA