Re: [RFC 16/16] xen/arm: Track page accessed between batch of Set/Way operations

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 8 Oct 2018, Julien Grall wrote:
> At the moment, the implementation of Set/Way operations will go through
> all the entries of the guest P2M and flush them. However, this is very
> expensive and may render unusable a guest OS using them.
> 
> For instance, Linux 32-bit will use Set/Way operations during secondary
> CPU bring-up. As the implementation is really expensive, it may be possible
> to hit the CPU bring-up timeout.
> 
> To limit the Set/Way impact, we track what pages has been of the guest
> has been accessed between batch of Set/Way operations. This is done
> using bit[0] (aka valid bit) of the P2M entry.

This is going to improve performance of ill-mannered guests at the cost
of hurting performance of well-mannered guests. Is it really a good
trade-off? Should this behavior at least be configurable with a Xen
command line?


> This patch adds a new per-arch helper is introduced to perform actions just
> before the guest is first unpaused. This will be used to invalidate the
> P2M to track access from the start of the guest.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
> 
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/arm/domain.c       | 14 ++++++++++++++
>  xen/arch/arm/domain_build.c |  7 +++++++
>  xen/arch/arm/p2m.c          | 32 +++++++++++++++++++++++++++++++-
>  xen/arch/x86/domain.c       |  4 ++++
>  xen/common/domain.c         |  5 ++++-
>  xen/include/asm-arm/p2m.h   |  2 ++
>  xen/include/xen/domain.h    |  2 ++
>  7 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index feebbf5a92..f439f4657a 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -738,6 +738,20 @@ int arch_domain_soft_reset(struct domain *d)
>      return -ENOSYS;
>  }
>  
> +void arch_domain_creation_finished(struct domain *d)
> +{
> +    /*
> +     * To avoid flushing the whole guest RAM on the first Set/Way, we
> +     * invalidate the P2M to track what has been accessed.
> +     *
> +     * This is only turned when IOMMU is not used or the page-table are
> +     * not shared because bit[0] (e.g valid bit) unset will result
> +     * IOMMU fault that could be not fixed-up.
> +     */
> +    if ( !iommu_use_hap_pt(d) )
> +        p2m_invalidate_root(p2m_get_hostp2m(d));
> +}
> +
>  static int is_guest_pv32_psr(uint32_t psr)
>  {
>      switch (psr & PSR_MODE_MASK)
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f552154e93..de96516faa 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2249,6 +2249,13 @@ int __init construct_dom0(struct domain *d)
>      v->is_initialised = 1;
>      clear_bit(_VPF_down, &v->pause_flags);
>  
> +    /*
> +     * XXX: We probably want a command line option to invalidate or not
> +     * the P2M. This is because invalidating the P2M will not work with
> +     * IOMMU, however if this is not done there will be an impact.

Why can't we check on iommu_use_hap_pt(d) like in
arch_domain_creation_finished?

In any case, I agree it is a good idea to introduce a command line
parameter to toggle the p2m_invalidate_root call at domain creation
on/off. There are cases where it should be off even if an IOMMU is
present.

Aside from these two questions, the rest of the patch looks correct.


> +     */
> +    p2m_invalidate_root(p2m_get_hostp2m(d));
>
>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index a3d4c563b1..8e0c31d7ac 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1079,6 +1079,22 @@ static void p2m_invalidate_table(struct p2m_domain *p2m, mfn_t mfn)
>      p2m->need_flush = true;
>  }
>  
> +/*
> + * Invalidate all entries in the root page-tables. This is
> + * useful to get fault on entry and do an action.
> + */
> +void p2m_invalidate_root(struct p2m_domain *p2m)
> +{
> +    unsigned int i;
> +
> +    p2m_write_lock(p2m);
> +
> +    for ( i = 0; i < P2M_ROOT_LEVEL; i++ )
> +        p2m_invalidate_table(p2m, page_to_mfn(p2m->root + i));
> +
> +    p2m_write_unlock(p2m);
> +}
> +
>  bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
> @@ -1539,7 +1555,8 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
>  
>      for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
>      {
> -        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
> +        bool valid;
> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order, &valid);
>  
>          next_gfn = gfn_next_boundary(start, order);
>  
> @@ -1547,6 +1564,13 @@ int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
>          if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) )
>              continue;
>  
> +        /*
> +         * Page with valid bit (bit [0]) unset does not need to be
> +         * cleaned
> +         */
> +        if ( !valid )
> +            continue;
> +
>          /* XXX: Implement preemption */
>          while ( gfn_x(start) < gfn_x(next_gfn) )
>          {
> @@ -1571,6 +1595,12 @@ static void p2m_flush_vm(struct vcpu *v)
>      /* XXX: Handle preemption */
>      p2m_cache_flush_range(v->domain, p2m->lowest_mapped_gfn,
>                            p2m->max_mapped_gfn);
> +
> +    /*
> +     * Invalidate the p2m to track which page was modified by the guest
> +     * between call of p2m_flush_vm().
> +     */
> +    p2m_invalidate_root(p2m);
>  }
>  
>  /*
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9371efc8c7..2b6d1c01a1 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -723,6 +723,10 @@ int arch_domain_soft_reset(struct domain *d)
>      return ret;
>  }
>  
> +void arch_domain_creation_finished(struct domain *d)
> +{
> +}
> +
>  /*
>   * These are the masks of CR4 bits (subject to hardware availability) which a
>   * PV guest may not legitimiately attempt to modify.
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 65151e2ac4..b402c635f9 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1100,8 +1100,11 @@ int domain_unpause_by_systemcontroller(struct domain *d)
>       * Creation is considered finished when the controller reference count
>       * first drops to 0.
>       */
> -    if ( new == 0 )
> +    if ( new == 0 && !d->creation_finished )
> +    {
>          d->creation_finished = true;
> +        arch_domain_creation_finished(d);
> +    }
>  
>      domain_unpause(d);
>  
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index c470f062db..2a4652e7f4 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -225,6 +225,8 @@ int p2m_set_entry(struct p2m_domain *p2m,
>                    p2m_type_t t,
>                    p2m_access_t a);
>  
> +void p2m_invalidate_root(struct p2m_domain *p2m);
> +
>  /*
>   * Clean & invalidate caches corresponding to a region [start,end) of guest
>   * address space.
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 5e393fd7f2..8d95ad4bf1 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -70,6 +70,8 @@ void arch_domain_unpause(struct domain *d);
>  
>  int arch_domain_soft_reset(struct domain *d);
>  
> +void arch_domain_creation_finished(struct domain *d);
> +
>  void arch_p2m_set_access_required(struct domain *d, bool access_required);
>  
>  int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
> -- 
> 2.11.0
> 

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