Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

On 12/18/2017 6:26 AM, Christoph Hellwig wrote:

On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote:

I think Christoph volunteered to do some restructuring, but I don't
know his timeframe.  If you can, I would probably wait for that
because there's so much overlap here.

I'll have some time over the holidays.  If you need it more urgent
than that feel free to take over.

We will wait for your changes. Thanks Christoph.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

On 12/18/2017 6:26 AM, Christoph Hellwig wrote:

On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote:

I think Christoph volunteered to do some restructuring, but I don't
know his timeframe.  If you can, I would probably wait for that
because there's so much overlap here.

I'll have some time over the holidays.  If you need it more urgent
than that feel free to take over.

We will wait for your changes. Thanks Christoph.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

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

So, we should consider one of these options.

- set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
- pcie_flr() should return if it is not supported

If we modify pcie_flr() to return error codes, then we need to modify
all existing modules that are calling this function.

Yes, of course.


Please let me know your preference, so that I can move accordingly. Thanks.

I think Christoph volunteered to do some restructuring, but I don't
know his timeframe.  If you can, I would probably wait for that
because there's so much overlap here.

OK.


The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues
and should be fixed, but again should wait for the revised pcie_flr()
interface.  And if they're not actually required for your Xen issue,
they sound like "nice to have" cleanups that will not gate your Xen
fixes.  I added this to my ever-growing list of cleanups to do.

For now, I am planning to use existing pcie_flr() after checking FLR
capability inside Xenpciback driver (like other existing pcie_flr()
usage). We will switch to revised pcie_flr() once it is available.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

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

So, we should consider one of these options.

- set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
- pcie_flr() should return if it is not supported

If we modify pcie_flr() to return error codes, then we need to modify
all existing modules that are calling this function.

Yes, of course.


Please let me know your preference, so that I can move accordingly. Thanks.

I think Christoph volunteered to do some restructuring, but I don't
know his timeframe.  If you can, I would probably wait for that
because there's so much overlap here.

OK.


The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues
and should be fixed, but again should wait for the revised pcie_flr()
interface.  And if they're not actually required for your Xen issue,
they sound like "nice to have" cleanups that will not gate your Xen
fixes.  I added this to my ever-growing list of cleanups to do.

For now, I am planning to use existing pcie_flr() after checking FLR
capability inside Xenpciback driver (like other existing pcie_flr()
usage). We will switch to revised pcie_flr() once it is available.

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]

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: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

Thanks Bjorn and Christophfor your response. Please see below for my 
comments.


On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:

[+cc Christoph]

On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:

-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
  {
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
  }
+EXPORT_SYMBOL_GPL(pcie_has_flr);

I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported.  Is that feasible?

Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.

If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.

I checked the current usage of pcie_has_flr() and pcie_flr(). I have
a couple
of questions or need some clarification.

1. pcie_has_flr() usage inside pci_probe_reset_function().

    This function is only calling pcie_has_flr() but not pcie_flr().
    Rest of the code is trying to do specific type of reset except
pcie_flr().

     rc = pci_dev_specific_reset(dev, 1);
     if (rc != -ENOTTY)
     return rc;
     if (pcie_has_flr(dev))
     return 0;
     rc = pci_af_flr(dev, 1);
     if (rc != -ENOTTY)
     return rc;

    In other-words, I can remove usage of pcie_has_flr() in all other places
    in pci.c except in above function.

I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
("PCI: Export pcie_flr()"), but revert the restructuring part.

Prior to a60a2b73ba69, we had

   int pcie_flr(struct pci_dev *dev, int probe);

like all the other reset methods.  AFAICT, the addition of
pcie_has_flr() was to optimize the path slightly because when drivers
call pcie_flr(), they should already know that their hardware supports
FLR.  But I don't think that optimization is worth the extra code
complexity.  If we do need to optimize it, we can check this in the
core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET
accordingly.

Christoph, chime in if I'm missing something here.

Not all code paths are aware of FLR capability and also, not
using pcie_flr().  For example,

arch/powerpc/platforms/powernv/eeh-powernv.c
drivers/crypto/cavium/nitrox/nitrox_main.c
drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c

So, we should consider one of these options.

- set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
- pcie_flr() should return if it is not supported

If we modify pcie_flr() to return error codes, then we need to modify
all existing modules that are calling this function.

Please let me know your preference, so that I can move accordingly. Thanks.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

Thanks Bjorn and Christophfor your response. Please see below for my 
comments.


On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:

[+cc Christoph]

On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:

-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
  {
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
  }
+EXPORT_SYMBOL_GPL(pcie_has_flr);

I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported.  Is that feasible?

Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.

If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.

I checked the current usage of pcie_has_flr() and pcie_flr(). I have
a couple
of questions or need some clarification.

