From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     "Liu, Yi L" <yi.l.liu@intel.com>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>
CC:     "cohuck@redhat.com" <cohuck@redhat.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "nicolinc@nvidia.com" <nicolinc@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>,
        "chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
        "yi.y.sun@linux.intel.com" <yi.y.sun@linux.intel.com>,
        "peterx@redhat.com" <peterx@redhat.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "shameerali.kolothum.thodi@huawei.com" 
        <shameerali.kolothum.thodi@huawei.com>,
        "lulu@redhat.com" <lulu@redhat.com>,
        "suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>
Subject: RE: [PATCH v2 09/14] vfio-iommufd: Add detach_ioas support for
 physical VFIO devices
Thread-Topic: [PATCH v2 09/14] vfio-iommufd: Add detach_ioas support for
 physical VFIO devices
Thread-Index: AQHZOgo/9nXrPVpMVEaNI/w6kla4hK7C/y4g
Date:   Tue, 7 Feb 2023 06:06:55 +0000
Message-ID: <BN9PR11MB5276E166F8BB56049A5CFEC48CDB9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20230206090532.95598-1-yi.l.liu@intel.com>
 <20230206090532.95598-10-yi.l.liu@intel.com>
In-Reply-To: <20230206090532.95598-10-yi.l.liu@intel.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?OiHscPtb8R7L/Hy0iZ3KPf5a7Fz6flFgJz7KARnSe3p8sbaz3AqUIneQLqJt?=
 =?us-ascii?Q?q9p7+pIB6aiL0O3MneOhDJ7jYSXcYoKhbXVEgWqHd+1Q49oMR0nRu0WMYD/D?=
 =?us-ascii?Q?OV1dbcwcAkZbjzalz6dYI48ksrmpOL6wZEczNglwzJzC3a+WtpNgeEzliua0?=
 =?us-ascii?Q?4lSp4GTJKocqb4QeVdOWjRjpsewDa0pqAfIWxYKgGdSZLuUMvOE32qnocP/h?=
 =?us-ascii?Q?ZQfyfmFlgYVueH2GCkJSzvIyPqZpEZhqpNhexE5fgnZhRDGFjRYrPkWXpaCT?=
 =?us-ascii?Q?4EUntMZzA6dz3BYYLeXfZklyUTB9gVTDUW8NZ75WUSFoAHIBxZjVH5o9PcSD?=
 =?us-ascii?Q?vl+CnnfqDVnTEZWWAkDodsD+V2nZTL1FsBVEAK1VKe2azGh12YuHCstVfNtG?=
 =?us-ascii?Q?9o5YfeUcaX6ZmfLd9N/CUqSao2yrXTqIbTrpSTw7GKbWMsdNh23I2f6LA/Hq?=
 =?us-ascii?Q?6n8I0drF2i1XQQg3vr8b3TR2SORdGPBgparUJeZTFW8AfK9fipOMf6uN9zgN?=
 =?us-ascii?Q?Wc+n22/cUKcD1CIC0PRSCXbnrdE53hI6YypZVe0BoFJNsjqVk2pZiG1h/1MS?=
 =?us-ascii?Q?M45mxqFTNJNqyktnMz+0EmH62FNWp02m6PXDuuuHlZ48k9jnLiqlMFDDpTc8?=
 =?us-ascii?Q?EhSaCCvvWHaWVbZ4DMcdH2T60bOwTzKQeh+4iApQC1BB7/o5wWhhW6eiuIH1?=
 =?us-ascii?Q?MXh0mIxI6ap0OVOQal+liIijhORaJiKDqxyCFLI1MIuqeFm3UkxeZsChzO9V?=
 =?us-ascii?Q?g6neA8vVwvFAllfusxr8MeCpqUTd65sOxF+KDtLSnOiVc/O0Jld6jGeJF0eQ?=
 =?us-ascii?Q?5S/sQsKfF0x13dL60O82HnoTEhx7gYtmzWV5ZlH+UOCW5nmMVorOZG0ogMmj?=
 =?us-ascii?Q?TVjZanZlkrTo91hKA7wV/rCSVDYViFHDj53EMPGwBdSloWl/bfGqbwAMSZo2?=
 =?us-ascii?Q?vUdnUXyqvQwt3EwuDl2Q6QRHcm46P1bnioQNA2AS9Xhl2/i5q7Nid09i0yGc?=
 =?us-ascii?Q?rBwYhpApufJCLdC54LIezKd1FxUXleu/p/aBB0Q2tExRmHfkCsPvfqMJE5jg?=
 =?us-ascii?Q?dDYoNtVgpPyv9Y4zLwAAPBc0Slef7fu2zgwhbUZU5jbGZEhDGII0dQFaqeep?=
 =?us-ascii?Q?KnCicar6r5DKXiGWmOi6kavLa9rdfRjkmPJP2Ki1eozCC4p/spJXt4TyzKMd?=
 =?us-ascii?Q?X58tXyMvgCKIdPrdzlHVoVhizEOgBUV26aMZm4ycEA637Eusp3Nh6p7dYycA?=
 =?us-ascii?Q?YAImaeVIWFdoyePCOCJJqLFo2g7dpB0cY3i5mM083No4CmVeQH8q7EbG5ejF?=
 =?us-ascii?Q?KEB7roY+AGTAySmAY6yUQ9pEjsDhUBNLWxaz3up4m+6yA4iXJRu2pU+9NHIc?=
 =?us-ascii?Q?KD9BQnnYWKfAS5XkDLqBGIQDGlmLL3i34bBPROVK1DRjz8YO/eWca7xluK8i?=
 =?us-ascii?Q?zNFBqQEmg7XV3v3lwm1IRiaoGLWaI0MhueMYmxTkB1HLsGtmODHW7DBEV+/v?=
 =?us-ascii?Q?sZtqexVh4eBU8m2GxdCWAJAop11eiWD7LLJFBChzSkH6diIYtCq7wBhKTvYx?=
 =?us-ascii?Q?G1SKATbKrx2oEAqossq/GjVQ/yzQxxaVD3TldInK?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f2ec3b93-e531-4946-067d-08db08d1840f
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Feb 2023 06:06:55.9196
 (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: AjCu//Yd605fH6l52G5tq2B1KbpsCf9xa8y+cVc5kFv+FsrQlcBdsav27zs70U4bShatRiSTrofvcbogVhTpQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7509
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, February 6, 2023 5:05 PM
>=20
> +static void __vfio_iommufd_detach(struct vfio_device *vdev)
> +{
> +	iommufd_device_detach(vdev->iommufd_device);
> +	vdev->iommufd_attached =3D false;
> +}
> +
>  void vfio_iommufd_physical_unbind(struct vfio_device *vdev)
>  {
>  	lockdep_assert_held(&vdev->dev_set->lock);
>=20
> -	if (vdev->iommufd_attached) {
> -		iommufd_device_detach(vdev->iommufd_device);
> -		vdev->iommufd_attached =3D false;
> -	}
> +	if (vdev->iommufd_attached)
> +		__vfio_iommufd_detach(vdev);

I'm not sure whether this abstraction really improves things.

Just two callers. and the old code reads clearer to me which
checks a flag, does something and then clear the flag.

> @@ -74,6 +74,7 @@ struct vfio_device {
>   * @unbind_iommufd: Opposite of bind_iommufd
>   * @attach_ioas: Called when attaching device to an IOAS/HWPT managed
> by the
>   *		 bound iommufd. Undo in unbind_iommufd.

"Undo in unbind_iommufd if @detach_ioas is not called".

> + * @detach_ioas: Opposite of attach_ioas, this is for runtime undo.

remove "this is for runtime undo" which is confusing.