| Message ID | 20230613101313.51326-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/amd-vi: fix checking for Invalidate All support in amd_iommu_resume() | expand |
On 13.06.2023 12:13, Roger Pau Monne wrote: > Do not rely on iommu local variable pointing to a valid amd_iommu "Do not rely" sounds like we might, but we choose not to. But we may not rely on this, as the pointer simply isn't valid to deref past the loop. Hence perhaps better "We cannot rely ..." or even "The iommu local variable does not point to ..."? > element after the call to for_each_amd_iommu(). Instead check whether > any IOMMU on the system doesn't support Invalidate All in order to > perform the per-domain and per-device flushes. > > Fixes: 9c46139de889 ('amd iommu: Support INVALIDATE_IOMMU_ALL command.') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > I don't have a way to test host suspend/resume, so the patch is only > build tested. > --- > xen/drivers/passthrough/amd/iommu_init.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c > index 9773ccfcb41f..4facff93d68b 100644 > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -1580,6 +1580,7 @@ void cf_check amd_iommu_crash_shutdown(void) > void cf_check amd_iommu_resume(void) > { > struct amd_iommu *iommu; > + bool invalidate_all = true; > > for_each_amd_iommu ( iommu ) > { > @@ -1589,10 +1590,12 @@ void cf_check amd_iommu_resume(void) > */ > disable_iommu(iommu); > enable_iommu(iommu); > + if ( !iommu->features.flds.ia_sup && invalidate_all ) You don't really need the right hand part of the condition, do you? Preferably with the adjustments (which I'd be happy to do while committing, as long as you agree) Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan > + invalidate_all = false; > } > > /* flush all cache entries after iommu re-enabled */ > - if ( !iommu->features.flds.ia_sup ) > + if ( !invalidate_all ) > { > invalidate_all_devices(); > invalidate_all_domain_pages();
On Tue, Jun 13, 2023 at 12:23:37PM +0200, Jan Beulich wrote: > On 13.06.2023 12:13, Roger Pau Monne wrote: > > Do not rely on iommu local variable pointing to a valid amd_iommu > > "Do not rely" sounds like we might, but we choose not to. But we may > not rely on this, as the pointer simply isn't valid to deref past > the loop. Hence perhaps better "We cannot rely ..." or even "The > iommu local variable does not point to ..."? "Xen cannot rely ..." doesn't modify the sentence too much and could likely be adjusted at commit if you agree? Otherwise your last suggestion would also be OK by me. > > element after the call to for_each_amd_iommu(). Instead check whether > > any IOMMU on the system doesn't support Invalidate All in order to > > perform the per-domain and per-device flushes. > > > > Fixes: 9c46139de889 ('amd iommu: Support INVALIDATE_IOMMU_ALL command.') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > I don't have a way to test host suspend/resume, so the patch is only > > build tested. > > --- > > xen/drivers/passthrough/amd/iommu_init.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c > > index 9773ccfcb41f..4facff93d68b 100644 > > --- a/xen/drivers/passthrough/amd/iommu_init.c > > +++ b/xen/drivers/passthrough/amd/iommu_init.c > > @@ -1580,6 +1580,7 @@ void cf_check amd_iommu_crash_shutdown(void) > > void cf_check amd_iommu_resume(void) > > { > > struct amd_iommu *iommu; > > + bool invalidate_all = true; > > > > for_each_amd_iommu ( iommu ) > > { > > @@ -1589,10 +1590,12 @@ void cf_check amd_iommu_resume(void) > > */ > > disable_iommu(iommu); > > enable_iommu(iommu); > > + if ( !iommu->features.flds.ia_sup && invalidate_all ) > > You don't really need the right hand part of the condition, do you? > > Preferably with the adjustments (which I'd be happy to do while > committing, as long as you agree) > Reviewed-by: Jan Beulich <jbeulich@suse.com> Wanted to avoid repeatedly setting `invalidate_all = false` if all the IOMMUs on the system don't support Invalidate All. I don't have a strong opinion, my first (local) version didn't have the right hand side of the condition, but then I realized that setting this for every IOMMU on the system could be wasteful. Thanks, Roger.
On 13.06.2023 12:33, Roger Pau Monné wrote: > On Tue, Jun 13, 2023 at 12:23:37PM +0200, Jan Beulich wrote: >> On 13.06.2023 12:13, Roger Pau Monne wrote: >>> Do not rely on iommu local variable pointing to a valid amd_iommu >> >> "Do not rely" sounds like we might, but we choose not to. But we may >> not rely on this, as the pointer simply isn't valid to deref past >> the loop. Hence perhaps better "We cannot rely ..." or even "The >> iommu local variable does not point to ..."? > > "Xen cannot rely ..." doesn't modify the sentence too much and could > likely be adjusted at commit if you agree? > > Otherwise your last suggestion would also be OK by me. I used that latter one, as being more concise. >>> --- a/xen/drivers/passthrough/amd/iommu_init.c >>> +++ b/xen/drivers/passthrough/amd/iommu_init.c >>> @@ -1580,6 +1580,7 @@ void cf_check amd_iommu_crash_shutdown(void) >>> void cf_check amd_iommu_resume(void) >>> { >>> struct amd_iommu *iommu; >>> + bool invalidate_all = true; >>> >>> for_each_amd_iommu ( iommu ) >>> { >>> @@ -1589,10 +1590,12 @@ void cf_check amd_iommu_resume(void) >>> */ >>> disable_iommu(iommu); >>> enable_iommu(iommu); >>> + if ( !iommu->features.flds.ia_sup && invalidate_all ) >> >> You don't really need the right hand part of the condition, do you? >> >> Preferably with the adjustments (which I'd be happy to do while >> committing, as long as you agree) >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Wanted to avoid repeatedly setting `invalidate_all = false` if all the > IOMMUs on the system don't support Invalidate All. > > I don't have a strong opinion, my first (local) version didn't have > the right hand side of the condition, but then I realized that setting > this for every IOMMU on the system could be wasteful. I've dropped that part: We don't care much about that tiny performance difference here (and it's unclear whether the extra check actually is a win or a loss), and imo the code is more clear with the simpler conditional. Jan
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index 9773ccfcb41f..4facff93d68b 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1580,6 +1580,7 @@ void cf_check amd_iommu_crash_shutdown(void) void cf_check amd_iommu_resume(void) { struct amd_iommu *iommu; + bool invalidate_all = true; for_each_amd_iommu ( iommu ) { @@ -1589,10 +1590,12 @@ void cf_check amd_iommu_resume(void) */ disable_iommu(iommu); enable_iommu(iommu); + if ( !iommu->features.flds.ia_sup && invalidate_all ) + invalidate_all = false; } /* flush all cache entries after iommu re-enabled */ - if ( !iommu->features.flds.ia_sup ) + if ( !invalidate_all ) { invalidate_all_devices(); invalidate_all_domain_pages();
Do not rely on iommu local variable pointing to a valid amd_iommu element after the call to for_each_amd_iommu(). Instead check whether any IOMMU on the system doesn't support Invalidate All in order to perform the per-domain and per-device flushes. Fixes: 9c46139de889 ('amd iommu: Support INVALIDATE_IOMMU_ALL command.') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- I don't have a way to test host suspend/resume, so the patch is only build tested. --- xen/drivers/passthrough/amd/iommu_init.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)