qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 3/3] vfio-pci: process non fatal error of AER


From: Alex Williamson
Subject: Re: [Qemu-devel] [PATCH v3 3/3] vfio-pci: process non fatal error of AER
Date: Tue, 25 Apr 2017 18:06:45 -0600

On Tue, 25 Apr 2017 23:32:52 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Tue, Mar 28, 2017 at 08:55:04PM -0600, Alex Williamson wrote:
> > On Wed, 29 Mar 2017 02:59:34 +0300
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >   
> > > On Tue, Mar 28, 2017 at 10:12:25AM -0600, Alex Williamson wrote:  
> > > > On Tue, 28 Mar 2017 21:49:17 +0800
> > > > Cao jin <address@hidden> wrote:
> > > >     
> > > > > On 03/25/2017 06:12 AM, Alex Williamson wrote:    
> > > > > > On Thu, 23 Mar 2017 17:09:23 +0800
> > > > > > Cao jin <address@hidden> wrote:
> > > > > >       
> > > > > >> Make use of the non fatal error eventfd that the kernel module 
> > > > > >> provide
> > > > > >> to process the AER non fatal error. Fatal error still goes into the
> > > > > >> legacy way which results in VM stop.
> > > > > >>
> > > > > >> Register the handler, wait for notification. Construct aer message 
> > > > > >> and
> > > > > >> pass it to root port on notification. Root port will trigger an 
> > > > > >> interrupt
> > > > > >> to signal guest, then guest driver will do the recovery.      
> > > > > > 
> > > > > > Can we guarantee this is the better solution in all cases or could
> > > > > > there be guests without AER support where the VM stop is the better
> > > > > > solution?
> > > > > >       
> > > > > 
> > > > > Currently, we only have VM stop on errors, that looks the same as a
> > > > > sudden power down to me.  With this solution, we have about
> > > > > 50%(non-fatal) chance to reduce the sudden power-down risk.    
> > > > 
> > > > If half of all faults are expected to be non-fatal, then you must have
> > > > some real examples of devices triggering non-fatal errors which can be
> > > > corrected in the guest driver that you can share to justify why it's a
> > > > good thing to enable this behavior.
> > > >     
> > > > > What if a guest doesn't support AER?  It looks the same as a host
> > > > > without AER support. Now I only can speculate the worst condition: 
> > > > > guest
> > > > > crash, would that be quite different from a sudden power-down?    
> > > > 
> > > > Yes, it's very different.  In one case we contain the fault by stopping
> > > > the guest, in the other case we allow the guest to continue operating
> > > > with a known fault in the device which may allow the fault to propagate
> > > > and perhaps go unnoticed.  We have established with the current
> > > > behavior that QEMU will prevent further propagation of a fault by
> > > > halting the VM.  To change QEMU's behavior here risks that a VM relying
> > > > on that behavior no longer has that protection.  So it seems we either
> > > > need to detect whether the VM is handling AER or we need to require the
> > > > VM administrator to opt-in to this new feature.    
> > > 
> > > An opt-in flag sounds very reasonable. It can also specify whether
> > > to log the errors. We have a similar flag for disk errors.  
> > 
> > An opt-in works, but is rather burdensome to the user.
> >    
> > > >  Real hardware has
> > > > these same issues and I believe there are handshakes that can be done
> > > > through ACPI to allow the guest to take over error handling from the
> > > > system.    
> > > 
> > > No, that's only for error reporting IIUC. Driver needs to be
> > > aware of a chance for errors to trigger and be able to
> > > handle them.  
> > 
> > See drivers/acpi/pci_root.c:negotiate_os_control(), it seems that the
> > OSPM uses an _OSC to tell ACPI via OSC_PCI_EXPRESS_AER_CONTROL.  Would
> > that not be a reasonable mechanism for the guest to indicate AER
> > support?  
> 
> I'm not sure - it seems to be designed for firmware that can drive
> AER natively. E.g. if we ever set FIRMWARE_FIRST then linux
> will not set this bit.
> 
> It's also global so doesn't really indicate a given driver
> supports AER.
> 
> Still - would this remove some or all of your concern?

Certainly not all, I have rather deep concerns about where we're going
here.

> Could you outline a set of requirements that can be satisfied
> to make you consider the feature for inclusion?

Like any enhancement, show me that it's useful, show me that we've done
due diligence in researching the problem and solution, show me that
we're not painting ourselves into a corner by only addressing a subset
of the problem, show me that it's been thoroughly tested and reviewed.
Currently, I have little confidence in any of this.  We seem to just be
tossing AER spitballs at the wall hoping one of them sticks.
 
> I tried by writing up
>       vfio/pci: guest error recovery proposal
> back in December and there didn't seem to be any objections, so I am
> quite surprised to see patches implementing that proposal more or less
> verbatim getting rejected.

Are the concerns from the patch review not valid?  I think this is
indicative of the problems we've had throughout this 2+ year
development process, a suggestion is made and it's implemented without
a thorough analysis of the surrounding issues (or even testing), patches
are sent out, issues are found, further suggestions are made, and a new
revision comes out with the same lack of insight.  Rinse, repeat.  It's
effectively writing patches by proxy.
 
> Could you write up a proposal of your own? It shouldn't take
> long but I don't believe progress can be made otherwise.

Why do you make it sound like this should be an easy task?  Clearly the
problem is hard and a cursory proposal only feeds into the process I
mention above.  If someone wants to make a legitimate attempt at
improving this space, I feel like they really need to own the problem
themselves, invest in the research to figure out all the nuances.

> Please note that at least in Linux most driver developers
> test using software error injection. Documentation/PCI/pcieaer-howto.txt
> actually says:
> 
>       4. Software error injection
> 
>       Debugging PCIe AER error recovery code is quite difficult because it
>       is hard to trigger real hardware errors. Software based error
>       injection can be used to fake various kinds of PCIe errors.
> 
> it might be that even though yes, we'd prefer real testing
> we just might have to be satisfied with software injection.

Of course artificial AER injection is the way to go for testing.  I
don't expect someone to stick a wet finger into a system to try to
generate transient hardware failures, but I do expect more testing than
we've seen in the past.

Really, to make progress in this space I think we need to first
determine the actual problem we're trying to solve.  Are non-fatal
errors really a significant problem to tackle on their own or are we
just trying to appease the patch submitter by helping to get anything
AER related upstream?  I don't think that really helps anyone long
term.  So what if we return to full AER support?  We were stuck on bus
resets and how to coordinate host recovery resets vs guest resets. It's
clearly a hard problem and your approach was to avoid it by handling a
sub-class of errors for which bus resets are perhaps unnecessary.  What
if instead of a sub-class of errors, we pick a sub-class of devices for
which there's no such thing as a bus reset.  Aren't SR-IOV VFs really
designed for this use case?  I think with RAS features we're really
targeting the enterprise use cases, where VFs are (or should be) the
predominant class of device.  Would this work?  I don't know, and I
really don't want this to be just another idea that gets caught in the
cyclic rut we're stuck in so far.  If someone wants to pursue this I'd
expect to see some research into how errors are propagated from PF to
VF and certainly testing on relevant hardware.  Thanks,

Alex



reply via email to

[Prev in Thread] Current Thread [Next in Thread]