Received-SPF: pass (google.com: domain of linux-kernel+bounces-28512-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
CC: "Liu, Yi L" <yi.l.liu@intel.com>, "joro@8bytes.org" <joro@8bytes.org>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>, "baolu.lu@linux.intel.com"
	<baolu.lu@linux.intel.com>, "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>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>, "Duan,
 Zhenzhong" <zhenzhong.duan@intel.com>, "joao.m.martins@oracle.com"
	<joao.m.martins@oracle.com>, "Zeng, Xin" <xin.zeng@intel.com>, "Zhao, Yan Y"
	<yan.y.zhao@intel.com>
Subject: RE: [PATCH 3/8] iommufd: Support attach/replace hwpt per pasid
Thread-Topic: [PATCH 3/8] iommufd: Support attach/replace hwpt per pasid
Thread-Index: AQHaIPvOp5q2xzg/c0eyE+dkyLAmKbDbbW0AgACBx3CAAMYTAIAA//LA
Date: Wed, 17 Jan 2024 04:17:39 +0000
Message-ID: <BN9PR11MB52763DDDE39C211E761A05168C722@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20231127063428.127436-1-yi.l.liu@intel.com>
 <20231127063428.127436-4-yi.l.liu@intel.com>
 <20240115172430.GN734935@nvidia.com>
 <BN9PR11MB52761349DFB5DAD2797C3EBF8C732@BN9PR11MB5276.namprd11.prod.outlook.com>
 <20240116125756.GB734935@nvidia.com>
In-Reply-To: <20240116125756.GB734935@nvidia.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?Jn1v7oDeGVKMvKz9jL6kScnO1tzLQT5TmlrG5Kyi7e0VGvpVj3+/tFRbjQSZ?=
 =?us-ascii?Q?/PrZEzOa9BPVhaR7L9qiojh5ja5jOthyKpXg/KCrrnsiTXP9SzSEKvPBLqns?=
 =?us-ascii?Q?bEwKwuVQVaytb5y0SbfJOA1UckT7xtZW7K/QIpKWxSSLfXU5FuIcPLG5KAZ7?=
 =?us-ascii?Q?a79CxDqU/HmcdMxD2NHT51I2fZEsW8Icu54muym+V0qrNb2LutAiSRLo5p5S?=
 =?us-ascii?Q?X8vnWc6tia+9nS6YZdBnCIRPkRFn13JtWQXb5XXr/O+GIYWuPcmVkHogo2vj?=
 =?us-ascii?Q?YWZokAKARYCB2LKAUSSsQ4yqoyI4aIXxwWdKwhDpRgcPnbNUY62yOY+JHCfr?=
 =?us-ascii?Q?GJXTYv5u8poPsHC6M2QvCjWknIgLrmaj7Lcxuz9tOGpKJVo1v8nWr4IBPMjZ?=
 =?us-ascii?Q?s+Cqgj9Pis1azg7slpAdQr40VYqMAPrA4A3ywEuVRo7fTdFieLdBZ+mSYtBz?=
 =?us-ascii?Q?nHl47HlYZ1VvAwqJFaLGTnlsIXqCWho7FpgihmrFrTsOo0mJ99MlYqxAGEJ3?=
 =?us-ascii?Q?skkZmRdSaewd3uAmPYu/tvW8OyIT8XdVb/qEo32UHyv+BPfudGTDSFGrclgK?=
 =?us-ascii?Q?FV77E7kuks5QaBTgu4oPqLHlGGVIZMgM+ZCFTQniMpOyYLanT21zdDjBUxvg?=
 =?us-ascii?Q?S/GBtFjKqH3bWgWPGFEz5zrNviiA0ZiMf9qpVF3yUPjDPe+x2/bOE4B8AKx7?=
 =?us-ascii?Q?wuNPwniVWi6RQ/WX1Ha5dcXOMuc2jpj8PAJ8Gyqz5hhxUQgUNasMoiXlkpOv?=
 =?us-ascii?Q?iT9IGI75Sdra9v8KakU9R3lxtSVcrN9Y0ydXMavA7TvWkln80JThKGgoLbqu?=
 =?us-ascii?Q?MLu3+qStKPz+5lE+q9B0jTg0607FtY1ks7KaGSACIIW44gJDDZBk01Mq+UPZ?=
 =?us-ascii?Q?Kyaz+Ng3iDz4fcWbVgBmXpgbtgmslEpgoa5S4+VklwIYkq+EL2l0WyMVzL9H?=
 =?us-ascii?Q?Bj4Ql6snh1Nq+yTeG5NwcttJMxxE5nitOBitTFtU/+mACxMEmtSVj0rTxVZU?=
 =?us-ascii?Q?zDTiPi9WgsAMrZFNFFsoXNwFfdEDLeU0IxwoOkAi3A3gY4FwY6UTggheet2J?=
 =?us-ascii?Q?N1uQidwzQ1VL5alD8MHHQecFvzL3mAuY2NasKSvQNFu/NGmXJbd51xHEkw2W?=
 =?us-ascii?Q?EdYdYDFMA0Ksf0ReOKCKRD6y2W5VMBl/RzxGwZbq7yhqBhOB/IMAl0c6tNUA?=
 =?us-ascii?Q?sOkLFkTbpeKESdER5rZ2fyKhhtwvifVaNUUCKI1+iOpjfQq6KM765AJISydl?=
 =?us-ascii?Q?76nyvnPm9nIJqBqGnZ962VpR9FXUx4x0RhatgjI+v+1VP+W2JEVlQGS6z+s/?=
 =?us-ascii?Q?BqNqXznu4zbxw5lwNUybuOncezN6TePLGNBL+j0JKdGX6iswgcpdsbzb8qHP?=
 =?us-ascii?Q?0aHFbjUV5sEHHU/XO/mIIse1fDtnWrJ26cHfs2hOiS0qnnwANrVyeTlT7f5G?=
 =?us-ascii?Q?Ww9iioBlxh5eB5mnedkcOzAiWLeeQP7sP0d1hiXpP2Ea3X3x35XXONtG1xl4?=
 =?us-ascii?Q?UdnzjcfSe/l4q7CsZ/ZpdQAin98oWgGkglpNo/aUAJ5gcE82dsvfC2fRzcPo?=
 =?us-ascii?Q?TF48EpZjL3w6Q6IsPBhHcF4c3eQrMOnmWXLpAjpj?=
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: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3e9e8353-9665-4943-aa9a-08dc17133e2d
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jan 2024 04:17:39.4192
 (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: ACdxOW1yFIyNI2Xyx4c96nVUwNx/MtxzIv3iy+vS+U5mEflMsA9TsRphWLR4VofbFeuN5mcZuGUGdcdo9KZBNA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR11MB8602
X-OriginatorOrg: intel.com

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, January 16, 2024 8:58 PM
>=20
> On Tue, Jan 16, 2024 at 01:18:12AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Tuesday, January 16, 2024 1:25 AM
> > >
> > > On Sun, Nov 26, 2023 at 10:34:23PM -0800, Yi Liu wrote:
> > > > +/**
> > > > + * iommufd_device_pasid_detach - Disconnect a {device, pasid} to a=
n
> > > iommu_domain
> > > > + * @idev: device to detach
> > > > + * @pasid: pasid to detach
> > > > + *
> > > > + * Undo iommufd_device_pasid_attach(). This disconnects the
> idev/pasid
> > > from
> > > > + * the previously attached pt_id.
> > > > + */
> > > > +void iommufd_device_pasid_detach(struct iommufd_device *idev, u32
> > > pasid)
> > > > +{
> > > > +	struct iommufd_hw_pagetable *hwpt;
> > > > +
> > > > +	hwpt =3D xa_load(&idev->pasid_hwpts, pasid);
> > > > +	if (!hwpt)
> > > > +		return;
> > > > +	xa_erase(&idev->pasid_hwpts, pasid);
> > > > +	iommu_detach_device_pasid(hwpt->domain, idev->dev, pasid);
> > > > +	iommufd_hw_pagetable_put(idev->ictx, hwpt);
> > > > +}
> > >
> > > None of this xarray stuff looks locked properly
> > >
> >
> > I had an impression from past discussions that the caller should not
> > race attach/detach/replace on same device or pasid, otherwise it is
> > already a problem in a higher level.
>=20
> I thought that was just at the iommu layer? We want VFIO to do the
> same? Then why do we need the dual xarrays?
>=20
> Still, it looks really wrong to have code like this, we don't need to
> - it can be locked properly, it isn't a performance path..

OK, let's add a lock for this.

>=20
> > and the original intention of the group lock was to ensure all devices
> > in the group have a same view. Not exactly to guard concurrent
> > attach/detach.
>=20
> We don't have a group lock here, this is in iommufd.

I meant the lock in iommufd_group.

>=20
> Use the xarray lock..
>=20
> eg
>=20
> hwpt =3D xa_erase(&idev->pasid_hwpts, pasid);
> if (WARN_ON(!hwpt))
>    return
>=20
> xa_erase is atomic.
>=20

yes, that's better.