Re: [PATCH 3/6] iommu/vt-d: Make helpers support modifying present pasid entry

Yi Liu
2024-06-28T10:52:55Z

On 2024/6/28 17:52, Baolu Lu wrote:
> On 2024/6/28 16:55, Yi Liu wrote:
>> To handle domain replacement, set_dev_pasid op needs to modify a present
>> pasid entry. One way is sharing the most logics of remove_dev_pasid() in
>> the beginning of set_dev_pasid() to remove the old config. But this means
>> the set_dev_pasid path needs to rollback to the old config if it fails to
>> set up the new pasid entry. This needs to invoke the set_dev_pasid op of
>> the old domain. It breaks the iommu layering a bit. Another way is
>> implementing the set_dev_pasid() without rollback to old hardware config.
>> This can be achieved by implementing it in the order of preparing the
>> dev_pasid info for the new domain, modify the pasid entry, then undo the
>> dev_pasid info of the old domain, and if failed, undo the dev_pasid info
>> of the new domain. This would keep the old domain unchanged.
>>
>> Following the second way, needs to make the pasid entry set up helpers
>> support modifying present pasid entry.
>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 37 ++++++++++++-------------------------
>>   1 file changed, 12 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index b18eebb479de..5d3a12b081a2 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu
>> *iommu,
>>           return -EINVAL;
>>       }
>> +    /* Clear the old configuration if it already exists */
>> +    intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
>> +

[1]

>>       spin_lock(&iommu->lock);
>>       pte = intel_pasid_get_entry(dev, pasid);
>>       if (!pte) {
>> @@ -321,13 +324,6 @@ int intel_pasid_setup_first_level(struct intel_iommu
>> *iommu,
>>           return -ENODEV;
>>       }
>> -    if (pasid_pte_is_present(pte)) {
>> -        spin_unlock(&iommu->lock);
>> -        return -EBUSY;
>> -    }
>> -
>> -    pasid_clear_entry(pte);
>> -
>>       /* Setup the first level page table pointer: */
>>       pasid_set_flptr(pte, (u64)__pa(pgd));
>
> The above changes the previous assumption that when a new page table is
> about to be set up on a PASID, there should be no existing one still in
> place.

actually, this does not break the assumption. In [1]. it already clears the
pasid entry.

> Is this a requirement for the replace functionality?

Replace would surely need to modify a present pasid entry. But this patch
only moves adds the pasid entry tear down and prq drain into the helpers.

--
Regards,
Yi Liu