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

[Baolu Lu <baolu.lu@linux.intel.com>]

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

Is this a requirement for the replace functionality?

Best regards,
baolu