Received-SPF: pass (google.com: domain of linux-kernel+bounces-53098-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <3f30d63a-0919-44a8-b05c-60d130ae1dcd@amd.com>
Date: Mon, 5 Feb 2024 09:25:47 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH vfio] vfio/pds: Rework and simplify reset flows
To: "Tian, Kevin" <kevin.tian@intel.com>,
 Brett Creeley <brett.creeley@amd.com>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
 "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
 "shameerali.kolothum.thodi@huawei.com"
 <shameerali.kolothum.thodi@huawei.com>,
 "yishaih@nvidia.com" <yishaih@nvidia.com>, "jgg@ziepe.ca" <jgg@ziepe.ca>
Cc: "shannon.nelson@amd.com" <shannon.nelson@amd.com>
References: <20240126183225.19193-1-brett.creeley@amd.com>
 <BL1PR11MB52712B162AE5FEC2E24614F98C472@BL1PR11MB5271.namprd11.prod.outlook.com>
Content-Language: en-US
From: Brett Creeley <bcreeley@amd.com>
In-Reply-To: <BL1PR11MB52712B162AE5FEC2E24614F98C472@BL1PR11MB5271.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?bjBYb3VLWUU2VEhUODBOMUI0eTN3ZFJCUGpsaFRqY2xSeGpZRkk1RHJBQmt1?=
 =?utf-8?B?WUtSdFRpeXJRc0ZqRm5xenBSMVJ3WWpHTjJxRVJvL0hHR2hYM3VCZ2VoUDEy?=
 =?utf-8?B?WlV5LytKNlhWeHdyVlVtR3FwTW1XM1pta2lPNjVVYmdTeVByczgvcEZ2c3cr?=
 =?utf-8?B?TWsrRWJQdlJONUlqNU5ZM3NGZDFHeWJtK0xYdE95QkdSZEhRZFhhbXZDdjZI?=
 =?utf-8?B?MkNYVjFOVFlmQncvb3lEZmRPSldEc2JBNEZMM1lJT0lXMXFVUHdOS2hhaWRL?=
 =?utf-8?B?dDJDRDlLYjZBZTZ1dkY4MGcxOW9YYzAvK3BKN3FRU2FQVWRac1FxSHhPSkxt?=
 =?utf-8?B?UDRjaWYvZVo5RHVTbitSczlrZGN4cERVZ294OW42Vm5VcytYdXNwK3ZzVDdt?=
 =?utf-8?B?QUhKRjRkOVlWVnBSejY2L3hNa0JyVDk0Sjl6b1puTnZYWW9rbi9KVUFsbis5?=
 =?utf-8?B?VWZCUnhybWJPV1IwRmsveFFXRURzUTNaNC9oUHdMdW9NUGl6cDlSRkgyd1Ns?=
 =?utf-8?B?bXY5VUV2N1oxSDJHMmYwTWo2NDNSeFJ4aFhFM1ZXZ1VHM0MxQ2hUQkFpVlpZ?=
 =?utf-8?B?SFhLNjJ6UktRcWJYVDY5dVluOFVXMVg1UDB6WGQ4RGxhdVQrTlJVQTI3VWxv?=
 =?utf-8?B?dkhBZTZSVFVTSzVJM2hqdHR6cCs3UW1OYnVXa2FFYlNBSDlrdkxjRElvOW9z?=
 =?utf-8?B?VUQzSDF1V3BidzV6NCtZMVhvejQ0a0RjdFdGSG1oS2hoT01vRFE2alR4d3NZ?=
 =?utf-8?B?TWFISnVGaW1tWGVla2NRSW1oNkNKNEQ3VUNCN0lGVGhIWDhRaFVqNldiaXky?=
 =?utf-8?B?RUh5RmtFem4yblU5eDBrSGovbytYUzJrbTRiMjczMGZESzU0Yk9PQjBhbDBq?=
 =?utf-8?B?b0hGUktEUzFJb3NNblBUZm9qcmVjdEU3eW05MkszOTlLbGZUWGFYS1pNeXVx?=
 =?utf-8?B?UEdPc3FJb0lVM2hIZE9KaFd1dXQ1dHpMN3oxYWtEUW1TUTNLUE1taitZbi9X?=
 =?utf-8?B?SitWelJmYUY5N2pkRFNWelV2ZzdvYkJzbTNmUndjRzRMSWRLL1lvSVU5ZG5V?=
 =?utf-8?B?RFRFaHF4bTVkMitpN0F1bjdXQ05UVElHbVFyM3VPTFJiZlFBZFhYUEVLWjJP?=
 =?utf-8?B?TkFRMU5ZUW0vbXhDeXR5K2hya2dnNkt2VDdYY0xKcFhVUUdwNFBSUzI2WVV6?=
 =?utf-8?B?dlBFejEyVncyeVYvZys3YzRhajBFOWtRN01FSzFLcC9TL0NwcU04UGZnQktv?=
 =?utf-8?B?K083b0VDQU1MUm9mcGF1UG92dENWKzBkUXVEWjU3d0REZXNvc2FJQ3NsMjll?=
 =?utf-8?B?amNVWUsrREk2cHIzSHlzL2lwSmh6bmxJMUZONW42bisxTWpWb0lKL1NuL2E5?=
 =?utf-8?B?NXZWODA4WWdxKzdHL1Jkd0JSWUtIcHg5Mk9HbWg1MHh4bkN6YUtLYmc3ck5U?=
 =?utf-8?B?MDR4bDFyQXQ1dHBHTDdxeDdHczdKZWtNeXdQNExNRDNFeGhhdC9JbWE2TUNS?=
 =?utf-8?B?VmhGT0hUYVFrdkFvVVl0N3lIS2IvdkRMYlBGVzhBZTNLdEx3NE5kZXRRY2hM?=
 =?utf-8?B?QnBUWGNCUmdvU0I2QjVvd1JCaUZ6a3JydzBsVTE5Z2dtajYxNHNuZUl1VlZo?=
 =?utf-8?B?bXlORTlwQWcraFZCVDVnRVFHUElHcWlNL1Y5TFBSS1ptVWcyaHNqNXk3R1Rz?=
 =?utf-8?B?YTJQdzU5dXVSbTBEWE9UWEVZNU85bXI3ZWl3dXZMN0d4NU4yZ25QZ3d2MmFB?=
 =?utf-8?B?bGxrb2YxTmdYNW1nMFJLV3JsQXpTUjV3UWJmNkRzQVY0OEtnKzdYRlBUTWZQ?=
 =?utf-8?B?TDlHZ28vQnFnSWlqTzRDRm5TbUNESmwwc2p1NTFYUm1ZNGpYajA5SGRaUG9u?=
 =?utf-8?B?bUZJdVFWYTdqNGtCOTZ3a2RrclFobndkYXdHOUliWm9OSSs5cDkweG43TEE3?=
 =?utf-8?B?eHFqVkg5ZnBhVHY2MkluQkxrUzFKYzFrbU1WY2lVWVJ2OTRFQ0xHK2VEdFdU?=
 =?utf-8?B?ZkxMeU1VS1RIYTRUWmFieGU2TUw4elBvRkxuMEE2Rnp4ZTgvVlpaK3NHL3dI?=
 =?utf-8?B?THM1UE5yQUt4SzE1S3h1YUNwVEUzOHVUcEpycjJMLys1WDgwbXFGaGdWZk53?=
 =?utf-8?Q?4Mmeer8Ye0B7azEtSBcxXDruH?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8969fb84-9495-4c32-4f09-08dc266f7ee9
X-MS-Exchange-CrossTenant-AuthSource: PH0PR12MB7982.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2024 17:25:49.3344
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 6iV426c40MM9AvaS1UkYEMhPo3mb9oEhd3OcFmyqOFyQ9T3phfAMUuBegutIYsMiiyhlLOGIhdf+d3YTy6KMMQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB7666

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