>> >> 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 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
