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

Jason Gunthorpe
2023-04-26T13:36:25Z

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