On 2024/10/22 17:38, Yi Liu wrote:
On 2024/10/22 17:23, Baolu Lu wrote:
On 2024/10/21 15:24, Yi Liu wrote:
On 2024/10/21 14:59, Baolu Lu wrote:
On 2024/10/21 14:35, Yi Liu wrote:
On 2024/10/21 14:13, Baolu Lu wrote:
On 2024/10/18 13:53, Yi Liu wrote:
intel_pasid_tear_down_entry() finds the pasid entry and tears it
down.
There are paths that need to get the pasid entry, tear it down and
re-configure it. Letting intel_pasid_tear_down_entry() return the
pasid
entry can avoid duplicate codes to get the pasid entry. No
functional
change is intended.
Signed-off-by: Yi Liu<yi.l.liu@xxxxxxxxx>
---
drivers/iommu/intel/pasid.c | 11 ++++++++---
drivers/iommu/intel/pasid.h | 5 +++--
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/
pasid.c
index 2898e7af2cf4..336f9425214c 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct
intel_iommu *iommu,
/*
* Caller can request to drain PRQ in this helper if it hasn't
done so,
* e.g. in a path which doesn't follow remove_dev_pasid().
+ * Return the pasid entry pointer if the entry is found or NULL
if no
+ * entry found.
*/
-void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
struct device *dev,
- u32 pasid, u32 flags)
+struct pasid_entry *
+intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct
device *dev,
+ u32 pasid, u32 flags)
{
struct pasid_entry *pte;
u16 did, pgtt;
@@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct
intel_iommu *iommu, struct device *dev,
pte = intel_pasid_get_entry(dev, pasid);
if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
spin_unlock(&iommu->lock);
- return;
+ goto out;
The pasid table entry is protected by iommu->lock. It's not
reasonable
to return the pte pointer which is beyond the lock protected range.
Per my understanding, the iommu->lock protects the content of the
entry,
so the modifications to the entry need to hold it. While, it looks not
necessary to protect the pasid entry pointer itself. The pasid
table should
exist during device probe and release. is it?
The pattern of the code that modifies a pasid table entry is,
spin_lock(&iommu->lock);
pte = intel_pasid_get_entry(dev, pasid);
... modify the pasid table entry ...
spin_unlock(&iommu->lock);
Returning the pte pointer to the caller introduces a potential race
condition. If the caller subsequently modifies the pte without re-
acquiring the spin lock, there's a risk of data corruption or
inconsistencies.
it appears that we are on the same page about if pte pointer needs to be
protected or not. And I agree the modifications to the pte should be
protected by iommu->lock. If so, will documenting that the caller
must hold
iommu->lock if is tries to modify the content of pte work? Also, it
might
be helpful to add lockdep to make sure all the modifications of pte
entry
are under protection.
People will soon forget about this lock and may modify the returned pte
pointer without locking, introducing a race condition silently.
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()?
Thanks,
baolu
