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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of Paul Durrant
> Sent: 09 October 2018 12:23
> To: Wei Liu <wei.liu2@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Jan
> Beulich <JBeulich@xxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] x86/vtd: fix IOMMU share PT destruction
> path
> 
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> > Sent: 09 October 2018 11:42
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Jan Beulich <JBeulich@xxxxxxxx>; Roger Pau Monne
> > <roger.pau@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Wei Liu
> > <wei.liu2@xxxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>
> > Subject: [PATCH] x86/vtd: fix IOMMU share PT destruction path
> >
> > 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.
> >
> 
> This is VT-d specific and the issue could affect AMD too.

Yes, it looks like deallocate_iommu_page_tables() will then try to deallocate 
page tables that would not have been allocated. This probably happens to be 
safe because the root table is allocated on demand, but that might change in 
future so I think it better to fix as I suggest below.

  Paul

> Can't you just
> switch round the call to teardown and setting IOMMU_status_disabled in
> xen/drivers/passthrough/iommu.c:iommu_teardown()?
> 
>   Paul
> 
> 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  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));
> > +
> > +    /*
> > +     * 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;
> >
> >      spin_lock(&hd->arch.mapping_lock);
> > --
> > 2.11.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel