libxl__device_pci_reset() questions

libxl__device_pci_reset() questions

[Jan Beulich]

All,

in the context of someone seeing "The kernel doesn't support reset
from sysfs for PCI device", is my understanding correct that the lack
of error checking in any caller (perhaps intentional) means that any
of the errors logged from this function are really just warnings, i.e.
don't prevent the assignment from taking place?

Furthermore I'm puzzled by the function first thing trying to access
a "do_flr" file supposedly made available by the pciback driver, yet
I can't see either the upstream or the old 2.6.18 driver surfacing
such a file. What am I missing here?

Thanks, Jan

Re: libxl__device_pci_reset() questions

[Ian Campbell]

On Thu, 2015-02-19 at 13:59 +0000, Jan Beulich wrote:
> All,
> 
> in the context of someone seeing "The kernel doesn't support reset
> from sysfs for PCI device", is my understanding correct that the lack
> of error checking in any caller (perhaps intentional) means that any
> of the errors logged from this function are really just warnings, i.e.
> don't prevent the assignment from taking place?

It was a long while ago, but I believe that was the intention, yes.

> Furthermore I'm puzzled by the function first thing trying to access
> a "do_flr" file supposedly made available by the pciback driver, yet
> I can't see either the upstream or the old 2.6.18 driver surfacing
> such a file. What am I missing here?

I'm not sure, on the basis of
http://lists.xen.org/archives/html/xen-devel/2014-06/msg03105.html and 
http://lists.xen.org/archives/html/xen-devel/2014-07/msg01108.html I've
added Konrad to the CC.

Ian.

Re: libxl__device_pci_reset() questions

[Sander Eikelenboom]

Thursday, February 19, 2015, 3:30:52 PM, you wrote:

> On Thu, 2015-02-19 at 13:59 +0000, Jan Beulich wrote:
>> All,
>> 
>> in the context of someone seeing "The kernel doesn't support reset
>> from sysfs for PCI device", is my understanding correct that the lack
>> of error checking in any caller (perhaps intentional) means that any
>> of the errors logged from this function are really just warnings, i.e.
>> don't prevent the assignment from taking place?

> It was a long while ago, but I believe that was the intention, yes.

>> Furthermore I'm puzzled by the function first thing trying to access
>> a "do_flr" file supposedly made available by the pciback driver, yet
>> I can't see either the upstream or the old 2.6.18 driver surfacing
>> such a file. What am I missing here?

> I'm not sure, on the basis of
> http://lists.xen.org/archives/html/xen-devel/2014-06/msg03105.html and 
> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01108.html I've
> added Konrad to the CC.

You seemed to have missed the actual CC, added now and also David who nacked 
these patches.

The fundamental issue seems to be:
A) Does this even have to be sysfs triggered functionality. The most prominent 
   reason for this construct with pciback deferring it to the toolstack which in 
   turn invokes a sysfs to pciback again were issues around locking.
   However vfio/virtio also has the bus/slot reset logic and seems to be able to
   work around said locking issues.

B) If it *has* to be done with a sysfs entry, the naming of the sysfs entry 
   "do_flr" is misleading since it doesn't do an FLR, but hooks up bus/slot 
   reset.

