Re: [PATCH v4 15/17] iommu: Allow IOMMU_RESV_DIRECT to work on ARM

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 20, 2023 at 08:00:28AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, April 12, 2023 9:52 PM
> >
> > @@ -2918,6 +2922,17 @@ static int iommu_setup_default_domain(struct
> > iommu_group *group,
> >  		}
> >  	}
> > 
> > +	/*
> > +	 * Drivers are supposed to allow mappings to be installed in a
> > domain
> > +	 * before device attachment, but some don't. Hack around this defect
> > by
> > +	 * trying again after attaching. If this happens it means the device
> > +	 * will not continuously have the IOMMU_RESV_DIRECT map.
> > +	 */
> > +	if (direct_failed) {
> > +		for_each_group_device(group, gdev)
> > +			iommu_create_device_direct_mappings(dom, gdev-
> > >dev);
> > +	}
> > +
> 
> Throw out a warning so user knows that the continuity might be broken
> here?

I suppose we should also fail the whole function if we hit failures
here?

So like this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f44e2c21e9d05e..7a3b249601ea49 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2873,8 +2873,12 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	 */
 	direct_failed = false;
 	for_each_group_device(group, gdev) {
-		if (iommu_create_device_direct_mappings(dom, gdev->dev))
+		if (iommu_create_device_direct_mappings(dom, gdev->dev)) {
 			direct_failed = true;
+			dev_warn_once(
+				gdev->dev->iommu->iommu_dev->dev,
+				"IOMMU driver was not able to establish FW requested direct mapping.");
+		}
 	}
 
 	/* We must set default_domain early for __iommu_device_set_domain */
@@ -2906,10 +2910,20 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	 * will not continuously have the IOMMU_RESV_DIRECT map.
 	 */
 	if (direct_failed) {
-		for_each_group_device(group, gdev)
-			iommu_create_device_direct_mappings(dom, gdev->dev);
+		for_each_group_device(group, gdev) {
+			ret = iommu_create_device_direct_mappings(dom, gdev->dev);
+			if (ret)
+				goto err_restore;
+		}
 	}
 
+err_restore:
+	if (old_dom) {
+		__iommu_group_set_domain_internal(
+			group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
+		iommu_domain_free(dom);
+		old_dom = NULL;
+	}
 out_free:
 	if (old_dom)
 		iommu_domain_free(old_dom);

Thanks,
Jason