Re: [PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Nov 06, 2023 at 06:09:47PM +0530, Vasant Hegde wrote:

> > I think this driver really suffers from not having the right
> > data structures to handle everything cleanly.
> > 
> > In smmuv3 I added a 'master_domain' structure that linked the PCI
> > device to the iommu_domain. ie when attach is done you'd create a new
> > master_domain that essentially stores the parameters required to do
> > invalidation.
> > 
> > For what is going on here I would say to do that an then put the
> > "domain id" inside the "master_domain". Decide when the domain is
> > attached if the domain id should by taken from the iommu_domain
> > (device does not support PASID) or from the device (device does
> > support PASID)
> 
> We still have single domain concept (at least until we introduce
> vIOMMU). All we are changing is how we allocate domain ID.
>
> Having another domain for each device just to keep invalidation info is
> complicates things. Also IMO its unnecessary.

It is not another domain, it is cleaning up the mess of keeping track
of what caches need to be invalidated for a single domain.

Today we have this:

struct protection_domain {
	struct list_head dev_list; /* List of all devices in this domain */
	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */

(and I'm sorry, but using a global array of iommus and this dev_iommu
thing is an insane design)

Now this adds a new concept domain_id_is_per_dev(), and it still
doesn't support PASID properly!

Instead write it like this:

struct attachment {
    struct list_head attachments_item;
    struct amd_iommu *iommu;
    struct iommu_dev_data *device;
    ioasid_t pasid
}

struct protection_domain {
    struct list_head attachments;


Where every ops->attach_dev allocates a new struct attachment and
threads it on the liked list of the protection_domain.

Then the invalidation logic become completely straightforward, no
confusing mess:

invalidate_iotlb_v1:
   list_for_each_iommu(elm, domain->attachments)
       build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
       iommu_queue_command(elm->iommu, &cmd);

invalidate_iotlb_v2:
   list_for_each_iommu(elm, domain->attachments)
       build_cmd_v2_invalidation(&cmd, device->gcr3_cache_tag, elm->pasid, ...);
       iommu_queue_command(elm->iommu, &cmd);

invalidate_ats:
   list_for_each(elm, domain->attachments)
       if (!elm->device->ats enabled)
             continue
       build_cmd_atc_invalidation(&cmd, elm->device, elm->pasid, ...);
       iommu_queue_command(elm->iommu, &cmd);

Where list_for_each_iommu de-duplicates the iommus from the sorted
list, Michael had a series that showed how to do this for SMMU.

Basically you precalculate exactly the invalidations required and
store it in a list associated with the domain. When it is time to do
an invalidation then you just walk the list and do exactly what it
says.

Jason