Re: [PATCH] iommu/amd-vi: fix checking for Invalidate All support in amd_iommu_resume()

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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@xxxxxxxx>

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