Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

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

On 2024/9/13 09:35, Baolu Lu wrote:
> On 9/12/24 9:04 PM, Yi Liu wrote:
>> set_dev_pasid op is going to support domain replacement and keep the old
>> hardware config if it fails. Make the Intel iommu driver be prepared for
>> it.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++-------------
>>   1 file changed, 65 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 80b587de226d..6f5a8e549f3f 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4248,8 +4248,8 @@ static int intel_iommu_iotlb_sync_map(struct 
>> iommu_domain *domain,
>>       return 0;
>>   }
>> -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t 
>> pasid,
>> -                     struct iommu_domain *domain)
>> +static void domain_remove_dev_pasid(struct iommu_domain *domain,
>> +                    struct device *dev, ioasid_t pasid)
>>   {
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       struct dev_pasid_info *curr, *dev_pasid = NULL;
>> @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid,
>>       struct dmar_domain *dmar_domain;
>>       unsigned long flags;
>> -    if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>> -        intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>> -        return;
>> -    }
>> -
>>       dmar_domain = to_dmar_domain(domain);
>>       spin_lock_irqsave(&dmar_domain->lock, flags);
>>       list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
>> @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid,
>>       domain_detach_iommu(dmar_domain, iommu);
>>       intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>       kfree(dev_pasid);
>> +}
>> +
>> +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t 
>> pasid,
>> +                     struct iommu_domain *domain)
>> +{
>> +    struct device_domain_info *info = dev_iommu_priv_get(dev);
>> +    struct intel_iommu *iommu = info->iommu;
>> +
>>       intel_pasid_tear_down_entry(iommu, dev, pasid,
>>                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
>> +        return;
> 
> The static identity domain is not capable of handling page requests.
> Therefore there is no need to drain PRQ for an identity domain removal.

oh, yes. so maybe for SI domain, there is no PRQ at all.

> So it probably should be something like this:
> 
>      if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>          intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>          return;
>      }
> 
>      intel_pasid_tear_down_entry(iommu, dev, pasid,
>                                      INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> 
>> +    domain_remove_dev_pasid(domain, dev, pasid);
>>   }
>> -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>> -                     struct device *dev, ioasid_t pasid,
>> -                     struct iommu_domain *old)
>> +static struct dev_pasid_info *
>> +domain_prepare_dev_pasid(struct iommu_domain *domain,
>> +             struct device *dev, ioasid_t pasid)
> 
> Why do you want to return a struct pointer instead of an integer? The
> returned pointer is not used after it is returned.

it's for the intel_iommu_debugfs_create_dev_pasid(). it needs the dev_pasid.

> Also, how about renaming this helper to domain_add_dev_pasid() to pair
> it with domain_remove_dev_pasid()?

sure.

>>   {
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct 
>> iommu_domain *domain,
>>       unsigned long flags;
>>       int ret;
>> -    if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
>> -        return -EOPNOTSUPP;
>> -
>> -    if (domain->dirty_ops)
>> -        return -EINVAL;
>> -
>> -    if (context_copied(iommu, info->bus, info->devfn))
>> -        return -EBUSY;
>> -
>>       ret = prepare_domain_attach_device(domain, dev);
>>       if (ret)
>> -        return ret;
>> +        return ERR_PTR(ret);
>>       dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>>       if (!dev_pasid)
>> -        return -ENOMEM;
>> +        return ERR_PTR(-ENOMEM);
>>       ret = domain_attach_iommu(dmar_domain, iommu);
>>       if (ret)
> 
> Thanks,
> baolu

-- 
Regards,
Yi Liu