Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Boris Ostrovsky]

On 12/18/2017 02:36 AM, Jan Beulich wrote:
 On 15.12.17 at 20:52,  wrote:
> +static int pcistub_device_reset(struct pci_dev *dev)
> +{
> + struct xen_pcibk_dev_data *dev_data;
> + bool slot = false, bus = false;
> + struct pcistub_args arg = {};
> +
> + if (!dev)
> + return -EINVAL;
> +
> + dev_dbg(>dev, "[%s]\n", __func__);
> +
> + /* First check and try FLR */
> + if (pcie_has_flr(dev)) {
> + dev_dbg(>dev, "resetting %s device using FLR\n",
> + pci_name(dev));
> + pcie_flr(dev);
 The lack of error check here puzzled me, but I see the function
 indeed returns void right now. I think the prereq patch should
 change this along with exporting the function - you really don't
 want the device to be handed to a guest when the FLR timed
 out.
>>> We will change pcie_flr() to return error code. I will make this change
>>> in the next version of this patch.
>> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
>> as some planto restructure pcie flr specific functions but I don't know
>> the exact time-frame. For now,I am planning to use existing pcie_flr()
>> after checking FLR capability. We will switchto revised pcie_flr() once
>> it is available.
>>
>> I hope you are fine with this approach. Please let me know. Thanks.
> I've seen that other discussion. I don't think the change here
> should be done prior to the error reporting being put in place,
> for security reasons. But in the end it'll be Konrad as the
> maintainer to judge.
>
> Or wait, looks like there's some confusion in ./MAINTAINERS:
> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
> list of files doesn't include pciback. So it would instead be Boris
> or Jürgen to give you a final word.


This is now 4.16 material so we can at least wait until closer to
opening of the merge window when we may have the PCI updates. (And I
just noticed that you responded to Christoph.)

Besides, we don't want to make kernel changes until the interface is
settled (i.e the toolstack changes are accepted).

-boris

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Boris Ostrovsky]

On 12/18/2017 02:36 AM, Jan Beulich wrote:
 On 15.12.17 at 20:52,  wrote:
> +static int pcistub_device_reset(struct pci_dev *dev)
> +{
> + struct xen_pcibk_dev_data *dev_data;
> + bool slot = false, bus = false;
> + struct pcistub_args arg = {};
> +
> + if (!dev)
> + return -EINVAL;
> +
> + dev_dbg(>dev, "[%s]\n", __func__);
> +
> + /* First check and try FLR */
> + if (pcie_has_flr(dev)) {
> + dev_dbg(>dev, "resetting %s device using FLR\n",
> + pci_name(dev));
> + pcie_flr(dev);
 The lack of error check here puzzled me, but I see the function
 indeed returns void right now. I think the prereq patch should
 change this along with exporting the function - you really don't
 want the device to be handed to a guest when the FLR timed
 out.
>>> We will change pcie_flr() to return error code. I will make this change
>>> in the next version of this patch.
>> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
>> as some planto restructure pcie flr specific functions but I don't know
>> the exact time-frame. For now,I am planning to use existing pcie_flr()
>> after checking FLR capability. We will switchto revised pcie_flr() once
>> it is available.
>>
>> I hope you are fine with this approach. Please let me know. Thanks.
> I've seen that other discussion. I don't think the change here
> should be done prior to the error reporting being put in place,
> for security reasons. But in the end it'll be Konrad as the
> maintainer to judge.
>
> Or wait, looks like there's some confusion in ./MAINTAINERS:
> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
> list of files doesn't include pciback. So it would instead be Boris
> or Jürgen to give you a final word.


This is now 4.16 material so we can at least wait until closer to
opening of the merge window when we may have the PCI updates. (And I
just noticed that you responded to Christoph.)

Besides, we don't want to make kernel changes until the interface is
settled (i.e the toolstack changes are accepted).

-boris

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Jan Beulich]

>>> On 15.12.17 at 20:52,  wrote:
 +static int pcistub_device_reset(struct pci_dev *dev)
 +{
 +  struct xen_pcibk_dev_data *dev_data;
 +  bool slot = false, bus = false;
 +  struct pcistub_args arg = {};
 +
 +  if (!dev)
 +  return -EINVAL;
 +
 +  dev_dbg(>dev, "[%s]\n", __func__);
 +
 +  /* First check and try FLR */
 +  if (pcie_has_flr(dev)) {
 +  dev_dbg(>dev, "resetting %s device using FLR\n",
 +  pci_name(dev));
 +  pcie_flr(dev);
>>> The lack of error check here puzzled me, but I see the function
>>> indeed returns void right now. I think the prereq patch should
>>> change this along with exporting the function - you really don't
>>> want the device to be handed to a guest when the FLR timed
>>> out.
>> We will change pcie_flr() to return error code. I will make this change
>> in the next version of this patch.
> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
> as some planto restructure pcie flr specific functions but I don't know
> the exact time-frame. For now,I am planning to use existing pcie_flr()
> after checking FLR capability. We will switchto revised pcie_flr() once
> it is available.
> 
> I hope you are fine with this approach. Please let me know. Thanks.

I've seen that other discussion. I don't think the change here
should be done prior to the error reporting being put in place,
for security reasons. But in the end it'll be Konrad as the
maintainer to judge.

Or wait, looks like there's some confusion in ./MAINTAINERS:
Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
list of files doesn't include pciback. So it would instead be Boris
or Jürgen to give you a final word.

Jan

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Jan Beulich]

>>> On 15.12.17 at 20:52,  wrote:
 +static int pcistub_device_reset(struct pci_dev *dev)
 +{
 +  struct xen_pcibk_dev_data *dev_data;
 +  bool slot = false, bus = false;
 +  struct pcistub_args arg = {};
 +
 +  if (!dev)
 +  return -EINVAL;
 +
 +  dev_dbg(>dev, "[%s]\n", __func__);
 +
 +  /* First check and try FLR */
 +  if (pcie_has_flr(dev)) {
 +  dev_dbg(>dev, "resetting %s device using FLR\n",
 +  pci_name(dev));
 +  pcie_flr(dev);
>>> The lack of error check here puzzled me, but I see the function
>>> indeed returns void right now. I think the prereq patch should
>>> change this along with exporting the function - you really don't
>>> want the device to be handed to a guest when the FLR timed
>>> out.
>> We will change pcie_flr() to return error code. I will make this change
>> in the next version of this patch.
> I exchanged some emails with Bjorn/Christoph and it looks like Christoph
> as some planto restructure pcie flr specific functions but I don't know
> the exact time-frame. For now,I am planning to use existing pcie_flr()
> after checking FLR capability. We will switchto revised pcie_flr() once
> it is available.
> 
> I hope you are fine with this approach. Please let me know. Thanks.

I've seen that other discussion. I don't think the change here
should be done prior to the error reporting being put in place,
for security reasons. But in the end it'll be Konrad as the
maintainer to judge.

Or wait, looks like there's some confusion in ./MAINTAINERS:
Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the
list of files doesn't include pciback. So it would instead be Boris
or Jürgen to give you a final word.

Jan

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Govinda Tatti]

Jan,

One quick update on pcie_flr() specific implementation. Please see below.

+static int pcistub_device_reset(struct pci_dev *dev)
+{
+   struct xen_pcibk_dev_data *dev_data;
+   bool slot = false, bus = false;
+   struct pcistub_args arg = {};
+
+   if (!dev)
+   return -EINVAL;
+
+   dev_dbg(>dev, "[%s]\n", __func__);
+
+   /* First check and try FLR */
+   if (pcie_has_flr(dev)) {
+   dev_dbg(>dev, "resetting %s device using FLR\n",
+   pci_name(dev));
+   pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

We will change pcie_flr() to return error code. I will make this change
in the next version of this patch.

I exchanged some emails with Bjorn/Christoph and it looks like Christoph
as some planto restructure pcie flr specific functions but I don't know
the exact time-frame. For now,I am planning to use existing pcie_flr()
after checking FLR capability. We will switchto revised pcie_flr() once
it is available.

I hope you are fine with this approach. Please let me know. Thanks.

Cheers
GOVINDA

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Govinda Tatti]

Jan,

One quick update on pcie_flr() specific implementation. Please see below.

+static int pcistub_device_reset(struct pci_dev *dev)
+{
+   struct xen_pcibk_dev_data *dev_data;
+   bool slot = false, bus = false;
+   struct pcistub_args arg = {};
+
+   if (!dev)
+   return -EINVAL;
+
+   dev_dbg(>dev, "[%s]\n", __func__);
+
+   /* First check and try FLR */
+   if (pcie_has_flr(dev)) {
+   dev_dbg(>dev, "resetting %s device using FLR\n",
+   pci_name(dev));
+   pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

We will change pcie_flr() to return error code. I will make this change
in the next version of this patch.

I exchanged some emails with Bjorn/Christoph and it looks like Christoph
as some planto restructure pcie flr specific functions but I don't know
the exact time-frame. For now,I am planning to use existing pcie_flr()
after checking FLR capability. We will switchto revised pcie_flr() once
it is available.

I hope you are fine with this approach. Please let me know. Thanks.

Cheers
GOVINDA

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Govinda Tatti]

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

On 12/8/2017 3:34 AM, Jan Beulich wrote:

On 07.12.17 at 23:21,  wrote:

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.

It took me a moment to figure that here you're referring to the
process of (un)binding, not the state. To avoid that ambiguity in
wording, how about "... we cannot do the reset while a device is
being bound (...) or while it is being unbound ..."?

Sure, I will fix it.

--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -11,3 +11,18 @@ 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/reset
+Date:   Dec 2017
+KernelVersion:  4.15
+Contact:xen-de...@lists.xenproject.org  
+Description:

+An option to perform a flr/slot/bus reset when a PCI device
+   is owned by Xen PCI backend. Writing a string of :BB:DD.F

:BB:DD.F (or else the D-s are ambiguous, the more that "domain"
in Xen code is ambiguous anyway - I continue to be mislead by struct
pcistub_device_id's domain field)

Thanks for catching this issue. I will fix it.

Also I assume the  part is optional (default zero), which
probably can and should be expressed in some way.

 can be 0 or non-zero, subject to system configuration.

--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
up_write(_sem);
  }
  
+struct pcistub_args {

+   const struct pci_dev *dev;
+   unsigned int dcount;

The sole use of this field is for a debug message. Why not drop it
and make "dev" the "data" argument without further indirection?
I prefer to keep this data structure since it will be helpful to debug 
any issues

orfor future enhancements.

+static int pcistub_device_search(struct pci_dev *dev, void *data)
+{
+   struct pcistub_device *psdev;
+   struct pcistub_args *arg = data;
+   bool found = false;
+   unsigned long flags;
+
+   spin_lock_irqsave(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found = true;
+   arg->dcount++;
+   break;

Neither here nor in the caller I can see a check whether the device
is currently assigned to a guest. Ownership by pciback alone imo is
not sufficient to allow a reset to be performed.

I can add the following check

if ((psdev->dev == dev) && (pci_is_dev_assigned(dev)))

+static int pcistub_device_reset(struct pci_dev *dev)
+{
+   struct xen_pcibk_dev_data *dev_data;
+   bool slot = false, bus = false;
+   struct pcistub_args arg = {};
+
+   if (!dev)
+   return -EINVAL;
+
+   dev_dbg(>dev, "[%s]\n", __func__);
+
+   /* First check and try FLR */
+   if (pcie_has_flr(dev)) {
+   dev_dbg(>dev, "resetting %s device using FLR\n",
+   pci_name(dev));
+   pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

We will change pcie_flr() to return error code. I will make this change
in the next version of this patch.

+   return 0;
+   }
+
+   if (!pci_probe_reset_slot(dev->slot))
+   slot = true;
+   else if ((!pci_probe_reset_bus(dev->bus)) &&
+(!pci_is_root_bus(dev->bus)))

Too many parentheses for my taste.

I will fix it.

+static ssize_t reset_store(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, , , , );
+   if (err)
+   return err;
+
+   psdev = pcistub_device_find(domain, bus, slot, func);
+   if (psdev) {
+   err = pcistub_device_reset(psdev->dev);
+   pcistub_device_put(psdev);
+   } else {
+   err = -ENODEV;
+   }
+
+   if (!err)
+   err = count;
+
+   return err;
+}
+static DRIVER_ATTR_WO(reset);

Would it be worth for reads of the file to return whether the device
can be reset this way (i.e. the result of the checks you do before
actually doing the reset)?
I don't think so. Plus, it makes this interface and its usage more 
complicated.


Cheers
GOVINDA

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Govinda Tatti]

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

On 12/8/2017 3:34 AM, Jan Beulich wrote:

On 07.12.17 at 23:21,  wrote:

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.

It took me a moment to figure that here you're referring to the
process of (un)binding, not the state. To avoid that ambiguity in
wording, how about "... we cannot do the reset while a device is
being bound (...) or while it is being unbound ..."?

Sure, I will fix it.

--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -11,3 +11,18 @@ 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/reset
+Date:   Dec 2017
+KernelVersion:  4.15
+Contact:xen-de...@lists.xenproject.org  
+Description:

+An option to perform a flr/slot/bus reset when a PCI device
+   is owned by Xen PCI backend. Writing a string of :BB:DD.F

:BB:DD.F (or else the D-s are ambiguous, the more that "domain"
in Xen code is ambiguous anyway - I continue to be mislead by struct
pcistub_device_id's domain field)

Thanks for catching this issue. I will fix it.

Also I assume the  part is optional (default zero), which
probably can and should be expressed in some way.

 can be 0 or non-zero, subject to system configuration.

--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
up_write(_sem);
  }
  
+struct pcistub_args {

+   const struct pci_dev *dev;
+   unsigned int dcount;

The sole use of this field is for a debug message. Why not drop it
and make "dev" the "data" argument without further indirection?
I prefer to keep this data structure since it will be helpful to debug 
any issues

orfor future enhancements.

+static int pcistub_device_search(struct pci_dev *dev, void *data)
+{
+   struct pcistub_device *psdev;
+   struct pcistub_args *arg = data;
+   bool found = false;
+   unsigned long flags;
+
+   spin_lock_irqsave(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found = true;
+   arg->dcount++;
+   break;

Neither here nor in the caller I can see a check whether the device
is currently assigned to a guest. Ownership by pciback alone imo is
not sufficient to allow a reset to be performed.

I can add the following check

if ((psdev->dev == dev) && (pci_is_dev_assigned(dev)))

+static int pcistub_device_reset(struct pci_dev *dev)
+{
+   struct xen_pcibk_dev_data *dev_data;
+   bool slot = false, bus = false;
+   struct pcistub_args arg = {};
+
+   if (!dev)
+   return -EINVAL;
+
+   dev_dbg(>dev, "[%s]\n", __func__);
+
+   /* First check and try FLR */
+   if (pcie_has_flr(dev)) {
+   dev_dbg(>dev, "resetting %s device using FLR\n",
+   pci_name(dev));
+   pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

We will change pcie_flr() to return error code. I will make this change
in the next version of this patch.

+   return 0;
+   }
+
+   if (!pci_probe_reset_slot(dev->slot))
+   slot = true;
+   else if ((!pci_probe_reset_bus(dev->bus)) &&
+(!pci_is_root_bus(dev->bus)))

Too many parentheses for my taste.

I will fix it.

+static ssize_t reset_store(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, , , , );
+   if (err)
+   return err;
+
+   psdev = pcistub_device_find(domain, bus, slot, func);
+   if (psdev) {
+   err = pcistub_device_reset(psdev->dev);
+   pcistub_device_put(psdev);
+   } else {
+   err = -ENODEV;
+   }
+
+   if (!err)
+   err = count;
+
+   return err;
+}
+static DRIVER_ATTR_WO(reset);

Would it be worth for reads of the file to return whether the device
can be reset this way (i.e. the result of the checks you do before
actually doing the reset)?
I don't think so. Plus, it makes this interface and its usage more 
complicated.


Cheers
GOVINDA

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Jan Beulich]

>>> On 12.12.17 at 15:48,  wrote:
> Thanks Jan for your review comments. Please see below for my comments.

First of all - can you please do something about your reply style?
HTML mail should be avoided. You'll see that the (plain text) reply
as a result is rather hard to follow, too.

> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,18 @@ 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/reset
> +Date:   Dec 2017
> +KernelVersion:  4.15
> +Contact:xen-de...@lists.xenproject.org 
> +Description:
> +An option to perform a flr/slot/bus reset when a PCI device
> + is owned by Xen PCI backend. Writing a string of :BB:DD.F  
> :BB:DD.F (or else the D-s are ambiguous, the more that "domain"
> in Xen code is ambiguous anyway - I continue to be mislead by struct
> pcistub_device_id's domain field)  Thanks for catching this issue. I will 
> fix it.
> 
> 
> Also I assume the  part is optional (default zero), which
> probably can and should be expressed in some way.   can be 0 or 
> non-zero, subject to system configuration.

The question isn't system configuration, but whether the field can
be omitted on input, with zero being assumed in such a case. That's
a common shorthand, considering that the vast majority of x86
(and maybe other) systems aren't using segments other than zero.

Jan

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Jan Beulich]

>>> On 12.12.17 at 15:48,  wrote:
> Thanks Jan for your review comments. Please see below for my comments.

First of all - can you please do something about your reply style?
HTML mail should be avoided. You'll see that the (plain text) reply
as a result is rather hard to follow, too.

> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,18 @@ 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/reset
> +Date:   Dec 2017
> +KernelVersion:  4.15
> +Contact:xen-de...@lists.xenproject.org 
> +Description:
> +An option to perform a flr/slot/bus reset when a PCI device
> + is owned by Xen PCI backend. Writing a string of :BB:DD.F  
> :BB:DD.F (or else the D-s are ambiguous, the more that "domain"
> in Xen code is ambiguous anyway - I continue to be mislead by struct
> pcistub_device_id's domain field)  Thanks for catching this issue. I will 
> fix it.
> 
> 
> Also I assume the  part is optional (default zero), which
> probably can and should be expressed in some way.   can be 0 or 
> non-zero, subject to system configuration.

The question isn't system configuration, but whether the field can
be omitted on input, with zero being assumed in such a case. That's
a common shorthand, considering that the vast majority of x86
(and maybe other) systems aren't using segments other than zero.

Jan

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Jan Beulich]

>>> On 07.12.17 at 23:21,  wrote:
> 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.

It took me a moment to figure that here you're referring to the
process of (un)binding, not the state. To avoid that ambiguity in
wording, how about "... we cannot do the reset while a device is
being bound (...) or while it is being unbound ..."?

> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,18 @@ 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/reset
> +Date:   Dec 2017
> +KernelVersion:  4.15
> +Contact:xen-de...@lists.xenproject.org 
> +Description:
> +An option to perform a flr/slot/bus reset when a PCI device
> + is owned by Xen PCI backend. Writing a string of :BB:DD.F

:BB:DD.F (or else the D-s are ambiguous, the more that "domain"
in Xen code is ambiguous anyway - I continue to be mislead by struct
pcistub_device_id's domain field)

Also I assume the  part is optional (default zero), which
probably can and should be expressed in some way.

> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>   up_write(_sem);
>  }
>  
> +struct pcistub_args {
> + const struct pci_dev *dev;
> + unsigned int dcount;

The sole use of this field is for a debug message. Why not drop it
and make "dev" the "data" argument without further indirection?

> +static int pcistub_device_search(struct pci_dev *dev, void *data)
> +{
> + struct pcistub_device *psdev;
> + struct pcistub_args *arg = data;
> + bool found = false;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_devices_lock, flags);
> +
> + list_for_each_entry(psdev, _devices, dev_list) {
> + if (psdev->dev == dev) {
> + found = true;
> + arg->dcount++;
> + break;

Neither here nor in the caller I can see a check whether the device
is currently assigned to a guest. Ownership by pciback alone imo is
not sufficient to allow a reset to be performed.

> +static int pcistub_device_reset(struct pci_dev *dev)
> +{
> + struct xen_pcibk_dev_data *dev_data;
> + bool slot = false, bus = false;
> + struct pcistub_args arg = {};
> +
> + if (!dev)
> + return -EINVAL;
> +
> + dev_dbg(>dev, "[%s]\n", __func__);
> +
> + /* First check and try FLR */
> + if (pcie_has_flr(dev)) {
> + dev_dbg(>dev, "resetting %s device using FLR\n",
> + pci_name(dev));
> + pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

> + return 0;
> + }
> +
> + if (!pci_probe_reset_slot(dev->slot))
> + slot = true;
> + else if ((!pci_probe_reset_bus(dev->bus)) &&
> +  (!pci_is_root_bus(dev->bus)))

Too many parentheses for my taste.

> +static ssize_t reset_store(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, , , , );
> + if (err)
> + return err;
> +
> + psdev = pcistub_device_find(domain, bus, slot, func);
> + if (psdev) {
> + err = pcistub_device_reset(psdev->dev);
> + pcistub_device_put(psdev);
> + } else {
> + err = -ENODEV;
> + }
> +
> + if (!err)
> + err = count;
> +
> + return err;
> +}
> +static DRIVER_ATTR_WO(reset);

Would it be worth for reads of the file to return whether the device
can be reset this way (i.e. the result of the checks you do before
actually doing the reset)?

Jan

Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute

[Jan Beulich]

>>> On 07.12.17 at 23:21,  wrote:
> 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.

It took me a moment to figure that here you're referring to the
process of (un)binding, not the state. To avoid that ambiguity in
wording, how about "... we cannot do the reset while a device is
being bound (...) or while it is being unbound ..."?

> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,18 @@ 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/reset
> +Date:   Dec 2017
> +KernelVersion:  4.15
> +Contact:xen-de...@lists.xenproject.org 
> +Description:
> +An option to perform a flr/slot/bus reset when a PCI device
> + is owned by Xen PCI backend. Writing a string of :BB:DD.F

:BB:DD.F (or else the D-s are ambiguous, the more that "domain"
in Xen code is ambiguous anyway - I continue to be mislead by struct
pcistub_device_id's domain field)

Also I assume the  part is optional (default zero), which
probably can and should be expressed in some way.

> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>   up_write(_sem);
>  }
>  
> +struct pcistub_args {
> + const struct pci_dev *dev;
> + unsigned int dcount;

The sole use of this field is for a debug message. Why not drop it
and make "dev" the "data" argument without further indirection?

> +static int pcistub_device_search(struct pci_dev *dev, void *data)
> +{
> + struct pcistub_device *psdev;
> + struct pcistub_args *arg = data;
> + bool found = false;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_devices_lock, flags);
> +
> + list_for_each_entry(psdev, _devices, dev_list) {
> + if (psdev->dev == dev) {
> + found = true;
> + arg->dcount++;
> + break;

Neither here nor in the caller I can see a check whether the device
is currently assigned to a guest. Ownership by pciback alone imo is
not sufficient to allow a reset to be performed.

> +static int pcistub_device_reset(struct pci_dev *dev)
> +{
> + struct xen_pcibk_dev_data *dev_data;
> + bool slot = false, bus = false;
> + struct pcistub_args arg = {};
> +
> + if (!dev)
> + return -EINVAL;
> +
> + dev_dbg(>dev, "[%s]\n", __func__);
> +
> + /* First check and try FLR */
> + if (pcie_has_flr(dev)) {
> + dev_dbg(>dev, "resetting %s device using FLR\n",
> + pci_name(dev));
> + pcie_flr(dev);

The lack of error check here puzzled me, but I see the function
indeed returns void right now. I think the prereq patch should
change this along with exporting the function - you really don't
want the device to be handed to a guest when the FLR timed
out.

> + return 0;
> + }
> +
> + if (!pci_probe_reset_slot(dev->slot))
> + slot = true;
> + else if ((!pci_probe_reset_bus(dev->bus)) &&
> +  (!pci_is_root_bus(dev->bus)))

Too many parentheses for my taste.

> +static ssize_t reset_store(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, , , , );
> + if (err)
> + return err;
> +
> + psdev = pcistub_device_find(domain, bus, slot, func);
> + if (psdev) {
> + err = pcistub_device_reset(psdev->dev);
> + pcistub_device_put(psdev);
> + } else {
> + err = -ENODEV;
> + }
> +
> + if (!err)
> + err = count;
> +
> + return err;
> +}
> +static DRIVER_ATTR_WO(reset);

Would it be worth for reads of the file to return whether the device
can be reset this way (i.e. the result of the checks you do before
actually doing the reset)?

Jan