Re: [PATCH v5 14/17] iommu/amd: Refactor GCR3 table helper functions

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

On 1/25/2024 7:16 AM, Jason Gunthorpe wrote:
> On Tue, Jan 23, 2024 at 02:24:51PM +0530, Vasant Hegde wrote:
> 
>> domain_id_is_per_dev() decides how to allocate domain ID. Right now for V2 and
>> pass through mode we allocate per-device-domain-ID as they can switch to SVA.
> 
> That makes no sense. You don't need a domain id for passthrough mode
> unless you are also installing a gcr3 table. You don't need to install
> a gcr3 table unless there is a PASID being attached too.
> 
> Pre-setting the domain ID to avoid setting it when the GCR3 is later
> loaded is spaghetti logic.
> 
>>> All this logic should be shared between the pasid and rid attach
>>> paths.>
>>> The passthrough thing is only an issue of DTE construction.
>>>
>>> If you build a DTE with a GCR3 table and RID=IDENTITY then you set
>>> some bits, and that is it. Detect that case directly when you build
>>> the DTE. It should have no effect on what domain ID is used to tag
>>> translations retrived from a GCR3 table.
>>
>> We build DTE as soon as we attach device to domain.
>>
>> Now moving domain ID allocation to setup_gcr3_table complicates things.
>>   - In attach_device() path we want to allocate domain ID but not GCR3 table
>>     We can allocate GCR3 table, but if we don't use it its waste of memory.
> 
> It is a waste to allocate the domain id for identity too.
> 
>>   - In SVA enablement path we want to allocate GCR3 table
>>     But by then domain ID should have been allocated. We don't want to allocate
>> another domain_ID and change ID in SVA enablement path as our domain ID is not
>> specific to GCR3.
> 
> Again, this seems to be a complication that is being created by the
> DTE construction. You shouldn't need the caller to carefully sequence
> what it is doing. The DTE programming should just install the correct
> DTE for the *current state*.
> 
> A RID only identity/passthrough DTE does not have a GCR3 table and
> does not need a unique domain ID.
> 
>>>> We need to handle passthrough as well. It doesn't make sense to allocate and
>>>> keep GCR3 table when we are not going to use it.
>>>
>>> Then don't, and my diff didn't - but check for the passthrough case
>>> directly against the attached domain as identity.
>>
>> We don't want to add condition check that depends on code path:
>>   like attach_device : allocate domain ID but not GCR3
>>   SVA path : Allocate GCR3 but not domain ID.
> 
> I'm not saying you should do that, I'm actively saying you should not
> do that! All paths should be symmetric.

I still think its not a good thing to do domain allocation inside
setup_gcr3_table(). Anyway I have changed it in v6.


-Vasant