vfio-pci: add a switch to disable PCI AER
PATCH 1/1 vfio-pci: add a switch to disable PCI AER
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.
--- hw/misc/vfio.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
hw/misc/vfio.c +12 -1
@@ -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),
Signature · 1 line
1.7.9.5
Quoted 8 lines · 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.
Quoted 8 lines
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
Quoted 43 lines
Signed-off-by Peter Lieven <[email protected]>--- hw/misc/vfio.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)hw/misc/vfio.c +12
-1@@ -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),
Quoted 17 lines · Alex Williamson
Quoted 8 lines · 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.Quoted 8 lines
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.
Quoted 6 lines
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
Quoted 48 lines
Alex
Quoted 43 lines
Signed-off-by Peter Lieven <[email protected]>--- hw/misc/vfio.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)hw/misc/vfio.c +12
-1@@ -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),
Quoted 30 lines · Peter Lieven
Quoted 17 lines · Alex Williamson
Quoted 8 lines · 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.Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 4 lines
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
Quoted 49 lines
Quoted 46 lines
Quoted 44 lines
Signed-off-by Peter Lieven <[email protected]>--- hw/misc/vfio.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)hw/misc/vfio.c +12
-1@@ -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),