On 9/30/26 5:50 PM, Matthew Rosato wrote: > >>> >>> 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/ > > That's an interesting one. This series doesn't change that path, so it > just resets the error state but we don't actually take any action, so I > guess we just hope the device will now work and just didn't need > recovery on the host or a disable/enable cycle. > >> 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. > > I get what you're saying, but the reality is that this reset code as > implemented is going to get driven in a few ways: > > 1) During the guest-initiated disable, quite likely in response to the > PEC we injected if the device is in ZPCI_FS_ERROR. Here the guest is > responsible for clearing their error state as you say, but they're > already on the way to doing that when you come through this path. > They're doing the disable now, next step will be the enable. > > 2) Subsystem reset that is potentially running in parallel with the > guest recovery action (or the guest is not taking recovery action at > all). Here I don't believe the guest is responsible for clearing the > error state actually -- either the device remains in an error state over > the course of the subsystem reset (the way it used to work, because we > had no way of fixing it) or, if we already did host recovery, we have
Just realized I got that part wrong -- before this patch we would also just clear the error state and mark the device disabled during reset. > reason to believe the device will now work and so it is free to be set > to disabled because the guest will no longer have the prior context to > believe it is responsible for clearing any error state -- it thinks it's > starting 'from the beginning' due to the subsystem reset. This is > exactly why we disable the device in the reset path for an enabled > device (that isn't in an error state) today, because that is the initial > state the guest expects. > > To say it another way, when the subsystem reset occurs it will trigger > the device reset for all devices on the bus (plus a special direct > invocation a bit earlier for ISM devices). And the expectation after a > subsystem reset is that the devices will be in a state such that the > guest can clp enable them. This is the case that I am concerned you are > missing with this implementation -- if we hit this path during reboot > (and we cannot assume the guest issued its own CLP disable during the > reboot) then the next thing the guest will try is a CLP enable during > boot, which I think will fail even though you will have already done the > host recovery action. And I actually think the failure will be not due > to the state == ZPCI_FS_ERROR (because enable ignores that) but actually > because you never disabled the handle -- so the guest will get > CLP_RC_SETPCIFN_FHOP. > > AFAICT the only way the guest will be able to clear that up and force > the device to be usable again is to do a disable/enable cycle AFTER the > subsystem reset (or use the mpcifc) to clear the handle. That doesn't > sound right either. Yes, this is likely a small window (subsystem reset > at the same time a PEC comes from the host) but I think it highlights > the problem with leaving the device in ZPCI_FS_ERROR / handle enabled. > > If you think placing the device into a disabled state straight away > won't cover architecture, we could consider a new state to track when a > device is in error state (ZPCI_FS_ERROR) vs a device that has been reset > and is ready for the guest to clear the error state (ZPCI_FS_RECOVERED > or something)? The disable path can go straight from > RECOVERED->DISABLED, but then the subsystem_reset path could have logic > that ensures all devices that are ZPCI_FS_RECOVERED get moved to > ZPCI_FS_DISABLED (and their handle masked to disabled) before the guest > starts running again. Any that are in STANDBY/RESERVED/ERROR stay that > way, and everything else should already be DISABLED. > > I'm not sure if that overhead buys us anything though vs short-pathing > right to 'disabled', because I can't think of a case where we reset the > device on a path where the guest isn't either already doing the > error-state-clearing action, is no longer responsible for it, or the > device is being removed from the guest configuration. > > Thanks, > Matt >
