Re: [PATCH v3 3/5] iommu: Add verisilicon IOMMU driver

[Benjamin Gaignard]

Le 20/06/2025 à 14:05, Jason Gunthorpe a écrit :
> On Fri, Jun 20, 2025 at 10:57:49AM +0200, Benjamin Gaignard wrote:
> > Le 19/06/2025 à 18:59, Jason Gunthorpe a écrit :
> > > On Thu, Jun 19, 2025 at 06:27:52PM +0200, Benjamin Gaignard wrote:
> > > > Le 19/06/2025 à 15:47, Jason Gunthorpe a écrit :
> > > > > Ugh. This ignores the gfp flags that are passed into map because you
> > > > > have to force atomic due to the spinlock that shouldn't be there :(
> > > > > This means it does not set GFP_KERNEL_ACCOUNT when required. It would
> > > > > be better to continue to use the passed in GFP flags but override them
> > > > > to atomic mode.
> > > > I will add a gfp_t parameter and use it like that:
> > > > page_table = iommu_alloc_pages_sz(gfp | GFP_ATOMIC | GFP_DMA32, SPAGE_SIZE);
> > > This will or together GFP_ATOMIC and GFP_KERNEL, I don't think that
> > > works..
> > I have test it, I don't see conflicts or errors. What worries you ?
> Just looking at the bitmaps I'm not sure? Did you run with lockdep?

Yes and it complains about that.
I see that sun50i driver have more less the same struct than my driver
but doesn't use lock. I will try see if I can remove the lock.

> > > > > > +static int vsi_iommu_attach_device(struct iommu_domain *domain,
> > > > > > +				   struct device *dev)
> > > > > > +{
> > > > > > +	struct vsi_iommu *iommu = dev_iommu_priv_get(dev);
> > > > > > +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
> > > > > > +	unsigned long flags;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (WARN_ON(!iommu))
> > > > > > +		return -ENODEV;
> > > > > > +
> > > > > > +	/* iommu already attached */
> > > > > > +	if (iommu->domain == domain)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	ret = vsi_iommu_identity_attach(&vsi_identity_domain, dev);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > Hurm, this is actually quite bad, now that it is clear the HW is in an
> > > > > identity mode it is actually a security problem for VFIO to switch the
> > > > > translation to identity during attach_device. I'd really prefer new
> > > > > drivers don't make this mistake.
> > > > >
> > > > > It seems the main thing motivating this is the fact a linked list has
> > > > > only a single iommu->node so you can't attach the iommu to both the
> > > > > new/old domain and atomically update the page table base.
> > > > >
> > > > > Is it possible for the HW to do a blocking behavior? That would be an
> > > > > easy fix.. You should always be able to force this by allocating a
> > > > > shared top page table level during probe time and making it entirely
> > > > > empty while staying always in the paging mode. Maybe there is a less
> > > > > expensive way.
> > > > >
> > > > > Otherwise you probably have work more like the other drivers and
> > > > > allocate a struct for each attachment so you can have the iommu
> > > > > attached two domains during the switch over and never drop to an
> > > > > identity mode.
> > > > I will remove the switch to identity domain and it will works fine.
> > > You'll loose invalidations..
> > >
> > > Maybe the easiest thing is to squish vsi_iommu_enable() and reorganize
> > > it so that the spinlock is held across the register programming and
> > > then you can atomically under the lock change the registers and change
> > > the linked list. The register write cannot fail so this is fine.
> > >
> > > This should probably also flush the iotlb inside the lock.
> > I will try to summarize:
> > in vsi_iommu_attach_device() I should:
> > - take the lock
> > - do nothing if the domain is the same.
> > - if iommu is used (ie powered up):
> >    - update the registers to enable the iommu
> >    - flush
> >    - update the link list
> > - update iommu->domain
> > - release the lock
> That sounds believable, yes.. Though can you do the "powered up" checks
> inside the spinlock - are they sleeping? Can they be done before the
> spinlock?
>
> > in vsi_iommu_identity_attach() I should:
> > - take the lock
> > - do nothing if the domain is the same.
> > - if iommu is used (ie powered up):
> >    - disable the iommu
> >    - remove the node from the list
> > - update iommu->domain
> > - release the lock
> And maybe flush too? How does the caching hw work at this point? You
> can't have stale entries in the cache when you switch back to an
> enabled/translating configuration. So either the HW flushes implicitly
> or you need to add a flush somewhere..

I do not have the documentation for the hardware but it seems that
enable/disable are enough to do the job.
 

>   
> > vsi_iommu_suspend() and vsi_iommu_resume() will also have to take the lock
> > before calling vsi_iommu_disable() and vsi_iommu_enable().
> Yes, if they use iommu->domain that seems good
>
> If the above locking is a problem then I'd use the group mutex instead
> during resume/suspend. The attach functions are already called with
> the group mutex held.

Does group mutex is also called when using vsi_iommu_map or vsi_iommu_unmap ?

> > Do I have to switch to identity domain in vsi_iommu_attach_device()
> > before applying the requested domain ?
> No, that is what creates the security problem. What you want is to
> update the HW registers in a way that the HW just changes hitlessly
> from one translation to another, then flush the cache.
>
> Jason