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

[Lukas Wunner <lukas@wunner.de>]

On Thu, Oct 09, 2025 at 11:12:03AM +0200, Niklas Schnelle wrote:
> On Wed, 2025-10-08 at 20:14 +0200, Lukas Wunner wrote:
> > 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.

Thanks, that's the high-level overview I was looking for.

It would be good to include something like this at least
in the cover letter or additionally in the commit messages
so that it's easier for reviewers to connect the dots.

I was expecting paravirtualized error handling, i.e. the
VM is aware it's virtualized and vfio essentially proxies
the pci_ers_result return value of the driver (e.g. nvme)
back to the host, thereby allowing the host to drive error
recovery normally.  I'm not sure if there are technical
reasons preventing such an approach.

If you do want to stick with your alternative approach,
maybe doing the error handling in the ->mmio_enabled() phase
instead of ->error_detected() would make more sense.
In that phase you're allowed to access the device,
you can also attempt a local reset and return
PCI_ERS_RESULT_RECOVERED on success.

You'd have to return PCI_ERS_RESULT_CAN_RECOVER though
from the ->error_detected() callback in order to progress
to the ->mmio_enabled() step.

Does that make sense?

Thanks,

Lukas