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

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

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.

> 
> 
> 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.





> +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.

> +
> +    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);
> 
>