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

[Alex Williamson]

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?

> So yes, some guests might have benefitted from VM stop
> on AER but
> 1. the stop happens asynchronously so if guest can't handle
>    errors there's a chance it is already crashed by the time we
>    try to do vm stop

I fully concede that it's asynchronous, bad data can propagate and a
guest crash is one potential outcome.  That's fine, a guest crash
indicates a problem.  A VM stop also indicates a problem.  Potential
lack of a crash or VM stop is the worrisome case.

> 2. it's more of a chance by-product - we never promised
>    guests that VMs would be more robust than bare metal

Does that make it not a regression if we change the behavior?  I
wouldn't exactly call it a chance by-product, perhaps it wasn't the
primary motivation, but it was considered.  Thanks,

Alex

> > > >> Signed-off-by: Dou Liyang <address@hidden>
> > > >> Signed-off-by: Cao jin <address@hidden>
> > > >> ---
> > > >>  hw/vfio/pci.c              | 202 
> > > >> +++++++++++++++++++++++++++++++++++++++++++++
> > > >>  hw/vfio/pci.h              |   2 +
> > > >>  linux-headers/linux/vfio.h |   2 +
> > > >>  3 files changed, 206 insertions(+)
> > > >>
> > > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > >> index 3d0d005..c6786d5 100644
> > > >> --- a/hw/vfio/pci.c
> > > >> +++ b/hw/vfio/pci.c
> > > >> @@ -2432,6 +2432,200 @@ static void vfio_put_device(VFIOPCIDevice 
> > > >> *vdev)
> > > >>      vfio_put_base_device(&vdev->vbasedev);
> > > >>  }
> > > >>  
> > > >> +static void vfio_non_fatal_err_notifier_handler(void *opaque)
> > > >> +{
> > > >> +    VFIOPCIDevice *vdev = opaque;
> > > >> +    PCIDevice *dev = &vdev->pdev;
> > > >> +    PCIEAERMsg msg = {
> > > >> +        .severity = PCI_ERR_ROOT_CMD_NONFATAL_EN,
> > > >> +        .source_id = pci_requester_id(dev),
> > > >> +    };
> > > >> +
> > > >> +    if 
> > > >> (!event_notifier_test_and_clear(&vdev->non_fatal_err_notifier)) {
> > > >> +        return;
> > > >> +    }
> > > >> +
> > > >> +    /* Populate the aer msg and send it to root port */
> > > >> +    if (dev->exp.aer_cap) {    
> > > > 
> > > > Why would we have registered this notifier otherwise?
> > > >     
> > > >> +        uint8_t *aer_cap = dev->config + dev->exp.aer_cap;
> > > >> +        uint32_t uncor_status;
> > > >> +        bool isfatal;
> > > >> +
> > > >> +        uncor_status = vfio_pci_read_config(dev,
> > > >> +                            dev->exp.aer_cap + PCI_ERR_UNCOR_STATUS, 
> > > >> 4);
> > > >> +        if (!uncor_status) {
> > > >> +            return;
> > > >> +        }
> > > >> +
> > > >> +        isfatal = uncor_status & pci_get_long(aer_cap + 
> > > >> PCI_ERR_UNCOR_SEVER);
> > > >> +        if (isfatal) {
> > > >> +            goto stop;
> > > >> +        }    
> > > > 
> > > > Huh?  How can we get a non-fatal error notice for a fatal error?  (and
> > > > why are we saving this to a variable rather than testing it within the
> > > > 'if' condition?
> > > >    
> > > 
> > > Both of these are for the unsure corner cases.
> > > Is it possible that register reading shows a fatal error?
> > > Saving it into a variable just is personal taste: more neat.  
> > 
> > Why are there unsure corner cases?  Shouldn't the kernel have done this
> > check if there was any doubt whether the error was fatal or not?
> > Signaling the user with a non-fatal trigger for a fatal error certainly
> > doesn't make me have much confidence in this code.
> >   
> > > >> +
> > > >> +        error_report("%s sending non fatal event to root port. uncor 
> > > >> status = "
> > > >> +                     "0x%"PRIx32, vdev->vbasedev.name, uncor_status);
> > > >> +        pcie_aer_msg(dev, &msg);
> > > >> +        return;
> > > >> +    }
> > > >> +
> > > >> +stop:
> > > >> +    /* Terminate the guest in case of fatal error */
> > > >> +    error_report("%s: Device detected a fatal error. VM stopped",
> > > >> +          vdev->vbasedev.name);
> > > >> +    vm_stop(RUN_STATE_INTERNAL_ERROR);    
> > > > 
> > > > Shouldn't we use the existing error index if we can't make use of
> > > > correctable errors?
> > > >     
> > > 
> > > Why? If register reading shows it is actually a fatal error, is it the
> > > same as fatal error handler is notified?  what we use the existing error
> > > index for?  
> > 
> > See below.
> >   
> > > >> @@ -2860,6 +3054,8 @@ static void vfio_realize(PCIDevice *pdev, Error 
> > > >> **errp)
> > > >>          }
> > > >>      }
> > > >>  
> > > >> +    vfio_register_passive_reset_notifier(vdev);
> > > >> +    vfio_register_non_fatal_err_notifier(vdev);    
> > > > 
> > > > I think it's wrong that we configure these unconditionally.  Why do we
> > > > care about these unless we're configuring the guest to receive AER
> > > > events?
> > > >     
> > > 
> > > But do we have ways to know whether the guest has AER support? For now,
> > > I don't think so.  
> > 
> > By unconditionally here, I'm referring to not even testing whether the
> > device is in a VM topology where it can receive AER events.  If we
> > cannot signal the device, we don't need these additional triggers and
> > therefore we don't need the aer_cap test in the non-fatal error
> > handler, we can use the existing error index instead.  Enabling these
> > triggers at the point where the guest takes over error handling from
> > the system would be even better.
> >    
> > > If guest don't have AER support, for the worst condition: guest crash,
> > > it is not worse than a sudden power-down.  
> > 
> > Worst case is that a non-fatal error introduces a data corruption which
> > was previously noted via a VM stop (even if asynchronous notification
> > allowed some propagation) and now potentially goes unnoticed.  That's a
> > very big difference.  Thanks,
> > 
> > Alex