All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions
@ 2016-09-20  2:31 Dongli Zhang
  2016-09-20  2:31 ` [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Dongli Zhang
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Dongli Zhang @ 2016-09-20  2:31 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, jinsong.liu,
	dario.faggioli, ian.jackson, tim, jbeulich, andrew.cooper3

This patch cleaned up the code by replacing complicated tlbflush check and
operation with inline functions. We should use those inline functions to
avoid the complicated tlbflush check and tlbflush operations when
implementing TODOs left in commit a902c12ee45fc9389eb8fe54eeddaf267a555c58
(More efficient TLB-flush filtering in alloc_heap_pages()).

"#include <asm/flushtlb.h>" is removed from xen/arch/x86/acpi/suspend.c to
avoid the compiling error after we include "<asm/flushtlb.h>" to
xen/include/xen/mm.h.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
Changed since v5:
  * Move the if() and its body of tlbflush check into inline function.

Changed since v4:
  * Wrap the filtered tlbflush mask operation as inline function (suggested
    by Jan).
  * Remove asm/flushtlb.h from suspend.c to avoid compiling error.

Changed since v3:
  * Wrap the complicated tlbflush condition check as inline function
    (suggested by Dario).

---
 xen/arch/x86/acpi/suspend.c |  1 -
 xen/common/page_alloc.c     | 19 ++-----------------
 xen/include/xen/mm.h        | 29 +++++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index 1d8344c..d5c67ee 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -10,7 +10,6 @@
 #include <asm/processor.h>
 #include <asm/msr.h>
 #include <asm/debugreg.h>
-#include <asm/flushtlb.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
 #include <asm/i387.h>
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 18ff6cf..d7ca3a0 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -827,14 +827,7 @@ static struct page_info *alloc_heap_pages(
         BUG_ON(pg[i].count_info != PGC_state_free);
         pg[i].count_info = PGC_state_inuse;
 
-        if ( pg[i].u.free.need_tlbflush &&
-             (pg[i].tlbflush_timestamp <= tlbflush_current_time()) &&
-             (!need_tlbflush ||
-              (pg[i].tlbflush_timestamp > tlbflush_timestamp)) )
-        {
-            need_tlbflush = 1;
-            tlbflush_timestamp = pg[i].tlbflush_timestamp;
-        }
+        accumulate_tlbflush(&need_tlbflush, &pg[i], &tlbflush_timestamp);
 
         /* Initialise fields which have other uses for free pages. */
         pg[i].u.inuse.type_info = 0;
@@ -849,15 +842,7 @@ static struct page_info *alloc_heap_pages(
     spin_unlock(&heap_lock);
 
     if ( need_tlbflush )
-    {
-        cpumask_t mask = cpu_online_map;
-        tlbflush_filter(mask, tlbflush_timestamp);
-        if ( !cpumask_empty(&mask) )
-        {
-            perfc_incr(need_flush_tlb_flush);
-            flush_tlb_mask(&mask);
-        }
-    }
+        filtered_flush_tlb_mask(tlbflush_timestamp);
 
     return pg;
 }
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index f470e49..50db01d 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -51,6 +51,7 @@
 #include <xen/spinlock.h>
 #include <xen/typesafe.h>
 #include <xen/kernel.h>
+#include <xen/perfc.h>
 #include <public/memory.h>
 
 TYPE_SAFE(unsigned long, mfn);
@@ -567,4 +568,32 @@ int prepare_ring_for_helper(struct domain *d, unsigned long gmfn,
                             struct page_info **_page, void **_va);
 void destroy_ring_for_helper(void **_va, struct page_info *page);
 
+#include <asm/flushtlb.h>
+
+static inline void accumulate_tlbflush(bool *need_tlbflush,
+                                       const struct page_info *page,
+                                       uint32_t *tlbflush_timestamp)
+{
+    if ( page->u.free.need_tlbflush &&
+         page->tlbflush_timestamp <= tlbflush_current_time() &&
+         (!*need_tlbflush ||
+          page->tlbflush_timestamp > *tlbflush_timestamp) )
+    {
+        *need_tlbflush = true;
+        *tlbflush_timestamp = page->tlbflush_timestamp;
+    }
+}
+
+static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
+{
+    cpumask_t mask = cpu_online_map;
+
+    tlbflush_filter(mask, tlbflush_timestamp);
+    if ( !cpumask_empty(&mask) )
+    {
+        perfc_incr(need_flush_tlb_flush);
+        flush_tlb_mask(&mask);
+    }
+}
+
 #endif /* __XEN_MM_H__ */
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation
  2016-09-20  2:31 [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions Dongli Zhang
@ 2016-09-20  2:31 ` Dongli Zhang
  2016-09-20  9:01   ` Jan Beulich
                     ` (2 more replies)
  2016-09-20  8:56 ` [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions Jan Beulich
  2016-09-20 11:19 ` George Dunlap
  2 siblings, 3 replies; 9+ messages in thread
