Le 20/06/2025 à 14:05, Jason Gunthorpe a écrit :

On Fri, Jun 20, 2025 at 10:57:49AM +0200, Benjamin Gaignard wrote:

quoted

Le 19/06/2025 à 18:59, Jason Gunthorpe a écrit :

quoted

On Thu, Jun 19, 2025 at 06:27:52PM +0200, Benjamin Gaignard wrote:

quoted

Le 19/06/2025 à 15:47, Jason Gunthorpe a écrit :

quoted

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.

quoted

quoted

quoted

quoted

quoted

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

quoted

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.
  

quoted

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 ?

quoted

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