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 <[email protected]>
>> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
>> Reviewed-by: Boris Ostrovsky <[email protected]>
>> ---
>> 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: [email protected]
>> +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
From 1583450716039652801@xxx Tue Nov 07 23:11:19 +0000 2017
X-GM-THRID: 1583346318922306893
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread