Re: [PATCH v6 07/15] iommu/amd: Setup GCR3 table in advance if domain is SVA capable

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

On 3/5/2024 5:41 AM, Jason Gunthorpe wrote:
> On Fri, Feb 09, 2024 at 11:29:22AM +0000, Vasant Hegde wrote:
>> SVA can be supported if domain is in passthrough mode or paging domain
>> with v2 page table. Current code sets up GCR3 table for domain with v2
>> page table only. Setup GCR3 table for all SVA capable domains.
>>
>>   - Move GCR3 init/destroy to separate function.
>>
>>   - Change default GCR3 table to use MAX supported PASIDs. Ideally it
>>     should use 1 level PASID table as its using PASID zero only. But we
>>     don't have support to extend PASID table yet. We will fix this later.
>>
>>   - When domain is configured with passthrough mode, allocate default GCR3
>>     table only if device is SVA capable.
>>
>> Note that in attach_device() path it will not know whether device will use
>> SVA or not. If device is attached to passthrough domain and if it doesn't
>> use SVA then GCR3 table will never be used. We will endup wasting memory
>> allocated for GCR3 table. This is done to avoid DTE update when
>> attaching PASID to device.
> 
> It is certainly not elegant, but I can appreciate why you don't want
> to tackle the DTE update right now.
> 
>> +/*
>> + * If domain is SVA capable then initialize GCR3 table. Also if domain is
>> + * in v2 page table mode then update GCR3[0].
>> + */
>> +static int init_gcr3_table(struct iommu_dev_data *dev_data,
>> +			   struct protection_domain *pdom)
>> +{
>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
>> +	int max_pasids = dev_data->max_pasids;
>> +	int ret = 0;
>> +
>> +	 /*
>> +	  * If domain is in pt mode then setup GCR3 table only if device
>> +	  * is PASID capable
>> +	  */
>> +	if (pdom_is_in_pt_mode(pdom) && !pdev_pasid_supported(dev_data))
>> +		return ret;
> 
> This logic seems like it would be clearer as:
> 
> if (WARN_ON(gcr3_info->gcr3_tbl))
>    return -EINVAL;
> if (pdom->domain.type == IOMMU_DOMAIN_IDENTITIY)
>     max_pasids = dev_data->max_pasids;
> else if (pdom_is_v2_pgtbl_mode(pdom))
>     max_pasids = min(1, dev_data->pasids);
> else
>     max_pasids = 0;
> 
> if (!max_pasids)
>     return 0;
> 
> ret = setup_gcr3_table(&dev_data->gcr3_info, iommu, max_pasids)
> if (ret)
>    return ret;
> return 0;

Will look into it later.

> 
>> +static void destroy_gcr3_table(struct iommu_dev_data *dev_data,
>> +			       struct protection_domain *pdom)
>> +{
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +
>> +	if (pdom_is_v2_pgtbl_mode(pdom))
>> +		update_gcr3(dev_data, 0, 0, false);
> 
> This doesn't make alot of sense, we are about to free this table
> memory, why zero it and flush? If the DTE still points here we are in
> trouble!

This is for GCR3[0].. But yeah. detach path we can just ignore this and free
gcr3 table. Will fix it later.

> 
> Similar comment for storing the gcr3 in init_gcr3_table(), that should
> just stay in the caller..
> 
>> @@ -2010,10 +2068,8 @@ static void do_detach(struct iommu_dev_data *dev_data)
>>  	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
>>  
>>  	/* Clear GCR3 table */
>> -	if (domain->pd_mode == PD_MODE_V2) {
>> -		update_gcr3(dev_data, 0, 0, false);
>> -		free_gcr3_table(&dev_data->gcr3_info);
>> -	}
>> +	if (pdom_is_sva_capable(domain))
>> +		destroy_gcr3_table(dev_data, domain);
> 
> if (dev_data->gcr3_info)
> 	destroy_gcr3_table(dev_data, domain);

Makes sense. I will post separate patch later.

> 
> ?
> 
> Don't really care how we got here..
> 
> Anyhow, this looks like the right thing in the big picture, nit picks
> aside, so:
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks
-Vasant