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

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

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

[Jan Beulich]

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

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

Jan


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

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

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

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

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

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

[Konrad Rzeszutek Wilk]

On Wed, Nov 29, 2017 at 09:40:20AM -0700, Jan Beulich wrote:
> >>> On 29.11.17 at 17:29,  wrote:
> > On Wed, Nov 29, 2017 at 08:35:33AM -0700, Jan Beulich wrote:
> >> >>> On 29.11.17 at 16:08,  wrote:
> >> > 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-devel@lists.xenproject.org 
> >> > +Description:
> >> > +An option to perform a slot or bus reset when a PCI 
> >> > device
> >> > +is owned by Xen PCI backend. Writing a string of 
> >> > :BB:DD.F
> >> > +will cause the pciback driver to perform a slot or bus 
> >> > reset
> >> > +if the device supports it. It also checks to make sure 
> >> > that
> >> > +all of the devices under the bridge are owned by Xen PCI
> >> > +backend.
> >>  Why do you name this "do_flr" when you don't even try FLR, but
> >>  go to slot or then bus reset right away.
> >> >>> Yes, I agree but xen toolstack has already been modified to
> >> >>> consume"do_flr" attribute. Hence, we are using the
> >> >>> function that matches with sysfs attribute.
> >> >> That's not a valid reason imo: Right now the driver doesn't expose
> >> >> such an attribute, so if the tool stack was trying to use it, it would
> >> >> not do what is intended at present anyway (i.e. the code could at
> >> >> best be called dead).
> >> > Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus 
> >> > reset".
> >> > Please let me knowyour preference.
> >> 
> >> Well, that's more a question to Konrad as the maintainer.
> >> Personally I'd prefer just "reset", as "pci" is redundant and "bus"
> > 
> > Can't do 'reset'.
> 
> Why?

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

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

It can be indeed called 'reset'.

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

Fair enough. Reset should work then.

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

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

[Pasi Kärkkäinen]

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

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

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

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

Just thinking out loud here..


> Cheers
> GOVINDA
> 


Thanks,

-- Pasi


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

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



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

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

[Jan Beulich]

>>> On 29.11.17 at 16:08,  wrote:
> 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-devel@lists.xenproject.org 
> +Description:
> +An option to perform a slot or bus reset when a PCI 
> device
> + is owned by Xen PCI backend. Writing a string of :BB:DD.F
> + will cause the pciback driver to perform a slot or bus reset
> + if the device supports it. It also checks to make sure that
> + all of the devices under the bridge are owned by Xen PCI
> + backend.
 Why do you name this "do_flr" when you don't even try FLR, but
 go to slot or then bus reset right away.
>>> Yes, I agree but xen toolstack has already been modified to
>>> consume"do_flr" attribute. Hence, we are using the
>>> function that matches with sysfs attribute.
>> That's not a valid reason imo: Right now the driver doesn't expose
>> such an attribute, so if the tool stack was trying to use it, it would
>> not do what is intended at present anyway (i.e. the code could at
>> best be called dead).
> Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus 
> reset".
> Please let me knowyour preference.

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

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

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

Jan



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

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-devel@lists.xenproject.org
+Description:
+An option to perform a slot or bus reset when a PCI device
+   is owned by Xen PCI backend. Writing a string of :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

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