1. pcie_has_flr() usage inside pci_probe_reset_function().

    This function is only calling pcie_has_flr() but not pcie_flr().
    Rest of the code is trying to do specific type of reset except
pcie_flr().

     rc = pci_dev_specific_reset(dev, 1);
     if (rc != -ENOTTY)
     return rc;
     if (pcie_has_flr(dev))
     return 0;
     rc = pci_af_flr(dev, 1);
     if (rc != -ENOTTY)
     return rc;

    In other-words, I can remove usage of pcie_has_flr() in all other places
    in pci.c except in above function.

I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
("PCI: Export pcie_flr()"), but revert the restructuring part.

Prior to a60a2b73ba69, we had

   int pcie_flr(struct pci_dev *dev, int probe);

like all the other reset methods.  AFAICT, the addition of
pcie_has_flr() was to optimize the path slightly because when drivers
call pcie_flr(), they should already know that their hardware supports
FLR.  But I don't think that optimization is worth the extra code
complexity.  If we do need to optimize it, we can check this in the
core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET
accordingly.

Christoph, chime in if I'm missing something here.

Not all code paths are aware of FLR capability and also, not
using pcie_flr().  For example,

arch/powerpc/platforms/powernv/eeh-powernv.c
drivers/crypto/cavium/nitrox/nitrox_main.c
drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c

So, we should consider one of these options.

- set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
- pcie_flr() should return if it is not supported

If we modify pcie_flr() to return error codes, then we need to modify
all existing modules that are calling this function.

Please let me know your preference, so that I can move accordingly. Thanks.

Cheers
GOVINDA

Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
  {
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
  }
+EXPORT_SYMBOL_GPL(pcie_has_flr);

I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported.  Is that feasible?

Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.

If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.
I checked the current usage of pcie_has_flr() and pcie_flr(). I have a 
couple

of questions or need some clarification.

1. pcie_has_flr() usage inside pci_probe_reset_function().

   This function is only calling pcie_has_flr() but not pcie_flr().
   Rest of the code is trying to do specific type of reset except 
pcie_flr().


    rc = pci_dev_specific_reset(dev, 1);
    if (rc != -ENOTTY)
    return rc;
    if (pcie_has_flr(dev))
    return 0;
    rc = pci_af_flr(dev, 1);
    if (rc != -ENOTTY)
    return rc;

   In other-words, I can remove usage of pcie_has_flr() in all other places
   in pci.c except in above function.

2. W.r.t pcie_flr(), I am planning to return error code. Currently, the 
following
   file/modules are calling this function. My plan is to add a check 
for return
   code and print a WANRING message if return code is NON-ZERO. I hope 
this is

   sufficient for this patch.

   drivers/crypto/qat/qat_common/adf_aer.c
   drivers/infiniband/hw/hfi1/chip.c (2 places)
   drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places)
   drivers/pci/quirks.c (2 places)

Cheers
GOVINDA

Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
  {
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
  }
+EXPORT_SYMBOL_GPL(pcie_has_flr);

I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported.  Is that feasible?

Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.

If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.
I checked the current usage of pcie_has_flr() and pcie_flr(). I have a 
couple

of questions or need some clarification.

1. pcie_has_flr() usage inside pci_probe_reset_function().

   This function is only calling pcie_has_flr() but not pcie_flr().
   Rest of the code is trying to do specific type of reset except 
pcie_flr().


    rc = pci_dev_specific_reset(dev, 1);
    if (rc != -ENOTTY)
    return rc;
    if (pcie_has_flr(dev))
    return 0;
    rc = pci_af_flr(dev, 1);
    if (rc != -ENOTTY)
    return rc;

   In other-words, I can remove usage of pcie_has_flr() in all other places
   in pci.c except in above function.

2. W.r.t pcie_flr(), I am planning to return error code. Currently, the 
following
   file/modules are calling this function. My plan is to add a check 
for return
   code and print a WANRING message if return code is NON-ZERO. I hope 
this is

   sufficient for this patch.

   drivers/crypto/qat/qat_common/adf_aer.c
   drivers/infiniband/hw/hfi1/chip.c (2 places)
   drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places)
   drivers/pci/quirks.c (2 places)

Cheers
GOVINDA

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

[Govinda Tatti]

On 12/12/2017 9:01 AM, Jan Beulich wrote:

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.

Sorry about it. I had an issue with my Thunderbird setting.



--- 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

Yes, it can be omitted if  is zero.I will add this information
to above documentation file.

Cheers
GOVINDA

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

[Govinda Tatti]

On 12/12/2017 9:01 AM, Jan Beulich wrote:

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.

Sorry about it. I had an issue with my Thunderbird setting.



--- 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

Yes, it can be omitted if  is zero.I will add this information
to above documentation file.

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 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

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

On 12/8/2017 2:24 PM, Bjorn Helgaas wrote:

On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote:

This patch exports pcie_has_flr() and it is being used by Xen pciback
driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS
attribute.

Signed-off-by: Govinda Tatti <govinda.ta...@oracle.com>
---
v3: -New

  drivers/pci/pci.c   | 3 ++-
  include/linux/pci.h | 1 +
  2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6078dfc..499e922 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev)
   * Returns true if the device advertises support for PCIe function level
   * resets.
   */
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
  {
u32 cap;
  
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)

pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
  }
+EXPORT_SYMBOL_GPL(pcie_has_flr);

I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported.  Is that feasible?
Sure, I will add pcie_has_flr() logic inside pcie_flr() and return 
appropriate

values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.

Please let me know.

Cheers
GOVINDA

Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

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

On 12/8/2017 2:24 PM, Bjorn Helgaas wrote:

On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote:

This patch exports pcie_has_flr() and it is being used by Xen pciback
driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS
attribute.

Signed-off-by: Govinda Tatti 
---
v3: -New

  drivers/pci/pci.c   | 3 ++-
  include/linux/pci.h | 1 +
  2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6078dfc..499e922 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev)
   * Returns true if the device advertises support for PCIe function level
   * resets.
   */
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
  {
u32 cap;
  
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)

pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
  }
+EXPORT_SYMBOL_GPL(pcie_has_flr);

I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported.  Is that feasible?
Sure, I will add pcie_has_flr() logic inside pcie_flr() and return 
appropriate

values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.

Please let me know.

Cheers
GOVINDA

[PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

This patch exports pcie_has_flr() and it is being used by Xen pciback
driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS
attribute.

Signed-off-by: Govinda Tatti <govinda.ta...@oracle.com>
---
v3: -New

 drivers/pci/pci.c   | 3 ++-
 include/linux/pci.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6078dfc..499e922 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev)
  * Returns true if the device advertises support for PCIe function level
  * resets.
  */
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
 {
u32 cap;
 
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
 }
+EXPORT_SYMBOL_GPL(pcie_has_flr);
 
 /**
  * pcie_flr - initiate a PCIe function level reset
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d16a7c0..44bf2b5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1089,6 +1089,7 @@ int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
  enum pcie_link_width *width);
+bool pcie_has_flr(struct pci_dev *dev);
 void pcie_flr(struct pci_dev *dev);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
-- 
2.9.5

[PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' 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 providing an option to perform a flr/slot/bus reset
when a PCI device is owned by Xen PCI backend. It will try to execute one
of these reset method, starting with FLR if it is supported. Otherwise,
it tries slot or bus reset method. For slot or bus reset method, it also
checks to make sure that all of the devices under the bridge are owned by
Xen PCI backend before applying those resets.

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 'reset' 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. BTW, previously defined "do_flr"
attribute has been renamed to "reset" since "do_flr" name doesn't represent
all PCI reset methods and plus, currently it is not being used. So, there
is no impact in renaming this sysfs attribute.

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.ta...@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
---
v1: - First posting
v2: - struct pcistub_args: Changed docunt field as unsigned int
- pcistub_reset_dev: initialization of "struct pcistub_args"
- pcistub_reset_dev: combined multiple if-statements
- pcistub_do_flr: removed goto statement
v3: - Resynced with linux kernel 4.14.4 and latest pci_stub.c changes.
- Renamed "do_flr" SysFS attribute to "reset". Plus, modified
  "reset" SysFS attribute code as per the latest changes in 4.14.4.
- struct pcistub_args: added "const" to "struct pci_dev *dev" field
- pcistub_device_search: Renamed found_dev to found
- pcistub_device_search: Modified comments and return statements
- pcistub_device_reset: introduced FLR reset code
- pcistub_device_reset: Modified all dev_dbg messages

 Documentation/ABI/testing/sysfs-driver-pciback |  15 +++
 drivers/xen/xen-pciback/pci_stub.c | 128 +
 2 files changed, 143 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-pciback 
b/Documentation/ABI/testing/sysfs-driver-pciback
index 6a733bf..d295b42 100644
--- 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 dev

[PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface

[Govinda Tatti]

This patch exports pcie_has_flr() and it is being used by Xen pciback
driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS
attribute.

Signed-off-by: Govinda Tatti 
---
v3: -New

 drivers/pci/pci.c   | 3 ++-
 include/linux/pci.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6078dfc..499e922 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev)
  * Returns true if the device advertises support for PCIe function level
  * resets.
  */
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
 {
u32 cap;
 
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
 }
