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

[Vasant Hegde <vasant.hegde@amd.com>]

On 11/6/2023 7:06 PM, Jason Gunthorpe wrote:
> 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)

Devices behind different IOMMU can be attached to same domain (like VFIO case).
We do need to track the IOMMUs and as part of invalidation we have a requirement
to send `completion` command to each IOMMU. So this links domain to IOMMUs.

Array is not a best thing here. I have it in my TODO list to change this to
xarray or something. But that's after SVA series as we are already making too
many changes to fundamental data structure in this series.

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


We support PASID only in V2 page table mode. V1 does not have PASID stuff. So we
just have list of devices in the domain. Then each device has PASID table (that
what this series does).

Also as mentioned above we have the requirement of `completion wait` call for
each IOMMU. Hence we track the IOMMU list. Having it per device like above
increases completion wait calls which is not good. IMO above changes
unnecessarily complicates stuff.

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

Ours is domain based invalidation. So our flushing logic is
	Flush IOMMU TLB for each IOMMUs
	If device has ATS
		Flush device IOTLB

	For each IOMMU (dev_iommu list)
		call completion wait
		

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

From driver point of view, fundamentally V2 invalidation is not too different as
we have single invalidation command. All we need is few extra param to tell its
guest page table invalidation with PASID.



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


Driver already does this.

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

I think we have most things already in protection domain. Only extra check we
have is checking `pdom->dev_iommu[i]` which will be fixed separately.

-Vasant