Re: [PATCH v2 05/11] iommu/amd: Add support to enable/disable PASID feature

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

Jason,


On 9/12/2023 10:27 PM, Jason Gunthorpe wrote:
> On Mon, Sep 11, 2023 at 12:10:40PM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> It seems iommu_dev_enable_feature(SVA) will be deprecated soon. Hence in
>> this path we just return success.
>>
>> Instead we add necessary check during pasid bind to device. IN this path
>> it checks whether device GCR3 table is setup or not. If not it will try
>> to setup the GCR3.
> 
> Yes, though be mindful that switching from IDENTITY v1 to IDENTITY v2
> on the RID (eg switching the DTE to/from GCR3 mode) should be atomic.

Right. Its atomic. It should work fine. I have to be careful when I add support
for PASID table expansion. But its doable.

> 
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 94eec3dac8f6..5a9749cfe14e 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2162,6 +2162,50 @@ static void protection_domain_free(struct protection_domain *domain)
>>  	kfree(domain);
>>  }
>>  
>> +/*******************************
>> + * PASID setup related helper functions
>> + */
>> +static inline bool pdom_is_pt_mode(struct protection_domain *pdom)
>> +{
>> +	return (pdom->domain.type == IOMMU_DOMAIN_IDENTITY);
>> +}
>> +
>> +static inline bool pdom_is_v2_pgtbl_mode(struct protection_domain *pdom)
>> +{
>> +	return (pdom->iop.pgd != NULL);
>> +}
>> +
>> +int amd_iommu_gcr3_init(struct iommu_dev_data *dev_data, int pasids)
>> +{
>> +	struct protection_domain *pdom = dev_data->domain;
>> +	int ret = 0;
>> +
>> +	lockdep_assert_held(&dev_data->lock);
>> +
>> +	/*
>> +	 * We cannot support PASID w/ existing v1 page table in the same domain
>> +	 * since it will be nested. However, existing domain w/ v2 page table
>> +	 * can be used for PASID.
>> +	 */
>> +	if (pdom->pd_mode == PD_MODE_V1)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* Allocate GCR3 table */
>> +	if (pdom_is_pt_mode(dev_data->domain))
> 
> It seems obfuscating to put this test for IOMMU_DOMAIN_IDENTITY in a
> function..

It needs to know how domain is configured. Hence I have this check.

> 
>> +		ret = setup_gcr3_table(dev_data, pasids);
> 
> This is the DTE change that should be atomic

It gets called with dev_data lock. And it doesn't break ongoing translations. So
it should be fine.

-Vasant