Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 09 October 2018 15:41
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Jan
> Beulich <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>; Kevin
> Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
> 
> On Tue, Oct 09, 2018 at 03:32:51PM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > > Sent: 09 October 2018 15:26
> > > To: xen-devel@lists.xenproject.org
> > > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei
> Liu
> > > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > > Subject: Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
> > >
> > > On Tue, Oct 09, 2018 at 11:42:17AM +0100, Wei Liu wrote:
> > > > Commit 2916951c1 ("mm / iommu: include need_iommu() test in
> > > > iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt and
> > > > 91d4eca7add (" mm / iommu: split need_iommu() into has_iommu_pt()
> and
> > > > need_iommu_pt_sync()") made things finer grain by spliting
> need_iommu
> > > > into three states.
> > > >
> > > > The destruction path can't use iommu_use_hap_pt because at the point
> > > > platform op is called, IOMMU is already switched to disabled state
> and
> > > > the shared PT test would always be false.
> > > >
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > >  xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > > b/xen/drivers/passthrough/vtd/iommu.c
> > > > index f94b522c73..d66d9e8ad0 100644
> > > > --- a/xen/drivers/passthrough/vtd/iommu.c
> > > > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > > > @@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct
> domain
> > > *d)
> > > >          xfree(mrmrr);
> > > >      }
> > > >
> > > > -    if ( iommu_use_hap_pt(d) )
> > > > +    ASSERT(hap_enabled(d));
> > >
> > > This needs to be moved into the predicate below because PV domain can
> > > enter this path too.
> > >
> > > > +
> > > > +    /*
> > > > +     * We can't use iommu_use_hap_pt here because the IOMMU state
> is
> > > already
> > > > +     * changed to IOMMU_STATUS_disabled at this point.
> > > > +     */
> > > > +    if ( iommu_hap_pt_share )
> > > >          return;
> >
> > I guess the other option is to track whether there is anything to tear
> down in the iommu struct and test that. We have status and need_sync now,
> but maybe that's too simplistic.
> 
> Using hap_enabled(d) && iommu_hap_pt_share should be good enough to me.
> Unless you feel strongly about this I'm not going to introduce more code
> churn than necessary.
> 

Ok. Fair enough. Let's keep the patch small.

  Paul

> Wei.
> 
> >
> >   Paul
> >
> > > >
> > > >      spin_lock(&hd->arch.mapping_lock);
> > > > --
> > > > 2.11.0
> > > >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel