[PATCH] Xen PCIbackend support for slot and bus reset (v3).

[PATCH] Xen PCIbackend support for slot and bus reset (v3).

[konrad]

These patches had been posted in the past and had been reworked
to address reviews. The original concerns with the patches was
the complex logic of doing an workqueue (or thread) that would
do the FLR/bus/slot reset outside the PCI lock. That idea proved
buggy. The other idea of usurping the 'reset' SysFS didn't work
either - as the the generic code will complain loudly about this.

The best so far mechanism is to use the 'xl' toolstack usage
of 'do_flr' and make that work in Xen pciback.

 Documentation/ABI/testing/sysfs-driver-pciback |   95 ++++++++++++++
 drivers/xen/xen-pciback/pci_stub.c             |  163 +++++++++++++++++++-----
 drivers/xen/xen-pciback/xenbus.c               |    2 +-
 3 files changed, 224 insertions(+), 36 deletions(-)

Konrad Rzeszutek Wilk (7):
      xen-pciback: Document the various parameters and attributes in SysFS
      xen/pciback: Don't deadlock when unbinding.
      xen/pciback: Move the FLR code to a function.
      xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute
      xen/pciback: Include the domain id if removing the device whilst still in use
      xen/pciback: Print out the domain owning the device.
      xen/pciback: Remove tons of dereferences

[PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[konrad]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Which hadn't been done with the initial commit.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 Documentation/ABI/testing/sysfs-driver-pciback |   84 ++++++++++++++++++++++++
 1 files changed, 84 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-pciback

diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
new file mode 100644
index 0000000..4d2860c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -0,0 +1,84 @@
+What:           /sys//module/xen_pciback/parameters/verbose_request
+Date:           Oct 2011
+KernelVersion:  3.1
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                Print to the console the PCI configuration changes caused
+                by the frontend.
+
+What:           /sys/module/xen_pciback/parameters/passthrough
+Date:           Oct 2011
+KernelVersion:  3.1
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                Option to specify how to export PCI topology to guest:
+                0 - (default) Hide the true PCI topology and makes the frontend
+                 there is a single PCI bus with only the exported devices on it.
+                 For example, a device at 03:05.0 will be re-assigned to 00:00.0
+                 while second device at 02:1a.1 will be re-assigned to 00:01.1.
+                1 - Passthrough provides a real view of the PCI topology to the
+                 frontend (for example, a device at 06:01.b will still appear at
+                 06:01.b to the frontend). This is similar to how Xen 2.0.x
+                 exposed PCI devices to its driver domains. This may be required
+                 for drivers which depend on finding their hardware in certain
+                 bus/slot locations.
+
+What:           /sys/module/xen_pciback/parameters/hide
+Date:           Oct 2011
+KernelVersion:  3.1
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                An parenthesis separated list of DDDD:BB:DD.F devices to hide
+                from their native device drivers - so they cannot use them -
+                and Xen pciback to become their owner. That is
+                Domain:Bus:Device.Function, where Domain is optional.
+                Wildcards are permitted. For example:
+                xen-pciback.hide=(04:00.0)(03:01.*) will make the Xen pciback
+                driver the owner of 04:00.0 device and any functions of the
+                03:01.0 device (so 03:01.1, 03:01.2, etc).
+
+What:           /sys/module/xen_pciback/parameters/permissive
+Date:           Oct 2011
+KernelVersion:  3.1
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                Allow non-emulated (raw) access to PCI configuration space by
+                the guest frontend. This can have adverse affect as the guest
+                can destabilize the initial domain.
+
+
+What:           /sys/bus/pci/drivers/pciback/quirks
+Date:           Oct 2011
+KernelVersion:  3.1
+Contact:        xen-devel@lists.xenproject.org
+Description:
+		If the permissive attribute is set, then writing a string in
+                the format of DDDD:BB:DD.F-REG:SIZE:MASK will allow the guest
+                to write and read from the PCI device. That is Domain:Bus:
+                Device.Function-Register:Size:Mask (Domain is optional).
+                For example:
+                #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/irq_handlers
+Date:           Oct 2011
+KernelVersion:  3.1
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                A list of all of the PCI devices owned by Xen PCI back and
+                whether Xen PCI backend will acknowledge the interrupts received
+                and the amount of interrupts received. Xen PCI back acknowledges
+                said interrupts only when they are level, shared with another
+                guest, and enabled by the guest.
+
+What:           /sys/bus/pci/drivers/pciback/irq_handler_state
+Date:           Oct 2011
+KernelVersion:  3.1
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                An option to toggle Xen PCI back to acknowledge (or stop)
+                interrupts for the specific device regardless of whether the
+                device is shared, enabled, or on a level interrupt line.
+                Writing a string of DDDD:BB:DD.F will toggle the state.
+                This is Domain:Bus:Device.Function where domain is optional.
-- 
1.7.7.6

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Andrew Cooper]

On 08/07/14 19:58, konrad@kernel.org wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> Which hadn't been done with the initial commit.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-pciback |   84 ++++++++++++++++++++++++
>  1 files changed, 84 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-pciback
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
> new file mode 100644
> index 0000000..4d2860c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -0,0 +1,84 @@
> +What:           /sys//module/xen_pciback/parameters/verbose_request
>

s_//_/_ ?

~Andrew

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[David Vrabel]

On 08/07/14 19:58, konrad@kernel.org wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Which hadn't been done with the initial commit.
[...]
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -0,0 +1,84 @@
> +What:           /sys//module/xen_pciback/parameters/verbose_request

It's my understanding that module parameters are not typically part of
the ABI.

> +What:           /sys/module/xen_pciback/parameters/permissive
> +Date:           Oct 2011
> +KernelVersion:  3.1
> +Contact:        xen-devel@lists.xenproject.org
> +Description:
> +                Allow non-emulated (raw) access to PCI configuration space by
> +                the guest frontend. This can have adverse affect as the guest
> +                can destabilize the initial domain.

WTF?  Why does it even have such an unsafe option?


> +What:           /sys/bus/pci/drivers/pciback/irq_handlers
> +Date:           Oct 2011
> +KernelVersion:  3.1
> +Contact:        xen-devel@lists.xenproject.org
> +Description:
> +                A list of all of the PCI devices owned by Xen PCI back and
> +                whether Xen PCI backend will acknowledge the interrupts received
> +                and the amount of interrupts received. Xen PCI back acknowledges
> +                said interrupts only when they are level, shared with another
> +                guest, and enabled by the guest.

This should be a device property or something in debugfs.

> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
> +Date:           Oct 2011
> +KernelVersion:  3.1
> +Contact:        xen-devel@lists.xenproject.org
> +Description:
> +                An option to toggle Xen PCI back to acknowledge (or stop)
> +                interrupts for the specific device regardless of whether the
> +                device is shared, enabled, or on a level interrupt line.
> +                Writing a string of DDDD:BB:DD.F will toggle the state.
> +                This is Domain:Bus:Device.Function where domain is optional.

I do not understand under what circumstances this should be used in.

David

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 01:17:06PM +0100, David Vrabel wrote:
> On 08/07/14 19:58, konrad@kernel.org wrote:
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > Which hadn't been done with the initial commit.
> [...]
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> > @@ -0,0 +1,84 @@
> > +What:           /sys//module/xen_pciback/parameters/verbose_request
> 
> It's my understanding that module parameters are not typically part of
> the ABI.
> 
> > +What:           /sys/module/xen_pciback/parameters/permissive
> > +Date:           Oct 2011
> > +KernelVersion:  3.1
> > +Contact:        xen-devel@lists.xenproject.org
> > +Description:
> > +                Allow non-emulated (raw) access to PCI configuration space by
> > +                the guest frontend. This can have adverse affect as the guest
> > +                can destabilize the initial domain.
> 
> WTF?  Why does it even have such an unsafe option?

For those users who want to those drivers to have full access to it.
> 
> 
> > +What:           /sys/bus/pci/drivers/pciback/irq_handlers
> > +Date:           Oct 2011
> > +KernelVersion:  3.1
> > +Contact:        xen-devel@lists.xenproject.org
> > +Description:
> > +                A list of all of the PCI devices owned by Xen PCI back and
> > +                whether Xen PCI backend will acknowledge the interrupts received
> > +                and the amount of interrupts received. Xen PCI back acknowledges
> > +                said interrupts only when they are level, shared with another
> > +                guest, and enabled by the guest.
> 
> This should be a device property or something in debugfs.

<nods>Good idea.

<puts it on the todo list>
> 
> > +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
> > +Date:           Oct 2011
> > +KernelVersion:  3.1
> > +Contact:        xen-devel@lists.xenproject.org
> > +Description:
> > +                An option to toggle Xen PCI back to acknowledge (or stop)
> > +                interrupts for the specific device regardless of whether the
> > +                device is shared, enabled, or on a level interrupt line.
> > +                Writing a string of DDDD:BB:DD.F will toggle the state.
> > +                This is Domain:Bus:Device.Function where domain is optional.
> 
> I do not understand under what circumstances this should be used in.

So that dom0 does not disable the IRQ line as it would be getting the IRQs
for the guest as well (because the IRQ line is level and another guest
uses an PCI device that is using the same line).
> 
> David
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Andrew Cooper]

On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
>
>>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
>>> +Date:           Oct 2011
>>> +KernelVersion:  3.1
>>> +Contact:        xen-devel@lists.xenproject.org
>>> +Description:
>>> +                An option to toggle Xen PCI back to acknowledge (or stop)
>>> +                interrupts for the specific device regardless of whether the
>>> +                device is shared, enabled, or on a level interrupt line.
>>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
>>> +                This is Domain:Bus:Device.Function where domain is optional.
>> I do not understand under what circumstances this should be used in.
> So that dom0 does not disable the IRQ line as it would be getting the IRQs
> for the guest as well (because the IRQ line is level and another guest
> uses an PCI device that is using the same line).

Why is this relevant?  Xen (and Xen alone) actually controls this aspect
of interrupts.  Xen manages passing line level interrupts to any domain
which might have a device hanging off a particular line, and has to wait
until all domains have EOI'd the line until it can clear the interrupt
at the IO-APIC.

~Andrew

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 03:05:56PM +0100, Andrew Cooper wrote:
> On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
> >
> >>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
> >>> +Date:           Oct 2011
> >>> +KernelVersion:  3.1
> >>> +Contact:        xen-devel@lists.xenproject.org
> >>> +Description:
> >>> +                An option to toggle Xen PCI back to acknowledge (or stop)
> >>> +                interrupts for the specific device regardless of whether the
> >>> +                device is shared, enabled, or on a level interrupt line.
> >>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
> >>> +                This is Domain:Bus:Device.Function where domain is optional.
> >> I do not understand under what circumstances this should be used in.
> > So that dom0 does not disable the IRQ line as it would be getting the IRQs
> > for the guest as well (because the IRQ line is level and another guest
> > uses an PCI device that is using the same line).
> 
> Why is this relevant?  Xen (and Xen alone) actually controls this aspect
> of interrupts.  Xen manages passing line level interrupts to any domain
> which might have a device hanging off a particular line, and has to wait
> until all domains have EOI'd the line until it can clear the interrupt
> at the IO-APIC.

Because Linux will think there is an IRQ storm as the event->IRQ points
to the default one. And then it will mask the event, which means dom0
will mask the PIRQ, and Xen will then also mask the IRQ.
> 
> ~Andrew

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Andrew Cooper]

On 09/07/14 15:13, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 09, 2014 at 03:05:56PM +0100, Andrew Cooper wrote:
>> On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
>>>>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
>>>>> +Date:           Oct 2011
>>>>> +KernelVersion:  3.1
>>>>> +Contact:        xen-devel@lists.xenproject.org
>>>>> +Description:
>>>>> +                An option to toggle Xen PCI back to acknowledge (or stop)
>>>>> +                interrupts for the specific device regardless of whether the
>>>>> +                device is shared, enabled, or on a level interrupt line.
>>>>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
>>>>> +                This is Domain:Bus:Device.Function where domain is optional.
>>>> I do not understand under what circumstances this should be used in.
>>> So that dom0 does not disable the IRQ line as it would be getting the IRQs
>>> for the guest as well (because the IRQ line is level and another guest
>>> uses an PCI device that is using the same line).
>> Why is this relevant?  Xen (and Xen alone) actually controls this aspect
>> of interrupts.  Xen manages passing line level interrupts to any domain
>> which might have a device hanging off a particular line, and has to wait
>> until all domains have EOI'd the line until it can clear the interrupt
>> at the IO-APIC.
> Because Linux will think there is an IRQ storm as the event->IRQ points
> to the default one. And then it will mask the event, which means dom0
> will mask the PIRQ, and Xen will then also mask the IRQ.

Xen will (and by this I mean 'should', and this was the behaviour last
time I delved in there) only mask the IRQ if dom0 is the only consumer
of these interrupts.

For any PCIPassthrough, dom0 will get line interrupts for passed-through
devices, but in this case pci-back should always handle the line
interrupts so Linux doesn't block them as an IRQ storm.

~Andrew

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 03:22:30PM +0100, Andrew Cooper wrote:
> On 09/07/14 15:13, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jul 09, 2014 at 03:05:56PM +0100, Andrew Cooper wrote:
> >> On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
> >>>>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
> >>>>> +Date:           Oct 2011
> >>>>> +KernelVersion:  3.1
> >>>>> +Contact:        xen-devel@lists.xenproject.org
> >>>>> +Description:
> >>>>> +                An option to toggle Xen PCI back to acknowledge (or stop)
> >>>>> +                interrupts for the specific device regardless of whether the
> >>>>> +                device is shared, enabled, or on a level interrupt line.
> >>>>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
> >>>>> +                This is Domain:Bus:Device.Function where domain is optional.
> >>>> I do not understand under what circumstances this should be used in.
> >>> So that dom0 does not disable the IRQ line as it would be getting the IRQs
> >>> for the guest as well (because the IRQ line is level and another guest
> >>> uses an PCI device that is using the same line).
> >> Why is this relevant?  Xen (and Xen alone) actually controls this aspect
> >> of interrupts.  Xen manages passing line level interrupts to any domain
> >> which might have a device hanging off a particular line, and has to wait
> >> until all domains have EOI'd the line until it can clear the interrupt
> >> at the IO-APIC.
> > Because Linux will think there is an IRQ storm as the event->IRQ points
> > to the default one. And then it will mask the event, which means dom0
> > will mask the PIRQ, and Xen will then also mask the IRQ.
> 
> Xen will (and by this I mean 'should', and this was the behaviour last
> time I delved in there) only mask the IRQ if dom0 is the only consumer
> of these interrupts.
> 
> For any PCIPassthrough, dom0 will get line interrupts for passed-through
> devices, but in this case pci-back should always handle the line
> interrupts so Linux doesn't block them as an IRQ storm.

And that is what it does - and this option provides the option to enable/disable
it the system admin wishes to do it.
> 
> ~Andrew

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[David Vrabel]

On 09/07/14 15:25, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 09, 2014 at 03:22:30PM +0100, Andrew Cooper wrote:
>> On 09/07/14 15:13, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Jul 09, 2014 at 03:05:56PM +0100, Andrew Cooper wrote:
>>>> On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
>>>>>>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
>>>>>>> +Date:           Oct 2011
>>>>>>> +KernelVersion:  3.1
>>>>>>> +Contact:        xen-devel@lists.xenproject.org
>>>>>>> +Description:
>>>>>>> +                An option to toggle Xen PCI back to acknowledge (or stop)
>>>>>>> +                interrupts for the specific device regardless of whether the
>>>>>>> +                device is shared, enabled, or on a level interrupt line.
>>>>>>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
>>>>>>> +                This is Domain:Bus:Device.Function where domain is optional.
>>>>>> I do not understand under what circumstances this should be used in.
>>>>> So that dom0 does not disable the IRQ line as it would be getting the IRQs
>>>>> for the guest as well (because the IRQ line is level and another guest
>>>>> uses an PCI device that is using the same line).
>>>> Why is this relevant?  Xen (and Xen alone) actually controls this aspect
>>>> of interrupts.  Xen manages passing line level interrupts to any domain
>>>> which might have a device hanging off a particular line, and has to wait
>>>> until all domains have EOI'd the line until it can clear the interrupt
>>>> at the IO-APIC.
>>> Because Linux will think there is an IRQ storm as the event->IRQ points
>>> to the default one. And then it will mask the event, which means dom0
>>> will mask the PIRQ, and Xen will then also mask the IRQ.
>>
>> Xen will (and by this I mean 'should', and this was the behaviour last
>> time I delved in there) only mask the IRQ if dom0 is the only consumer
>> of these interrupts.
>>
>> For any PCIPassthrough, dom0 will get line interrupts for passed-through
>> devices, but in this case pci-back should always handle the line
>> interrupts so Linux doesn't block them as an IRQ storm.
> 
> And that is what it does - and this option provides the option to enable/disable
> it the system admin wishes to do it.

I still don't understand why someone would want to flip the handler to
a broken mode.

The original commit isn't very enlightening either.

David

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 03:45:03PM +0100, David Vrabel wrote:
> On 09/07/14 15:25, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jul 09, 2014 at 03:22:30PM +0100, Andrew Cooper wrote:
> >> On 09/07/14 15:13, Konrad Rzeszutek Wilk wrote:
> >>> On Wed, Jul 09, 2014 at 03:05:56PM +0100, Andrew Cooper wrote:
> >>>> On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
> >>>>>>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
> >>>>>>> +Date:           Oct 2011
> >>>>>>> +KernelVersion:  3.1
> >>>>>>> +Contact:        xen-devel@lists.xenproject.org
> >>>>>>> +Description:
> >>>>>>> +                An option to toggle Xen PCI back to acknowledge (or stop)
> >>>>>>> +                interrupts for the specific device regardless of whether the
> >>>>>>> +                device is shared, enabled, or on a level interrupt line.
> >>>>>>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
> >>>>>>> +                This is Domain:Bus:Device.Function where domain is optional.
> >>>>>> I do not understand under what circumstances this should be used in.
> >>>>> So that dom0 does not disable the IRQ line as it would be getting the IRQs
> >>>>> for the guest as well (because the IRQ line is level and another guest
> >>>>> uses an PCI device that is using the same line).
> >>>> Why is this relevant?  Xen (and Xen alone) actually controls this aspect
> >>>> of interrupts.  Xen manages passing line level interrupts to any domain
> >>>> which might have a device hanging off a particular line, and has to wait
> >>>> until all domains have EOI'd the line until it can clear the interrupt
> >>>> at the IO-APIC.
> >>> Because Linux will think there is an IRQ storm as the event->IRQ points
> >>> to the default one. And then it will mask the event, which means dom0
> >>> will mask the PIRQ, and Xen will then also mask the IRQ.
> >>
> >> Xen will (and by this I mean 'should', and this was the behaviour last
> >> time I delved in there) only mask the IRQ if dom0 is the only consumer
> >> of these interrupts.
> >>
> >> For any PCIPassthrough, dom0 will get line interrupts for passed-through
> >> devices, but in this case pci-back should always handle the line
> >> interrupts so Linux doesn't block them as an IRQ storm.
> > 
> > And that is what it does - and this option provides the option to enable/disable
> > it the system admin wishes to do it.
> 
> I still don't understand why someone would want to flip the handler to
> a broken mode.

The intent was to allow you to flip to the 'enable' mode in case Linux
did not detect it correctly.
> 
> The original commit isn't very enlightening either.

Thoughts then on what this documentation patch should say to make it
clear of its intent?

> 
> David

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[David Vrabel]

On 09/07/14 15:47, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 09, 2014 at 03:45:03PM +0100, David Vrabel wrote:
>> On 09/07/14 15:25, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Jul 09, 2014 at 03:22:30PM +0100, Andrew Cooper wrote:
>>>> On 09/07/14 15:13, Konrad Rzeszutek Wilk wrote:
>>>>> On Wed, Jul 09, 2014 at 03:05:56PM +0100, Andrew Cooper wrote:
>>>>>> On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
>>>>>>>>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
>>>>>>>>> +Date:           Oct 2011
>>>>>>>>> +KernelVersion:  3.1
>>>>>>>>> +Contact:        xen-devel@lists.xenproject.org
>>>>>>>>> +Description:
>>>>>>>>> +                An option to toggle Xen PCI back to acknowledge (or stop)
>>>>>>>>> +                interrupts for the specific device regardless of whether the
>>>>>>>>> +                device is shared, enabled, or on a level interrupt line.
>>>>>>>>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
>>>>>>>>> +                This is Domain:Bus:Device.Function where domain is optional.
>>>>>>>> I do not understand under what circumstances this should be used in.
>>>>>>> So that dom0 does not disable the IRQ line as it would be getting the IRQs
>>>>>>> for the guest as well (because the IRQ line is level and another guest
>>>>>>> uses an PCI device that is using the same line).
>>>>>> Why is this relevant?  Xen (and Xen alone) actually controls this aspect
>>>>>> of interrupts.  Xen manages passing line level interrupts to any domain
>>>>>> which might have a device hanging off a particular line, and has to wait
>>>>>> until all domains have EOI'd the line until it can clear the interrupt
>>>>>> at the IO-APIC.
>>>>> Because Linux will think there is an IRQ storm as the event->IRQ points
>>>>> to the default one. And then it will mask the event, which means dom0
>>>>> will mask the PIRQ, and Xen will then also mask the IRQ.
>>>>
>>>> Xen will (and by this I mean 'should', and this was the behaviour last
>>>> time I delved in there) only mask the IRQ if dom0 is the only consumer
>>>> of these interrupts.
>>>>
>>>> For any PCIPassthrough, dom0 will get line interrupts for passed-through
>>>> devices, but in this case pci-back should always handle the line
>>>> interrupts so Linux doesn't block them as an IRQ storm.
>>>
>>> And that is what it does - and this option provides the option to enable/disable
>>> it the system admin wishes to do it.
>>
>> I still don't understand why someone would want to flip the handler to
>> a broken mode.
> 
> The intent was to allow you to flip to the 'enable' mode in case Linux
> did not detect it correctly.

We should not provide sysfs knobs to work around kernel bugs.

>> The original commit isn't very enlightening either.
> 
> Thoughts then on what this documentation patch should say to make it
> clear of its intent?

I think it should be removed.

It also has non-standard behaviour of /toggling/ with every write
instead of using a write of a 1 or a 0 like every other sysfs file.

David

Re: [Xen-devel] [PATCH v3 1/7] xen-pciback: Document the various parameters and attributes in SysFS

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 03:57:15PM +0100, David Vrabel wrote:
> On 09/07/14 15:47, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jul 09, 2014 at 03:45:03PM +0100, David Vrabel wrote:
> >> On 09/07/14 15:25, Konrad Rzeszutek Wilk wrote:
> >>> On Wed, Jul 09, 2014 at 03:22:30PM +0100, Andrew Cooper wrote:
> >>>> On 09/07/14 15:13, Konrad Rzeszutek Wilk wrote:
> >>>>> On Wed, Jul 09, 2014 at 03:05:56PM +0100, Andrew Cooper wrote:
> >>>>>> On 09/07/14 14:59, Konrad Rzeszutek Wilk wrote:
> >>>>>>>>> +What:           /sys/bus/pci/drivers/pciback/irq_handler_state
> >>>>>>>>> +Date:           Oct 2011
> >>>>>>>>> +KernelVersion:  3.1
> >>>>>>>>> +Contact:        xen-devel@lists.xenproject.org
> >>>>>>>>> +Description:
> >>>>>>>>> +                An option to toggle Xen PCI back to acknowledge (or stop)
> >>>>>>>>> +                interrupts for the specific device regardless of whether the
> >>>>>>>>> +                device is shared, enabled, or on a level interrupt line.
> >>>>>>>>> +                Writing a string of DDDD:BB:DD.F will toggle the state.
> >>>>>>>>> +                This is Domain:Bus:Device.Function where domain is optional.
> >>>>>>>> I do not understand under what circumstances this should be used in.
> >>>>>>> So that dom0 does not disable the IRQ line as it would be getting the IRQs
> >>>>>>> for the guest as well (because the IRQ line is level and another guest
> >>>>>>> uses an PCI device that is using the same line).
> >>>>>> Why is this relevant?  Xen (and Xen alone) actually controls this aspect
> >>>>>> of interrupts.  Xen manages passing line level interrupts to any domain
> >>>>>> which might have a device hanging off a particular line, and has to wait
> >>>>>> until all domains have EOI'd the line until it can clear the interrupt
> >>>>>> at the IO-APIC.
> >>>>> Because Linux will think there is an IRQ storm as the event->IRQ points
> >>>>> to the default one. And then it will mask the event, which means dom0
> >>>>> will mask the PIRQ, and Xen will then also mask the IRQ.
> >>>>
> >>>> Xen will (and by this I mean 'should', and this was the behaviour last
> >>>> time I delved in there) only mask the IRQ if dom0 is the only consumer
> >>>> of these interrupts.
> >>>>
> >>>> For any PCIPassthrough, dom0 will get line interrupts for passed-through
> >>>> devices, but in this case pci-back should always handle the line
> >>>> interrupts so Linux doesn't block them as an IRQ storm.
> >>>
> >>> And that is what it does - and this option provides the option to enable/disable
> >>> it the system admin wishes to do it.
> >>
> >> I still don't understand why someone would want to flip the handler to
> >> a broken mode.
> > 
> > The intent was to allow you to flip to the 'enable' mode in case Linux
> > did not detect it correctly.
> 
> We should not provide sysfs knobs to work around kernel bugs.

It is already in the code. This is just a documentation patch.
But I can drop this chunk from it if you would like.
> 
> >> The original commit isn't very enlightening either.
> > 
> > Thoughts then on what this documentation patch should say to make it
> > clear of its intent?
> 
> I think it should be removed.
> 
> It also has non-standard behaviour of /toggling/ with every write
> instead of using a write of a 1 or a 0 like every other sysfs file.

The 'unbind' and 'bind' expect an BDF as well and they are part
of the SysFS and in documentation.

> 
> David

[PATCH v3 2/7] xen/pciback: Don't deadlock when unbinding.

[konrad]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

As commit 0a9fd0152929db372ff61b0d6c280fdd34ae8bdb
'xen/pciback: Document the entry points for 'pcistub_put_pci_dev''
explained there are four entry points in this function.
Two of them are when the user fiddles in the SysFS to
unbind a device which might be in use by a guest or not.

Both 'unbind' states will cause a deadlock as the the PCI lock has
already been taken, which then pci_device_reset tries to take.

We can omit this by detecting this and using the  'already
locked' variant of the pci_device_reset - but we don't need to
do either as that is done much later when the PCIback device
structures are destroyed. As such lets use the 'try_reset'
variant. That means we still end up FLR-ing the device in
between swapping it from one guest to another. The FLR when
'unbind' part is still taken care by pci_device_reset.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 8a69a9f..b1fb099 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -277,10 +277,15 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	 * (so it's ready for the next domain)
 	 */
 
-	/* This is OK - we are running from workqueue context
-	 * and want to inhibit the user from fiddling with 'reset'
+	/* We might be holding the PCI lock (see comment at the top of the
+	 * function) - as such try lock and if we can't then don't worry -
+	 * as either:
+	 *  - we are 'unbind' in which case 'pcistub_device_release' gets
+	 *    called which does the locked version of this.
+	 *  - the toolstack has the smarts to do it and has done the
+	 *    reset on SysFS before assigning this device to another guest.
 	 */
-	pci_reset_function(dev);
+	pci_try_reset_function(dev);
 	pci_restore_state(dev);
 
 	/* This disables the device. */
-- 
1.7.7.6

Re: [PATCH v3 2/7] xen/pciback: Don't deadlock when unbinding.

[David Vrabel]

On 08/07/14 19:58, konrad@kernel.org wrote:
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -277,10 +277,15 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>  	 * (so it's ready for the next domain)
>  	 */
>  
> -	/* This is OK - we are running from workqueue context
> -	 * and want to inhibit the user from fiddling with 'reset'
> +	/* We might be holding the PCI lock (see comment at the top of the
> +	 * function) - as such try lock and if we can't then don't worry -
> +	 * as either:
> +	 *  - we are 'unbind' in which case 'pcistub_device_release' gets
> +	 *    called which does the locked version of this.
> +	 *  - the toolstack has the smarts to do it and has done the
> +	 *    reset on SysFS before assigning this device to another guest.
>  	 */
> -	pci_reset_function(dev);
> +	pci_try_reset_function(dev);

Why not use __pci_reset_function_locked() and ensure all callers of
pcistub_put_pci_dev() are holding the device lock?

>  	pci_restore_state(dev);

If we didn't reset the device there is no need to restore its state.

David

Re: [PATCH v3 2/7] xen/pciback: Don't deadlock when unbinding.

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 01:21:12PM +0100, David Vrabel wrote:
> On 08/07/14 19:58, konrad@kernel.org wrote:
> > --- a/drivers/xen/xen-pciback/pci_stub.c
> > +++ b/drivers/xen/xen-pciback/pci_stub.c
> > @@ -277,10 +277,15 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> >  	 * (so it's ready for the next domain)
> >  	 */
> >  
> > -	/* This is OK - we are running from workqueue context
> > -	 * and want to inhibit the user from fiddling with 'reset'
> > +	/* We might be holding the PCI lock (see comment at the top of the
> > +	 * function) - as such try lock and if we can't then don't worry -
> > +	 * as either:
> > +	 *  - we are 'unbind' in which case 'pcistub_device_release' gets
> > +	 *    called which does the locked version of this.
> > +	 *  - the toolstack has the smarts to do it and has done the
> > +	 *    reset on SysFS before assigning this device to another guest.
> >  	 */
> > -	pci_reset_function(dev);
> > +	pci_try_reset_function(dev);
> 
> Why not use __pci_reset_function_locked() and ensure all callers of
> pcistub_put_pci_dev() are holding the device lock?

That could also be done.
> 
> >  	pci_restore_state(dev);
> 
> If we didn't reset the device there is no need to restore its state.

<nods>
> 
> David

[PATCH v3 3/7] xen/pciback: Move the FLR code to a function.

[konrad]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Moving the bulk of the code its own function to aid
in making the 'xen/pciback: implement PCI reset slot
or bus with 'do_flr' SysFS attribute' easier.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c |   42 ++++++++++++++++++++++-------------
 1 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index b1fb099..03badac 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -242,6 +242,31 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
 	return found_dev;
 }
 
+static void pcistub_reset_pci_dev(struct pci_dev *dev)
+{
+	/* This is OK - we are running from workqueue context
+	 * and want to inhibit the user from fiddling with 'reset'
+	 */
+
+	dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
+	/* We might be holding the PCI lock (see comment at the top of the
+	 * function) - as such try lock and if we can't then don't worry -
+	 * as either:
+	 *  - we are 'unbind' in which case 'pcistub_device_release' gets
+	 *    called which does the locked version of this.
+	 *  - the toolstack has the smarts to do it and has done the
+	 *    reset on SysFS before assigning this device to another guest.
+	 */
+	pci_try_reset_function(dev);
+	pci_restore_state(dev);
+
+	/* This disables the device. */
+	xen_pcibk_reset_device(dev);
+
+	/* And cleanup up our emulated fields. */
+	xen_pcibk_config_reset_dev(dev);
+}
+
 /*
  * Called when:
  *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
@@ -276,23 +301,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	/* Cleanup our device
 	 * (so it's ready for the next domain)
 	 */
+	pcistub_reset_pci_dev(dev);
 
-	/* We might be holding the PCI lock (see comment at the top of the
-	 * function) - as such try lock and if we can't then don't worry -
-	 * as either:
-	 *  - we are 'unbind' in which case 'pcistub_device_release' gets
-	 *    called which does the locked version of this.
-	 *  - the toolstack has the smarts to do it and has done the
-	 *    reset on SysFS before assigning this device to another guest.
-	 */
-	pci_try_reset_function(dev);
-	pci_restore_state(dev);
-
-	/* This disables the device. */
-	xen_pcibk_reset_device(dev);
-
-	/* And cleanup up our emulated fields. */
-	xen_pcibk_config_reset_dev(dev);
 	xen_pcibk_config_free_dyn_fields(dev);
 
 	xen_unregister_device_domain_owner(dev);
-- 
1.7.7.6

[PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[konrad]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

It starts with the device being binded to us - for which
we do a device function reset (and done via SysFS
so the PCI lock is held)

If the device is unbinded from us - we also do a function
reset (also 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).

All on the individual PCI function level (so bus:device:function).

Unfortunatly a function reset is not adequate for certain
PCIe devices. The reset for an individual PCI function "means
device must support FLR (PCIe or AF), PM reset on D3hot->D0
device specific reset, or be a singleton device on a bus
a secondary bus reset.  FLR does not have widespread support,
reset is not very reliable, and bus topology is dictated by the
and device design.  We need to provide a means for a user to
a bus reset in cases where the existing mechanisms are not
 or not reliable. " (Adam Williamson, 'vfio-pci: PCI hot reset
interface' commit 8b27ee60bfd6bbb84d2df28fa706c5c5081066ca).

As such to do a slot or a bus reset is we need another mechanism.
This is not exposed 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 off 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 an slot or bus reset (if
slot not supported) if all of the functions of a PCIe
device belong to Xen PCIback. We do not care if the device is
in-use as we depend on the toolstack to be aware of this -
however if it is we will WARN the user.

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

Putting the reset function in a workqueue 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 toolstack
doing the right thing. As such 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 inadvertly 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 do not do
the bus (or slot) reset.

We also warn the user if the device is in use - but still
continue with the reset. This should not happen as the toolstack
also does the check.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 Documentation/ABI/testing/sysfs-driver-pciback |   11 ++
 drivers/xen/xen-pciback/pci_stub.c             |  124 +++++++++++++++++++-----
 2 files changed, 110 insertions(+), 25 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
index 4d2860c..812eb1a 100644
--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -82,3 +82,14 @@ Description:
                 device is shared, enabled, or on a level interrupt line.
                 Writing a string of DDDD:BB:DD.F will toggle the state.
                 This is Domain:Bus:Device.Function where domain is optional.
+
+What:           /sys/bus/pci/drivers/pciback/do_flr
+Date:           July 2014
+KernelVersion:  3.16
+Contact:        xen-devel@lists.xenproject.org
+Description:
+                An option to slot or bus reset an PCI device owned by
+                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
+                the driver to perform an slot or bus reset if the device
+                supports. 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 03badac..6bb6be8 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -100,14 +100,9 @@ static void pcistub_device_release(struct kref *kref)
 
 	xen_unregister_device_domain_owner(dev);
 
-	/* Call the reset function which does not take lock as this
-	 * is called from "unbind" which takes a device_lock mutex.
-	 */
-	__pci_reset_function_locked(dev);
+	/* Reset is done by the toolstack by using 'do_flr' on the SysFS. */
 	if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
 		dev_dbg(&dev->dev, "Could not reload PCI state\n");
-	else
-		pci_restore_state(dev);
 
 	if (dev->msix_cap) {
 		struct physdev_pci_device ppdev = {
@@ -123,9 +118,6 @@ static void pcistub_device_release(struct kref *kref)
 				 err);
 	}
 
-	/* Disable the device */
-	xen_pcibk_reset_device(dev);
-
 	kfree(dev_data);
 	pci_set_drvdata(dev, NULL);
 
@@ -242,22 +234,64 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
 	return found_dev;
 }
 
-static void pcistub_reset_pci_dev(struct pci_dev *dev)
+struct wrapper_args {
+	struct pci_dev *dev;
+	int in_use;
+};
+
+static int pcistub_pci_walk_wrapper(struct pci_dev *dev, void *data)
 {
-	/* This is OK - we are running from workqueue context
-	 * and want to inhibit the user from fiddling with 'reset'
-	 */
+	struct pcistub_device *psdev, *found_psdev = NULL;
+	struct wrapper_args *arg = data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pcistub_devices_lock, flags);
+	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
+		if (psdev->dev == dev) {
+			found_psdev = psdev;
+			if (psdev->pdev)
+				arg->in_use++;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
+	dev_dbg(&dev->dev, "%s\n", found_psdev ? "OK" : "not owned by us!");
+
+	if (!found_psdev)
+		arg->dev = dev;
+	return found_psdev ? 0 : 1;
+}
+
+static int pcistub_reset_pci_dev(struct pci_dev *dev)
+{
+	struct wrapper_args arg = { .dev = NULL, .in_use = 0 };
+	bool slot = false, bus = false;
+	int rc;
+
+	if (!dev)
+		return -EINVAL;
+
+	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;
+	}
+	dev_dbg(&dev->dev, "resetting (FLR, D3, %s %s) the device\n",
+		slot ? "slot" : "", bus ? "bus" : "");
+
+	pci_walk_bus(dev->bus, pcistub_pci_walk_wrapper, &arg);
+
+	if (arg.in_use)
+		dev_err(&dev->dev, "is in use!\n");
 
-	dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
-	/* We might be holding the PCI lock (see comment at the top of the
-	 * function) - as such try lock and if we can't then don't worry -
-	 * as either:
-	 *  - we are 'unbind' in which case 'pcistub_device_release' gets
-	 *    called which does the locked version of this.
-	 *  - the toolstack has the smarts to do it and has done the
-	 *    reset on SysFS before assigning this device to another guest.
+	/*
+	 * Takes the PCI lock. OK to do it as we are never called
+	 * from 'unbind' state and don't deadlock.
 	 */
-	pci_try_reset_function(dev);
+	pci_reset_function(dev);
+
 	pci_restore_state(dev);
 
 	/* This disables the device. */
@@ -265,6 +299,18 @@ static void pcistub_reset_pci_dev(struct pci_dev *dev)
 
 	/* And cleanup up our emulated fields. */
 	xen_pcibk_config_reset_dev(dev);
+
+	if (!bus && !slot)
+		return 0;
+
+	/* All slots or devices under the bus should be part of pcistub! */
+	if (arg.dev) {
+		dev_err(&dev->dev, "depends on: %s!\n", pci_name(arg.dev));
+		return -EBUSY;
+	}
+	rc = slot ? pci_try_reset_slot(dev->slot): pci_try_reset_bus(dev->bus);
+
+	return rc;
 }
 
 /*
@@ -298,10 +344,10 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	* pcistub and xen_pcibk when AER is in processing
 	*/
 	down_write(&pcistub_sem);
-	/* Cleanup our device
-	 * (so it's ready for the next domain)
+	/* Cleanup our device (so it's ready for the next domain)
+	 * That is the job of the toolstack which has to call 'do_flr' before
+	 * providing the PCI device to a guest (see pcistub_do_flr).
 	 */
-	pcistub_reset_pci_dev(dev);
 
 	xen_pcibk_config_free_dyn_fields(dev);
 
@@ -1388,6 +1434,29 @@ static ssize_t permissive_show(struct device_driver *drv, char *buf)
 static DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show,
 		   permissive_add);
 
+static ssize_t pcistub_do_flr(struct device_driver *drv, const char *buf,
+				size_t count)
+{
+	int domain, bus, slot, func;
+	int err;
+	struct pcistub_device *psdev;
+
+	err = str_to_slot(buf, &domain, &bus, &slot, &func);
+	if (err)
+		goto out;
+
+	psdev = pcistub_device_find(domain, bus, slot, func);
+	if (psdev) {
+		err = pcistub_reset_pci_dev(psdev->dev);
+		pcistub_device_put(psdev);
+	} else
+		err = -ENODEV;
+out:
+	if (!err)
+		err = count;
+	return err;
+}
+static DRIVER_ATTR(do_flr, S_IWUSR, NULL, pcistub_do_flr);
 static void pcistub_exit(void)
 {
 	driver_remove_file(&xen_pcibk_pci_driver.driver, &driver_attr_new_slot);
@@ -1401,6 +1470,8 @@ static void pcistub_exit(void)
 			   &driver_attr_irq_handlers);
 	driver_remove_file(&xen_pcibk_pci_driver.driver,
 			   &driver_attr_irq_handler_state);
+	driver_remove_file(&xen_pcibk_pci_driver.driver,
+			   &driver_attr_do_flr);
 	pci_unregister_driver(&xen_pcibk_pci_driver);
 }
 
@@ -1494,6 +1565,9 @@ static int __init pcistub_init(void)
 	if (!err)
 		err = driver_create_file(&xen_pcibk_pci_driver.driver,
 					&driver_attr_irq_handler_state);
+	if (!err)
+		err = driver_create_file(&xen_pcibk_pci_driver.driver,
+					&driver_attr_do_flr);
 	if (err)
 		pcistub_exit();
 
-- 
1.7.7.6

Re: [PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[David Vrabel]

On 08/07/14 19:58, konrad@kernel.org wrote:
> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -82,3 +82,14 @@ Description:
>                  device is shared, enabled, or on a level interrupt line.
>                  Writing a string of DDDD:BB:DD.F will toggle the state.
>                  This is Domain:Bus:Device.Function where domain is optional.
> +
> +What:           /sys/bus/pci/drivers/pciback/do_flr
> +Date:           July 2014
> +KernelVersion:  3.16
> +Contact:        xen-devel@lists.xenproject.org
> +Description:
> +                An option to slot or bus reset an PCI device owned by
> +                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
> +                the driver to perform an slot or bus reset if the device
> +                supports. It also checks to make sure that all of the devices
> +                under the bridge are owned by Xen PCI backend.

Not sure I like this new interface.  I solved this by adding a new reset
file that looked like the regular one the pci would have if it supported
FLR.  I'm fairly sure I posted a series for this.  Was there a reason
you didn't do this?

David

Re: [PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[Konrad Rzeszutek Wilk]

On Tue, Jul 08, 2014 at 07:02:51PM +0100, David Vrabel wrote:
> On 08/07/14 19:58, konrad@kernel.org wrote:
> > --- a/Documentation/ABI/testing/sysfs-driver-pciback
> > +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> > @@ -82,3 +82,14 @@ Description:
> >                  device is shared, enabled, or on a level interrupt line.
> >                  Writing a string of DDDD:BB:DD.F will toggle the state.
> >                  This is Domain:Bus:Device.Function where domain is optional.
> > +
> > +What:           /sys/bus/pci/drivers/pciback/do_flr
> > +Date:           July 2014
> > +KernelVersion:  3.16
> > +Contact:        xen-devel@lists.xenproject.org
> > +Description:
> > +                An option to slot or bus reset an PCI device owned by
> > +                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
> > +                the driver to perform an slot or bus reset if the device
> > +                supports. It also checks to make sure that all of the devices
> > +                under the bridge are owned by Xen PCI backend.
> 
> Not sure I like this new interface.  I solved this by adding a new reset
> file that looked like the regular one the pci would have if it supported
> FLR.  I'm fairly sure I posted a series for this.  Was there a reason
> you didn't do this?

It did not work.

During bootup kobject would complain about a secondary 'reset' SysFS
on the PCI device.

> 
> David

Re: [PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[Konrad Rzeszutek Wilk]

On Tue, Jul 08, 2014 at 02:46:26PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 08, 2014 at 07:02:51PM +0100, David Vrabel wrote:
> > On 08/07/14 19:58, konrad@kernel.org wrote:
> > > --- a/Documentation/ABI/testing/sysfs-driver-pciback
> > > +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> > > @@ -82,3 +82,14 @@ Description:
> > >                  device is shared, enabled, or on a level interrupt line.
> > >                  Writing a string of DDDD:BB:DD.F will toggle the state.
> > >                  This is Domain:Bus:Device.Function where domain is optional.
> > > +
> > > +What:           /sys/bus/pci/drivers/pciback/do_flr
> > > +Date:           July 2014
> > > +KernelVersion:  3.16
> > > +Contact:        xen-devel@lists.xenproject.org
> > > +Description:
> > > +                An option to slot or bus reset an PCI device owned by
> > > +                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
> > > +                the driver to perform an slot or bus reset if the device
> > > +                supports. It also checks to make sure that all of the devices
> > > +                under the bridge are owned by Xen PCI backend.
> > 
> > Not sure I like this new interface.  I solved this by adding a new reset
> > file that looked like the regular one the pci would have if it supported
> > FLR.  I'm fairly sure I posted a series for this.  Was there a reason
> > you didn't do this?
> 
> It did not work.
> 
> During bootup kobject would complain about a secondary 'reset' SysFS
> on the PCI device.

Here is what I saw:
   20.441332] Key type dns_resolver registered
[   20.446548] registered taskstats version 1
[   20.452843] ------------[ cut here ]------------
[   20.457731] WARNING: CPU: 0 PID: 1 at /home/konrad/ssd/konrad/linux/fs/sysfs/dir.c:31 sysfs_warn_dup+0x6a/0x80()
[   20.468594] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1c.0/0000:02:00.0/0000:03:01.0/0000:04:00.0/reset'
[   20.481207] Modules linked in:
[   20.484508] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W     3.16.0-rc2-00027-g777b409 #1
[   20.493701] Hardware name:                  /DQ67SW, BIOS SWQ6710H.86A.0066.2012.1105.1504 11/05/2012
[   20.503561]  000000000000001f ffff8801192abcc8 ffffffff816e5bc2 000000000000001f
[   20.511479]  ffff8801192abd18 ffff8801192abd08 ffffffff810a0187 0000000000000000
[   20.519396]  ffff88010e8ff000 ffffffff819b632b ffff880118e421e8 ffff880118e410a8
[   20.527337] Call Trace:
[   20.530022]  [<ffffffff816e5bc2>] dump_stack+0x51/0x6b
[   20.535541]  [<ffffffff810a0187>] warn_slowpath_common+0x87/0xb0
[   20.541986]  [<ffffffff810a0251>] warn_slowpath_fmt+0x41/0x50
[   20.548155]  [<ffffffff81245243>] ? kernfs_path+0x53/0x70
[   20.553966]  [<ffffffff812473fa>] sysfs_warn_dup+0x6a/0x80
[   20.559855]  [<ffffffff81246e56>] sysfs_add_file_mode_ns+0x126/0x160
[   20.566668]  [<ffffffff81246eb5>] sysfs_create_file_ns+0x25/0x30
[   20.573110]  [<ffffffff81476343>] device_create_file+0x43/0xb0
[   20.579361]  [<ffffffff81369d13>] pci_create_sysfs_dev_files+0x2c3/0x3e0
[   20.586546]  [<ffffffff81d1ffa5>] pci_sysfs_init+0x1f/0x51
[   20.592429]  [<ffffffff81d1ff86>] ? pci_driver_init+0x12/0x12
[   20.598592]  [<ffffffff81d1ff86>] ? pci_driver_init+0x12/0x12
[   20.604748]  [<ffffffff81002089>] do_one_initcall+0x89/0x1b0
[   20.610833]  [<ffffffff81ce4b30>] kernel_init_freeable+0x167/0x1fd
[   20.617487]  [<ffffffff81ce4bc6>] ? kernel_init_freeable+0x1fd/0x1fd
[   20.624296]  [<ffffffff816dd7c0>] ? rest_init+0x90/0x90
[   20.629910]  [<ffffffff816dd7c9>] kernel_init+0x9/0xf0
[   20.635433]  [<ffffffff816eb27c>] ret_from_fork+0x7c/0xb0
[   20.641219]  [<ffffffff816dd7c0>] ? rest_init+0x90/0x90
[   20.646846] ---[ end trace 956618df162d6136 ]---
[   20.661457]   Magic number: 6:892:343

with this patch and command line:
debug console=hvc0 xen-blkback.log_stats=1 kgdboc=hvc0 xen-pciback.hide=(04:00.0)(07:00.0)(00:16.*)(03:01.0)(03:02.0)


>From 777b4097bc6f2d83a8690d844a2a6285f392b7fa Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 21 Apr 2014 16:32:23 -0400
Subject: [PATCH] xen/pciback: PCI reset slot or bus - David's

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c |   84 ++++++++++++++++++++++++++++++------
 1 files changed, 70 insertions(+), 14 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 030ac8f..21754fe 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -49,6 +49,8 @@ struct pcistub_device {
 
 	struct pci_dev *dev;
 	struct xen_pcibk_device *pdev;/* non-NULL if struct pci_dev is in use */
+
+	bool created_reset_file;
 };
 
 /* Access to pcistub_devices & seized_devices lists and the initialize_devices
@@ -63,6 +65,7 @@ static LIST_HEAD(pcistub_devices);
 static int initialize_devices;
 static LIST_HEAD(seized_devices);
 
+void pcistub_device_reset(struct work_struct *work);
 static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev;
@@ -85,6 +88,8 @@ static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 	return psdev;
 }
 
+static void pcistub_remove_reset_file(struct pcistub_device *psdev);
+
 /* Don't call this directly as it's called by pcistub_device_put */
 static void pcistub_device_release(struct kref *kref)
 {
@@ -100,14 +105,11 @@ static void pcistub_device_release(struct kref *kref)
 
 	xen_unregister_device_domain_owner(dev);
 
-	/* Call the reset function which does not take lock as this
-	 * is called from "unbind" which takes a device_lock mutex.
-	 */
-	__pci_reset_function_locked(dev);
+	/* Reset is done by the toolstack by using 'reset' on the SysFS. */
 	if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
 		dev_dbg(&dev->dev, "Could not reload PCI state\n");
-	else
-		pci_restore_state(dev);
+
+	pcistub_remove_reset_file(psdev);
 
 	if (dev->msix_cap) {
 		struct physdev_pci_device ppdev = {
@@ -123,9 +125,6 @@ static void pcistub_device_release(struct kref *kref)
 				 err);
 	}
 
-	/* Disable the device */
-	xen_pcibk_reset_device(dev);
-
 	kfree(dev_data);
 	pci_set_drvdata(dev, NULL);
 
@@ -136,17 +135,22 @@ static void pcistub_device_release(struct kref *kref)
 	dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
 	pci_dev_put(dev);
 
+	dev_dbg(&dev->dev, "pcistub_device_release finished. Device gone\n");
+
 	kfree(psdev);
 }
 
 static inline void pcistub_device_get(struct pcistub_device *psdev)
 {
 	kref_get(&psdev->kref);
+	pr_debug("%s, ref count is NOW at %d, %p\n", __func__, atomic_read(&psdev->kref.refcount), pci_get_drvdata(psdev->dev));
 }
 
 static inline void pcistub_device_put(struct pcistub_device *psdev)
 {
+	pr_debug("%s, ref count is at %d %p\n", __func__, atomic_read(&psdev->kref.refcount), pci_get_drvdata(psdev->dev));
 	kref_put(&psdev->kref, pcistub_device_release);
+	pr_debug("%s, ref count is at %d %p\n", __func__, atomic_read(&psdev->kref.refcount), pci_get_drvdata(psdev->dev));
 }
 
 static struct pcistub_device *pcistub_device_find(int domain, int bus,
@@ -248,9 +252,10 @@ static void pcistub_reset_pci_dev(struct pci_dev *dev)
 	 * and want to inhibit the user from fiddling with 'reset'
 	 */
 
-	dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
+	dev_dbg(&dev->dev, "resetting (FLR, D3, bus, slot, etc) the device\n");
 
 	pci_reset_function(dev);
+
 	pci_restore_state(dev);
 
 	/* This disables the device. */
@@ -258,6 +263,52 @@ static void pcistub_reset_pci_dev(struct pci_dev *dev)
 
 	/* And cleanup up our emulated fields. */
 	xen_pcibk_config_reset_dev(dev);
+
+	/* Implement the rest. */
+}
+
+static ssize_t pcistub_reset_store(struct device *dev,
+                                  struct device_attribute *attr,
+                                  const char *buf, size_t count)
+{
+       struct pci_dev *pdev = to_pci_dev(dev);
+       unsigned long val;
+       ssize_t result = strict_strtoul(buf, 0, &val);
+
+       if (result < 0)
+               return result;
+
+       if (val != 1)
+               return -EINVAL;
+
+	pcistub_reset_pci_dev(pdev);
+
+	return 0;
+}
+static DEVICE_ATTR(reset, 0200, NULL, pcistub_reset_store);
+static void pcistub_remove_reset_file(struct pcistub_device *psdev)
+{
+       if (psdev && psdev->created_reset_file)
+               device_remove_file(&psdev->dev->dev, &dev_attr_reset);
+}
+
+static int pcistub_try_create_reset_file(struct pcistub_device *psdev)
+{
+       struct device *dev = &psdev->dev->dev;
+       struct kernfs_node *reset_dirent;
+       int ret;
+
+       reset_dirent = sysfs_get_dirent(dev->kobj.sd, "reset");
+       if (reset_dirent) {
+               sysfs_put(dev->kobj.sd);
+               return 0;
+       }
+
+       ret = device_create_file(dev, &dev_attr_reset);
+       if (ret < 0)
+               return ret;
+       psdev->created_reset_file = true;
+       return 0;
 }
 
 /*
@@ -291,10 +342,10 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	* pcistub and xen_pcibk when AER is in processing
 	*/
 	down_write(&pcistub_sem);
-	/* Cleanup our device
-	 * (so it's ready for the next domain)
+	/* Cleanup our device (so it's ready for the next domain)
+	 * That is the job of the toolstack which has to call 'reset' before
+	 * providing the PCI device to a guest (see pcistub_reset_store).
 	 */
-	pcistub_reset_pci_dev(dev);
 
 	xen_unregister_device_domain_owner(dev);
 
@@ -409,7 +460,7 @@ static int pcistub_init_device(struct pci_dev *dev)
 	if (!dev_data->pci_saved_state)
 		dev_err(&dev->dev, "Could not store PCI conf saved state!\n");
 	else {
-		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
+		dev_dbg(&dev->dev, "resetting (FLR, D4, etc) the device\n");
 		__pci_reset_function_locked(dev);
 		pci_restore_state(dev);
 	}
@@ -483,6 +534,10 @@ static int pcistub_seize(struct pci_dev *dev)
 	if (!psdev)
 		return -ENOMEM;
 
+       err = pcistub_try_create_reset_file(psdev);
+       if (err < 0)
+               goto out;
+
 	spin_lock_irqsave(&pcistub_devices_lock, flags);
 
 	if (initialize_devices) {
@@ -502,6 +557,7 @@ static int pcistub_seize(struct pci_dev *dev)
 
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 
+out:
 	if (err)
 		pcistub_device_put(psdev);
 
-- 
1.7.7.6

> 
> > 
> > David

Re: [PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[David Vrabel]

On 08/07/14 19:46, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 08, 2014 at 07:02:51PM +0100, David Vrabel wrote:
>> On 08/07/14 19:58, konrad@kernel.org wrote:
>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>>> @@ -82,3 +82,14 @@ Description:
>>>                  device is shared, enabled, or on a level interrupt line.
>>>                  Writing a string of DDDD:BB:DD.F will toggle the state.
>>>                  This is Domain:Bus:Device.Function where domain is optional.
>>> +
>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>>> +Date:           July 2014
>>> +KernelVersion:  3.16
>>> +Contact:        xen-devel@lists.xenproject.org
>>> +Description:
>>> +                An option to slot or bus reset an PCI device owned by
>>> +                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
>>> +                the driver to perform an slot or bus reset if the device
>>> +                supports. It also checks to make sure that all of the devices
>>> +                under the bridge are owned by Xen PCI backend.
>>
>> Not sure I like this new interface.  I solved this by adding a new reset
>> file that looked like the regular one the pci would have if it supported
>> FLR.  I'm fairly sure I posted a series for this.  Was there a reason
>> you didn't do this?
> 
> It did not work.
> 
> During bootup kobject would complain about a secondary 'reset' SysFS
> on the PCI device.

I think this because of pciback registering a driver too early, before
the device is fully initialized.  You can see in the trace that it is
the common pci code that is trying to add the "reset" file so it must be
doing this /after/ pciback's probe has been called.

I would consider:

1. Removing the "hide" module parameter -- it doesn't work if pciback is
a module anyway.

2. Making pciback initialize like a regular driver module (no
fs_initcall() shenanigans).

3. Require userspace to sort out binding the device to pciback (e.g.,
libxl already does the unbind if requested).

4. Finally, I would consider generic driver core functionality for
prioritizing drivers so they get probed first.

David

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

[David Vrabel]

On 09/07/14 13:32, David Vrabel wrote:
> On 08/07/14 19:46, Konrad Rzeszutek Wilk wrote:
>> On Tue, Jul 08, 2014 at 07:02:51PM +0100, David Vrabel wrote:
>>>
>>> Not sure I like this new interface.  I solved this by adding a new reset
>>> file that looked like the regular one the pci would have if it supported
>>> FLR.  I'm fairly sure I posted a series for this.  Was there a reason
>>> you didn't do this?
>>
>> It did not work.
>>
>> During bootup kobject would complain about a secondary 'reset' SysFS
>> on the PCI device.
[...]
> 
> I would consider:

Or try the v2 of the patch which defers creating the sysfs to the late init.

David

Re: [PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 01:32:10PM +0100, David Vrabel wrote:
> On 08/07/14 19:46, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jul 08, 2014 at 07:02:51PM +0100, David Vrabel wrote:
> >> On 08/07/14 19:58, konrad@kernel.org wrote:
> >>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> >>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> >>> @@ -82,3 +82,14 @@ Description:
> >>>                  device is shared, enabled, or on a level interrupt line.
> >>>                  Writing a string of DDDD:BB:DD.F will toggle the state.
> >>>                  This is Domain:Bus:Device.Function where domain is optional.
> >>> +
> >>> +What:           /sys/bus/pci/drivers/pciback/do_flr
> >>> +Date:           July 2014
> >>> +KernelVersion:  3.16
> >>> +Contact:        xen-devel@lists.xenproject.org
> >>> +Description:
> >>> +                An option to slot or bus reset an PCI device owned by
> >>> +                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
> >>> +                the driver to perform an slot or bus reset if the device
> >>> +                supports. It also checks to make sure that all of the devices
> >>> +                under the bridge are owned by Xen PCI backend.
> >>
> >> Not sure I like this new interface.  I solved this by adding a new reset
> >> file that looked like the regular one the pci would have if it supported
> >> FLR.  I'm fairly sure I posted a series for this.  Was there a reason
> >> you didn't do this?
> > 
> > It did not work.
> > 
> > During bootup kobject would complain about a secondary 'reset' SysFS
> > on the PCI device.
> 
> I think this because of pciback registering a driver too early, before
> the device is fully initialized.  You can see in the trace that it is
> the common pci code that is trying to add the "reset" file so it must be
> doing this /after/ pciback's probe has been called.
> 
> I would consider:
> 
> 1. Removing the "hide" module parameter -- it doesn't work if pciback is
> a module anyway.

I find it incredibly useful and so do a lot of other people.

> 
> 2. Making pciback initialize like a regular driver module (no
> fs_initcall() shenanigans).

The point is to take the PCI device before the drivers touch it.

We want it to be in a pristine state so that the device driver
domains can use it.
> 
> 3. Require userspace to sort out binding the device to pciback (e.g.,
> libxl already does the unbind if requested).

How would you do the bus/slot reset? Or are you thinking that at
that point the 'reset' functionality would be over-written to point
to Xen pciback and it would do the job?

> 
> 4. Finally, I would consider generic driver core functionality for
> prioritizing drivers so they get probed first.

Not sure I understand why you want the drivers to use the device
first? The point is that we can 'hide' them from the generic
drivers and present them to the backend domains.

Regardless of these - I am curious to why you don't like do_flr
as it is even implemented in the the toolstack (but buggy) and
it does a good job of allowing us to do slot/bus reset?

> 
> David

Re: [PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[David Vrabel]

On 09/07/14 15:12, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 09, 2014 at 01:32:10PM +0100, David Vrabel wrote:
>> On 08/07/14 19:46, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Jul 08, 2014 at 07:02:51PM +0100, David Vrabel wrote:
>>>> On 08/07/14 19:58, konrad@kernel.org wrote:
>>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
>>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
>>>>> @@ -82,3 +82,14 @@ Description:
>>>>>                  device is shared, enabled, or on a level interrupt line.
>>>>>                  Writing a string of DDDD:BB:DD.F will toggle the state.
>>>>>                  This is Domain:Bus:Device.Function where domain is optional.
>>>>> +
>>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
>>>>> +Date:           July 2014
>>>>> +KernelVersion:  3.16
>>>>> +Contact:        xen-devel@lists.xenproject.org
>>>>> +Description:
>>>>> +                An option to slot or bus reset an PCI device owned by
>>>>> +                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
>>>>> +                the driver to perform an slot or bus reset if the device
>>>>> +                supports. It also checks to make sure that all of the devices
>>>>> +                under the bridge are owned by Xen PCI backend.
>>>>
>>>> Not sure I like this new interface.  I solved this by adding a new reset
>>>> file that looked like the regular one the pci would have if it supported
>>>> FLR.  I'm fairly sure I posted a series for this.  Was there a reason
>>>> you didn't do this?
>>>
>>> It did not work.
>>>
>>> During bootup kobject would complain about a secondary 'reset' SysFS
>>> on the PCI device.
>>
>> I think this because of pciback registering a driver too early, before
>> the device is fully initialized.  You can see in the trace that it is
>> the common pci code that is trying to add the "reset" file so it must be
>> doing this /after/ pciback's probe has been called.
>>
>> I would consider:
>>
>> 1. Removing the "hide" module parameter -- it doesn't work if pciback is
>> a module anyway.
> 
> I find it incredibly useful and so do a lot of other people.

PCI passthrough must work well without hide and without pciback being
built-in (and it does with the "reset" change).

What are you using "hide" for?

>> 2. Making pciback initialize like a regular driver module (no
>> fs_initcall() shenanigans).
> 
> The point is to take the PCI device before the drivers touch it.
> 
> We want it to be in a pristine state so that the device driver
> domains can use it.

But hide only ensures this the first time the device is assigned.  Using
a function reset ensures this all the time.

>> 3. Require userspace to sort out binding the device to pciback (e.g.,
>> libxl already does the unbind if requested).
> 
> How would you do the bus/slot reset? Or are you thinking that at
> that point the 'reset' functionality would be over-written to point
> to Xen pciback and it would do the job?

I'm not sure I understand your question.  libxl already does the
function reset (by writing to "reset").

>> 4. Finally, I would consider generic driver core functionality for
>> prioritizing drivers so they get probed first.
> 
> Not sure I understand why you want the drivers to use the device
> first? The point is that we can 'hide' them from the generic
> drivers and present them to the backend domains.

The pciback driver would be prioritized, so it would be probed first.

> Regardless of these - I am curious to why you don't like do_flr
> as it is even implemented in the the toolstack (but buggy) and
> it does a good job of allowing us to do slot/bus reset?

Because there is already a documented interface for resetting devices
(the "reset" file), we don't want a second interface.

David

Re: [PATCH v3 4/7] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute

[Konrad Rzeszutek Wilk]

On Wed, Jul 09, 2014 at 03:26:05PM +0100, David Vrabel wrote:
> On 09/07/14 15:12, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jul 09, 2014 at 01:32:10PM +0100, David Vrabel wrote:
> >> On 08/07/14 19:46, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Jul 08, 2014 at 07:02:51PM +0100, David Vrabel wrote:
> >>>> On 08/07/14 19:58, konrad@kernel.org wrote:
> >>>>> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> >>>>> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> >>>>> @@ -82,3 +82,14 @@ Description:
> >>>>>                  device is shared, enabled, or on a level interrupt line.
> >>>>>                  Writing a string of DDDD:BB:DD.F will toggle the state.
> >>>>>                  This is Domain:Bus:Device.Function where domain is optional.
> >>>>> +
> >>>>> +What:           /sys/bus/pci/drivers/pciback/do_flr
> >>>>> +Date:           July 2014
> >>>>> +KernelVersion:  3.16
> >>>>> +Contact:        xen-devel@lists.xenproject.org
> >>>>> +Description:
> >>>>> +                An option to slot or bus reset an PCI device owned by
> >>>>> +                Xen PCI backend. Writing a string of DDDD:BB:DD.F will cause
> >>>>> +                the driver to perform an slot or bus reset if the device
> >>>>> +                supports. It also checks to make sure that all of the devices
> >>>>> +                under the bridge are owned by Xen PCI backend.
> >>>>
> >>>> Not sure I like this new interface.  I solved this by adding a new reset
> >>>> file that looked like the regular one the pci would have if it supported
> >>>> FLR.  I'm fairly sure I posted a series for this.  Was there a reason
> >>>> you didn't do this?
> >>>
> >>> It did not work.
> >>>
> >>> During bootup kobject would complain about a secondary 'reset' SysFS
> >>> on the PCI device.
> >>
> >> I think this because of pciback registering a driver too early, before
> >> the device is fully initialized.  You can see in the trace that it is
> >> the common pci code that is trying to add the "reset" file so it must be
> >> doing this /after/ pciback's probe has been called.
> >>
> >> I would consider:
> >>
> >> 1. Removing the "hide" module parameter -- it doesn't work if pciback is
> >> a module anyway.
> > 
> > I find it incredibly useful and so do a lot of other people.
> 
> PCI passthrough must work well without hide and without pciback being
> built-in (and it does with the "reset" change).
> 
> What are you using "hide" for?

For hiding the AHCI driver from the likes of multipath and lvm.
I want the device driver domain to set those up and don't want the
initial domain have to create this and then have to tear it down.

Ditto for the bttv - it ends up loading tons of drivers and
just sorting out the dependency is tedious. If I hide it away
I can easily pass it in the backend.
> 
> >> 2. Making pciback initialize like a regular driver module (no
> >> fs_initcall() shenanigans).
> > 
> > The point is to take the PCI device before the drivers touch it.
> > 
> > We want it to be in a pristine state so that the device driver
> > domains can use it.
> 
> But hide only ensures this the first time the device is assigned.  Using
> a function reset ensures this all the time.

The hide also saves the registers:

 514         pci_save_state(dev);
 515         dev_data->pci_saved_state = pci_store_saved_state(dev);

before they are used by device drivers. The function reset (or bus
reset) won't ensure that those registers will always be the same
from the initial bootup state. As in: device drivers -> pciback ->
save states -> FLR -> give to guest. There is still the taint of the
device driver modifying the registers.

> 
> >> 3. Require userspace to sort out binding the device to pciback (e.g.,
> >> libxl already does the unbind if requested).
> > 
> > How would you do the bus/slot reset? Or are you thinking that at
> > that point the 'reset' functionality would be over-written to point
> > to Xen pciback and it would do the job?
> 
> I'm not sure I understand your question.  libxl already does the
> function reset (by writing to "reset").

Right.  And it also does 'do_flr'.
> 
> >> 4. Finally, I would consider generic driver core functionality for
> >> prioritizing drivers so they get probed first.
> > 
> > Not sure I understand why you want the drivers to use the device
> > first? The point is that we can 'hide' them from the generic
> > drivers and present them to the backend domains.
> 
> The pciback driver would be prioritized, so it would be probed first.

Which would require it to be some early_initcall variant and use
the override (which I think is going in 3.17?) for 'struct device'?
(Alex implemented it). I think that is what you are saying? However
that looks to be orthogonal to what this patch tries to do?

> 
> > Regardless of these - I am curious to why you don't like do_flr
> > as it is even implemented in the the toolstack (but buggy) and
> > it does a good job of allowing us to do slot/bus reset?
> 
> Because there is already a documented interface for resetting devices
> (the "reset" file), we don't want a second interface.

Which just does the FLR. This is doing bus/slot reset and doing an
override of the 'reset' in SysFS does not work without some clever
coding.

The 'do_flr' seems to be already baked in libxl - why not use it?
> 
> David

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

[Andrew Cooper]

On 08/07/14 19:58, konrad@kernel.org wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> The life-cycle of a PCI device in Xen pciback is complex
> and is constrained by the PCI generic locking mechanism.
>
> It starts with the device being binded to us - for which

"being bound to us"

~Andrew

[PATCH v3 5/7] xen/pciback: Include the domain id if removing the device whilst still in use

[konrad]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Cleanup the function a bit - also include the id of the
domain that is using the device.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 6bb6be8..df485ec 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -615,12 +615,14 @@ static void pcistub_remove(struct pci_dev *dev)
 	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
 
 	if (found_psdev) {
-		dev_dbg(&dev->dev, "found device to remove - in use? %p\n",
-			found_psdev->pdev);
+		dev_dbg(&dev->dev, "found device to remove %s\n",
+			found_psdev->pdev ? "- in-use" : "");
 
 		if (found_psdev->pdev) {
-			pr_warn("****** removing device %s while still in-use! ******\n",
-			       pci_name(found_psdev->dev));
+			int domid = xen_find_device_domain_owner(dev);
+
+			pr_warn("****** removing device %s while still in-use by domain %d! ******\n",
+			       pci_name(found_psdev->dev), domid);
 			pr_warn("****** driver domain may still access this device's i/o resources!\n");
 			pr_warn("****** shutdown driver domain before binding device\n");
 			pr_warn("****** to other drivers or domains\n");
-- 
1.7.7.6

Re: [PATCH v3 5/7] xen/pciback: Include the domain id if removing the device whilst still in use

[David Vrabel]

On 08/07/14 19:58, konrad@kernel.org wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Cleanup the function a bit - also include the id of the
> domain that is using the device.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

[PATCH v3 6/7] xen/pciback: Print out the domain owning the device.

[konrad]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

We had been printing it only if the device was built with
debug enabled. But this information is usuable in the field
to troubleshoot.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/xenbus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index 81d5068..f18174a 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -246,7 +246,7 @@ static int xen_pcibk_export_device(struct xen_pcibk_device *pdev,
 	if (err)
 		goto out;
 
-	dev_dbg(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
+	dev_info(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
 	if (xen_register_device_domain_owner(dev,
 					     pdev->xdev->otherend_id) != 0) {
 		dev_err(&dev->dev, "Stealing ownership from dom%d.\n",
-- 
1.7.7.6

Re: [PATCH v3 6/7] xen/pciback: Print out the domain owning the device.

[David Vrabel]

On 08/07/14 19:58, konrad@kernel.org wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> We had been printing it only if the device was built with
> debug enabled. But this information is usuable in the field
> to troubleshoot.

"useful"

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

[PATCH v3 7/7] xen/pciback: Remove tons of dereferences

[konrad]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

A little cleanup. No functional difference.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pci_stub.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index df485ec..8f74ae0 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -692,10 +692,12 @@ static pci_ers_result_t common_process(struct pcistub_device *psdev,
 {
 	pci_ers_result_t res = result;
 	struct xen_pcie_aer_op *aer_op;
+	struct xen_pcibk_device *pdev = psdev->pdev;
+	struct xen_pci_sharedinfo *sh_info = pdev->sh_info;
 	int ret;
 
 	/*with PV AER drivers*/
-	aer_op = &(psdev->pdev->sh_info->aer_op);
+	aer_op = &(sh_info->aer_op);
 	aer_op->cmd = aer_cmd ;
 	/*useful for error_detected callback*/
 	aer_op->err = state;
@@ -716,36 +718,36 @@ static pci_ers_result_t common_process(struct pcistub_device *psdev,
 	* this flag to judge whether we need to check pci-front give aer
 	* service ack signal
 	*/
-	set_bit(_PCIB_op_pending, (unsigned long *)&psdev->pdev->flags);
+	set_bit(_PCIB_op_pending, (unsigned long *)&pdev->flags);
 
 	/*It is possible that a pcifront conf_read_write ops request invokes
 	* the callback which cause the spurious execution of wake_up.
 	* Yet it is harmless and better than a spinlock here
 	*/
 	set_bit(_XEN_PCIB_active,
-		(unsigned long *)&psdev->pdev->sh_info->flags);
+		(unsigned long *)&sh_info->flags);
 	wmb();
-	notify_remote_via_irq(psdev->pdev->evtchn_irq);
+	notify_remote_via_irq(pdev->evtchn_irq);
 
 	ret = wait_event_timeout(xen_pcibk_aer_wait_queue,
 				 !(test_bit(_XEN_PCIB_active, (unsigned long *)
-				 &psdev->pdev->sh_info->flags)), 300*HZ);
+				 &sh_info->flags)), 300*HZ);
 
 	if (!ret) {
 		if (test_bit(_XEN_PCIB_active,
-			(unsigned long *)&psdev->pdev->sh_info->flags)) {
+			(unsigned long *)&sh_info->flags)) {
 			dev_err(&psdev->dev->dev,
 				"pcifront aer process not responding!\n");
 			clear_bit(_XEN_PCIB_active,
-			  (unsigned long *)&psdev->pdev->sh_info->flags);
+			  (unsigned long *)&sh_info->flags);
 			aer_op->err = PCI_ERS_RESULT_NONE;
 			return res;
 		}
 	}
-	clear_bit(_PCIB_op_pending, (unsigned long *)&psdev->pdev->flags);
+	clear_bit(_PCIB_op_pending, (unsigned long *)&pdev->flags);
 
 	if (test_bit(_XEN_PCIF_active,
-		(unsigned long *)&psdev->pdev->sh_info->flags)) {
+		(unsigned long *)&sh_info->flags)) {
 		dev_dbg(&psdev->dev->dev,
 			"schedule pci_conf service in " DRV_NAME "\n");
 		xen_pcibk_test_and_schedule_op(psdev->pdev);
-- 
1.7.7.6

Re: [Xen-devel] [PATCH] Xen PCIbackend support for slot and bus reset (v3).

[Sander Eikelenboom]

Tuesday, July 8, 2014, 8:58:22 PM, you wrote:

> These patches had been posted in the past and had been reworked
> to address reviews. The original concerns with the patches was
> the complex logic of doing an workqueue (or thread) that would
> do the FLR/bus/slot reset outside the PCI lock. That idea proved
> buggy. The other idea of usurping the 'reset' SysFS didn't work
> either - as the the generic code will complain loudly about this.

> The best so far mechanism is to use the 'xl' toolstack usage
> of 'do_flr' and make that work in Xen pciback.

>  Documentation/ABI/testing/sysfs-driver-pciback |   95 ++++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c             |  163 +++++++++++++++++++-----
>  drivers/xen/xen-pciback/xenbus.c               |    2 +-
>  3 files changed, 224 insertions(+), 36 deletions(-)

> Konrad Rzeszutek Wilk (7):
>       xen-pciback: Document the various parameters and attributes in SysFS
>       xen/pciback: Don't deadlock when unbinding.
>       xen/pciback: Move the FLR code to a function.
>       xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute
>       xen/pciback: Include the domain id if removing the device whilst still in use
>       xen/pciback: Print out the domain owning the device.
>       xen/pciback: Remove tons of dereferences

Could you push these to your kernel.org git tree ?

--
Sander