Re: [Xen-devel] [PATCH v12 8/9] mm / iommu: include need_iommu() test in iommu_use_hap_pt()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> Sent: 01 October 2018 11:37
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Subject: Re: [PATCH v12 8/9] mm / iommu: include need_iommu() test in
> iommu_use_hap_pt()
> 
> Hi Paul,
> 
> On 09/27/2018 03:33 PM, Paul Durrant wrote:
> > The name 'iommu_use_hap_pt' suggests that that P2M table is in use as
> the
> > domain's IOMMU pagetable which, prior to this patch, is not strictly
> true
> > since the macro did not test whether the domain actually has IOMMU
> > mappings.
> 
> Well, I think we assume that iommu_use_hap_pt() will only be called when
> need_iommu(d) == 1.

Yes.

> At the end, you will still need to check
> need_iommu(d) outside because iommu_use_hap_pt() may now return 0 for
> domain without IOMMU or for domain not sharing p2m with the IOMMU.
> 
> Do you expect an improvement in long-term?
> 

Yes. The ultimate goal is the patch to split up need_iommu() into separate 
macros for 'domain has IOMMU pt' and 'domain needs explicit sync of IOMMU pt' 
and this seems like a logical step. For 'need explicit sync of IOMMU pt' to 
true then 'has IOMMU pt' clearly needs to be true, so I wanted the macro for 
'sharing IOMMU pt' to have the same predicate.

  Paul

> Cheers,
> 
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > ---
> > Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> >
> > v11:
> >   - Pulled in from v6 of the full PV-IOMMU series.
> >
> > v4:
> >   - New in v4.
> > ---
> >   xen/arch/x86/mm/p2m-ept.c       | 6 +++---
> >   xen/arch/x86/mm/p2m-pt.c        | 6 +++---
> >   xen/arch/x86/mm/p2m.c           | 2 +-
> >   xen/drivers/passthrough/iommu.c | 2 +-
> >   xen/include/asm-arm/iommu.h     | 2 +-
> >   xen/include/asm-x86/iommu.h     | 5 +++--
> >   6 files changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > index af7674f7e1..f0d93856f2 100644
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -863,12 +863,12 @@ out:
> >           ept_sync_domain(p2m);
> >
> >       /* For host p2m, may need to change VT-d page table.*/
> > -    if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> > +    if ( rc == 0 && p2m_is_hostp2m(p2m) &&
> >            need_modify_vtd_table )
> >       {
> > -        if ( iommu_hap_pt_share )
> > +        if ( iommu_use_hap_pt(d) )
> >               rc = iommu_pte_flush(d, gfn, &ept_entry->epte, order,
> vtd_pte_present);
> > -        else
> > +        else if ( need_iommu(d) )
> >           {
> >               dfn_t dfn = _dfn(gfn);
> >
> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> > index 607046f31b..6b30337f62 100644
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -677,8 +677,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_,
> mfn_t mfn,
> >            && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
> >           p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;
> >
> > -    if ( iommu_enabled && need_iommu(p2m->domain) &&
> > -         (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn))
> )
> > +    if ( iommu_enabled && (iommu_old_flags != iommu_pte_flags ||
> > +                           old_mfn != mfn_x(mfn)) )
> >       {
> >           ASSERT(rc == 0);
> >
> > @@ -687,7 +687,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_,
> mfn_t mfn,
> >               if ( iommu_old_flags )
> >                   amd_iommu_flush_pages(p2m->domain, gfn, page_order);
> >           }
> > -        else
> > +        else if ( need_iommu(p2m->domain) )
> >           {
> >               dfn_t dfn = _dfn(gfn);
> >
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index 537add65bb..97bb3feacc 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -2091,7 +2091,7 @@ static unsigned int mmio_order(const struct domain
> *d,
> >        * - exclude PV guests, should execution reach this code for such.
> >        * So be careful when altering this.
> >        */
> > -    if ( !need_iommu(d) || !iommu_use_hap_pt(d) ||
> > +    if ( !iommu_use_hap_pt(d) ||
> >            (start_fn & ((1UL << PAGE_ORDER_2M) - 1)) || !(nr >>
> PAGE_ORDER_2M) )
> >           return PAGE_ORDER_4K;
> >
> > diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> > index 9cb1793144..ea7ccbace7 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -475,7 +475,7 @@ int iommu_do_domctl(
> >
> >   void iommu_share_p2m_table(struct domain* d)
> >   {
> > -    if ( iommu_enabled && iommu_use_hap_pt(d) )
> > +    if ( iommu_use_hap_pt(d) )
> >           iommu_get_ops()->share_p2m(d);
> >   }
> >
> > diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
> > index 57d9b1e14a..8d1506c6f7 100644
> > --- a/xen/include/asm-arm/iommu.h
> > +++ b/xen/include/asm-arm/iommu.h
> > @@ -21,7 +21,7 @@ struct arch_iommu
> >   };
> >
> >   /* Always share P2M Table between the CPU and the IOMMU */
> > -#define iommu_use_hap_pt(d) (1)
> > +#define iommu_use_hap_pt(d) (need_iommu(d))
> >
> >   const struct iommu_ops *iommu_get_ops(void);
> >   void __init iommu_set_ops(const struct iommu_ops *ops);
> > diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
> > index 0ed4a9e86d..7c3187c8ec 100644
> > --- a/xen/include/asm-x86/iommu.h
> > +++ b/xen/include/asm-x86/iommu.h
> > @@ -89,8 +89,9 @@ static inline int iommu_hardware_setup(void)
> >       return -ENODEV;
> >   }
> >
> > -/* Does this domain have a P2M table we can use as its IOMMU pagetable?
> */
> > -#define iommu_use_hap_pt(d) (hap_enabled(d) && iommu_hap_pt_share)
> > +/* Are we using the domain P2M table as its IOMMU pagetable? */
> > +#define iommu_use_hap_pt(d) \
> > +    (hap_enabled(d) && need_iommu(d) && iommu_hap_pt_share)
> >
> >   void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg,
> unsigned int value);
> >   unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int
> reg);
> >
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel