Re: [Xen-devel] [PATCH for-4.12 v3 5/5] xen/arm: Track page accessed between batch of Set/Way operations

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 14 Dec 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 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@xxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

> ---
> 
> While we can spread d->creation_finished all over the code, the per-arch
> helper to perform actions just before the guest is first unpaused can
> bring a lot of benefit for both architecture. For instance, on Arm, the
> flush to the instruction cache could be delayed until the domain is
> first run. This would improve greatly the performance of creating guest.
> 
> I am still doing the benchmark whether having a command line option is
> worth it. I will provide numbers as soon as I have them.
> 
>     Changes in v3:
>         - Add Jan reviewed-by for non-ARM pieces
> 
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain.c     | 14 ++++++++++++++
>  xen/arch/arm/p2m.c        | 29 +++++++++++++++++++++++++++--
>  xen/arch/x86/domain.c     |  4 ++++
>  xen/common/domain.c       |  5 ++++-
>  xen/include/asm-arm/p2m.h |  2 ++
>  xen/include/xen/domain.h  |  2 ++
>  6 files changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 1d926dcb29..41f101746e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -767,6 +767,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/p2m.c b/xen/arch/arm/p2m.c
> index 125d858d02..347028c325 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)
>  }
>  
>  /*
> + * 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);
> +}
> +
> +/*
>   * Resolve any translation fault due to change in the p2m. This
>   * includes break-before-make and valid bit cleared.
>   */
> @@ -1587,10 +1603,12 @@ int p2m_cache_flush_range(struct domain *d, gfn_t 
> *pstart, gfn_t end)
>           */
>          if ( gfn_eq(start, next_block_gfn) )
>          {
> -            mfn = p2m_get_entry(p2m, start, &t, NULL, &order, NULL);
> +            bool valid;
> +
> +            mfn = p2m_get_entry(p2m, start, &t, NULL, &order, &valid);
>              next_block_gfn = gfn_next_boundary(start, order);
>  
> -            if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) )
> +            if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) || !valid )
>              {
>                  count++;
>                  start = next_block_gfn;
> @@ -1624,6 +1642,7 @@ int p2m_cache_flush_range(struct domain *d, gfn_t 
> *pstart, gfn_t end)
>   */
>  void p2m_flush_vm(struct vcpu *v)
>  {
> +    struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>      int rc;
>      gfn_t start = _gfn(0);
>  
> @@ -1643,6 +1662,12 @@ void p2m_flush_vm(struct vcpu *v)
>                  "P2M has not been correctly cleaned (rc = %d)\n",
>                  rc);
>  
> +    /*
> +     * Invalidate the p2m to track which page was modified by the guest
> +     * between call of p2m_flush_vm().
> +     */
> +    p2m_invalidate_root(p2m);
> +
>      v->arch.need_flush_to_ram = false;
>  }
>  
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f0e0cdbb0e..3729887d00 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -762,6 +762,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 78cc5249e8..c623daec56 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1116,8 +1116,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 79abcb5a63..01cd3ee4b5 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -231,6 +231,8 @@ int p2m_set_entry(struct p2m_domain *p2m,
>  
>  bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
>  
> +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 33e41486cb..d1bfc82f57 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel