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+QmVuzFLJejK7PjtZwgABzeQCACkyk0IADEJoAgAB38jA=
Date:   Fri, 24 Feb 2023 08:07:41 +0000
Message-ID: <BN9PR11MB52761AE29AC1B52D2BFF45DC8CA89@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>
 <BN9PR11MB5276A3EABA1F00DED4B5B5DF8CAA9@BN9PR11MB5276.namprd11.prod.outlook.com>
 <Y/gLiNs42UyO58zJ@nvidia.com>
In-Reply-To: <Y/gLiNs42UyO58zJ@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?hIZm7FW2t/EiW5o5qk3sDoY2NNTrJhLuDKu/vodqDZ8fFO6T6euD15GP7S4I?=
 =?us-ascii?Q?jgEhzlYVDTYO+b4ioR1CSwN+U7s120W/DIO70iNO1jaesaxEk8R7QfIDbBzf?=
 =?us-ascii?Q?P7EoEsewVdxq3TCgaMzySjIfFb+tgU36r/ZCieV2++eD36ZJ9ZE1B3C6SlDZ?=
 =?us-ascii?Q?NcG64o0eMjetBeOM2zbcT5nruOd2dHWMzFW4qSfzjOYXo50+uMieo+cTIkjk?=
 =?us-ascii?Q?I19NCnXaTZ9Zg+6rMG+DtBmw/przokNyie3z/qSxREMtQnANhmQowHT1a161?=
 =?us-ascii?Q?OXOJLjA0EpySkffrRNVFB0W4RPAaLe7R9wjBQ9eyIJK9juSE+TF/d47LjAwU?=
 =?us-ascii?Q?Pwz/65WMwSrrdSNEI2XqmA22S11LrWsK9tiUOs1dbGd9MWS1me8n4bbba4yC?=
 =?us-ascii?Q?0KT7rP0dvcPDK4XcnmgWvs2bH5GT24XhDV0PnuRiIPLCfcFiDXP9x7aDWmQq?=
 =?us-ascii?Q?STFT6zC8JVOUCJIlsxmCiSWks4er9TivbhIbNeb0L4sn6ZVmsi1Ws/hxBBrU?=
 =?us-ascii?Q?zjY6SIlUDUJa2HBK/UmUtAp0iurtD/eoLnW5SSa4ckG5b2qJJfqh6ZUYzMWh?=
 =?us-ascii?Q?L3Wvs12PrOM9HMN19MSOF8zJN/sqyrpQhY2yCE9qLqyNG9f6jAZLZS+RROAv?=
 =?us-ascii?Q?PmdtBdJ7J2a+dn8j1W//S+t+Ifgd8Z9s/rZnBZzuqgIKAOseQa/6j4RaVRwD?=
 =?us-ascii?Q?og/NHWktQSDCHYs2Z3remYk5lVml4lXHSh7ovXI158VUqo8ZQEA7LRYpw6Ng?=
 =?us-ascii?Q?RrWPNzb6dektdnMQYXkaD3hvZ/Qe3CDABCQPLNNeEL5GMjrZq+EZo8rT6roM?=
 =?us-ascii?Q?2Rnxc0GsTAUTxLI5BKCKdpVsf6dCGVO/yUn6kANU4c5XkF9VmdzFTBFYDnP2?=
 =?us-ascii?Q?6Ev7KNTzuOVo2NGwtCB03HaaCK4uwQO79FVASspIouFOWD+QWSLsUgdbEFjn?=
 =?us-ascii?Q?Th2q/xx9wRTnS6FrPAPdL3qivUJbbNSKod0OFegPDvAXeRmxE70+JJ+9UYhk?=
 =?us-ascii?Q?STXR9Bpgc7s+HTYvWoBnoVChxTzWGZkpRUwnUXcPv1kamVllIXEuyVeKBIqh?=
 =?us-ascii?Q?s0+HtYR3mdkf2L5KuMrLFCfxzerCH5RnbznVi9LuwC8+V3VehlKY0AvlGvN2?=
 =?us-ascii?Q?Ti+IB/bV4nvvyzAu0QTCRmpK7NKAdR5FAgaevIkuATvNUf6TPWu0miqqq5TE?=
 =?us-ascii?Q?j+UzTz4NFC2IdNWFOKTGwwlwYuzd6zqEnNwW7WFATcCJhwSknNMCnOZrR1A3?=
 =?us-ascii?Q?UDhr48mXbG1CymyuCDAIoNiXtLB+Fp9AdrC0TdoLgTF3UaZKEGFA22LfwH4p?=
 =?us-ascii?Q?TyU9iugd1ptIp81QFpBH5YU9eAsq3tk4+N4iBILf0fCsfHnLhvKKp6pQH4eX?=
 =?us-ascii?Q?aSm6WLuaHkXgyHZHinvZ/HyGaXru9KZFna8nE4Cimng7ZFVFg1UoNZk4mBQ5?=
 =?us-ascii?Q?zyAczlmyQz9C4kged3lPfR6UGdnjXiEhC7nIe8zJTI0eHWw06w/RxoIatEHS?=
 =?us-ascii?Q?ry58UTRfNMn05NT+1aJ5TUaROPZ1JLBA7Db6p54XWVikqsaNSbflu3xbkFOl?=
 =?us-ascii?Q?wNYKxKJQKrfuu/k8fdbGEHefItgHakIY+sdnYU3z?=
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: 1fe7615f-ae6d-498a-d137-08db163e33b4
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Feb 2023 08:07:41.4172
 (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: mhkOC/pr5RCF/MTknOBqPhcITobGgFcmmvFttmDTSA61op5yHvkizZFjomJd6rVBNYHFUHfkeOHPdEMvgvh6Ew==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7695
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: Friday, February 24, 2023 8:58 AM
>=20
> On Wed, Feb 22, 2023 at 02:11:39AM +0000, Tian, Kevin wrote:
>=20
> > > > 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;
> > >
> > > 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 dro=
p
> > > all references to the prior domain, not put them back.
> > >
> > > Failures are WARN_ON events not error recovery.
> > >
> >
> > 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.
>=20
> Sigh, this is too much.
>=20
> I made a series to clean up all the domain attach logic so the error
> handling is all in one place and all the same.
>=20
> What do you think?
>=20
> https://github.com/jgunthorpe/linux/commits/iommufd_hwpt
>=20

Yeah, that sounds a right cleanup at a glance.