Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

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

On 2024/10/22 21:25, Yi Liu wrote:
>>>>> Or any suggestion from you given a path that needs to get pte 
>>>>> first, check
>>>>> if it exists and then call intel_pasid_tear_down_entry(). For 
>>>>> example the
>>>>> intel_pasid_setup_first_level() [1], in my series, I need to call the
>>>>> unlock iommu->lock and call intel_pasid_tear_down_entry() and then 
>>>>> lock
>>>>> iommu->lock and do more modifications on the pasid entry. It would 
>>>>> invoke
>>>>> the intel_pasid_get_entry() twice if no change to
>>>>> intel_pasid_tear_down_entry().
>>>>
>>>> There is no need to check the present of a pte entry before calling 
>>>> into
>>>> intel_pasid_tear_down_entry(). The helper will return directly if the
>>>> pte is not present:
>>>>
>>>>          spin_lock(&iommu->lock);
>>>>          pte = intel_pasid_get_entry(dev, pasid);
>>>>          if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>                  spin_unlock(&iommu->lock);
>>>>                  return;
>>>>          }
>>>>
>>>> Does it work for you?
>>>
>>> This is not I'm talking about. My intention is to avoid duplicated
>>> intel_pasid_get_entry() call when calling 
>>> intel_pasid_tear_down_entry() in
>>> intel_pasid_setup_first_level(). Both the two functions call the
>>> intel_pasid_get_entry() to get pte pointer. So I think it might be 
>>> good to
>>> save one of them.
>>
>> Then, perhaps you can add a pasid_entry_tear_down() helper which asserts
>> iommu->lock and call it in both intel_pasid_tear_down_entry() and
>> intel_pasid_setup_first_level()?
> 
> hmmm. I still have a doubt. Only part of the intel_pasid_tear_down_entry()
> holds the iommu->lock. I'm afraid it's uneasy to split the
> intel_pasid_tear_down_entry() without letting the cache flush code under
> the iommu->lock. But it seems unnecessary to do cache flush under the
> iommu->lock. What about your thought? or am I getting you correctly?
> Also, I suppose this split allows the caller of the new
 > pasid_entry_tear_down() helper to pass in the pte pointer. is it?

Okay, so you want to implement a "replace" on a PASID. I think there are
two ways to achieve this. First, we can transition the PASID to the
blocking state and then replace it with a new translation. Second, we
can implement a native replacement by directly modifying the present
PASID entry.

For the first solution, we could do something like this:

	/* blocking the translation on the PASID */
	intel_pasid_tear_down_entry(dev, pasid);
	... ...
	/* setup the new domain on the PASID */
	ret = intel_pasid_setup_first_level(domain, dev, pasid);
	if (ret)
		intel_pasid_setup_first_level(old_domain, dev, pasid);

For the second solution, we need to implement a new helper function,
intel_pasid_replace_first_level(), and use it like this:

	ret = intel_pasid_replace_first_level(domain, dev, pasid);

The PASID entry remains unchanged if an error occurs.

I don't see a need of refactoring current PASID tear_down and setup
helpers.

Thanks,
baolu