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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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

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.