On 2/4/2024 10:58 PM, Tian, Kevin wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
From: Brett Creeley <brett.creeley@xxxxxxx>
Sent: Saturday, January 27, 2024 2:32 AM
The current logic for handling resets based on
whether they were initiated from the DSC or
host/VMM is slightly confusing and incorrect.
The incorrect behavior can cause the VF device
to be unusable on the destination on failed
migrations due to incompatible configurations.
Fix this by setting the state back to
VFIO_DEVICE_STATE_RUNNING when an FLR is
triggered, so the VF device is put back in
an "initial" pre-configured state after failures.
any reason for putting short lines (<50 chars) in commit msg?
No, I will make the lines longer in the next commits.
Also, while here clean-up the reset logic to
make the source of the reset more obvious.
as a fix the a 'Fixed' tag is preferred and CC stable
also separate the real fix from the cleanup so stable kernel doesn't need
to backport unnecessary code.
Sure, I can split this into 2 changes as you suggested.
btw the commit msg is not clear to me. It says fixing the problem
by setting the state to _ERROR for the DSC path and to _RUNNING for
the FLR path.
But looks it's already such case with old code:
pds_vfio_recovery()
pds_vfio->deferred_reset = true;
pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_ERROR;
pds_vfio_reset()
pds_vfio->deferred_reset = true;
pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_RUNNING;
pds_vfio_state_mutex_unlock()
if (pds_vfio->deferred_reset) {
...
pds_vfio->state = pds_vfio->deferred_reset_state;
...
}
it's same as what this patch does:
pds_vfio_recovery()
pds_vfio->deferred_reset_type = PDS_VFIO_DEVICE_RESET;
pds_vfio_reset()
pds_vfio->deferred_reset_state = PDS_VFIO_HOST_RESET;
pds_vfio_state_mutex_unlock()
if (pds_vfio->deferred_reset) {
...
if (pds_vfio->deferred_reset_type == PDS_VFIO_HOST_RESET)
pds_vfio->state = VFIO_DEVICE_STATE_RUNNING;
else
pds_vfio->state = VFIO_DEVICE_STATE_ERROR;
...
}
looks the actual functional difference is from below change:
@@ -32,13 +32,14 @@ void pds_vfio_state_mutex_unlock(struct
pds_vfio_pci_device *pds_vfio)
mutex_lock(&pds_vfio->reset_mutex);
if (pds_vfio->deferred_reset) {
pds_vfio->deferred_reset = false;
- if (pds_vfio->state == VFIO_DEVICE_STATE_ERROR) {
- pds_vfio_put_restore_file(pds_vfio);
- pds_vfio_put_save_file(pds_vfio);
+ pds_vfio_put_restore_file(pds_vfio);
+ pds_vfio_put_save_file(pds_vfio);
above two are changed from conditional to always.
+ if (pds_vfio->deferred_reset_type == PDS_VFIO_HOST_RESET)
{
+ pds_vfio->state = VFIO_DEVICE_STATE_RUNNING;
+ } else {
pds_vfio_dirty_disable(pds_vfio, false);
and this is now only for the DSC path.
need a better explanation here.
I will clean up this patch by separating it into 2 patches and improving
the commit descriptions before sending a v2.
Thanks for the review,
Brett
