Received-SPF: pass (google.com: domain of linux-kernel+bounces-27397-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 16 Jan 2024 08:57:56 -0400
From: Jason Gunthorpe <jgg@nvidia.com>
To: "Tian, Kevin" <kevin.tian@intel.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
Message-ID: <20240116125756.GB734935@nvidia.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>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BN9PR11MB52761349DFB5DAD2797C3EBF8C732@BN9PR11MB5276.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?e3RfVj6HVioK6FxITrwVwVcwBi7kVZ5VP7w/s6APblmIXXPCUBsB8RNxdjgP?=
 =?us-ascii?Q?QFURSCFgDl6MOhNAkwjo1Tjo7Pce58ZWgA6mCG6/00z+06p/OfJ4vVR1ie3/?=
 =?us-ascii?Q?nmTRni5EgIZv/xQafFtTsp8QwPNweh1+utnbb1++74h09gEjs69Qzjp/A7qF?=
 =?us-ascii?Q?r8zGygKMJoYNF3dCjlBzp/P0y2o9+zkkPatRfXVi3i7g22WFOX90Cqrr8Zdi?=
 =?us-ascii?Q?GtZFlI4tWP9Ie4WOUlI9FpaQGEVofJgHQPjbwn+S6u7ZJ0ofe4e6wwZGEIvg?=
 =?us-ascii?Q?wl4XOX0Mxf3Q498c3IWuvaEews+qQHJB55jlqpi5kCQ2JQY2fzRDhzNGOQKr?=
 =?us-ascii?Q?XJiztz3rVYAJLyOyYD4eVYKIsQfA1UnbL9kKie2cn4yOZSfHa4VRURFBPAb2?=
 =?us-ascii?Q?gR/0tCs9+9ND0N4TKgHN869SIgjhwHU3c9Sn4qFoOUNpSv5h9uflsTlCdffo?=
 =?us-ascii?Q?9c+7FS4+xcSjxAAj/J1NH4bbo57i3znKYU+xf1yo6nzIagLWfV/aDO2MOAfX?=
 =?us-ascii?Q?1UIjsm+I1Kufh/eoceKKmvsbGe7sSsx78YUZyN2TLOHIREJEUNNPpbe0bvwT?=
 =?us-ascii?Q?a2aqzNrurxxlEWCX3x5/N9ZQX3E87G0pcfdnJ28XEzPVjbTign4j09EcSLOl?=
 =?us-ascii?Q?2LmwHApE1h9eqrrGQpu1cq84Gm61fe/sz9ogzoNjy0iFBpJwWSnkKYdzoY9D?=
 =?us-ascii?Q?KJmdhb4zkUaL8fgaeAT2gDt/x+sua2zZ1eSFUn4K5k6uu2rQTnz82nte1VI6?=
 =?us-ascii?Q?H2/a88dgyPEqOsoG+17OfpUGPVprfarBQJZ2iLWA57o3cPaQ43uIHkXN3u4/?=
 =?us-ascii?Q?bjpx7XZMTa2dFwy4BTWAmReT2DBJZxUMn1C5mutSvLRVctEvnoFcJZNYv1z7?=
 =?us-ascii?Q?hJ0QdDZkQ+low7mutC5g++Vb8q2HhjCSBPDKhgubMPwn1etU/luVtx6+J7Vo?=
 =?us-ascii?Q?QImhnGhvUHV2jg+LO/wsMi8H0qsoskXEbbBE/CmJtNPwFU2k7hntH2SVIfJW?=
 =?us-ascii?Q?A5hxEEL1JEWJJMOxTJfGUFcSVkBehRhyJxJ78+aosNlCwDBmsSlaG7C1EJMF?=
 =?us-ascii?Q?o0609ND+hdYu8BKW0ImYBzATIx6m+PHs7ssFZDQ8VbbttfVXAdxOn77DNnsR?=
 =?us-ascii?Q?nGlzpJqzIpUyQ8VO4xLxZuWD8w6Au4fXYq744YYPJr/g0F3iYnK8Li3idnF3?=
 =?us-ascii?Q?s8c1FvAiWZf4EWWm1jIq6RYbVzlCFimZE0bK+3pygA83yzPlnK0uXs9cRkRE?=
 =?us-ascii?Q?37xIrKwCfKtU9+Re+kleTzp2m4KJpt6YaAsZ8m/Qw5EMdduGHjx/tunlPr1v?=
 =?us-ascii?Q?uA1ylgDcdwBNZay0ERuPTyXRZbAXl52UUPKEi0/UrAB9XI229tyCtZOBF8/S?=
 =?us-ascii?Q?045LdncVA42kYSLhhliYIoFLYX1YdnfXh+H0I1zqGYPAGrugVQPSxo7+zTqF?=
 =?us-ascii?Q?aq5ka6dBYUFjun9Tk63Bc0l8g8NA50X6EhsGBiR7B6BQsapBv3Iek094Kyr3?=
 =?us-ascii?Q?NxcA2sDc8V1b4tgV/HpM/zBZOTyn/sSCW2hoXSNtY6za+lmXxCIcjj8AcTLu?=
 =?us-ascii?Q?45/FeXWv6jd1GrlmIKXcZ0mC9mYElcdEL9l6Uvvf?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 735883be-f44a-40c7-eccb-08dc1692c351
X-MS-Exchange-CrossTenant-AuthSource: LV2PR12MB5869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jan 2024 12:57:57.9065
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nTmoybUVVfN2aQOnQMJXuKDbxgQwWILi4HvodTF1VpLHhZ8sCaRbw3aZiZovvFIL
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB5018

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

I thought that was just at the iommu layer? We want VFIO to do the
same? Then why do we need the dual xarrays?

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

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

We don't have a group lock here, this is in iommufd.

Use the xarray lock..

eg 

hwpt = xa_erase(&idev->pasid_hwpts, pasid);
if (WARN_ON(!hwpt))
   return

xa_erase is atomic.

Jason