Re: [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation

[George Dunlap <george.dunlap@citrix.com>]

On 20/09/16 03:31, Dongli Zhang wrote:
> This patch implemented parts of TODO left in commit id
> a902c12ee45fc9389eb8fe54eeddaf267a555c58 (More efficient TLB-flush
> filtering in alloc_heap_pages()). It moved TLB-flush filtering out into
> populate_physmap. Because of TLB-flush in alloc_heap_pages, it's very slow
> to create a guest with memory size of more than 100GB on host with 100+
> cpus.
> 
> This patch introduced a "MEMF_no_tlbflush" bit to memflags to indicate
> whether TLB-flush should be done in alloc_heap_pages or its caller
> populate_physmap.  Once this bit is set in memflags, alloc_heap_pages will
> ignore TLB-flush. To use this bit after vm is created might lead to
> security issue, that is, this would make pages accessible to the guest B,
> when guest A may still have a cached mapping to them.
> 
> Therefore, this patch also introduced a "creation_finished" field to struct
> domain to indicate whether this domain has ever got unpaused by hypervisor.
> MEMF_no_tlbflush can be set only during vm creation phase when
> creation_finished is still false before this domain gets unpaused for the
> first time.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> Changed since v5:
>   * Remove conditional check before "d->creation_finished = true;".
>   * Place "bool creation_finished;" next to the other group of booleans.
>   * Remove duplicate "only" in comments.
> 
> Changed since v4:
>   * Rename is_ever_unpaused to creation_finished.
>   * Change bool_t to bool.
>   * Polish comments.
> 
> Changed since v3:
>   * Set the flag to true in domain_unpause_by_systemcontroller when
>     unpausing the guest domain for the first time.
>   * Use true/false for all boot_t variables.
>   * Add unlikely to optimize "if statement".
>   * Correct comment style.
> 
> Changed since v2:
>   * Limit this optimization to domain creation time.
> 
> ---
>  xen/common/domain.c     |  7 +++++++
>  xen/common/memory.c     | 22 ++++++++++++++++++++++
>  xen/common/page_alloc.c |  4 +++-
>  xen/include/xen/mm.h    |  2 ++
>  xen/include/xen/sched.h |  6 ++++++
>  5 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index a8804e4..3abaca9 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1004,6 +1004,13 @@ int domain_unpause_by_systemcontroller(struct domain *d)
>  {
>      int old, new, prev = d->controller_pause_count;
>  
> +    /*
> +     * We record this information here for populate_physmap to figure out
> +     * that the domain has finished being created. In fact, we're only
> +     * allowed to set the MEMF_no_tlbflush flag during VM creation.
> +     */
> +    d->creation_finished = true;
> +
>      do
>      {
>          old = prev;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index cc0f69e..21797ca 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -141,6 +141,8 @@ static void populate_physmap(struct memop_args *a)
>      unsigned int i, j;
>      xen_pfn_t gpfn, mfn;
>      struct domain *d = a->domain, *curr_d = current->domain;
> +    bool need_tlbflush = false;
> +    uint32_t tlbflush_timestamp = 0;
>  
>      if ( !guest_handle_subrange_okay(a->extent_list, a->nr_done,
>                                       a->nr_extents-1) )
> @@ -150,6 +152,17 @@ static void populate_physmap(struct memop_args *a)
>                              max_order(curr_d)) )
>          return;
>  
> +    /*
> +     * With MEMF_no_tlbflush set, alloc_heap_pages() will ignore
> +     * TLB-flushes. After VM creation, this is a security issue (it can
> +     * make pages accessible to guest B, when guest A may still have a
> +     * cached mapping to them). So we do this only during domain creation,
> +     * when the domain itself has not yet been unpaused for the first
> +     * time.
> +     */
> +    if ( unlikely(!d->creation_finished) )
> +        a->memflags |= MEMF_no_tlbflush;
> +
>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
>          if ( i != a->nr_done && hypercall_preempt_check() )
> @@ -214,6 +227,13 @@ static void populate_physmap(struct memop_args *a)
>                      goto out;
>                  }
>  
> +                if ( unlikely(a->memflags & MEMF_no_tlbflush) )
> +                {
> +                    for ( j = 0; j < (1U << a->extent_order); j++ )
> +                        accumulate_tlbflush(&need_tlbflush, &page[j],
> +                                            &tlbflush_timestamp);
> +                }
> +
>                  mfn = page_to_mfn(page);
>              }
>  
> @@ -232,6 +252,8 @@ static void populate_physmap(struct memop_args *a)
>      }
>  
>  out:
> +    if ( need_tlbflush )
> +        filtered_flush_tlb_mask(tlbflush_timestamp);
>      a->nr_done = i;
>  }
>  
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index d7ca3a0..ae2476d 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -827,7 +827,9 @@ static struct page_info *alloc_heap_pages(
>          BUG_ON(pg[i].count_info != PGC_state_free);
>          pg[i].count_info = PGC_state_inuse;
>  
> -        accumulate_tlbflush(&need_tlbflush, &pg[i], &tlbflush_timestamp);
> +        if ( !(memflags & MEMF_no_tlbflush) )
> +            accumulate_tlbflush(&need_tlbflush, &pg[i],
> +                                &tlbflush_timestamp);
>  
>          /* Initialise fields which have other uses for free pages. */
>          pg[i].u.inuse.type_info = 0;
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 50db01d..76fbb82 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -222,6 +222,8 @@ struct npfec {
>  #define  MEMF_exact_node  (1U<<_MEMF_exact_node)
>  #define _MEMF_no_owner    5
>  #define  MEMF_no_owner    (1U<<_MEMF_no_owner)
> +#define _MEMF_no_tlbflush 6
> +#define  MEMF_no_tlbflush (1U<<_MEMF_no_tlbflush)
>  #define _MEMF_node        8
>  #define  MEMF_node_mask   ((1U << (8 * sizeof(nodeid_t))) - 1)
>  #define  MEMF_node(n)     ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 2f9c15f..cd05e55 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -386,6 +386,12 @@ struct domain
>      bool_t           disable_migrate;
>      /* Is this guest being debugged by dom0? */
>      bool_t           debugger_attached;
> +    /*
> +     * Set to true at the very end of domain creation, when the domain is
> +     * unpaused for the first time by the systemcontroller.
> +     */
> +    bool creation_finished;
> +
>      /* Which guest this guest has privileges on */
>      struct domain   *target;
>  
> 


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