+EXPORT_SYMBOL_GPL(pcie_has_flr);
 
 /**
  * pcie_flr - initiate a PCIe function level reset
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d16a7c0..44bf2b5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1089,6 +1089,7 @@ int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
  enum pcie_link_width *width);
+bool pcie_has_flr(struct pci_dev *dev);
 void pcie_flr(struct pci_dev *dev);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
-- 
2.9.5

[PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' 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 providing an option to perform a flr/slot/bus reset
when a PCI device is owned by Xen PCI backend. It will try to execute one
of these reset method, starting with FLR if it is supported. Otherwise,
it tries slot or bus reset method. For slot or bus reset method, it also
checks to make sure that all of the devices under the bridge are owned by
Xen PCI backend before applying those resets.

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 'reset' 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. BTW, previously defined "do_flr"
attribute has been renamed to "reset" since "do_flr" name doesn't represent
all PCI reset methods and plus, currently it is not being used. So, there
is no impact in renaming this sysfs attribute.

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 
Signed-off-by: Konrad Rzeszutek Wilk 
Reviewed-by: Boris Ostrovsky 
---
v1: - First posting
v2: - struct pcistub_args: Changed docunt field as unsigned int
- pcistub_reset_dev: initialization of "struct pcistub_args"
- pcistub_reset_dev: combined multiple if-statements
- pcistub_do_flr: removed goto statement
v3: - Resynced with linux kernel 4.14.4 and latest pci_stub.c changes.
- Renamed "do_flr" SysFS attribute to "reset". Plus, modified
  "reset" SysFS attribute code as per the latest changes in 4.14.4.
- struct pcistub_args: added "const" to "struct pci_dev *dev" field
- pcistub_device_search: Renamed found_dev to found
- pcistub_device_search: Modified comments and return statements
- pcistub_device_reset: introduced FLR reset code
- pcistub_device_reset: Modified all dev_dbg messages

 Documentation/ABI/testing/sysfs-driver-pciback |  15 +++
 drivers/xen/xen-pciback/pci_stub.c | 128 +
 2 files changed, 143 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-pciback 
b/Documentation/ABI/testing/sysfs-driver-pciback
index 6a733bf..d295b42 100644
--- 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
+  

[PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS attribute

[Govinda Tatti]

This patch contains Xen pciback driver changes to support PCI reset
(flr/slot/bus) based on SysFS 'reset' attribute. The following Xen
libxl patch depends on these kernel patches.
- Xen/libxl: Perform PCI reset using 'reset' SysFS attribute

Govinda Tatti (2):
  Drivers/PCI: Export pcie_has_flr() interface
  Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS
attribute

 Documentation/ABI/testing/sysfs-driver-pciback |  15 +++
 drivers/pci/pci.c  |   3 +-
 drivers/xen/xen-pciback/pci_stub.c | 128 +
 include/linux/pci.h|   1 +
 4 files changed, 146 insertions(+), 1 deletion(-)

-- 
2.9.5

[PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS attribute

[Govinda Tatti]

This patch contains Xen pciback driver changes to support PCI reset
(flr/slot/bus) based on SysFS 'reset' attribute. The following Xen
libxl patch depends on these kernel patches.
- Xen/libxl: Perform PCI reset using 'reset' SysFS attribute

Govinda Tatti (2):
  Drivers/PCI: Export pcie_has_flr() interface
  Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS
attribute

 Documentation/ABI/testing/sysfs-driver-pciback |  15 +++
 drivers/pci/pci.c  |   3 +-
 drivers/xen/xen-pciback/pci_stub.c | 128 +
 include/linux/pci.h|   1 +
 4 files changed, 146 insertions(+), 1 deletion(-)

-- 
2.9.5

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

[Govinda Tatti]

Jan,

Do you have any further comments on the current version of this patch?. 
Otherwise, I will work on the review comments and publish next version 
of this patch later this week. Please let me know. Thanks.


Cheers
GOVINDA
On 12/1/2017 10:16 AM, Govinda Tatti wrote:


On 11/30/2017 8:46 AM, Jan Beulich wrote:

On 30.11.17 at 15:15, <govinda.ta...@oracle.com> wrote:

On 11/30/2017 2:27 AM, Jan Beulich wrote:

On 29.11.17 at 18:38, <govinda.ta...@oracle.com> wrote:
In the case of bus or slot reset, our goal is to reset connected 
PCIe

fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?
For PCIe platforms, both slot and bus reset endup resetting all 
connected

device/functions on thesecondary bus (behind the root-port or
downstream-port).

According to my understanding this contradicts the comment
ahead of pci_reset_slot(), which talks of multiple slots per bus.
In such a setup, I can't see why resetting on slot would affect
other slots on the same bus. At the same time the comment
says that the slot reset may resolve to a bus one when there's
just a single slot on the bus.

For legacy PCI/PCI-X, we can have multiple slots per bus but not with
PCI-Express
(each link will be on a separate bus).

Is that true even for root complex integrated end points? A
random system's lspci output doesn't seem to agree with what
you say. A typical example would be USB controllers all sitting
on bus 0, but having different slot numbers. You clearly won't
be able to ever bus-reset these, and if you checked all devices
on bus 0 you would then also not be able to slot-reset them.

Here, slot reset refers to any PCIe slot that implements or supports
hotplug feature. The slot reset ultimately invokes "pciehp_reset_slot()".
W.r.t integrated endpoints, these can be reset either through FLR or
secondary bus reset methods only.

Cheers
GOVINDA

___
Xen-devel mailing list
xen-de...@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Govinda Tatti]

Jan,

Do you have any further comments on the current version of this patch?. 
Otherwise, I will work on the review comments and publish next version 
of this patch later this week. Please let me know. Thanks.


Cheers
GOVINDA
On 12/1/2017 10:16 AM, Govinda Tatti wrote:


On 11/30/2017 8:46 AM, Jan Beulich wrote:

On 30.11.17 at 15:15,  wrote:

On 11/30/2017 2:27 AM, Jan Beulich wrote:

On 29.11.17 at 18:38,  wrote:
In the case of bus or slot reset, our goal is to reset connected 
PCIe

fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?
For PCIe platforms, both slot and bus reset endup resetting all 
connected

device/functions on thesecondary bus (behind the root-port or
downstream-port).

According to my understanding this contradicts the comment
ahead of pci_reset_slot(), which talks of multiple slots per bus.
In such a setup, I can't see why resetting on slot would affect
other slots on the same bus. At the same time the comment
says that the slot reset may resolve to a bus one when there's
just a single slot on the bus.

For legacy PCI/PCI-X, we can have multiple slots per bus but not with
PCI-Express
(each link will be on a separate bus).

Is that true even for root complex integrated end points? A
random system's lspci output doesn't seem to agree with what
you say. A typical example would be USB controllers all sitting
on bus 0, but having different slot numbers. You clearly won't
be able to ever bus-reset these, and if you checked all devices
on bus 0 you would then also not be able to slot-reset them.

Here, slot reset refers to any PCIe slot that implements or supports
hotplug feature. The slot reset ultimately invokes "pciehp_reset_slot()".
W.r.t integrated endpoints, these can be reset either through FLR or
secondary bus reset methods only.

Cheers
GOVINDA

___
Xen-devel mailing list
xen-de...@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Govinda Tatti]

On 11/30/2017 8:46 AM, Jan Beulich wrote:

On 30.11.17 at 15:15,  wrote:

On 11/30/2017 2:27 AM, Jan Beulich wrote:

On 29.11.17 at 18:38,  wrote:
In the case of bus or slot reset, our goal is to reset connected PCIe
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?

For PCIe platforms, both slot and bus reset endup resetting all connected
device/functions on thesecondary bus (behind the root-port or
downstream-port).

According to my understanding this contradicts the comment
ahead of pci_reset_slot(), which talks of multiple slots per bus.
In such a setup, I can't see why resetting on slot would affect
other slots on the same bus. At the same time the comment
says that the slot reset may resolve to a bus one when there's
just a single slot on the bus.

For legacy PCI/PCI-X, we can have multiple slots per bus but not with
PCI-Express
(each link will be on a separate bus).

Is that true even for root complex integrated end points? A
random system's lspci output doesn't seem to agree with what
you say. A typical example would be USB controllers all sitting
on bus 0, but having different slot numbers. You clearly won't
be able to ever bus-reset these, and if you checked all devices
on bus 0 you would then also not be able to slot-reset them.

Here, slot reset refers to any PCIe slot that implements or supports
hotplug feature. The slot reset ultimately invokes "pciehp_reset_slot()".
W.r.t integrated endpoints, these can be reset either through FLR or
secondary bus reset methods only.

Cheers
GOVINDA

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

[Govinda Tatti]

On 11/30/2017 8:46 AM, Jan Beulich wrote:

On 30.11.17 at 15:15,  wrote:

On 11/30/2017 2:27 AM, Jan Beulich wrote:

On 29.11.17 at 18:38,  wrote:
In the case of bus or slot reset, our goal is to reset connected PCIe
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?

For PCIe platforms, both slot and bus reset endup resetting all connected
device/functions on thesecondary bus (behind the root-port or
downstream-port).

According to my understanding this contradicts the comment
ahead of pci_reset_slot(), which talks of multiple slots per bus.
In such a setup, I can't see why resetting on slot would affect
other slots on the same bus. At the same time the comment
says that the slot reset may resolve to a bus one when there's
just a single slot on the bus.

For legacy PCI/PCI-X, we can have multiple slots per bus but not with
PCI-Express
(each link will be on a separate bus).

Is that true even for root complex integrated end points? A
random system's lspci output doesn't seem to agree with what
you say. A typical example would be USB controllers all sitting
on bus 0, but having different slot numbers. You clearly won't
be able to ever bus-reset these, and if you checked all devices
on bus 0 you would then also not be able to slot-reset them.

Here, slot reset refers to any PCIe slot that implements or supports
hotplug feature. The slot reset ultimately invokes "pciehp_reset_slot()".
W.r.t integrated endpoints, these can be reset either through FLR or
secondary bus reset methods only.

Cheers
GOVINDA

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

[Govinda Tatti]

On 11/30/2017 2:27 AM, Jan Beulich wrote:

On 29.11.17 at 18:38,  wrote:
In the case of bus or slot reset, our goal is to reset connected PCIe
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?

For PCIe platforms, both slot and bus reset endup resetting all connected
device/functions on thesecondary bus (behind the root-port or
downstream-port).

According to my understanding this contradicts the comment
ahead of pci_reset_slot(), which talks of multiple slots per bus.
In such a setup, I can't see why resetting on slot would affect
other slots on the same bus. At the same time the comment
says that the slot reset may resolve to a bus one when there's
just a single slot on the bus.
For legacy PCI/PCI-X, we can have multiple slots per bus but not with 
PCI-Express
(each link will be on a separate bus). In anycase, we need to 
walk-through other
device/functions on the same bus/slot and check their status before 
using slot/bus

reset.

Cheers
GOVINDA

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

[Govinda Tatti]

On 11/30/2017 2:27 AM, Jan Beulich wrote:

On 29.11.17 at 18:38,  wrote:
In the case of bus or slot reset, our goal is to reset connected PCIe
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?

For PCIe platforms, both slot and bus reset endup resetting all connected
device/functions on thesecondary bus (behind the root-port or
downstream-port).

According to my understanding this contradicts the comment
ahead of pci_reset_slot(), which talks of multiple slots per bus.
In such a setup, I can't see why resetting on slot would affect
other slots on the same bus. At the same time the comment
says that the slot reset may resolve to a bus one when there's
just a single slot on the bus.
For legacy PCI/PCI-X, we can have multiple slots per bus but not with 
PCI-Express
(each link will be on a separate bus). In anycase, we need to 
walk-through other
device/functions on the same bus/slot and check their status before 
using slot/bus

reset.

Cheers
GOVINDA

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,  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

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,  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

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

[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

[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]

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 V2] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

In the case of bus or slot reset, our goal is to reset connected PCIe
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?

For PCIe platforms, both slot and bus reset endup resetting all connected
device/functions on thesecondary bus (behind the root-port or 
downstream-port).


Cheers
GOVINDA

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

[Govinda Tatti]

In the case of bus or slot reset, our goal is to reset connected PCIe
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same
walk-through and checking
is needed irrespective of type of reset being used.

I don't follow: The scope of other devices/functions possibly
affected by a reset depends on the type of reset, doesn't it?

For PCIe platforms, both slot and bus reset endup resetting all connected
device/functions on thesecondary bus (behind the root-port or 
downstream-port).


Cheers
GOVINDA

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

[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 V2] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

[Govinda Tatti]

Jan,

Please see below for my comments.

On 11/9/2017 2:49 AM, Jan Beulich wrote:

On 09.11.17 at 00:06,  wrote:

--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -244,6 +244,91 @@ struct pci_dev *pcistub_get_pci_dev(struct 
xen_pcibk_device *pdev,
return found_dev;
  }
  
+struct pcistub_args {

+   struct pci_dev *dev;

Please don't ignore prior review comments: Either carry out what
was requested, or explain why the request can't be fulfilled. You
saying "This field will point to first device that is not owned by
pcistub" to Roger's request to make this a pointer to const is not a
valid reason to not do the adjustment; in fact your reply is entirely
unrelated to the request.

We can use "const" with above field since we will set this field only once.
I will change it.



+static int pcistub_search_dev(struct pci_dev *dev, void *data)
+{
+   struct pcistub_device *psdev;
+   struct pcistub_args *arg = data;
+   bool found_dev = false;

Purely cosmetical, but anyway: Why not just "found"? What else
could be (not) found here other than the device in question?

Sure, I will change it to "found".




+   unsigned long flags;
+
+   spin_lock_irqsave(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found_dev = true;
+   arg->dcount++;
+   break;
+   }
+   }
+
+   spin_unlock_irqrestore(_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;

Despite the function needing to return int, this can be simplified to
"return !found_dev". I'd also like to note that the part of the
earlier comment related to this is sort of disconnected. How about

/* Device not owned by pcistub, someone owns it. Abort the walk */
if (!found_dev) {
arg->dev = dev;
return 1;
}

