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

Tian, Kevin
2023-04-26T23:48:18Z

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, April 26, 2023 9:36 PM
>
> 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?

agree

>
> 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