Re: [PATCH v11 9/9] vfio: Remove the pcie check for VFIO_PCI_ERR_IRQ_INDEX

[Alex Williamson]

On Tue, 24 Mar 2026 16:26:02 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:

> On Mon, Mar 16, 2026 at 12:15:44PM -0700, Farhan Ali wrote:
> > We are configuring the error signaling on the vast majority of devices and  
> 
> Who is "we"?  If a function configures error signaling, can you
> mention the name?
> 
> > it's extremely rare that it fires anyway. This allows userspace to
> > be notified on errors for legacy PCI devices. The Internal Shared
> > Memory (ISM) device on s390 is one such device.   
> 
> This commit log talks about things that could be done, but doesn't
> actually say what the patch does or what makes it safe and effective,
> and I'm not VFIO-literate enough for it to be clear.
> 
> These pci_is_pcie() tests were added by dad9f8972e04 ("VFIO-AER:
> Vfio-pci driver changes for supporting AER"), so I suppose the
> dad9f8972e04 assumption was that AER was the only error reporting
> mechanism, and AER only exists on PCIe devices?

Yes, that's the conclusion we came to in previous discussions that
Farhan notes in their reply.

> But s390 can report errors for conventional PCI devices, and you want
> VFIO to support that as well?
> 
> Obviously this change needs to be safe for all arches, not just s390.
> I suppose it's safe to report the VFIO_PCI_ERR_IRQ_INDEX info
> everywhere; it's just that it will never be used except on s390?  And
> I guess powerpc, which can get to vfio_pci_core_aer_err_detected() via
> eeh_report_failure().
> 
> It looks like vfio_pci_driver provides vfio_pci_core_err_handlers
> whether the device is conventional PCI or PCIe, and s390 can already
> call vfio_pci_core_aer_err_detected() (the .error_detected() hook) via
> zpci_event_notify_error_detected(), so this patch makes it possible
> for the guest (QEMU, etc) to learn about it?
> 
> > For PCI devices on IBM s390 error
> > recovery involves platform firmware and notification to operating system
> > is done by architecture specific way. So the ISM device can still be
> > recovered when notified of an error.  
> 
> I guess this error recovery part would be done by the guest ISM
> driver, triggered when when something like QEMU receives the eventfd
> signal from vfio_pci_core_aer_err_detected()?
> 
> > Reviewed-by: Julian Ruess <julianr@xxxxxxxxxxxxx>
> > Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>  
> 
> I don't maintain VFIO, so I'm just kibbitzing here.  Hopefully Alex
> will chime in.

It's the previous patch about restoring open state of the device on
.reset_done that gives me more anxiety than just reporting that
non-PCIe (non-AER) devices can report errors.  At worst here, I think
userspace might be wiring an interrupt on a conventional device that
cannot fire, whereas most PCIe device have AER.  The number of
conventional device in use with vfio-pci is probably not enough to
worry about though.

For completeness, I'll note that QEMU sets a "pci_aer" flag based on
whether this error IRQ is exposed, where I think we had intended this
might interact with emulated AER.  However, it never made it that far
and just registers a handler that stops the VM.

Farhan, this and the previous patch should use "vfio/pci:" as their
title prefix.  Thanks,

Alex

> > ---
> >  drivers/vfio/pci/vfio_pci_core.c  | 8 ++------
> >  drivers/vfio/pci/vfio_pci_intrs.c | 3 +--
> >  2 files changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index f1bd1266b88f..cfd9a51cd194 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -786,8 +786,7 @@ static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_typ
> >  			return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
> >  		}
> >  	} else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX) {
> > -		if (pci_is_pcie(vdev->pdev))
> > -			return 1;
> > +		return 1;
> >  	} else if (irq_type == VFIO_PCI_REQ_IRQ_INDEX) {
> >  		return 1;
> >  	}
> > @@ -1163,11 +1162,8 @@ static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev,
> >  	switch (info.index) {
> >  	case VFIO_PCI_INTX_IRQ_INDEX ... VFIO_PCI_MSIX_IRQ_INDEX:
> >  	case VFIO_PCI_REQ_IRQ_INDEX:
> > -		break;
> >  	case VFIO_PCI_ERR_IRQ_INDEX:
> > -		if (pci_is_pcie(vdev->pdev))
> > -			break;
> > -		fallthrough;
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> > index 33944d4d9dc4..64f80f64ff57 100644
> > --- a/drivers/vfio/pci/vfio_pci_intrs.c
> > +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> > @@ -859,8 +859,7 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_core_device *vdev, uint32_t flags,
> >  	case VFIO_PCI_ERR_IRQ_INDEX:
> >  		switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> >  		case VFIO_IRQ_SET_ACTION_TRIGGER:
> > -			if (pci_is_pcie(vdev->pdev))
> > -				func = vfio_pci_set_err_trigger;
> > +			func = vfio_pci_set_err_trigger;
> >  			break;
> >  		}
> >  		break;
> > -- 
> > 2.43.0
> >