Re: [PATCH v14 4/7] s390/pci: Store PCI error information for passthrough devices

[Farhan Ali]

On 4/30/2026 12:48 AM, Niklas Schnelle wrote:
> On Wed, 2026-04-29 at 09:48 -0700, Farhan Ali wrote:
> > On 4/29/2026 4:41 AM, Niklas Schnelle wrote:
> > > On Tue, 2026-04-21 at 09:30 -0700, Farhan Ali wrote:
> > > > For a passthrough device we need co-operation from user space to recover
> > > > the device. This would require to bubble up any error information to user
> > > > space.  Let's store this error information for passthrough devices, so it
> > > > can be retrieved later.
> > > >
> > > > We can now have userspace drivers (vfio-pci based) on s390x. The userspace
> > > > drivers will not have any KVM fd and so no kzdev associated with them. So
> > > > we need to update the logic for detecting passthrough devices to not depend
> > > > on struct kvm_zdev.
> > > >
> > > > Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> > > > Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> > > > ---
> > > >    arch/s390/include/asm/pci.h      |  30 ++++++++
> > > >    arch/s390/pci/pci.c              |   1 +
> > > >    arch/s390/pci/pci_event.c        | 117 +++++++++++++++++--------------
> > > >    drivers/vfio/pci/vfio_pci_zdev.c |   9 ++-
> > > >    4 files changed, 105 insertions(+), 52 deletions(-)
> > > --- snip ---
> > > > +
> > > > +void zpci_start_mediated_recovery(struct zpci_dev *zdev)
> > > > +{
> > > > +	guard(mutex)(&zdev->pending_errs_lock);
> > > > +	zdev->pending_errs.mediated_recovery = true;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(zpci_start_mediated_recovery);
> > > > +
> > > > +void zpci_stop_mediated_recovery(struct zpci_dev *zdev)
> > > > +{
> > > > +	struct pci_dev *pdev = NULL;
> > > > +
> > > > +	guard(mutex)(&zdev->pending_errs_lock);
> > > > +	zdev->pending_errs.mediated_recovery = false;
> > > > +	pdev = pci_get_slot(zdev->zbus->bus, zdev->devfn);
> > > > +	if (zdev->pending_errs.count)
> > > > +		pr_info("%s: Unhandled PCI error events count=%d",
> > > > +			pci_name(pdev), zdev->pending_errs.count);
> > > Sashiko notes a possible ABBA locking issue here between
> > > pending_errs_lock and the pci_bus_sem inside pci_get_slot(). I wonder
> > > if that would also be visible with lockdep? Also Sashiko notes that
> > > zdev->zbus->bus could be NULL I don't think this is possible at the
> > > current callsites via vfio-pci though. Similarly I don't think
> > > pci_get_slot() can really be NULL at the current call sites. This makes
> > > me wonder however, would it maybe be cleaner to pass a struct pci_dev
> > > here so you don't need the pci_get_slot() at all? Both
> > > vfio_pci_zdev_open_device() and vfio_pci_zdev_close_device() have that
> > > readily available via vdev->pdev.
> > The pdev here was meant for helpful error message. On second thought
> > maybe removing the pdev usage, and using the fid would be better?
> Either that or maybe just return the number of left over errors and do
> the print in the caller?
>
> > > > +	memset(&zdev->pending_errs, 0, sizeof(struct zpci_ccdf_pending));
> > > > +	pci_dev_put(pdev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(zpci_stop_mediated_recovery);
> > > > +
> > > >    static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev,
> > > >    							 struct pci_driver *driver)
> > > >    {
> > > > @@ -175,7 +190,8 @@ static pci_ers_result_t zpci_event_do_reset(struct pci_dev *pdev,
> > > >     * and the platform determines which functions are affected for
> > > >     * multi-function devices.
> > > >     */
> > > > -static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
> > > > +static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev,
> > > > +							  struct zpci_ccdf_err *ccdf)
> > > >    {
> --- snip ---
> > > >    
> > > > +	scoped_guard(mutex, &zdev->pending_errs_lock) {
> > > > +		if (zdev->pending_errs.mediated_recovery) {
> > > > +			pr_info("%s: Leaving recovery of pass-through device to user-space\n",
> > > > +				pci_name(pdev));
> > > > +			ers_res = PCI_ERS_RESULT_RECOVERED;
> > > > +			status_str = "in progress";
> > > > +			goto out_unlock;
> > > > +		}
> > > > +	}
> > > > +
> > > Sashiko notes that mixing goto unlock and lock guards within one
> > > function is discouraged. Here it's not that hard to read since it is
> > > just a scoped guard but I think we should still not mix it.
> > >
> > > However if we also convert the device_lock() to a guard lock here the
> > > goto would still make sense to go to the zpci_report_status() and that
> > > is still a bit odd with guarded locks. So I think an alternative simple
> > > fix, that makes this overall cleaner too, is to put the whole
> > > scoped_guard() block above into a helper function then you can do the
> > > goto out_unlock on that helper returning PCI_ERS_RESULT_RECVOERED in
> > > line with e.g. zpci_event_notify_error_detected(). That way you don't
> > > need to touch existing locks and you get to keep the guard locking.
> > How about changing it to mutex_lock/mutex_unlock? I think the block is
> > small enough that it shouldn't be too confusing. Moving to a function
> > opens up the possibility of using a stale value for mediated_recovery.
> I'm probably missing something but I don't see how moving it to a
> helper function changes whether the mediated_recovery can be stale?
> You'd still use the same pending_errs_lock and passed in struct
> zpci_dev * then you just return PCI_ERS_RESULT_RECOVERED if mediated
> recovery is in effect? And you can do the pr_info() at the caller so
> you don't need to pass in the pdev too.

I think Alex raised a concern about a similar approach in a previous 
version[1]. IIUC there could be a device open/close when we check the 
mediated_recovery and return PCI_ERS_RESULT_RECOVERED to when execute 
the goto. So i think its safer to do this with the pending_errs lock held.

[1]https://lore.kernel.org/all/20260407093814.3d95263a@xxxxxxxxxxx/

Thanks

Farhan