| Message ID | 20241018055402.23277-4-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make set_dev_pasid op supporting domain replacement | expand |
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@intel.com> > --- > 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. Thanks, baolu
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@intel.com> >> --- >> 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?
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@intel.com> >>> --- >>> 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. Thanks, baolu
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@intel.com> >>>> --- >>>> 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. 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(). [1] https://github.com/torvalds/linux/blob/master/drivers/iommu/intel/pasid.c#L317
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@intel.com> >>>>> --- >>>>> 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? Thanks, baolu
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@intel.com> >>>>>> --- >>>>>> 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.
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@intel.com> >>>>>>> --- >>>>>>> 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
On 2024/10/22 19:23, Baolu Lu wrote: > 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@intel.com> >>>>>>>> --- >>>>>>>> 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()? 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? void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev, u32 pasid, bool fault_ignore) { struct pasid_entry *pte; u16 did, pgtt; 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; } did = pasid_get_domain_id(pte); pgtt = pasid_pte_get_pgtt(pte); intel_pasid_clear_entry(dev, pasid, fault_ignore); spin_unlock(&iommu->lock); if (!ecap_coherent(iommu->ecap)) clflush_cache_range(pte, sizeof(*pte)); pasid_cache_invalidation_with_pasid(iommu, did, pasid); if (pgtt == PASID_ENTRY_PGTT_PT || pgtt == PASID_ENTRY_PGTT_FL_ONLY) qi_flush_piotlb(iommu, did, pasid, 0, -1, 0); else iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH); devtlb_invalidation_with_pasid(iommu, dev, pasid); }
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
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; } did = pasid_get_domain_id(pte); @@ -273,6 +276,8 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev, if (flags & INTEL_PASID_TEARDOWN_DRAIN_PRQ) intel_drain_pasid_prq(dev, pasid); +out: + return pte; } /* diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h index 7dc9e4dfbd88..9b2351325b0e 100644 --- a/drivers/iommu/intel/pasid.h +++ b/drivers/iommu/intel/pasid.h @@ -306,8 +306,9 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev, #define INTEL_PASID_TEARDOWN_IGNORE_FAULT BIT(0) #define INTEL_PASID_TEARDOWN_DRAIN_PRQ BIT(1) -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); void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu, struct device *dev, u32 pasid); int intel_pasid_setup_sm_context(struct device *dev);
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@intel.com> --- drivers/iommu/intel/pasid.c | 11 ++++++++--- drivers/iommu/intel/pasid.h | 5 +++-- 2 files changed, 11 insertions(+), 5 deletions(-)