From:   "Liu, Yi L" <yi.l.liu@intel.com>
To:     "Tian, Kevin" <kevin.tian@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/pRhdQAVXPEC6P100tLKEcK7DAK2AgAAvH+A=
Date:   Tue, 7 Feb 2023 08:56:48 +0000
Message-ID: <DS0PR11MB75299F77D98FAF3FF197A304C3DB9@DS0PR11MB7529.namprd11.prod.outlook.com>
References: <20230206090532.95598-1-yi.l.liu@intel.com>
 <20230206090532.95598-10-yi.l.liu@intel.com>
 <BN9PR11MB5276E166F8BB56049A5CFEC48CDB9@BN9PR11MB5276.namprd11.prod.outlook.com>
In-Reply-To: <BN9PR11MB5276E166F8BB56049A5CFEC48CDB9@BN9PR11MB5276.namprd11.prod.outlook.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?+NuYKXkHoc9ixkf0Wut546i1QRDG9YLUV36bUJ73wHYysBstjuTG97FDiGhZ?=
 =?us-ascii?Q?A0FhWSyRuUW9vFsI7/5hlvKEWWx5cpPa4F6UBPElKqvaR9wxM53W1EmdJlYx?=
 =?us-ascii?Q?0RMlfSz2N3xk1bcMp1FkSyTmTLe6EsZXDWHQqUljh+sFI0bXiC9fF1E5k3fB?=
 =?us-ascii?Q?ypv1UAJkwmiCdV7A/v5kI2kjtwMx+6UK/7OtiDmUA5tI4MoAeRXBqq5hC1Js?=
 =?us-ascii?Q?7n/FKHI7l4Y+JEp+DiUOVckrmAX5DxEOSFzP12DRxKPmvwV/dkBLCmSrxJyD?=
 =?us-ascii?Q?YBj18z66W47qvszQnGiDxc1YX0R+zRzLd6/YV5/KIpnqC/lUf8B7idBL/G3S?=
 =?us-ascii?Q?WSkI84rvlwkJAxuYbV/YnrzDY/vDEwoUbFnMj1uo5Ww8/7Me7MtgqFxtCkEG?=
 =?us-ascii?Q?1dmWGHqcy3MzLhadrDIsHoUMGlGOVckQ/Wmb+fHyirZIOIa2n166sdNnZDZw?=
 =?us-ascii?Q?kUVYWAZaLJfKlulkpPgQhDkIa7fu72OcP4KKt53sO8xdnhavoGomjzK03KJN?=
 =?us-ascii?Q?DwuTD3d3O4DHAjkQmU3J4WlC2Rlg1zqR9AuMOe7F2Ya9hxujdliVfsMZovaI?=
 =?us-ascii?Q?AED5xHBgf/GLoQnaxTsuJ/VEi1MLt3E5aNI94He0OkHRrGLDHwW4zQCLs1pD?=
 =?us-ascii?Q?BWFJUO6Lb2+WwOhbCBOj0z3DBAYY+9j1mRrhcmnF3tXihZEIzgyzU0H1+SJq?=
 =?us-ascii?Q?VEOZ3B/TBhPFQ7CtF3kJEjaVqBmg9MoBl5NROWsstxfLQA6wTPwVdpc5Qr0U?=
 =?us-ascii?Q?dY4z/qATUN7qlMMyhpzRUSwDKpTeZzSd6Gl+lA/8Lys6A0VUltbNAFd1GKW6?=
 =?us-ascii?Q?p+NtVPHRaQ9QUOMFRPJM7ysLk6e3KhD8dQpB+NxkLROi+7wTw2IX5AnSLTAW?=
 =?us-ascii?Q?6Mtq1aj4XEzKlBXGVxOy5tBvJO+Y3qjlWHp/+r6j3H5rfUNrhvmLypq81MHD?=
 =?us-ascii?Q?zVBeMDJ082CDeQc4VKw7NaS21JJMqpYgZBGq8H7rdKmnhFodv0v2wFShZmdg?=
 =?us-ascii?Q?EhGFDhP7uQbIhoXAv8n146noj/UiSvkv98hMK+NP4pXydUWtjii9OkGC/1Iv?=
 =?us-ascii?Q?thWYxc9dWqBSHUz5L3uVCFvQ16aUrbana4DPUlxBzr0fNNm5KADkNPGWwdjt?=
 =?us-ascii?Q?XJvrfmRmpr3/OPaHuHlE7Z806x49yLCY0N/HYtG3UBNoUsUBwx46MPKoX/bo?=
 =?us-ascii?Q?egvN6aKKK0zK2nyB0hq9SZTjyaLYgthnIjmzy0tT1hPj7RTZE49Aki/dU7wj?=
 =?us-ascii?Q?uCdUhGpXx7RqglfJxW141VlTkDNV2GrXIPkPiWMInAYj7Ya5vsLtxmZ4V1ko?=
 =?us-ascii?Q?z6DD6tyDYuCDfbV8U4XhdRGWWU5WFDl5iDYEOKebpVzPvq/H8DVY0XycFLVA?=
 =?us-ascii?Q?VEg+MdJ6iQdyaSje8wncyJfpjib3099xo+CgRqsksvU2z99nV3i21YCUWYbu?=
 =?us-ascii?Q?2RO2DCdCr+JB8oPpZlaji6gwZaZUaLqun1rbRDXAFlOVl9JJxpT1UsvbmN68?=
 =?us-ascii?Q?s5+P0LFtRoQY8ux+aWO4useHkkxiwVXrzQg39lf701Uvm0wytB/PnNngOHnh?=
 =?us-ascii?Q?JoQdJ9u17ytgcf21Vdvuc31dwl9Xi2Ji2LYtuv19?=
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: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f2c59d04-811f-4380-0e42-08db08e93f98
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Feb 2023 08:56:49.0077
 (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: UA+d/JF9V/jM+PvL0CswLtYI5n26+IhyOwiPpL3IjrVQhfCm3vs47kmgT3Y/+cGJhWngldGstYH2QjaPAs/SRw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB6868
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Tuesday, February 7, 2023 2:07 PM
>=20
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Monday, February 6, 2023 5:05 PM
> >
> > +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);
> >
> > -	if (vdev->iommufd_attached) {
> > -		iommufd_device_detach(vdev->iommufd_device);
> > -		vdev->iommufd_attached =3D false;
> > -	}
> > +	if (vdev->iommufd_attached)
> > +		__vfio_iommufd_detach(vdev);
>=20
> I'm not sure whether this abstraction really improves things.
>=20
> Just two callers. and the old code reads clearer to me which
> checks a flag, does something and then clear the flag.

Ok. Will revert it.

>=20
> > @@ -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.
>=20
> "Undo in unbind_iommufd if @detach_ioas is not called".
>
> > + * @detach_ioas: Opposite of attach_ioas, this is for runtime undo.
>=20
> remove "this is for runtime undo" which is confusing.

Ok, clear with your above suggestion.

Regards,
Yi Liu