Re: [Qemu-devel] [PATCH] vfio-pci: add a switch to disable PCI AER

[Alex Williamson]

On Mon, 2014-12-15 at 16:05 +0100, Peter Lieven wrote:
> AER is meant to let a device driver recover from
> errors discovered on the PCIe bus. However, the
> current implementation of vfio-pci does not distingish
> between correctable or uncorrectalbe as well as fatal
> vs. non-fatal errors. Any kind of error can trigger the
> error correction interrupt and cause all vServers to
> switch to RUN_STATE_INTERNAL_ERROR.
> 
> I have observed correctable non-fatal errors on a
> PCI root hub which where then propagated to all vServers
> on this root hub causing them to shut down.
> 
> I added this switch to be able to ignore AER interrupts
> until a proper interface to propagate the error type from
> kernel to qemu is there. That would be the old behaviour
> of pci-assign or a host kernel not supporting AER.

I don't think it's been proven that the kernel is signaling on all the
error types claimed here.  Our QE has certainly done testing on
corrected errors, the question is whether uncorrected, non-fatal errors
are getting through.  If they are and if those errors should not be
signaled, I'd rather fix it in the kernel than require users to add
obscure device parameters to their VMs.  Thanks,

Alex

> Signed-off-by: Peter Lieven <address@hidden>
> ---
>  hw/misc/vfio.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index fd318a1..b479708 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -217,6 +217,8 @@ typedef struct VFIODevice {
>      uint32_t features;
>  #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>  #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
> +#define VFIO_FEATURE_ENABLE_PCI_AER_BIT 1
> +#define VFIO_FEATURE_ENABLE_PCI_AER (1 << VFIO_FEATURE_ENABLE_PCI_AER_BIT)
>      int32_t bootindex;
>      uint8_t pm_cap;
>      bool reset_works;
> @@ -4025,7 +4027,13 @@ static int vfio_get_device(VFIOGroup *group, const 
> char *name, VFIODevice *vdev)
>          DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure: %m\n");
>          ret = 0;
>      } else if (irq_info.count == 1) {
> -        vdev->pci_aer = true;
> +        vdev->pci_aer = !!(vdev->features & VFIO_FEATURE_ENABLE_PCI_AER);
> +        if (!vdev->pci_aer) {
> +            error_report("vfio: %04x:%02x:%02x.%x "
> +                         "Ignoring error recovery interrupts for the device",
> +                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> +                         vdev->host.function);
> +        }
>      } else {
>          error_report("vfio: %04x:%02x:%02x.%x "
>                       "Could not enable error recovery for the device",
> @@ -4381,6 +4389,9 @@ static Property vfio_pci_dev_properties[] = {
>                         intx.mmap_timeout, 1100),
>      DEFINE_PROP_BIT("x-vga", VFIODevice, features,
>                      VFIO_FEATURE_ENABLE_VGA_BIT, false),
> +    DEFINE_PROP_BIT("pci-aer", VFIODevice, features,
> +                    VFIO_FEATURE_ENABLE_PCI_AER, true),
> +
>      /*
>       * TODO - support passed fds... is this necessary?
>       * DEFINE_PROP_STRING("vfiofd", VFIODevice, vfiofd_name),