Re: [PATCH v6 2/2] s390x/pci: Reset a device in error state

[Matthew Rosato <mjrosato@linux.ibm.com>]

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