That said, i'm running for quite some time now with the mentioned do_flr patches
from Konrad applied, these fix issues with pci devices not being properly reset 
when stopping and starting VM's with pci devices passed through. Most 
prominently VGA cards (without it they won't work any more after a VM shutdown
and you would have to restart the host (you also see the screen isn't reset to 
all black after the VM shuts down.)

So the functionality is valuable, but it would be nice if the implementation 
could be contained within pciback (when a device is being mappend/ unmapped 
from a guest) and the sysfs attribute could be dropped (solves the naming issue)

--
Sander

> Ian.

Re: libxl__device_pci_reset() questions

[Ian Campbell]

On Thu, 2015-02-19 at 16:09 +0100, Sander Eikelenboom wrote:
> Thursday, February 19, 2015, 3:30:52 PM, you wrote:
> 
> > On Thu, 2015-02-19 at 13:59 +0000, Jan Beulich wrote:
> >> All,
> >> 
> >> in the context of someone seeing "The kernel doesn't support reset
> >> from sysfs for PCI device", is my understanding correct that the lack
> >> of error checking in any caller (perhaps intentional) means that any
> >> of the errors logged from this function are really just warnings, i.e.
> >> don't prevent the assignment from taking place?
> 
> > It was a long while ago, but I believe that was the intention, yes.
> 
> >> Furthermore I'm puzzled by the function first thing trying to access
> >> a "do_flr" file supposedly made available by the pciback driver, yet
> >> I can't see either the upstream or the old 2.6.18 driver surfacing
> >> such a file. What am I missing here?
> 
> > I'm not sure, on the basis of
> > http://lists.xen.org/archives/html/xen-devel/2014-06/msg03105.html and 
> > http://lists.xen.org/archives/html/xen-devel/2014-07/msg01108.html I've
> > added Konrad to the CC.
> 
> You seemed to have missed the actual CC, added now

Nope, Konrad's mailman configuration is such that the list copy of the
mail ends up with his CC stripped -- that's really a feature of mailman!

> and also David who nacked these patches.

I didn't know about this.

Oh well, I'll let other fight it out as to what the correct approach is.

> 
> The fundamental issue seems to be:
> A) Does this even have to be sysfs triggered functionality. The most prominent 
>    reason for this construct with pciback deferring it to the toolstack which in 
>    turn invokes a sysfs to pciback again were issues around locking.
>    However vfio/virtio also has the bus/slot reset logic and seems to be able to
>    work around said locking issues.
> 
> B) If it *has* to be done with a sysfs entry, the naming of the sysfs entry 
>    "do_flr" is misleading since it doesn't do an FLR, but hooks up bus/slot 
>    reset.
> 
> That said, i'm running for quite some time now with the mentioned do_flr patches
> from Konrad applied, these fix issues with pci devices not being properly reset 
> when stopping and starting VM's with pci devices passed through. Most 
> prominently VGA cards (without it they won't work any more after a VM shutdown
> and you would have to restart the host (you also see the screen isn't reset to 
> all black after the VM shuts down.)
> 
> So the functionality is valuable, but it would be nice if the implementation 
> could be contained within pciback (when a device is being mappend/ unmapped 
> from a guest) and the sysfs attribute could be dropped (solves the naming issue)
> 
> --
> Sander
> 
> > Ian.
> 
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: libxl__device_pci_reset() questions

[David Vrabel]

On 19/02/15 13:59, Jan Beulich wrote:
> All,
> 
> in the context of someone seeing "The kernel doesn't support reset
> from sysfs for PCI device", is my understanding correct that the lack
> of error checking in any caller (perhaps intentional) means that any
> of the errors logged from this function are really just warnings, i.e.
> don't prevent the assignment from taking place?
> 
> Furthermore I'm puzzled by the function first thing trying to access
> a "do_flr" file supposedly made available by the pciback driver, yet
> I can't see either the upstream or the old 2.6.18 driver surfacing
> such a file. What am I missing here?

"do_flr" is an old XenServer-ism.  libxl doesn't need to try using it.

David

Re: libxl__device_pci_reset() questions

[Jan Beulich]

>>> On 19.02.15 at 15:30, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-02-19 at 13:59 +0000, Jan Beulich wrote:
>> All,
>> 
>> in the context of someone seeing "The kernel doesn't support reset
>> from sysfs for PCI device", is my understanding correct that the lack
>> of error checking in any caller (perhaps intentional) means that any
>> of the errors logged from this function are really just warnings, i.e.
>> don't prevent the assignment from taking place?
> 
> It was a long while ago, but I believe that was the intention, yes.
> 
>> Furthermore I'm puzzled by the function first thing trying to access
>> a "do_flr" file supposedly made available by the pciback driver, yet
>> I can't see either the upstream or the old 2.6.18 driver surfacing
>> such a file. What am I missing here?
> 
> I'm not sure, on the basis of
> http://lists.xen.org/archives/html/xen-devel/2014-06/msg03105.html and 
> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01108.html I've
> added Konrad to the CC.

Konrad?

Thanks, Jan

Re: libxl__device_pci_reset() questions

[Konrad Rzeszutek Wilk]

On Thu, Feb 26, 2015 at 02:28:34PM +0000, Jan Beulich wrote:
> >>> On 19.02.15 at 15:30, <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-02-19 at 13:59 +0000, Jan Beulich wrote:
> >> All,
> >> 
> >> in the context of someone seeing "The kernel doesn't support reset
> >> from sysfs for PCI device", is my understanding correct that the lack
> >> of error checking in any caller (perhaps intentional) means that any
> >> of the errors logged from this function are really just warnings, i.e.
> >> don't prevent the assignment from taking place?
> > 
> > It was a long while ago, but I believe that was the intention, yes.
> > 
> >> Furthermore I'm puzzled by the function first thing trying to access
> >> a "do_flr" file supposedly made available by the pciback driver, yet
> >> I can't see either the upstream or the old 2.6.18 driver surfacing
> >> such a file. What am I missing here?
> > 
> > I'm not sure, on the basis of
> > http://lists.xen.org/archives/html/xen-devel/2014-06/msg03105.html and 
> > http://lists.xen.org/archives/html/xen-devel/2014-07/msg01108.html I've
> > added Konrad to the CC.
> 
> Konrad?

I talked with David about this and his point was that:
 1). If the device advertises it can 'reset' it be better be able to do it.

 2). However there are some that lie. If they exist we should have an quirk for them
     in the PCI layer so that we don't think we have this feature available.

 3). In the case where the PCI device has none of the mechanism to do the reset
     we should provide on via xen-pciback.

The 3) David had a patch which is in XenServer which does the work - it first
figures out whether the PCI device reports as being able to do the reset. If it
is not, then we install our own 'reset' SysFS which will do the bus reset.

However looking at how VFIO and QEMU does it - there is also an check on 
the user-space part - where it decideds in some cases to ignore the 'reset'
from SysFS and do its bus-reset via the VFIO ioctl. I hadn't yet digged
completlely in the code to understand what the logic states it has to
use the VFIO ioctl bus reset instead of the PCI reset mechanism.

> 
> Thanks, Jan
>