Date:   Wed, 15 Feb 2023 08:52:38 -0400
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     "Tian, Kevin" <kevin.tian@intel.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
Message-ID: <Y+zVliMV/R0ALrlT@nvidia.com>
References: <cover.1675802050.git.nicolinc@nvidia.com>
 <fa81379dca611c1d9f50f9d8cd2bca0d4ec7f965.1675802050.git.nicolinc@nvidia.com>
 <BN9PR11MB5276941167B52F7C0A7A4BA68CA39@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BN9PR11MB5276941167B52F7C0A7A4BA68CA39@BN9PR11MB5276.namprd11.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?VLDy1il7TsSMhKbPNPEx6o/kFONDjuXDu4+6vBPxNdg+ktWsGVBcGMh8gNsE?=
 =?us-ascii?Q?J4+vxVkWKCCMWXSV0Q5bXFY75g1Wj0khfeWVkXRxnqiqVhabCdPauDYX1WvL?=
 =?us-ascii?Q?BTVPHCIxM4FT37yCKUkDgjOBWTPagT3f7R96NGOoWrC8ZvKI5KUCBor8tB4F?=
 =?us-ascii?Q?VVTLzC156ZIcKRoSP0h7x1LCX+cVDrrgkSU9zSMaKi6128Cu2+WEDXXFvpjK?=
 =?us-ascii?Q?CPjhb5HZuVYl32Ru2UjvU60sb9U69QK0Ebvod2V9slY1rEdV3jVW4eBlJE7w?=
 =?us-ascii?Q?OQhIKYdYfKg0iypd9xtTYOWlE11upS9O+Slal+jghB14Qs/gGD9fntUPsFtb?=
 =?us-ascii?Q?MzONpCEim33GHwDKpxY19L/aGj7BDgAW/9tVcBuwnHmEk3syYV/Fp1IXPAya?=
 =?us-ascii?Q?uuU6oLEri8fFzcTJNVjDjpFsbxXxmuys3U4Qel6ktHxlNKs4v91srasgKx1K?=
 =?us-ascii?Q?huAlDx01wtYIaqi2NiBZYSZzihqEA/NZwsFOmxgC1PJ5s0bEPE0qLW9nYjQP?=
 =?us-ascii?Q?nE26EgXmC8b+CfWT/YpSD7SRcOcbPawU3KgBBj5g2mBudxgXIRT94zcaAiFe?=
 =?us-ascii?Q?tjuq1jCRYp8iLvDxsqaK/LmXAxkn0jamQ5Qk+Tu9Ht4SqVagbh2T0mSOc9xM?=
 =?us-ascii?Q?ehkfdnmhVsfakMBRRcK9twbWxg+AZM0baNcpttrNkMfh8E4ALizKU2hqhGze?=
 =?us-ascii?Q?HpxHaBjD3AixxCN2qvYxiVhhMQkqLIJNaTBrcV/DsWZIeL/Ew27Tf2CjSfVf?=
 =?us-ascii?Q?RGqXyjgi9Y8NKBLrdFziEORl+8r49h+AVl9ZwerSufmeKGhMpQbBnYHNfjC5?=
 =?us-ascii?Q?k3I6fZxroRVeYi7NLP5ecCV63UE7wcgYVH0ZvU5L/Ic5BNHLF93AAGpfWkJc?=
 =?us-ascii?Q?ieN/+nXQ+Eluh0dnwLTe7bF5ry4OH7JE93vbckflLQgBt8dOFqP66egUhzDj?=
 =?us-ascii?Q?shoDGkeDQJKuYs7ENtG95DKCCUn7NsngwiBKRhIW4k0XgW7hSX8iB9VpAn6F?=
 =?us-ascii?Q?FM2ySsiycm1TMowKtWFfOUcyGf6wikcwPfPzWV7gG5966LCIaFpwNfEpjUco?=
 =?us-ascii?Q?mI/zAjSbdfney9br/tzASJL1IKJHJ7p5FCz1SLf46tFTa+CYuLNFou3svDV3?=
 =?us-ascii?Q?EBdL5JzKUxD4VQZRu79dKXHhQFEIRbu4mZj2LaQnbsO7ZEkgU5e8C4KyKGEi?=
 =?us-ascii?Q?w9rFwVptRB9ubd7a5OfZvdNFO8MN3oMnGxNhurVQwbYPgRnK0E7GtRDKQzt7?=
 =?us-ascii?Q?esSnboBxKHZHUDUoya69l+NYQiL+vPPvvLcBSybVu4lQ42xQqYS6MFKniJ7I?=
 =?us-ascii?Q?vZFTri7aWANjYg1vjlXCKSJKwKH4dr0VZv07QWhLcR9xCCfhkZSqkGqwQ8i4?=
 =?us-ascii?Q?CD/UCScK4hRgIa7nlzbmxepxWfb9pLQkoUA3unBfaUIm1RL3WN8h9NY1EfgY?=
 =?us-ascii?Q?a+Oqd9YExdE1ZAj5reR0En9t9Xqbz+yq9WSjQl91rsGGuPnzHGzLJ3FUAL+0?=
 =?us-ascii?Q?bft9IhfgeUUR3WCZ/YoNRqccelHU3rRJYt85A6NdyZAzIuU14Ir/c3+57X83?=
 =?us-ascii?Q?tzbgWpUro7FX2HTPtW1+ujCRFIekkbfKC2+c43rp?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 39bf6b21-e462-470d-bdd7-08db0f5384d4
X-MS-Exchange-CrossTenant-AuthSource: LV2PR12MB5869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2023 12:52:39.3293
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /VkbmFghUA01e4go0DWsZiGzIu9gMJZkBtT87SAOiMfJtkv18Xx/t1N4NwYuv0Ze
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB7890
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 = __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 == 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 = __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 = 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 drop
all references to the prior domain, not put them back.

Failures are WARN_ON events not error recovery.

Jason