From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Jason Gunthorpe <jgg@nvidia.com>
CC:     Nicolin Chen <nicolinc@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>,
        "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+QmVuzFLJejK7PjtZwgABzeQCACkyk0A==
Date:   Wed, 22 Feb 2023 02:11:39 +0000
Message-ID: <BN9PR11MB5276A3EABA1F00DED4B5B5DF8CAA9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <cover.1675802050.git.nicolinc@nvidia.com>
 <fa81379dca611c1d9f50f9d8cd2bca0d4ec7f965.1675802050.git.nicolinc@nvidia.com>
 <BN9PR11MB5276941167B52F7C0A7A4BA68CA39@BN9PR11MB5276.namprd11.prod.outlook.com>
 <Y+zVliMV/R0ALrlT@nvidia.com>
In-Reply-To: <Y+zVliMV/R0ALrlT@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?nUF+fAsOoUrrcfIJxD5hl4TkkJkeBS4pvRxEW8Dk+s/9C1OmG+iqdtgPBmoA?=
 =?us-ascii?Q?CsWjgxFNnIxjNHMSgzBwnuK410te3p3fBUSfORJb064rB3WyyGbLIEP/MBpa?=
 =?us-ascii?Q?C2MDfMcY4ybCO4z5mQHhCNOJhGmQzRl5ZxwlWKiJtIE/eIDhLoIIV+Q5Uq7v?=
 =?us-ascii?Q?8Nwq/d6Yz8VL6NAtuxvP8CZ8x2AD056/wmErosfzQrQJ0eX3k6TdPigLFNnd?=
 =?us-ascii?Q?imnjHUbIVqE8+9Ep10N1nNk6bAYotZtHZ4SYRKQH1E5yRUL+9qm4PkNnXVNT?=
 =?us-ascii?Q?i0ZG+XDiAu1Jw421El8sXzS5fvYLatNZpUTmFnTgKmZRWZ6MJD6KF0W2aYZP?=
 =?us-ascii?Q?4gQ6J/x411L0AokDF7I4Er0PoLHpN3StBqbaeRlx5UxH8876Mzc2cCnhCpnT?=
 =?us-ascii?Q?7+1MPAorv3F3/Cir8NHGKMy/V+WR/BuQctCagbePk0phOhN0tAbMOMvZfJZC?=
 =?us-ascii?Q?8ZQCRw5TV4Ew7fhwVJE9jqI6KLSfRMMrALIRU2w0WE4jrPi0TdUESqoP5kJL?=
 =?us-ascii?Q?Ff7B/CfanWZi5l3YSDwI+s7Er66LHkm2nG4siCxxG2ePCKImOQ+HHkL/TehX?=
 =?us-ascii?Q?+TD2Kt/3AG0SGjpUTueLnifmBnziyw7VknWK2nAYVJAg2BngnmFfiXtRghig?=
 =?us-ascii?Q?PVv1CAfIphETDWJWdyOBu5ZK3rEIQrtevQ3dMJ1MaE56dGLCV83SFEhqJINP?=
 =?us-ascii?Q?jrBOq8M/Rm/1Wvzef0WgHWFPTz4l4muQ7jnsQolNITD12Rl5MJtkllPOEFW+?=
 =?us-ascii?Q?XF22uud21zv1y2v5BCXPcmI7cx7Q8bMbf/hXFWJAH8SGbyhIekPGNLhFyIpG?=
 =?us-ascii?Q?3sOA2BAww6rsixtLXVGvCOlxp64Ozj9mLKfe1Q7nQDqMzBO4hSMYiKM0HJpd?=
 =?us-ascii?Q?eeYH4sYjyYdVuaSjVAEhOUQIXoC01bgXv0066AhWp76kai97xVH7VZevD7vS?=
 =?us-ascii?Q?mm3Qze6ehCY2gvGtMYSJH7qEYv3fByFrDIh92XDK3R/Zbwa7ZA66y/Kikegr?=
 =?us-ascii?Q?Ot73E+FQ94sdb6FHhsZPGC8P9VaRJ7uPkfVdu4gqYJbK/pQLeOdX6e1jsTWf?=
 =?us-ascii?Q?ugtN6HMJypyeV7lTmnN9iScJG0UP2KrM08hbTR4U6YbphQ/dKayx4os+p5V2?=
 =?us-ascii?Q?aasf2WYR+h7VH4M8I9nS/ZSGcgvP8cvO+MFVEf8XNXlH13EilaZKTqDRbTZs?=
 =?us-ascii?Q?pQMPMg0FcIGQqjD7KwJpn0OuvqAoX1R+t7oIJaFi6AU7aopUxx5FY3Ir+pJq?=
 =?us-ascii?Q?3JkiN3KGxLTL1n5i90SyNMdLBpKOpy5fVzcI8D3sgE30c0eLcNWw965YpJhC?=
 =?us-ascii?Q?LAA0dHQUYFvj2chem2Zd2u6csRYrkYwVsjCrP2f8RjFEaSrNSKjmzh0w9H/r?=
 =?us-ascii?Q?BWgPcz+Wt/a8h2CVQ3dCgxBetuO0o5m52W7Jsckj08QqwfsCZ2I4tCIz1Vwz?=
 =?us-ascii?Q?Q0rZZFxLadghJrbO9QNs7cGN9NPGzpRabRycQcvKY2alkvE/dbA6WzpU4zyr?=
 =?us-ascii?Q?7f0FrvBrbQZK9NB7S+KmWkEDv1GkYQfurRc21IrL47SZhJex94p7avAi8NPr?=
 =?us-ascii?Q?sGPCv8lrh9i/8opwf7WLl29tY7oV2nv0oX/ep2vu?=
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: 55428316-3551-46a8-0917-08db147a2279
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Feb 2023 02:11:39.9342
 (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: qImDmkHA9YBcnv5++j+vm2w/+J6ObS1+zU3qD0W0fY7qTOPVcKDwHdxoxPA7FHJEx06bhM9GPlQV3wnD5frwhQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7252
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, February 15, 2023 8:53 PM
>=20
> On Wed, Feb 15, 2023 at 06:10:47AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Wednesday, February 8, 2023 5:18 AM
> > >
> > > +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(), b=
y
> > 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;
>=20
> We talked about this already, some times this is not the correct
> recovery case, eg if we are going to a blocking domain we need to drop
> all references to the prior domain, not put them back.
>=20
> Failures are WARN_ON events not error recovery.
>=20

OK, I remember that. Then here looks we also need temporarily
set group->domain to NULL before calling set_domain() to recover,
as [1] does.

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