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

[Julien Grall <julien.grall@xxxxxxx>]

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. 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?

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