Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

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

On 11/6/2023 10:59 PM, Jason Gunthorpe wrote:
> On Fri, Oct 13, 2023 at 03:16:49PM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> To use the new helper function for setting up GCR3 table.
>>
>> If system is booted with V2 page table then setup default GCR3 with
>> domain GCR3 pointer. 
> 
> Lets stop talking about "booted with V2 page table" - "If the domain
> uses the V2 format then setup a GCR3 table in the device to point to it"
> 
>> +static int _init_gcr3_tbl(struct iommu_dev_data *dev_data)
>> +{
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +
>> +	/* By default, GCR3 is set to support non-PASID devices. */
>> +	gcr3_info->giov = true;
> 
> I admit I find it really hard to read the AMD spec here.. In terms of

Sorry. I can't help.

> the SW model what modes are actually supportable by HW?
> 
> No PASIDs being used:
>  RID domain=IDENTITY - yes [V=0]

That's still valid. DTE[V]=1

>  RID domain=BLOCKED - yes [V=1, TV=1, GV=0, mode == 0]

May be TV=1 is valid here, but I am not too sure. I am yet to explore this one.
Rest of the flags are fine.

>  RID domain=v1 - Yes [V=1, TV=1, GV=0, mode != 0]
>  RID domain=v2 - Yes [V=1, TV=0, GV=1, GIOV=1]

TV=1. Rest of the flags are fine.


> 
> Some kind of PASID in use:
>  RID domain=v2 & PASID - Yes [V=1, TV=0, GV=1, GIOV=1]

Correct.

>  RID domain=V1 & PASID - No??

Correct. No PASID support.

>  RID domain=IDENTITY & PASID - ?? [V=1, TV=1 GV=1, mode=0, GIOV=0] (Section 2.2.7.1?)

This is supported.

>  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 

I haven't thought this scenario. Why do we even need this case?

> 
> Did I get it right? If so GIOV should ultimately be deduced based on
> what domain the RID has?
> 
> Look at how the SMMUv3 stuff ended up. Their STE is the same purpose
> as the AMD DTE. There are alot of combinations here, it was hard to
> make a code flow that was clean. It turned out pretty good when the
> DTE was generated in the ops->attach based on a calculation of exactly
> what the current configuration is, because we already know what we are
> in alot of detail at that point.
> 
> eg we know if we are attaching an identity domain and PASIDs are in
> use that a single specific DTE should be created. So just call a
> function directly to get the required DTE.
> 
> IOW - I'm not sure it really makes logical sense to store giov in
> gcr3_info.

We had a choice of having a giov flag inside gcr3_info as it tells how to
configure GCR3 related bits in DTE -OR- having a extra logic to calculate it
every time. I can be calculated.


> 
>> +static int do_attach(struct iommu_dev_data *dev_data,
>> +		     struct protection_domain *domain)
>>  {
>>  	struct amd_iommu *iommu;
>> +	int ret = 0;
>>  
>>  	iommu = get_amd_iommu_from_dev(dev_data->dev);
>>  	if (!iommu)
>> -		return;
>> +		return -EINVAL;
> 
> iommu can't be null here, have a dev_data.

Fixed.

> 
>>  	dev_data->domain = domain;
>> @@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
>>  	if (domain_id_is_per_dev(domain))
>>  		dev_data->domid = domain_id_alloc();
> 
> At some point this is the wrong place to put this, the domain ID is
> logically associated with the gcr3 table, it should never be used if
> there is no gcr table allocated, and it should be freed once the gcr3
> table is freed.

Domain ID is decided based on page table type (and may be based on PASID later).
 That's why I have a function to decide whether to allocate ID or not and it
should be done in this path only. So that we can configure DTE.



> 
>> +	/* Init GCR3 table */
>> +	if (domain->pd_mode == PD_MODE_V2) {
> 
> Is there a case where domain_id_is_per_dev() but we are attaching a v1
> table?

Not today.

-Vasant