Received-SPF: pass (google.com: domain of linux-kernel+bounces-30159-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 18 Jan 2024 09:38:57 -0400
From: Jason Gunthorpe <jgg@nvidia.com>
To: Yi Liu <yi.l.liu@intel.com>
Cc: "Tian, Kevin" <kevin.tian@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: <20240118133857.GK734935@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>
 <20240116125756.GB734935@nvidia.com>
 <BN9PR11MB52763DDDE39C211E761A05168C722@BN9PR11MB5276.namprd11.prod.outlook.com>
 <88e46f6c-4d64-4357-be2a-833797e6de15@intel.com>
 <20240117125625.GF734935@nvidia.com>
 <459d6a3a-0ad9-4980-be37-103211e927c2@intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <459d6a3a-0ad9-4980-be37-103211e927c2@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?b0+sbh6bRhFXqlOWvZh0LDHFK1o2HEyH+X7jDy7zN5rlZURnB8j/E684fccz?=
 =?us-ascii?Q?AlQv0y9m58XjSIDu9G9W3L6bAA8oS7uddM5R9ffyJ3jIXV3owtLIl2s57DT2?=
 =?us-ascii?Q?2jpylfnk2p+8te5pApmVz4KzgIwnB3JvroNPR+ojR8zuP3RBnB18urpy0V1N?=
 =?us-ascii?Q?4Gl8CgDBRpKKAGgYCcz22xDvnhTKswBDSMbIB97wgwD2eyM+QEo9z1oM1weV?=
 =?us-ascii?Q?gQ7vlm7jBxGqkK7ClAwPqKsb1v2XICdVOF39AvaiGb7qR7vljlsq7TcAhv4I?=
 =?us-ascii?Q?o5vDo5cAu7bYg19DiBlN5RIuCSUjD73I9w0pzZvkS9bkcAHuEiisYIz4AJKb?=
 =?us-ascii?Q?kVwTZGNDnGKXiGwMvdspgE3L3v93s5ubRmBC9bKxha9eRIHT2v8TsPH1qco/?=
 =?us-ascii?Q?PolKovq+cvsnancmehaC16nJIctbFSI/Y9dCp9TxFkRDszXwYwSQqA0GftI0?=
 =?us-ascii?Q?EN/UbM/5zuOfQmmde6wRlUVbuXFmI0fbLud2Rh/WuRcFYBsxa+YuG/YINNG9?=
 =?us-ascii?Q?CzBygWpErbU4IjjwmLCVpO81/nn156rfc70SJCl98ApXWCwFKwGr9A30eLir?=
 =?us-ascii?Q?Rq4dCnQT0IjWes/gQCX4K6PREmG8HJaevVnzgClneXos41CIjTfHOSq/bFQS?=
 =?us-ascii?Q?8q0LiPu8FwK2TSbhps6P4iKqlq45LqujDhI021qylIn/unGGcQ1NBObrIeUI?=
 =?us-ascii?Q?RigyphcPPYw/fDYa+BuWLXYHlDzmtUJ+3GZZnOQa+ODmHl4NsMKvSKVtrBR6?=
 =?us-ascii?Q?9orLG9jToqmGInRDFfjGDwuCdmWrVqLHUoHUg+Wf07YeUd4u2VssJ2r0mSXI?=
 =?us-ascii?Q?gMMAkGGtOttWL83S9Tzf+tw+worqAe/OtLxHkVNyAV2aJ4sI3jnDFNIk9vFX?=
 =?us-ascii?Q?IoIEa98jRDxF7XB5rch8a/nKkzjgrAPyZfw0QL7eiAgaFoCb88P78ZHpsI8L?=
 =?us-ascii?Q?gclV5nos3afkDZPl5WYowUv7k7+kkDRj5G00CtV4w0Brps8x2ajFelT9VRD/?=
 =?us-ascii?Q?AA5BO8jd0Gtfvvb7GUqoabIJ0//XNAf2yiJlGO6oQqcjJinrvbBOp5nYEq/M?=
 =?us-ascii?Q?q9oD/Z3BrBSwq3pi3V67TdpxAivofTtuDVvUcvwBks2VZGYqJ1kLdpHknq6c?=
 =?us-ascii?Q?UJCRFXTQw6j2BH6PUHjD0rJqPSisyPCk68XMT+2ft8K44eCehULVm3v/gvV9?=
 =?us-ascii?Q?8r2qAeM3rlOv3K03M0EcHYxxUXAHxD8eonsYeGhuwZSJWaDuaV2hratsMxsn?=
 =?us-ascii?Q?G5j1XZ5Cyx367a6ktOhV2hVSMDlVw0zFLn+C+2JjcjM5Bz2BfSmaGncNbvhn?=
 =?us-ascii?Q?fibKCzt/rXAnrcMKqIpHMlLh0I24vCUqCaSjbAmgUthAU3g3+myyG3kSoLY7?=
 =?us-ascii?Q?ddEwQIKoOdN/YsFuHmIiRY3BiBWylylXZbNbQxBBVHfB24wVG61/bHaSKI4A?=
 =?us-ascii?Q?ahllO4PLJs4RNpHayLyWLCag8RxngbTJdzqIkK9DHrAptabgtdhN+2TCcGcI?=
 =?us-ascii?Q?ElaKtnIRcIFgb0xDugOfsLbwOWlp8BONV5Nqwjw1UHhzKp5Wmzcas4xqqg51?=
 =?us-ascii?Q?XNQHnlOEkQ8CpK/eMZM=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 17a89a60-47a2-46b3-7dea-08dc182ad337
X-MS-Exchange-CrossTenant-AuthSource: LV2PR12MB5869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jan 2024 13:38:59.2166
 (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: E4cFMC4kOvlsnw3/UHHioj1Atsq8oRgtb4+8toLs3TaEsx1E+Ut5sg1Yoh3BurX2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4062

On Thu, Jan 18, 2024 at 05:28:01PM +0800, Yi Liu wrote:
> On 2024/1/17 20:56, Jason Gunthorpe wrote:
> > On Wed, Jan 17, 2024 at 04:24:24PM +0800, Yi Liu wrote:
> > > Above indeed makes more sense if there can be concurrent attach/replace/detach
> > > on a single pasid. Just have one doubt should we add lock to protect the
> > > whole attach/replace/detach paths. In the attach/replace path[1] [2], the
> > > xarray entry is verified firstly, and then updated after returning from
> > > iommu attach/replace API. It is uneasy to protect the xarray operations only
> > > with xa_lock as a detach path can acquire xa_lock right after attach/replace
> > > path checks the xarray. To avoid it, may need a mutex to protect the whole
> > > attach/replace/detach path to avoid race. Or maybe the attach/replace path
> > > should mark the corresponding entry as a special state that can block the
> > > other path like detach until the attach/replace path update the final hwpt to
> > > the xarray. Is there such state in xarray?
> > 
> > If the caller is not allowed to make concurrent attaches/detaches to
> > the same pasid then you can document that in a comment,
> 
> yes. I can document it. Otherwise, we may need a mutex for pasid to allow
> concurrent attaches/detaches.
> 
> > but it is
> > still better to use xarray in a self-consistent way.
> 
> sure. I'll try. At least in the detach path, xarray should be what you've
> suggested in prior email. Currently in the attach path, the logic is as
> below. Perhaps I can skip the check on old_hwpt since
> iommu_attach_device_pasid() should fail if there is an existing domain
> attached on the pasid. Then the xarray should be more consistent. what
> about your opinion?
> 
> 	old_hwpt = xa_load(&idev->pasid_hwpts, pasid);
> 	if (old_hwpt) {
> 		/* Attach does not allow overwrite */
> 		if (old_hwpt == hwpt)
> 			return NULL;
> 		else
> 			return ERR_PTR(-EINVAL);
> 	}
> 
> 	rc = iommu_attach_device_pasid(hwpt->domain, idev->dev, pasid);
> 	if (rc)
> 		return ERR_PTR(rc);
> 
> 	refcount_inc(&hwpt->obj.users);
> 	xa_store(&idev->pasid_hwpts, pasid, hwpt, GFP_KERNEL);

Use xa_cmpxchg()

Jason