From: Dongli Zhang @ 2016-09-20  2:31 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, jinsong.liu,
	dario.faggioli, ian.jackson, tim, jbeulich, andrew.cooper3

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>
---
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;
 
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions
  2016-09-20  2:31 [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions Dongli Zhang
  2016-09-20  2:31 ` [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Dongli Zhang
@ 2016-09-20  8:56 ` Jan Beulich
  2016-09-20 11:19 ` George Dunlap
  2 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2016-09-20  8:56 UTC (permalink / raw)
  To: Dongli Zhang
  Cc: tim, sstabellini, wei.liu2, George.Dunlap, jinsong.liu,
	dario.faggioli, ian.jackson, xen-devel, andrew.cooper3

>>> On 20.09.16 at 04:31, <dongli.zhang@oracle.com> wrote:
> This patch cleaned up the code by replacing complicated tlbflush check and
> operation with inline functions. We should use those inline functions to
> avoid the complicated tlbflush check and tlbflush operations when
> implementing TODOs left in commit a902c12ee45fc9389eb8fe54eeddaf267a555c58
> (More efficient TLB-flush filtering in alloc_heap_pages()).
> 
> "#include <asm/flushtlb.h>" is removed from xen/arch/x86/acpi/suspend.c to
> avoid the compiling error after we include "<asm/flushtlb.h>" to
> xen/include/xen/mm.h.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation
  2016-09-20  2:31 ` [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Dongli Zhang
@ 2016-09-20  9:01   ` Jan Beulich
  2016-09-20 11:20   ` George Dunlap
  2016-09-20 14:54   ` Wei Liu
  2 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2016-09-20  9:01 UTC (permalink / raw)
  To: Dongli Zhang
  Cc: tim, sstabellini, wei.liu2, George.Dunlap, jinsong.liu,
	dario.faggioli, ian.jackson, xen-devel, andrew.cooper3

>>> On 20.09.16 at 04:31, <dongli.zhang@oracle.com> 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: Jan Beulich <jbeulich@suse.com>
with ...

> --- 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;

... blank padding added here to match the style of the surrounding
code. I'll try to remember to take care of this during commit, but I'd
appreciate if you'd look at neighboring code next time round.

Jan


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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions
  2016-09-20  2:31 [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions Dongli Zhang
  2016-09-20  2:31 ` [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Dongli Zhang
  2016-09-20  8:56 ` [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions Jan Beulich
@ 2016-09-20 11:19 ` George Dunlap
  2016-09-20 12:14   ` Dario Faggioli
  2 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2016-09-20 11:19 UTC (permalink / raw)
  To: Dongli Zhang, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, jinsong.liu,
	dario.faggioli, ian.jackson, tim, jbeulich, andrew.cooper3

On 20/09/16 03:31, Dongli Zhang wrote:
> This patch cleaned up the code by replacing complicated tlbflush check and
> operation with inline functions. We should use those inline functions to
> avoid the complicated tlbflush check and tlbflush operations when
> implementing TODOs left in commit a902c12ee45fc9389eb8fe54eeddaf267a555c58
> (More efficient TLB-flush filtering in alloc_heap_pages()).
> 
> "#include <asm/flushtlb.h>" is removed from xen/arch/x86/acpi/suspend.c to
> avoid the compiling error after we include "<asm/flushtlb.h>" to
> xen/include/xen/mm.h.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

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

> ---
> Changed since v5:
>   * Move the if() and its body of tlbflush check into inline function.
> 
> Changed since v4:
>   * Wrap the filtered tlbflush mask operation as inline function (suggested
>     by Jan).
>   * Remove asm/flushtlb.h from suspend.c to avoid compiling error.
> 
> Changed since v3:
>   * Wrap the complicated tlbflush condition check as inline function
>     (suggested by Dario).
> 
> ---
>  xen/arch/x86/acpi/suspend.c |  1 -
>  xen/common/page_alloc.c     | 19 ++-----------------
>  xen/include/xen/mm.h        | 29 +++++++++++++++++++++++++++++
>  3 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
> index 1d8344c..d5c67ee 100644
> --- a/xen/arch/x86/acpi/suspend.c
> +++ b/xen/arch/x86/acpi/suspend.c
> @@ -10,7 +10,6 @@
>  #include <asm/processor.h>
>  #include <asm/msr.h>
>  #include <asm/debugreg.h>
> -#include <asm/flushtlb.h>
>  #include <asm/hvm/hvm.h>
>  #include <asm/hvm/support.h>
>  #include <asm/i387.h>
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 18ff6cf..d7ca3a0 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -827,14 +827,7 @@ static struct page_info *alloc_heap_pages(
>          BUG_ON(pg[i].count_info != PGC_state_free);
>          pg[i].count_info = PGC_state_inuse;
>  
> -        if ( pg[i].u.free.need_tlbflush &&
> -             (pg[i].tlbflush_timestamp <= tlbflush_current_time()) &&
> -             (!need_tlbflush ||
> -              (pg[i].tlbflush_timestamp > tlbflush_timestamp)) )
> -        {
> -            need_tlbflush = 1;
> -            tlbflush_timestamp = pg[i].tlbflush_timestamp;
> -        }
> +        accumulate_tlbflush(&need_tlbflush, &pg[i], &tlbflush_timestamp);
>  
>          /* Initialise fields which have other uses for free pages. */
>          pg[i].u.inuse.type_info = 0;
> @@ -849,15 +842,7 @@ static struct page_info *alloc_heap_pages(
>      spin_unlock(&heap_lock);
>  
>      if ( need_tlbflush )
> -    {
> -        cpumask_t mask = cpu_online_map;
> -        tlbflush_filter(mask, tlbflush_timestamp);
> -        if ( !cpumask_empty(&mask) )
> -        {
> -            perfc_incr(need_flush_tlb_flush);
> -            flush_tlb_mask(&mask);
> -        }
> -    }
> +        filtered_flush_tlb_mask(tlbflush_timestamp);
>  
>      return pg;
>  }
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index f470e49..50db01d 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -51,6 +51,7 @@
>  #include <xen/spinlock.h>
>  #include <xen/typesafe.h>
>  #include <xen/kernel.h>
> +#include <xen/perfc.h>
>  #include <public/memory.h>
>  
>  TYPE_SAFE(unsigned long, mfn);
> @@ -567,4 +568,32 @@ int prepare_ring_for_helper(struct domain *d, unsigned long gmfn,
>                              struct page_info **_page, void **_va);
>  void destroy_ring_for_helper(void **_va, struct page_info *page);
>  
> +#include <asm/flushtlb.h>
> +
> +static inline void accumulate_tlbflush(bool *need_tlbflush,
> +                                       const struct page_info *page,
> +                                       uint32_t *tlbflush_timestamp)
> +{
> +    if ( page->u.free.need_tlbflush &&
> +         page->tlbflush_timestamp <= tlbflush_current_time() &&
> +         (!*need_tlbflush ||
> +          page->tlbflush_timestamp > *tlbflush_timestamp) )
> +    {
> +        *need_tlbflush = true;
> +        *tlbflush_timestamp = page->tlbflush_timestamp;
> +    }
> +}
> +
> +static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
> +{
> +    cpumask_t mask = cpu_online_map;
> +
> +    tlbflush_filter(mask, tlbflush_timestamp);
> +    if ( !cpumask_empty(&mask) )
> +    {
> +        perfc_incr(need_flush_tlb_flush);
> +        flush_tlb_mask(&mask);
> +    }
> +}
> +
>  #endif /* __XEN_MM_H__ */
> 


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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation
  2016-09-20  2:31 ` [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Dongli Zhang
  2016-09-20  9:01   ` Jan Beulich
@ 2016-09-20 11:20   ` George Dunlap
  2016-09-20 12:14     ` Dario Faggioli
  2016-09-20 14:54   ` Wei Liu
  2 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2016-09-20 11:20 UTC (permalink / raw)
  To: Dongli Zhang, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, jinsong.liu,
	dario.faggioli, ian.jackson, tim, jbeulich, andrew.cooper3

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation
  2016-09-20 11:20   ` George Dunlap
@ 2016-09-20 12:14     ` Dario Faggioli
  0 siblings, 0 replies; 9+ messages in thread
From: Dario Faggioli @ 2016-09-20 12:14 UTC (permalink / raw)
  To: George Dunlap, Dongli Zhang, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, jinsong.liu, ian.jackson,
	tim, jbeulich, andrew.cooper3


[-- Attachment #1.1: Type: text/plain, Size: 1802 bytes --]

On Tue, 2016-09-20 at 12:20 +0100, George Dunlap wrote:
> 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>
> 
FWIW, and if I'm still in time:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions
  2016-09-20 11:19 ` George Dunlap
@ 2016-09-20 12:14   ` Dario Faggioli
  0 siblings, 0 replies; 9+ messages in thread
From: Dario Faggioli @ 2016-09-20 12:14 UTC (permalink / raw)
  To: George Dunlap, Dongli Zhang, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, jinsong.liu, ian.jackson,
	tim, jbeulich, andrew.cooper3


[-- Attachment #1.1: Type: text/plain, Size: 1133 bytes --]

On Tue, 2016-09-20 at 12:19 +0100, George Dunlap wrote:
> On 20/09/16 03:31, Dongli Zhang wrote:
> > 
> > This patch cleaned up the code by replacing complicated tlbflush
> > check and
> > operation with inline functions. We should use those inline
> > functions to
> > avoid the complicated tlbflush check and tlbflush operations when
> > implementing TODOs left in commit
> > a902c12ee45fc9389eb8fe54eeddaf267a555c58
> > (More efficient TLB-flush filtering in alloc_heap_pages()).
> > 
> > "#include <asm/flushtlb.h>" is removed from
> > xen/arch/x86/acpi/suspend.c to
> > avoid the compiling error after we include "<asm/flushtlb.h>" to
> > xen/include/xen/mm.h.
> > 
> > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> 
> Acked-by: George Dunlap <george.dunlap@citrix.com>
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation
  2016-09-20  2:31 ` [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Dongli Zhang
  2016-09-20  9:01   ` Jan Beulich
  2016-09-20 11:20   ` George Dunlap
@ 2016-09-20 14:54   ` Wei Liu
  2 siblings, 0 replies; 9+ messages in thread
From: Wei Liu @ 2016-09-20 14:54 UTC (permalink / raw)
  To: Dongli Zhang
  Cc: tim, sstabellini, wei.liu2, George.Dunlap, jinsong.liu,
	dario.faggioli, ian.jackson, xen-devel, jbeulich, andrew.cooper3

On Tue, Sep 20, 2016 at 10:31:04AM +0800, 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.
> 

Do you have some actual numbers on how much faster after applying this
patch?

This is mostly for writing release note etc, so it is fine if you don't
have numbers at hand.

Wei.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-20 14:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-20  2:31 [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions Dongli Zhang
2016-09-20  2:31 ` [PATCH v6 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation Dongli Zhang
2016-09-20  9:01   ` Jan Beulich
2016-09-20 11:20   ` George Dunlap
2016-09-20 12:14     ` Dario Faggioli
2016-09-20 14:54   ` Wei Liu
2016-09-20  8:56 ` [PATCH v6 1/2] xen: replace tlbflush check and operation with inline functions Jan Beulich
2016-09-20 11:19 ` George Dunlap
2016-09-20 12:14   ` Dario Faggioli

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.