Re: [PATCH vfio] vfio/pds: Rework and simplify reset flows

[Brett Creeley <bcreeley@amd.com>]

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@amd.com>
>> 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