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

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

[Peter Lieven]

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.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 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),
-- 
1.7.9.5

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 <pl@kamp.de>
> ---
>  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),

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

[Peter Lieven]

On 15.12.2014 16:19, Alex Williamson wrote:
> 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,

As far as I understand it is the task of the vfio-pci kernel driver
to signal if it has recovered or not by the return code of the
error handler. Currently it signals the VM unconditionally in that
error handler.

I added this switch because it fixes my and potentials others'
issues. Fixing in the kernel sounds better, but in my case
its easier to add a fix to qemu than compiling or obtaining
a new kernel.

Peter


>
> Alex
>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   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),
>
>

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

[Alex Williamson]

On Mon, 2014-12-15 at 16:28 +0100, Peter Lieven wrote:
> On 15.12.2014 16:19, Alex Williamson wrote:
> > 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,
> 
> As far as I understand it is the task of the vfio-pci kernel driver
> to signal if it has recovered or not by the return code of the
> error handler. Currently it signals the VM unconditionally in that
> error handler.
> 
> I added this switch because it fixes my and potentials others'
> issues. Fixing in the kernel sounds better, but in my case
> its easier to add a fix to qemu than compiling or obtaining
> a new kernel.

You can also just boot with pci=noaer if you just want a quick and dirty
fix for the problem.  Let's not forget that to get this problem you need
to have hardware that causes an AER error, in this case an ACS violation
which probably means that on reset the device is briefly sending
transactions with a bogus requester ID that falls outside of the bus
range below this root port.  If a device was doing this maliciously we'd
call it spoofing and perhaps be thankful that we're being notified about
it.

AER is not an area that I've spent much time, but the intention of the
contributed vfio AER code was to only signal on fatal errors.  I'd
certainly appreciate patches to make that the case if it's not already.
Thanks,

Alex

> >
> >> Signed-off-by: Peter Lieven <pl@kamp.de>
> >> ---
> >>   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),
> >
> >
> 
> 
>