Re: [PATCH 06/20] iommu/exynos: Implement an IDENTITY domain

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, May 03, 2023 at 04:31:39PM +0100, Robin Murphy wrote:
> > -				    struct device *dev)
> > +static int exynos_iommu_identity_attach(struct iommu_domain *identity_domain,
> > +					struct device *dev)
> >   {
> > -	struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> >   	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
> > -	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> > +	struct exynos_iommu_domain *domain;
> > +	phys_addr_t pagetable;
> >   	struct sysmmu_drvdata *data, *next;
> >   	unsigned long flags;
> > -	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
> > -		return;
> > +	if (!owner)
> > +		return -ENODEV;
> 
> That can't be true - devices can't be attached without having already
> dereferenced their group, which means they've been through probe_device
> successfully.

Yes, the current code is wrong to check has_sysmmu(), I removed it

> > +	if (owner->domain == identity_domain)
> > +		return 0;
> > +
> > +	domain = to_exynos_domain(owner->domain);
> > +	pagetable = virt_to_phys(domain->pgtable);
> 
> Identity domains by definition shouldn't have a pagetable? I don't think
> virt_to_phys(NULL) can be assumed to be valid or safe in general.

Read again, the first if excludes that owner->domain is identity so
it must be paging. Thus pgtable mst be valid.

More broadly the struct exynos_iommu_domain is now always a paging
domain.

> >   	mutex_lock(&owner->rpm_lock);
> > @@ -1009,15 +1015,25 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> >   		list_del_init(&data->domain_node);
> >   		spin_unlock(&data->lock);
> >   	}
> 
> This iterates the whole domain->clients list, which may include other
> devices from other groups belonging to other IOMMU instances. I think that's
> technically an issue already given that we support cross-instance domain
> attach here, which the DRM drivers rely on. I can't quite work out off-hand
> if this is liable to make it any worse or not :/

Yeah, that looks wrong today - it should be a strict pairing with
exynos_iommu_attach_device() so it needs to check if each
client's sysmmu_drvdata is in the exynos_iommu_owner->controller list.
Marek?

At least this transformation shouldn't make it worse as we will still
call this code in all the same places as we always did. The identity
domain is also not threaded on this list.

> > -	owner->domain = NULL;
> > +	owner->domain = identity_domain;
> >   	spin_unlock_irqrestore(&domain->lock, flags);
> >   	mutex_unlock(&owner->rpm_lock);
> >   	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
> 
> This no longer makes much sense.

	dev_dbg(dev, "%s: Restored IOMMU to IDENTITY from pgtable %pa\n",
		__func__, &pagetable);

Better?

> > @@ -1457,6 +1458,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
> >   		INIT_LIST_HEAD(&owner->controllers);
> >   		mutex_init(&owner->rpm_lock);
> > +		owner->domain = &exynos_identity_domain;
> 
> I think strictly this would be more of a probe_device thing than an of_xlate
> thing, but it's not super-important.

The full code block is this:

		owner = kzalloc(sizeof(*owner), GFP_KERNEL);
		if (!owner) {
			put_device(&sysmmu->dev);
			return -ENOMEM;
		}

		INIT_LIST_HEAD(&owner->controllers);
		mutex_init(&owner->rpm_lock);
		owner->domain = &exynos_identity_domain;
		dev_iommu_priv_set(dev, owner);

So we set the domain at the same time we allocate and initialize the
owner struct.

Why this is allocated in of_xlate is beyond me, but it is the right
place to put this initialization, we never want NULL to be stored
here.

Thanks,
Jason