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(-)

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();
-- 
2.40.0

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