Received-SPF: pass (google.com: domain of linux-kernel+bounces-110113-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <a477bb58-951c-491a-b4cc-b5cfe9cca646@intel.com>
Date: Thu, 21 Mar 2024 21:58:08 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/8] iommu: Introduce a replace API for device pasid
Content-Language: en-US
To: Jason Gunthorpe <jgg@nvidia.com>
CC: Baolu Lu <baolu.lu@linux.intel.com>, "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>,
	"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: <c831bf5e-f623-402d-9347-8718987d1610@intel.com>
 <BN9PR11MB52766161477C2540969C83568C242@BN9PR11MB5276.namprd11.prod.outlook.com>
 <585423de-9173-4c97-b596-71e1564d8b4e@intel.com>
 <87a2be0d-6a24-4ca8-be30-35287072dda4@linux.intel.com>
 <749b23c7-ab0e-42b4-9992-e1867fc7d4d7@intel.com>
 <20240318165247.GD5825@nvidia.com>
 <13645a9f-239a-46c9-bde2-a1d5c365df4f@intel.com>
 <20240320123803.GD159172@nvidia.com>
 <65c517a9-72dc-4342-b2f2-c3c44735bfad@intel.com>
 <7a4777dd-2359-4bcd-839e-c2d0b5f6be14@intel.com>
 <20240321122059.GF159172@nvidia.com>
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <20240321122059.GF159172@nvidia.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?Q1IvS0JOcHNFdDB5QVhFRlhGQ3o5Rlo2U3Ztbk14SXpJc2V0L2xqeFh6eUtM?=
 =?utf-8?B?ZzdxZldvT3hMaUlXTUxpNVF3aFJNTWIyVmY2S2hVRWRXSjQySzdTN1I2ZStX?=
 =?utf-8?B?amkva2FOYjhKRkxWS3cwbzk5cDBJS05YQXQ1ZFNWc05LeS9QZlpSb3l5V1dC?=
 =?utf-8?B?Vi9zbC80bzFLeGx6Y08yZHFRL3ZNN1BrM3Rlc1oydHpKUjJWNjRDNkUzcnZW?=
 =?utf-8?B?T2VmT0Z0OW1CdThadkluSWs3d1Q0SFRTZnREVEM1L1RzRHhqY0diSFQ1UzAx?=
 =?utf-8?B?VHRWbm9jRmxMSWdDNjBhS3VvUk5xOVlBVE1NWWVDd3FKbVdFaDBIS0RDLzE3?=
 =?utf-8?B?OG0rSXVydFpVM0piWXd3U3ZwaVdudTBQWnZrUGRGa3E3Q005Nkx4a1BhL0pC?=
 =?utf-8?B?RThBSVMyQWhhalk1dm9XOXp3RDZiZFRXOHAyRjFNMzVtSG94ZjlwTjB1YURV?=
 =?utf-8?B?bFBwakZJd1hhKy9MRWRQcXVvSVJqVkh5NGV2c0NjTkgwSkd0SzlXMUNSMlk2?=
 =?utf-8?B?d2hkblVMcy9WbFZPdzlJd2VFS05zSUJlN2E2dDBnNzZxTjZEU0grY2s4ZGFE?=
 =?utf-8?B?OGcwSGV5cm5GNTdGV0tGM1ZTd2Q1STdQSWptZEt6STdEQWw3SFdVUzFRNVF6?=
 =?utf-8?B?SDRYWVZLczRYV1k1WU4yWnAzZE9ubmxsd0krRDByRmlqSXZDRU5iMEFPSlow?=
 =?utf-8?B?aTcrUlV0UE9jc0czZTg0WGRVdDFUelg5Rnh5T3ViNzZRVTJuTVpzNm5pRUo3?=
 =?utf-8?B?YUxtTzRPSWY3V2lKM0w1eWJJQjM4ZElrSW5qbkhWTnYra2JhRHBERHA5d01a?=
 =?utf-8?B?QnQxbTJCL3Y5cVlIMGptK2xOeGpJQk00Z2pnd2lhZVZaRU01aE9BRm5NTE9T?=
 =?utf-8?B?TmVGRzFZaTNJQ2s5Tkpra21QVjR3Z29jeW82bnJncjlyUi9kZnVEdXYwNkxn?=
 =?utf-8?B?ZnRCVjdqbDBpaDlpbFFPZ01aOVVoNDZVVVFRQ1prMml0K1FBVHh2QXFPOVRY?=
 =?utf-8?B?ZjRGTDB3QnRHU0xwWlg3Y2NUNG5ybm9nbkVGRWlZeHJYaFhzZE1MWExORStm?=
 =?utf-8?B?QkUyeGV6dHp3RWtQcWN0OVhNVUZQNTdlOUdRdk12NWRlS1VNd1JSbUxNVXJX?=
 =?utf-8?B?a3ZqVC9KemV0a1dqS20vY2dId3VOMkRwTUY3d2l3bUxhb3VuK3VSS2lXSmdM?=
 =?utf-8?B?QWhCaGg3aGs2VU92RndTWXVWcWdOaE5ZS1ArUDlzZys4ZnJCNTNhVVpVV2Rq?=
 =?utf-8?B?TXZDMjRzZVM2azRzUGdGSjVxUmUzaUNWYTc4QWM2VVU2VDI5QzNha1BUUXZP?=
 =?utf-8?B?WHZTYW41d2ZoNGpEbmxOYmk5Z21DZ2JHRGl3U2hmaVBjOEFaTTFyOG5PNmZQ?=
 =?utf-8?B?QWhQb0JVYXZ6aVE0bUcwZ3VpTGRuajd5Z0hTdUcreW9sbGV6WlpPTldKRDVZ?=
 =?utf-8?B?Y3J1RUJTd1NCQ0tRVnRuRGg1TGF5a2xXYkM3OVdBb1RDZ21ndGtEN1pMamVF?=
 =?utf-8?B?cU9neGp4Y1IvMFdjekR5YTJ5Y1YrTE9ZajdQTnYrTmJuTHF0Qzc2Ry9oUGVk?=
 =?utf-8?B?elUyMnkvY2M0VlQramRnS0U5UjNFajZ5VW92Mkd1WU5RR1Y5U2g5NmJRWWZx?=
 =?utf-8?B?WTV4WW1GOVp3WkJOL01LOTFySjZ6YU45NUd1Y3pvNG1vNFhoaFhpcHdDN05q?=
 =?utf-8?B?elc3VlF5eHB1SHdZdG53MVNFc1kvaW9aOWVBUVVYZjFTcXFpMzZYa3VGUExj?=
 =?utf-8?B?dkFXZFM1R2wxN2ZsRW51MURqelRlWC9rZnhZRHE2RWFOdCs3Rk9rU0RlSmNa?=
 =?utf-8?B?T0M5WGc1N3NmcWNzQ2wrM25UUDlmR1Zzb01KclkzdzJmN3RoMDFlY2xJcVhJ?=
 =?utf-8?B?Vkc1RG5xYWVBNEN2WWJFR25CZW1pZ3NhRS9FRjlldE9mUjVyU3dPTHQzQzUr?=
 =?utf-8?B?RG05b0JzUWx1N3ZJaGJMSm9UbFVkaDdqVm1HN3RXNFB2UHZCMnJEdlNBUXZs?=
 =?utf-8?B?am5wdTRsUnZjajNMdkV3OW5ZVWFXZ1J2cDVkQmdVZ0FidUVTU0E2ckFrUmdt?=
 =?utf-8?B?UGgvSVNJVXpoS09jcGpjRTAvZ3pncUdFcjlpTmNsT0NXd2VYV3Z2enNLVmhm?=
 =?utf-8?Q?7Gl89QdHJSZL//z94U3+aTmY1?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 388047f2-0487-47c0-6f5d-08dc49ae7582
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2024 13:54:42.7807
 (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: pP5ZRgbD8IN5CoR6wWdkBMiLiwBoECAbtYYF5jOYUuJ4kbYE4nZwiAo0k6Ko90OXKr+Ul7RjOVp1Ui3hvH7kkA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8762
X-OriginatorOrg: intel.com

On 2024/3/21 20:20, Jason Gunthorpe wrote:
> On Thu, Mar 21, 2024 at 07:26:41PM +0800, Yi Liu wrote:
>>> yes, the correct way is to undo what have been done before the fail
>>> device. However, I somehow remember that pasid capability is only
>>> available when the group is singleton. So iterate all devices of the
>>> devices just means one device in fact. If this is true, then the
>>> current code is fine although a bit confusing.
> 
> Platform devicse don't have that limitation.. It is PCI only.

ok.

>>>> And the whole thing is easier to reason about if an input argument
>>>> specifies the current attached domain instead of having the driver
>>>> read it from the xarray.
>>>
>>> yep, will correct it as a fix patch.
>>
>> Hi Jason,
>>
>> It appears there are two solutions here.
>>
>> First, only undo the devices that have set_dev_pasid successfully in
>> the __iommu_set_group_pasid(), so the problematic
>> __iommu_remove_group_pasid() call at line 3378 [1] would go away.
>> This also makes the helper more self-contained. Draft patch in [2]
>>
>> Second, pass in the domain to remove_dev_pasid(). Draft patch in [3]
>>
>> Either of the above two should be able to solve the mistake you mentioned.
>> BTW. They are orthogonal, so it's also possible to apply both of them.
>> Which one is your preference then?
> 
> I would do both because I also think it is not nice that the drivers
> always have to have the boiler plate to read the xarray in their
> remove..

sure. I'll send the two patches as Fix series.

-- 
Regards,
Yi Liu