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