return 0;

Fine with me.


And finally - I don't think the comment is entirely correct - the
device not being owned by pciback doesn't necessarily mean it's
owned by another driver. It could as well be unowned.
OK. I will modify this comment as " Device not owned by pcistub. Abort 
the walk".



+static int pcistub_reset_dev(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__);
+
+   if (!pci_probe_reset_slot(dev->slot))
+   slot = true;
+   else if ((!pci_probe_reset_bus(dev->bus)) &&
+(!pci_is_root_bus(dev->bus)))
+   bus = true;
+
+   if (!bus && !slot)
+   return -EOPNOTSUPP;
+
+   /*
+* Make sure all devices on this bus are owned by the
+* PCI backend so that we can safely reset the whole bus.
+*/

Is that really the case when you mean to do a slot reset? It was for
a reason that I had asked about a missing "else" in v1 review,
rather than questioning the conditional around the logic.


In the case of bus or slot reset, our goal is to reset connected PCIe 
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same 
walk-through and checking

is needed irrespective of type of reset being used.




+   pci_walk_bus(dev->bus, pcistub_search_dev, );
+
+   /* All devices under the bus should be part of pcistub! */
+   if (arg.dev) {
+   dev_err(>dev, "%s device on bus 0x%x is not owned by 
pcistub\n",

%#x

Yet then, thinking about what would be useful information should the
situation really arise, I'm not convinced printing a bare bus number
here is useful either. Especially for the case of multiple parallel
requests you want to make it possible to match each message to the
original request (guest start or whatever). Hence I think you want
something like

"%s on the same bus as %s is not owned by " DRV_NAME "\n"

Sure, I will make this change.



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

While here the original device is perhaps not necessary to print,
the bare bus number doesn't carry enough information: You'll
want to prefix it by the segment number. Plus you'll want to use
canonical formatting (:bb), so one can get matches when
suitably grep-ing the log. Perhaps bus->name is what you're
after.

Sure, I can change it to

dev_dbg(>dev, "pcistub owns %d devices on PCI Bus %04x:%02x", 
pci_domain_nr(bus), bus->number);

Cheers
GOVINDA

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

[Govinda Tatti]

Jan,

Please see below for my comments.

On 11/9/2017 2:49 AM, Jan Beulich wrote:

On 09.11.17 at 00:06,  wrote:

--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -244,6 +244,91 @@ struct pci_dev *pcistub_get_pci_dev(struct 
xen_pcibk_device *pdev,
return found_dev;
  }
  
+struct pcistub_args {

+   struct pci_dev *dev;

Please don't ignore prior review comments: Either carry out what
was requested, or explain why the request can't be fulfilled. You
saying "This field will point to first device that is not owned by
pcistub" to Roger's request to make this a pointer to const is not a
valid reason to not do the adjustment; in fact your reply is entirely
unrelated to the request.

We can use "const" with above field since we will set this field only once.
I will change it.



+static int pcistub_search_dev(struct pci_dev *dev, void *data)
+{
+   struct pcistub_device *psdev;
+   struct pcistub_args *arg = data;
+   bool found_dev = false;

Purely cosmetical, but anyway: Why not just "found"? What else
could be (not) found here other than the device in question?

Sure, I will change it to "found".




+   unsigned long flags;
+
+   spin_lock_irqsave(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found_dev = true;
+   arg->dcount++;
+   break;
+   }
+   }
+
+   spin_unlock_irqrestore(_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;

Despite the function needing to return int, this can be simplified to
"return !found_dev". I'd also like to note that the part of the
earlier comment related to this is sort of disconnected. How about

/* Device not owned by pcistub, someone owns it. Abort the walk */
if (!found_dev) {
arg->dev = dev;
return 1;
}

return 0;

Fine with me.


And finally - I don't think the comment is entirely correct - the
device not being owned by pciback doesn't necessarily mean it's
owned by another driver. It could as well be unowned.
OK. I will modify this comment as " Device not owned by pcistub. Abort 
the walk".



+static int pcistub_reset_dev(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__);
+
+   if (!pci_probe_reset_slot(dev->slot))
+   slot = true;
+   else if ((!pci_probe_reset_bus(dev->bus)) &&
+(!pci_is_root_bus(dev->bus)))
+   bus = true;
+
+   if (!bus && !slot)
+   return -EOPNOTSUPP;
+
+   /*
+* Make sure all devices on this bus are owned by the
+* PCI backend so that we can safely reset the whole bus.
+*/

Is that really the case when you mean to do a slot reset? It was for
a reason that I had asked about a missing "else" in v1 review,
rather than questioning the conditional around the logic.


In the case of bus or slot reset, our goal is to reset connected PCIe 
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same 
walk-through and checking

is needed irrespective of type of reset being used.




+   pci_walk_bus(dev->bus, pcistub_search_dev, );
+
+   /* All devices under the bus should be part of pcistub! */
+   if (arg.dev) {
+   dev_err(>dev, "%s device on bus 0x%x is not owned by 
pcistub\n",

%#x

Yet then, thinking about what would be useful information should the
situation really arise, I'm not convinced printing a bare bus number
here is useful either. Especially for the case of multiple parallel
requests you want to make it possible to match each message to the
original request (guest start or whatever). Hence I think you want
something like

"%s on the same bus as %s is not owned by " DRV_NAME "\n"

Sure, I will make this change.



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

While here the original device is perhaps not necessary to print,
the bare bus number doesn't carry enough information: You'll
want to prefix it by the segment number. Plus you'll want to use
canonical formatting (:bb), so one can get matches when
suitably grep-ing the log. Perhaps bus->name is what you're
after.

Sure, I can change it to

dev_dbg(>dev, "pcistub owns %d devices on PCI Bus %04x:%02x", 
pci_domain_nr(bus), bus->number);

Cheers
GOVINDA

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,  wrote:

On 11/7/2017 8:40 AM, Jan Beulich wrote:

On 06.11.17 at 18:48,  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-de...@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 :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

[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,  wrote:

On 11/7/2017 8:40 AM, Jan Beulich wrote:

On 06.11.17 at 18:48,  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-de...@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 :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

[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.ta...@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-de...@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 :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

[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,  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-de...@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 :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

[PATCH V2] 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.ta...@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
---
 Documentation/ABI/testing/sysfs-driver-pciback |  12 +++
 drivers/xen/xen-pciback/pci_stub.c | 119 +
 2 files changed, 131 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-de...@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 :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..e2677a6 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -244,6 +244,91 @@ struct pci_dev *pcistub_get_pci_dev(struct 
xen_pcibk_device *pdev,
return found_dev;
 }
 
+struct pcistub_args {
+   struct pci_dev *dev;
+   unsigned 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(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found_dev = true;
+   arg->dcount++;
+   break;
+   }
+   }
+
+   spin_unlock_irqrestore(_devices_lock, flags);
+
+   /* Device not owned by pcistub, someone owns it. Abor

[PATCH V2] 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 
Signed-off-by: Konrad Rzeszutek Wilk 
Reviewed-by: Boris Ostrovsky 
---
 Documentation/ABI/testing/sysfs-driver-pciback |  12 +++
 drivers/xen/xen-pciback/pci_stub.c | 119 +
 2 files changed, 131 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-de...@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 :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..e2677a6 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -244,6 +244,91 @@ struct pci_dev *pcistub_get_pci_dev(struct 
xen_pcibk_device *pdev,
return found_dev;
 }
 
+struct pcistub_args {
+   struct pci_dev *dev;
+   unsigned 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(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found_dev = true;
+   arg->dcount++;
+   break;
+   }
+   }
+
+   spin_unlock_irqrestore(_devices_lock, flags);
+
+   /* Device not owned by pcistub, someone owns it. Abort the walk */
+   if (!found_dev)
+   arg->dev = dev;
+
+   ret

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,  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-de...@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 :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, "[%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, );
+
+   /* All devices under the bus should be part of pcistub! */
+   if (arg.dev) {
+   dev_err(>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, "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

[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,  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-de...@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 :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, "[%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, );
+
+   /* All devices under the bus should be part of pcistub! */
+   if (arg.dev) {
+   dev_err(>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, "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

[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]

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 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.ta...@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@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-de...@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 :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

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 
Signed-off-by: Konrad Rzeszutek Wilk 
Reviewed-by: Boris Ostrovsky 
---
  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-de...@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 :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(_devices

[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.ta...@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@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-de...@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 :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(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found_dev = true;
+   arg->dcount++;
+   break;
+   }
+   }
+
+   spin_unlock_irqrestore(_devices_lock, flags);
+
+   /* Device not owned by pcistub, someone owns it. Abor

[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 
Signed-off-by: Konrad Rzeszutek Wilk 
Reviewed-by: Boris Ostrovsky 
---
 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-de...@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 :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(_devices_lock, flags);
+
+   list_for_each_entry(psdev, _devices, dev_list) {
+   if (psdev->dev == dev) {
+   found_dev = true;
+   arg->dcount++;
+   break;
+   }
+   }
+
+   spin_unlock_irqrestore(_devices_lock, flags);
+
+   /* Device not owned by pcistub, someone owns it. Abort the walk */
+   if (!found_dev)
+   arg->dev = dev;
+
+   return found_dev ? 0