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/7/2023 6:51 PM, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 11:00:18AM +0530, Vasant Hegde wrote:
>>
>>
>> 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.
> 
> I understand how it works.

Great.

> 
>> 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.
> 
> 'unsigned dev_iommu' is the problem not the array.

I know. I will fix this after SVA 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).
> 
> Doesn't matter, v1 uses the dev_iommu and the point is to consolidate
> alll of this.
> 
>> 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.
> 
> It is not per device, it is still done per-iommu. I wrote:
> 
>     list_for_each_iommu(elm, domain->attachments)
>         build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
>         iommu_queue_command(elm->iommu, &cmd);
> 
> Which is the same work as iterating over protection_domain->dev_iommu,
> the iommus are extracted from the device list which is needed anyhow
> for ATS, PASID, and V2. So just use it everywhere.
> 
>>> 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
> 
> This is what I wrote.
> 
>>> 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.
> 
> From a SW perspective it is totally different because V1 invalidates a
> single domain id per IOMMU and V2 invalidates a domain_id&PASID for
> every device.

Right. I'd prefer to do it in steps. I would like to limit this series to
current form. (GCR3 movement and domain ID). Then will have separate series for:
  - reworking protection domain structure
  - Handle domain allocation path (adding BLOCKED domain, some cleanup etc)
  - Fix attach/detach path
  - Other enhancements like PASID table expansion support, per-device-domain-ID
improvement

-Vasant

> 
>>> 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.
> 
> The SVA series had code like this, I'm saying you need to generalize
> it.
> 
>>> 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.
> 
> You have it but it is not structured in a logical way, that is why
> this series has introduced nonsensical things like a PASID for a
> domain, encoding the V1/v2 state ina PASID/etc, and then did a half
> version of this list anyway to make SVA work.
> 
> Bring the list from the SVA series into this series, use it
> consistently, remove the weird stuff and then it will make sense.
> 
> Do not have a list *and* a bunch of weird stuff, that is moving
> further away from what it needs to look like..
> 
> Jason