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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3f30d63a-0919-44a8-b05c-60d130ae1dcd@amd.com>
Date: Mon, 5 Feb 2024 09:25:47 -0800
From: Brett Creeley <bcreeley@....com>
To: "Tian, Kevin" <kevin.tian@...el.com>,
 Brett Creeley <brett.creeley@....com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
 "shameerali.kolothum.thodi@...wei.com"
 <shameerali.kolothum.thodi@...wei.com>,
 "yishaih@...dia.com" <yishaih@...dia.com>, "jgg@...pe.ca" <jgg@...pe.ca>
Cc: "shannon.nelson@....com" <shannon.nelson@....com>
Subject: Re: [PATCH vfio] vfio/pds: Rework and simplify reset flows

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