Re: [PATCH v4 01/10] PCI: Avoid saving error values for config space

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Wed, 2025-10-08 at 20:14 +0200, Lukas Wunner wrote:
> On Wed, Oct 08, 2025 at 10:56:35AM -0700, Farhan Ali wrote:
> > On 10/8/2025 6:34 AM, Lukas Wunner wrote:
> > > I'm not sure yet.  Let's back up a little:  I'm missing an
> > > architectural description how you're intending to do error
> > > recovery in the VM.  If I understand correctly, you're
> > > informing the VM of the error via the ->error_detected() callback.
> > > 
> > > You're saying you need to check for accessibility of the device
> > > prior to resetting it from the VM, does that mean you're attempting
> > > a reset from the ->error_detected() callback?
> > > 
> > > According to Documentation/PCI/pci-error-recovery.rst, the device
> > > isn't supposed to be considered accessible in ->error_detected().
> > > The first callback which allows access is ->mmio_enabled().
> > > 
> > 
> > The ->error_detected() callback is used to inform userspace of an error. In
> > the case of a VM, using QEMU as a userspace, once notified of an error QEMU
> > will inject an error into the guest in s390x architecture specific way [1]
> > (probably should have linked the QEMU series in the cover letter). Once
> > notified of the error VM's device driver will drive the recovery action. The
> > recovery action require a reset of the device and on s390x PCI devices are
> > reset using architecture specific instructions (zpci_device_hot_reset()).
> 
> According to Documentation/PCI/pci-error-recovery.rst:
> 
>    "STEP 1: Notification
>     --------------------
>     Platform calls the error_detected() callback on every instance of
>     every driver affected by the error.
>     At this point, the device might not be accessible anymore, [...]
>     it gives the driver a chance to cleanup, waiting for pending stuff
>     (timers, whatever, etc...) to complete; it can take semaphores,
>     schedule, etc... everything but touch the device."
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> And yet you're touching the device by trying to reset it.
> 
> The code you're introducing in patch [01/10] only becomes necessary
> because you're not following the above-quoted protocol.  If you
> follow the protocol, patch [01/10] becomes unnecessary.
> 

I agree with your point above error_detected() should not touch the
device. My understanding of Farhan's series though is that it follows
that rule. As I understand it error_detected() is only used to inject
the s390 specific PCI error event into the VM using the information
stored in patch 7. As before vfio-pci returns
PCI_ERS_RESULT_CAN_RECOVER from error_detected() but then with patch 7
the pass-through case is detected and this gets turned into
PCI_ERS_RESULT_RECOVERED and the rest of the s390 recovery code gets
skipped. And yeah, writing it down I'm not super happy with this part,
maybe it would be better to have an explicit
PCI_ERS_RESULT_LEAVE_AS_IS.

Either way this leaves the PCI device in the error state just like for
the host the platform leaves the device in the error state. Up until
this point even if the VM/QEMU tried to do a reset already it would get
blocked on at least the zdev->state_lock until the recovery code is
done. Only after the VM would run its recovery code and with that drive
the reset.

> > > I also don't quite understand why the VM needs to perform a reset.
> > > Why can't you just let the VM tell the host that a reset is needed
> > > (PCI_ERS_RESULT_NEED_RESET) and then the host resets the device on
> > > behalf of the VM?

The reason is that we want the behavior from the VMs perspective to
follow s390's PCI error event handling architecture. In this model
however there is no mechanism to synchroniously ask the OS "An error
occurred would you want the device reset?" or to tell it that we as
hypervisor already unblocked MMIO/DMA or performed a reset. So instead
our idea was that we just do the error_detected() part in the host's
recovery code and then leave the device as is driving the rest from the
guest.

Thanks,
Niklas