Received-SPF: pass (google.com: domain of linux-kernel+bounces-52101-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Tian, Kevin" <kevin.tian@intel.com>
To: 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>
Subject: RE: [PATCH vfio] vfio/pds: Rework and simplify reset flows
Thread-Topic: [PATCH vfio] vfio/pds: Rework and simplify reset flows
Thread-Index: AQHaUIYcXfafP7iWiUS0/Oox6wrptLD7WkUQ
Date: Mon, 5 Feb 2024 06:58:51 +0000
Message-ID: <BL1PR11MB52712B162AE5FEC2E24614F98C472@BL1PR11MB5271.namprd11.prod.outlook.com>
References: <20240126183225.19193-1-brett.creeley@amd.com>
In-Reply-To: <20240126183225.19193-1-brett.creeley@amd.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cxa1mYF/Ed4wW/uhgo9EqemgAQ5J6xVKRL3K0JxbVryb3FeN/ygR0XBqwxNe?=
 =?us-ascii?Q?lXeN715jjm078hh4lw8QXakfnN5sG/XczQz51ecc+ayvpaZPpbpqHiaZ+kSi?=
 =?us-ascii?Q?ZPpOPLgbCGmMQFaDWXxCR3DsqEDOwFFQzsiUNVTjul5Q4lh9oatfJtlmzCIN?=
 =?us-ascii?Q?yJYWjlSDNhHnrcxS+B3HeDBkfrblGrQpLu4R+XeqFJ+78vFqma+tXy5mnZx1?=
 =?us-ascii?Q?36zwCwjMcYZHNuZ7RuWAIXjHfeeO1OzfX9WgP0h5vrn965WWw/0/MDLeCggN?=
 =?us-ascii?Q?tLxW3BJ5a0xdm6dXsI4ycch8Qg65cANIMGtVEB+oyNnwL9BNS9BHAWpO5j3x?=
 =?us-ascii?Q?/ITsy00MFoh/6IxJBONSTAyuDukpdjHvfkg4shdC+BgruHTXcgRj6fBANQvl?=
 =?us-ascii?Q?kI7Trys6MMi7vUL6/uBSV21EXPo0IMOZ/b6iGfpZIsPwZTHuua/NN7PqNNp2?=
 =?us-ascii?Q?QOSeQ4n9+J3LEK1TBsjBO3X8WqUENjHDvMKdzpGBzHcyqajE48URM984tzNY?=
 =?us-ascii?Q?9StQ9gMuVE6KiujcwxGYNPssfe/BPlO/ahEorHDmAbEuK4ZaROuH9PyDfALZ?=
 =?us-ascii?Q?n0imsu5UyU6t00IJv1jPFDbaNtiIHRWoyGD4KLQb10GfvooxJdYPsSDrfgN+?=
 =?us-ascii?Q?dpjxlu0QxS5+Xmcp5B10KTi7I8YrRSsnplKFMLbaUeGjW6J2oRLFCib3yOHR?=
 =?us-ascii?Q?r/KtWLwvZGF3fwCujHR6cR8OHFKsRD3i9Fe3FdLfPvyxz6dSv2rq4M3QYB7J?=
 =?us-ascii?Q?Fpg1pcbAvjK0kuOTbCeUrSrQoBrKuR5iLWzRU5Gr6r9snI6a+jCKXKWKgQ3D?=
 =?us-ascii?Q?06s18cwUof72h6gNcPlApVUpFeY8pYG6Ln+INshNUDRwyAK5LDVPHnx0QhL+?=
 =?us-ascii?Q?27h185Q/IG7klgDB8U4IT7/ngE9JBgcvs3l9nGFszKFB4v/80PHZZfBJUfWI?=
 =?us-ascii?Q?Nmi++Awa0Klqrib80s6GbTDxCeu1UVguSEsvI+LOzTpEddfXl1xAHqbKrFP6?=
 =?us-ascii?Q?VAwQl7YOSCoN+5IQuzueIC0hmxqWNFFAHKj2JtIznWgrzDAaLjwZ28NmflL0?=
 =?us-ascii?Q?VMYhp05kaHD4f8lX+gCxtCkWIA99RC8wEJ0TkQ2mjABf80aBxPnTxr82Z5Sb?=
 =?us-ascii?Q?ivHmprjJ5lSP+WEzx1WJAjM8Gld979DFxMZJ3MEoQup3paA7OWVXMGWyhJdO?=
 =?us-ascii?Q?up5fvgCl/RitZcPON8ESQ/YdPUg0yXcrjiBibAmdaXVfxkwYFDyLcmtyUp9E?=
 =?us-ascii?Q?AVc9HthbY965MbtdabrOlSYJAMjxhypHEc5euA3Yoz2rVpwYntf4qaVeEe77?=
 =?us-ascii?Q?gOEeuLzeKj5ps4hFgJ4wGJCocXr8wrzh5hcSg4QiKKYL+oKsfPdFD8b0WDxn?=
 =?us-ascii?Q?Z0qV8Rit2GLOpv7czyCGbvXVa4PPo4VJZ4YWc86Uxy8w0bx8Nx47bnjx+G2N?=
 =?us-ascii?Q?JN2f1C01tX45wf5U4ELUVxYPuFJaDvF/oSMwS7AxsZBzN4HqZ68ZqegYKGbP?=
 =?us-ascii?Q?aQ8zxJpH7eGvRNevLiS4fABHjI90rItbedi/bgNdM5r56Y+H0xgFzsh87IsP?=
 =?us-ascii?Q?jqzs0UQHYFWEcc9VqUbtUUTimzH03d+V7jV+uVkT?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5271.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 93e20388-26c5-4fd0-eb34-08dc2617e912
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Feb 2024 06:58:51.6120
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: hzwhZbtCdbyKE6cDJ4JZh8ouLo1lNpZxNfPJ7CWxVjs+rq9o2I+WG6x+k4QpFXlJycmsvGbo9ByACyGVrODUsw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4795
X-OriginatorOrg: intel.com

> From: Brett Creeley <brett.creeley@amd.com>
> Sent: Saturday, January 27, 2024 2:32 AM
>=20
> 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?

>=20
> 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.

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 =3D true;
	pds_vfio->deferred_reset_state =3D VFIO_DEVICE_STATE_ERROR;

pds_vfio_reset()
	pds_vfio->deferred_reset =3D true;
	pds_vfio->deferred_reset_state =3D VFIO_DEVICE_STATE_RUNNING;

pds_vfio_state_mutex_unlock()
	if (pds_vfio->deferred_reset) {
		...
		pds_vfio->state =3D pds_vfio->deferred_reset_state;
		...
	}

it's same as what this patch does:

pds_vfio_recovery()
	pds_vfio->deferred_reset_type =3D PDS_VFIO_DEVICE_RESET;

pds_vfio_reset()
	pds_vfio->deferred_reset_state =3D PDS_VFIO_HOST_RESET;

pds_vfio_state_mutex_unlock()
	if (pds_vfio->deferred_reset) {
		...
		if (pds_vfio->deferred_reset_type =3D=3D PDS_VFIO_HOST_RESET)
			pds_vfio->state =3D VFIO_DEVICE_STATE_RUNNING;
		else
			pds_vfio->state =3D 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 =3D false;
> -		if (pds_vfio->state =3D=3D 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 =3D=3D PDS_VFIO_HOST_RESET)
> {
> +			pds_vfio->state =3D 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.