From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Nicolin Chen <nicolinc@nvidia.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "joro@8bytes.org" <joro@8bytes.org>,
        "will@kernel.org" <will@kernel.org>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "shuah@kernel.org" <shuah@kernel.org>
CC:     "Liu, Yi L" <yi.l.liu@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "baolu.lu@linux.intel.com" <baolu.lu@linux.intel.com>
Subject: RE: [PATCH v2 02/10] iommu: Introduce a new
 iommu_group_replace_domain() API
Thread-Topic: [PATCH v2 02/10] iommu: Introduce a new
 iommu_group_replace_domain() API
Thread-Index: AQHZOzoL2+M6IjP2sU+QmVuzFLJejK7PjtZw
Date:   Wed, 15 Feb 2023 06:10:47 +0000
Message-ID: <BN9PR11MB5276941167B52F7C0A7A4BA68CA39@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <cover.1675802050.git.nicolinc@nvidia.com>
 <fa81379dca611c1d9f50f9d8cd2bca0d4ec7f965.1675802050.git.nicolinc@nvidia.com>
In-Reply-To: <fa81379dca611c1d9f50f9d8cd2bca0d4ec7f965.1675802050.git.nicolinc@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?qV3nHsfwX8KECC1uGgE75VMpoDdfyAiL4YJNAjES5/FdAjTL4ZXSQf4sppqy?=
 =?us-ascii?Q?FcfjULHMdMQxmaZPA8aB7xQXRtQi8az07cxVyjj3koGHGUyuRBWUKzux5yN8?=
 =?us-ascii?Q?7c2RMZ2c4QEuIPnmEDdVG3ymlgPExzrqIJ+zi/QhieIFzP1AElvZFUe2so9p?=
 =?us-ascii?Q?A+oQJibxVA1YmIcEpXpKu4bTy4p623MYRnBwm/ttSk8UqquP6VUoD275GbO8?=
 =?us-ascii?Q?/96zHcmkPU9DLvS/yGzNfJJo8U58H/QMBa41OJVxLprGBZQk1UWnQWuc8iFP?=
 =?us-ascii?Q?RunjdIY0bOKqpkm0G86XsCpMqLxdOY8qtH9Wo4dY1EZo5Yzo/Pi264XSbfee?=
 =?us-ascii?Q?RQ55rhx5k2uNTjjqTDsyQSUprrR916TtO/JUa+cojJ7dizEeMc2yaKUWzCKn?=
 =?us-ascii?Q?UGe4BU8SRLoHe63+s4+uOZ2JM6tiJtN5gBpgzQUZ6yOPzXP3mPe9OiEi6RLG?=
 =?us-ascii?Q?6NMihXgImzEoWqZG/DdZnIdictzIQrYt2iOcfCz7tjrehzDxn0fjn22BiMsV?=
 =?us-ascii?Q?MMWGZ4IU+dbtY4Il3NOMjSNVz+qpMs0MjWyLdtTYiFQWlCGB4WILL4zyaYpU?=
 =?us-ascii?Q?UAglXnFMs3YmnGSbmcl7zTrs+a7WATtJHBUW/I8ZJVABmQiAI3qw6BulRwzM?=
 =?us-ascii?Q?ojO91PujFm0Vj02Xo3Vx6KZb12ZlVXFhiq4xAMyReLBuaJC2LAQc/18b/KQp?=
 =?us-ascii?Q?uPb6ji03fiXIm7F1vgGcu8t/3mXuO3MYPHX/MFCPAk713OU0jYDlvFdnUbOn?=
 =?us-ascii?Q?OoCnPeYBhAVAh6Ah+kTam8zT+8xu7bmWUMbAxz+izu5KyYi2Mbjasz5gm2Vp?=
 =?us-ascii?Q?SXL98UAsyrTkgw2mVnFfNuX/bGpAGMgEpy4qnoh/GaXjMgUeg+0AVhwI1GB4?=
 =?us-ascii?Q?ddKb0K8XC7rpRV0EKuX2CoBgCK1EAIun6enmALqgnqqTvCONIkY26LYY2MD1?=
 =?us-ascii?Q?umZAPdqioXTrQSWw9cwztA+RGZiVd61C5symtgzu/gR4SuEQJimCsEKMOMdm?=
 =?us-ascii?Q?fvqi2VvKxZoKpD33dZ2j7yqM70MxtqkHO75GrEWX3HiECMva3rCfg6aBvuAc?=
 =?us-ascii?Q?Mks39cw7ESuAa4eV2tdVvHSdWrKdZwdTikJUFnxeL0mkciZG4WhWCwwkUb3P?=
 =?us-ascii?Q?aoVhnC/FJ20lD6cUEMQZ8SFutcSvRy76kMmZ4mdUu/VEdtNPt+jHfN/DjO8W?=
 =?us-ascii?Q?VfIuFFEAz/LTMQ6QqitT8WsRnu1gxC2+Ej1vt4JSDEjiGyc4PZdBumgqGB6m?=
 =?us-ascii?Q?EKkFaSbDEkOvL3EnFA7pqO9srHohRAlLFuk1jp0639rmaFD3FW/HKKQg/Wvu?=
 =?us-ascii?Q?EMTYUHh7XlsgheYrsu3rzD+P1346wtaD+Kucqpu0f5sRss1gC8jxNzHmZ4uv?=
 =?us-ascii?Q?AZ6LV98aTNUW6pszBQ6fdlO8+0Utprle4isrkGAC+a3u+Zh/LNSDB1Gsc5sU?=
 =?us-ascii?Q?X4/IVwWxhPF9y+WUkRLEPQAtHrc0/0cvg2L56mzqtGYtl3HksGKfwd8H31ap?=
 =?us-ascii?Q?JGoNdKcbTRLYvyb580xGOulzjSFpUWtl7dqxR8qn6Ez+UL8bjpLt3KAxH27H?=
 =?us-ascii?Q?VnGv3lPe1KshQPG3m2KwUSz30BLVhDKK5EtxZnDz?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
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: 8638d591-5931-41b7-cbce-08db0f1b6127
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2023 06:10:47.1120
 (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: 4Lquvn7wS7T6BuzF6afsiQh7pYz5BeUkIDmUTT3plXmNCvsT1oMDHQ0jHdbMZXyiFCsdPCchCwGzOc9cqoTx7w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7545
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, February 8, 2023 5:18 AM
>=20
> +int iommu_group_replace_domain(struct iommu_group *group,
> +			       struct iommu_domain *new_domain)
> +{
> +	int ret;
> +
> +	if (!new_domain)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +	ret =3D __iommu_group_set_domain(group, new_domain);
> +	if (ret)
> +		__iommu_group_set_domain(group, group->domain);

Just realize the error unwind is a nop given below:

__iommu_group_set_domain()
{
	if (group->domain =3D=3D new_domain)
		return 0;

	...

There was an attempt [1] to fix error unwind in iommu_attach_group(), by
temporarily set group->domain to NULL before calling set_domain().

Jason, I wonder why this recovering cannot be done in
__iommu_group_set_domain() directly, e.g.:

	ret =3D __iommu_group_for_each_dev(group, new_domain,
					  iommu_group_do_attach_device);
	if (ret) {
		__iommu_group_for_each_dev(group, group->domain,
					  iommu_group_do_attach_device);
		return ret;
	}
	group->domain =3D new_domain;

Thanks,
Kevin

[1] https://lore.kernel.org/linux-iommu/20230215052642.6016-1-vasant.hegde@=
amd.com/