Received-SPF: pass (google.com: domain of linux-kernel+bounces-28638-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <88e46f6c-4d64-4357-be2a-833797e6de15@intel.com>
Date: Wed, 17 Jan 2024 16:24:24 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/8] iommufd: Support attach/replace hwpt per pasid
To: "Tian, Kevin" <kevin.tian@intel.com>, Jason Gunthorpe <jgg@nvidia.com>
CC: "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>
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>
Content-Language: en-US
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <BN9PR11MB52763DDDE39C211E761A05168C722@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?c29BVFdYQ0w3T1RnZEpUUzdLUys1V0dtOVUzQURsdFZjMnRuSjVhbEJuYnBj?=
 =?utf-8?B?RFhHN25DVEhjY0pOUlF1N0pSRzNGME0reHdMVE5oSVN4dng3WXhhbUNUY29x?=
 =?utf-8?B?T3RmWFY3UGxGVnhJT1BNS2grakFVVjhSLzBUTlp5bnpYR3lhVWFuTVdXQVFB?=
 =?utf-8?B?RmlvSDloKzBjb1I4akV1L01tZUdPazhUYTN2c2JWMGt5dmNHbVNyd2JkZEEr?=
 =?utf-8?B?VW14NWg2blhadHVvU0hVR1Brc3BnK3VaaGhYcUJJdWI5Z2p0cHpFNzhlcXdR?=
 =?utf-8?B?SUs1bk1pM0w4YWdIcC9tNUFWNlNJbnRCKzlDRXJkQW5mdG5RYjJCczYzbVlw?=
 =?utf-8?B?OGFSSGxxRGRKeERYejRubnZuWmhDK2luUmcvcmhFTWN1Si8vbVc0UkkzbTdn?=
 =?utf-8?B?V0FqWUhSU09TYVFNejZYUTl4dEluWkpRV3Fta0pMTzlicWlVK0x5T1oyb1NT?=
 =?utf-8?B?bjF1d0FkenNhRnB4a1JzOC9xeWpobzlJL0x2a0Z6UzEzR0lLRy9oZC9sYThG?=
 =?utf-8?B?SVZTQjZHY1pPRFRMeS9HQkxzaURPN20yNW9teVRFK1VNTEhVK2RpZ3N6MXpr?=
 =?utf-8?B?RjU1UmJqWmo5bklBUmJERUV6cFNqOFJyRGgxcUpaeFAyaVZpTFBqakR6ellS?=
 =?utf-8?B?bjdaZERUM1ZDUll1bVRoUkN5andROG9hLzR6UlBMM0JEQTFIR3ZsSHlxQ0lE?=
 =?utf-8?B?RUdzZm15L3paT09Od0oyVzRUWCtwOGZiVEViOUE0WVk5em0reG4rQm1xL1dK?=
 =?utf-8?B?MG9rUUlaWitZREpGc1pJdVJxN3FXMnlGWGpZczlHbWpFSWVlc3JTSUJXV0Ft?=
 =?utf-8?B?TTc5VVQrVWkva1hGWS9lQ0RSaUxOWU5yMUNDQi9Oc2hNQjEwM3dmR285UWdO?=
 =?utf-8?B?RDVzU0tUMkdlK3EwdHZvUytJOUJkNlIrYWN0enRIL0lMT0g2ZGVTY2ZLek9J?=
 =?utf-8?B?TWIvZFdPcXVRa2ZpRFBRdlJlV21EL2dzbGh1Q2FWN0xMN3Y2NXQ1T1dYTHZt?=
 =?utf-8?B?OG5IWlBUVG1VUDV3bCtzdVVtNDhnaGovTElyYWJGK2hLZ3BraW54UGU3STVM?=
 =?utf-8?B?ZHZsZkVXZmlGUEV5OVFBbmpLTytaL2JBbnlSbHl0cXlCOG41NUtnemdsT2RK?=
 =?utf-8?B?ZWFpR0RaWGhKZjdzWmZSYUx1dldvc3JYUUVhN05QZ2JMdDF2NmhRVDBwOFla?=
 =?utf-8?B?L0h0bVVVT3UvS3NFSWZGYUYzNHJXaUFCMU1DTjUrZVlHdU5jZ0dsU2RDR2s1?=
 =?utf-8?B?WldNZDc3SEM3cGd6VzBJZDlzKzZuVEtVd3haSVpDQklSOUJ5c2U1TG1GZERz?=
 =?utf-8?B?QWk1Kzcyd1Y1MW5EbU16VkZ3UUJjeXRmcDNjVDdkOFVqMWRzSWZYZjcwZUlE?=
 =?utf-8?B?VU5yTWJlblpFR0M1Y0NOd05kbklIbFNkdlNGRlFlN3NGT2JxTnBjVjhscUxT?=
 =?utf-8?B?dXJ6U0N2K1NQVWpsT1crSXoxZ2NnWUtOc2FJNUhYRlU4QzBZSEtYbitad2RG?=
 =?utf-8?B?SHpqZGRlL2xlVVJRdGhPSjgvSG52V0VXbThnOEFyS25JaldEbkk2Y09GK2hK?=
 =?utf-8?B?eUJURTRBbjcvR1VEZXpkK2ZTbnlzMmdCQXZIUERBdnlWZVFoNnl4QXlGb1Zp?=
 =?utf-8?B?amluNXdSWWFkek14U3FnVWRnVGJYQkRRYzlEeDlmVHFMYUdFcDJSWXJzdEE2?=
 =?utf-8?B?SllvQTJlc1ZsWk5xd0JlS216bWhEQ3ZTc0dDdk5qTzJncFZLZDJ0TVVJUGpn?=
 =?utf-8?B?Zjlqa3NwRjF0WHVBcVFBZVFFR0FHQzFoVHM5VkNSTVIvMmFOV09Lb1VXZmc2?=
 =?utf-8?B?bGFtaCtLZFlONFo2TjJNb1JYcU54cGtmNTdKeXNRbkdjWllTVXRlL3BLd29h?=
 =?utf-8?B?Z0N1QTZJaHpmYzI0SXE1YjMzTytTdm04SmN4NHNBSjFjVlErbEp6UXNVWm9p?=
 =?utf-8?B?dUpwRDVJRnZmUFFnblpJNktJN1drdGhacXBHOHVkR3dqNWhFcW0yK2xXcUNs?=
 =?utf-8?B?RllINDA4VHF5eHNIYnlDZlNSeGp4T01neEZpWk5HZXdsK1UrTEZjeE1ZZkNJ?=
 =?utf-8?B?R3NmWXhnUmNqcXNoQW52L2FsRXlpbG1zallsaUxaUENkQU5GN0I5cTN0bGxR?=
 =?utf-8?Q?uFQZ6kSnt8kvIs5wq9Gr2dlJb?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 954e9de2-61f3-45f8-a469-08dc17354bb0
X-MS-Exchange-CrossTenant-AuthSource: SN7PR11MB7540.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2024 08:21:25.4782
 (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: gnmM13Vn4z4dyOZRVDCjASM9EGftXKstWxKiWQdKAv+eiKZbpwpZxS6OkqAotzod48vtVCiCKRVdxPOJzXriGg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7924
X-OriginatorOrg: intel.com

On 2024/1/17 12:17, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Tuesday, January 16, 2024 8:58 PM
>>
>> 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..
> 
> OK, let's add a lock for this.
> 
>>
>>> 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.
> 
> I meant the lock in iommufd_group.
> 
>>
>> Use the xarray lock..
>>
>> eg
>>
>> hwpt = xa_erase(&idev->pasid_hwpts, pasid);
>> if (WARN_ON(!hwpt))
>>     return
>>
>> xa_erase is atomic.
>>
> 
> yes, that's better.

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?

[1] iommufd_device_pasid_attach() -> iommufd_device_pasid_do_attach() -> 
__iommufd_device_pasid_do_attach()
[2] iommufd_device_pasid_replace -> iommufd_device_pasid_do_replace -> 
__iommufd_device_pasid_do_attach

Regards,
Yi Liu