Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Yi Liu <yi.l.liu@intel.com>]

On 2024/10/23 19:10, Vasant Hegde wrote:
> Hi Yi,
> 
> 
> On 10/22/2024 6:21 PM, Yi Liu wrote:
>> On 2024/10/21 20:33, Jason Gunthorpe wrote:
>>> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote:
>>>> On 2024/10/18 22:39, Jason Gunthorpe wrote:
>>>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
>>>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by
>>>>>> extending the blocked_domain to support PASID. However, this cannot be
>>>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
>>>>>> supported it, while the AMD iommu driver has not yet. During this
>>>>>> transition, the IOMMU core needs to support both ways to destroy the
>>>>>> attachment of device/PASID and domain.
>>>>>
>>>>> Let's just fix AMD?
>>>>
>>>> cool.
>>>
>>> You could probably do better on this and fixup
>>> amd_iommu_remove_dev_pasid() to have the right signature directly,
>>> like the other drivers did
>>
>> It might make sense to move the amd_iommu_remove_dev_pasid() to the
>> drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid().
> 
> I wanted to keep all PASID code in pasid.c. I'd say for now lets keep it in
> pasid.c only.

ok. If so, we may just let the blocked_domain_set_dev_pasid() call
amd_iommu_remove_dev_pasid().

>>
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index b11b014fa82d..55ac1ad10fb3 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom);
>>   int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
>>                   struct device *dev, ioasid_t pasid,
>>                   struct iommu_domain *old);
>> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>> -                struct iommu_domain *domain);
>> +void remove_pdom_dev_pasid(struct protection_domain *pdom,
>> +               struct device *dev, ioasid_t pasid);
>>
>>   /* SVA/PASID */
>>   bool amd_iommu_pasid_supported(void);
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 8364cd6fa47d..f807c4956a75 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct
>> iommu_domain *domain,
>>       return 0;
>>   }
>>
> 
> May be we should add comment here or at least explain it in patch description.
> Otherwise it may create confusion. Something like below
> 
> 
> Remove PASID from old domain and device GCR3 table. No need to attach PASID to
> blocked domain as clearing PASID from GCR3 table will make sure all DMAs for
> that PASID is blocked.

got it.

> 
> 
> 
> 
>> +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
>> +                    struct device *dev, ioasid_t pasid,
>> +                    struct iommu_domain *old)
>> +{
>> +    struct protection_domain *pdom = to_pdomain(old);
>> +    unsigned long flags;
>> +
>> +    if (old->type != IOMMU_DOMAIN_SVA)
>> +        return -EINVAL;
>> +
>> +    if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
>> +        return 0;
>> +
>> +    pdom = to_pdomain(domain);
> 
> This is redundant as you already set pdom to old domain.

yes.

>> +
>> +    spin_lock_irqsave(&pdom->lock, flags);
>> +
>> +    /* Remove PASID from dev_data_list */
>> +    remove_pdom_dev_pasid(pdom, dev, pasid);
>> +
>> +    spin_unlock_irqrestore(&pdom->lock, flags);
>> +    return 0;
>> +}
>> +
>>   static struct iommu_domain blocked_domain = {
>>       .type = IOMMU_DOMAIN_BLOCKED,
>>       .ops = &(const struct iommu_domain_ops) {
>> diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
>> index 8c73a30c2800..c43c7286c872 100644
>> --- a/drivers/iommu/amd/pasid.c
>> +++ b/drivers/iommu/amd/pasid.c
>> @@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data *pdom_dev_data)
>>   }
>>
>>   /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */
>> -static void remove_pdom_dev_pasid(struct protection_domain *pdom,
>> -                  struct device *dev, ioasid_t pasid)
>> +void remove_pdom_dev_pasid(struct protection_domain *pdom,
>> +               struct device *dev, ioasid_t pasid)
>>   {
>>       struct pdom_dev_data *pdom_dev_data;
>>       struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> @@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
>>       return ret;
>>   }
>>
>> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>> -                struct iommu_domain *domain)
>> -{
>> -    struct protection_domain *sva_pdom;
>> -    unsigned long flags;
>> -
>> -    if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
>> -        return;
>> -
>> -    sva_pdom = to_pdomain(domain);
>> -
>> -    spin_lock_irqsave(&sva_pdom->lock, flags);
>> -
>> -    /* Remove PASID from dev_data_list */
>> -    remove_pdom_dev_pasid(sva_pdom, dev, pasid);
>> -
>> -    spin_unlock_irqrestore(&sva_pdom->lock, flags);
>> -}
>> -
>>   static void iommu_sva_domain_free(struct iommu_domain *domain)
>>   {
>>       struct protection_domain *sva_pdom = to_pdomain(domain);
>>
>>

-- 
Regards,
Yi Liu