On 9/30/2026 8:32 AM, Matthew Rosato wrote:
--- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -1505,6 +1505,8 @@ static void s390_pci_device_reset(DeviceState *dev) return; case ZPCI_FS_STANDBY: break; + case ZPCI_FS_ERROR: + break; default: pbdev->fh &= ~FH_MASK_ENABLE; pbdev->state = ZPCI_FS_DISABLED; @@ -1517,6 +1519,11 @@ static void s390_pci_device_reset(DeviceState *dev) } else if (pbdev->summary_ind) { pci_dereg_irqs(pbdev); } + + if (pbdev->state == ZPCI_FS_ERROR) { + s390_pci_reset(pbdev); + }I know it was mentioned in the prior version, so apologies for not chiming in til now. But you really need a comment block explaining why it's OK to reset the device yet leave it in the ZPCI_FS_ERROR state after the fact. That's confusing.
I can add a comment block explaining why we don't change the state. But my understanding is the guest needs to drive the change of the function state.
You mentioned in the last version it's because this happens on the disable path, and there yes we will set the device to ZPCI_FS_DISABLED right after this -- but what about any other case where we drive this reset path (e.g. subsystem reset, reboot, and the ISM-specific extra paths) Note that the non-error cases are ensuring we either leave this function with the device listed as disabled or standby/reserved. Would there be harm in, after performing the reset, doing pbdev->fh &= ~FH_MASK_ENABLE; pbdev->state = ZPCI_FS_DISABLED; for the ZPCI_FS_ERROR case now?
One reason I didn't change the state was also because per my understanding of architecture we need the guest to clear the error state. This could be done via a mpcifc instruction (oc=7 ZPCI_MOD_FC_RESET_ERROR) or through a CLP set pci function enable/disable cycle. So if we leave the device in disabled state after a reset without the guest driving any change, I think it might present a wrong view to the guest. I am open to suggestions if you think it would be more appropriate to keep the device in disabled state.
Thanks Farhan
Consider that: 1) For a guest-initiated disable, we already checked those were valid before reaching this point. 2) For all other reset cases, if the device were not in error/reserved/standby we were leaving here with the device disabled before this patch. If the device happened to be in error, now you fix it up but isn't the proper fix state now 'disabled' to match what an enabled device would have been? Thanks, Matt
