All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH 0/2] x86: improve assisted tlb flush and use it in guest mode
@ 2019-12-24 13:26 Roger Pau Monne
  2019-12-24 13:26 ` [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs Roger Pau Monne
  2019-12-24 13:26 ` [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available Roger Pau Monne
  0 siblings, 2 replies; 11+ messages in thread
From: Roger Pau Monne @ 2019-12-24 13:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monne

Hello,

The following series aims to improve the tlb flush times when running
nested Xen, and it's specially beneficial when running in shim mode.

See patch #2 for a comparison on the performance of the L0 assisted
flush vs using x2APIC shorthand.

Thanks, Roger.

Roger Pau Monne (2):
  x86/hvm: improve performance of HVMOP_flush_tlbs
  x86/tlb: use Xen L0 assisted TLB flush when available

 xen/arch/x86/guest/xen/xen.c    | 11 +++++++
 xen/arch/x86/hvm/hvm.c          | 55 ++++++++++++++++++++++-----------
 xen/arch/x86/smp.c              |  6 ++++
 xen/include/asm-x86/guest/xen.h |  7 +++++
 4 files changed, 61 insertions(+), 18 deletions(-)

-- 
2.24.1


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs
  2019-12-24 13:26 [Xen-devel] [PATCH 0/2] x86: improve assisted tlb flush and use it in guest mode Roger Pau Monne
@ 2019-12-24 13:26 ` Roger Pau Monne
  2019-12-27 14:52   ` Andrew Cooper
  2020-01-03 15:17   ` Jan Beulich
  2019-12-24 13:26 ` [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available Roger Pau Monne
  1 sibling, 2 replies; 11+ messages in thread
From: Roger Pau Monne @ 2019-12-24 13:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monne

There's no need to call paging_update_cr3 unless CR3 trapping is
enabled, and that's only the case when using shadow paging or when
requested for introspection purposes, otherwise there's no need to
pause all the vCPUs of the domain in order to perform the flush.

Check whether CR3 trapping is currently in use in order to decide
whether the vCPUs should be paused, otherwise just perform the flush.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/hvm.c | 55 ++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4dfaf35566..7dcc16afc6 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3985,25 +3985,36 @@ bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
     static DEFINE_PER_CPU(cpumask_t, flush_cpumask);
     cpumask_t *mask = &this_cpu(flush_cpumask);
     struct domain *d = current->domain;
+    /*
+     * CR3 trapping is only enabled when running with shadow paging or when
+     * requested for introspection purposes, otherwise there's no need to call
+     * paging_update_cr3 and hence pause all vCPUs.
+     */
+    bool trap_cr3 = !paging_mode_hap(d) ||
+                    (d->arch.monitor.write_ctrlreg_enabled &
+                    monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3));
     struct vcpu *v;
 
-    /* Avoid deadlock if more than one vcpu tries this at the same time. */
-    if ( !spin_trylock(&d->hypercall_deadlock_mutex) )
-        return false;
+    if ( trap_cr3 )
+    {
+        /* Avoid deadlock if more than one vcpu tries this at the same time. */
+        if ( !spin_trylock(&d->hypercall_deadlock_mutex) )
+            return false;
 
-    /* Pause all other vcpus. */
-    for_each_vcpu ( d, v )
-        if ( v != current && flush_vcpu(ctxt, v) )
-            vcpu_pause_nosync(v);
+        /* Pause all other vcpus. */
+        for_each_vcpu ( d, v )
+            if ( v != current && flush_vcpu(ctxt, v) )
+                vcpu_pause_nosync(v);
 
-    /* Now that all VCPUs are signalled to deschedule, we wait... */
-    for_each_vcpu ( d, v )
-        if ( v != current && flush_vcpu(ctxt, v) )
-            while ( !vcpu_runnable(v) && v->is_running )
-                cpu_relax();
+        /* Now that all VCPUs are signalled to deschedule, we wait... */
+        for_each_vcpu ( d, v )
+            if ( v != current && flush_vcpu(ctxt, v) )
+                while ( !vcpu_runnable(v) && v->is_running )
+                    cpu_relax();
 
-    /* All other vcpus are paused, safe to unlock now. */
-    spin_unlock(&d->hypercall_deadlock_mutex);
+        /* All other vcpus are paused, safe to unlock now. */
+        spin_unlock(&d->hypercall_deadlock_mutex);
+    }
 
     cpumask_clear(mask);
 
@@ -4015,8 +4026,15 @@ bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
         if ( !flush_vcpu(ctxt, v) )
             continue;
 
-        paging_update_cr3(v, false);
+        if ( trap_cr3 )
+            paging_update_cr3(v, false);
 
+        /*
+         * It's correct to do this flush without pausing the vCPUs: any vCPU
+         * context switch will already flush the tlb and the worse that could
+         * happen is that Xen ends up performing flushes on pCPUs that are no
+         * longer running the target vCPUs.
+         */
         cpu = read_atomic(&v->dirty_cpu);
         if ( is_vcpu_dirty_cpu(cpu) )
             __cpumask_set_cpu(cpu, mask);
@@ -4026,9 +4044,10 @@ bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
     flush_tlb_mask(mask);
 
     /* Done. */
-    for_each_vcpu ( d, v )
-        if ( v != current && flush_vcpu(ctxt, v) )
-            vcpu_unpause(v);
+    if ( trap_cr3 )
+        for_each_vcpu ( d, v )
+            if ( v != current && flush_vcpu(ctxt, v) )
+                vcpu_unpause(v);
 
     return true;
 }
-- 
2.24.1


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available
  2019-12-24 13:26 [Xen-devel] [PATCH 0/2] x86: improve assisted tlb flush and use it in guest mode Roger Pau Monne
  2019-12-24 13:26 ` [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs Roger Pau Monne
@ 2019-12-24 13:26 ` Roger Pau Monne
  2019-12-25 16:13   ` Wei Liu
                     ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Roger Pau Monne @ 2019-12-24 13:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monne

Use Xen's L0 HVMOP_flush_tlbs hypercall when available in order to
perform flushes. This greatly increases the performance of tlb flushes
when running with a high amount of vCPUs as a Xen guest, and is
specially important when running in shim mode.

The following figures are from a PV guest running `make -j342 xen` in
shim mode with 32 vCPUs.

Using x2APIC and ALLBUT shorthand:
real	4m35.973s
user	4m35.110s
sys	36m24.117s

Using L0 assisted flush:
real	1m17.391s
user	4m42.413s
sys	6m20.773s

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/guest/xen/xen.c    | 11 +++++++++++
 xen/arch/x86/smp.c              |  6 ++++++
 xen/include/asm-x86/guest/xen.h |  7 +++++++
 3 files changed, 24 insertions(+)

diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
index 6dbc5f953f..e6493caecf 100644
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -281,6 +281,17 @@ int xg_free_unused_page(mfn_t mfn)
     return rangeset_remove_range(mem, mfn_x(mfn), mfn_x(mfn));
 }
 
+int xg_flush_tlbs(void)
+{
+    int rc;
+
+    do {
+        rc = xen_hypercall_hvm_op(HVMOP_flush_tlbs, NULL);
+    } while ( rc == -ERESTART );
+
+    return rc;
+}
+
 static void ap_resume(void *unused)
 {
     map_vcpuinfo();
diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index 427c33db9d..a892db28c1 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -15,6 +15,7 @@
 #include <xen/perfc.h>
 #include <xen/spinlock.h>
 #include <asm/current.h>
+#include <asm/guest.h>
 #include <asm/smp.h>
 #include <asm/mc146818rtc.h>
 #include <asm/flushtlb.h>
@@ -235,6 +236,11 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
     {
         bool cpus_locked = false;
 
+        if ( xen_guest &&
+             !(flags & ~(FLUSH_TLB | FLUSH_TLB_GLOBAL | FLUSH_VA_VALID)) &&
+             !xg_flush_tlbs() )
+            return;
+
         spin_lock(&flush_lock);
         cpumask_and(&flush_cpumask, mask, &cpu_online_map);
         cpumask_clear_cpu(cpu, &flush_cpumask);
diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h
index 2042a9a0c2..f0de9e4d71 100644
--- a/xen/include/asm-x86/guest/xen.h
+++ b/xen/include/asm-x86/guest/xen.h
@@ -36,6 +36,7 @@ extern uint32_t xen_cpuid_base;
 const struct hypervisor_ops *xg_probe(void);
 int xg_alloc_unused_page(mfn_t *mfn);
 int xg_free_unused_page(mfn_t mfn);
+int xg_flush_tlbs(void);
 
 DECLARE_PER_CPU(unsigned int, vcpu_id);
 DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
@@ -47,6 +48,12 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
 
 static inline const struct hypervisor_ops *xg_probe(void) { return NULL; }
 
+static inline int xg_flush_tlbs(void)
+{
+    ASSERT_UNREACHABLE();
+    return -ENOSYS;
+}
+
 #endif /* CONFIG_XEN_GUEST */
 #endif /* __X86_GUEST_XEN_H__ */
 
-- 
2.24.1


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available
  2019-12-24 13:26 ` [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available Roger Pau Monne
@ 2019-12-25 16:13   ` Wei Liu
  2019-12-27 14:55   ` Andrew Cooper
  2020-01-03 15:23   ` Jan Beulich
  2 siblings, 0 replies; 11+ messages in thread
From: Wei Liu @ 2019-12-25 16:13 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper

On Tue, Dec 24, 2019 at 02:26:16PM +0100, Roger Pau Monne wrote:
> Use Xen's L0 HVMOP_flush_tlbs hypercall when available in order to
> perform flushes. This greatly increases the performance of tlb flushes
> when running with a high amount of vCPUs as a Xen guest, and is
> specially important when running in shim mode.
> 
> The following figures are from a PV guest running `make -j342 xen` in
> shim mode with 32 vCPUs.
> 
> Using x2APIC and ALLBUT shorthand:
> real	4m35.973s
> user	4m35.110s
> sys	36m24.117s
> 
> Using L0 assisted flush:
> real	1m17.391s
> user	4m42.413s
> sys	6m20.773s
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/guest/xen/xen.c    | 11 +++++++++++
>  xen/arch/x86/smp.c              |  6 ++++++
>  xen/include/asm-x86/guest/xen.h |  7 +++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
> index 6dbc5f953f..e6493caecf 100644
> --- a/xen/arch/x86/guest/xen/xen.c
> +++ b/xen/arch/x86/guest/xen/xen.c
> @@ -281,6 +281,17 @@ int xg_free_unused_page(mfn_t mfn)
>      return rangeset_remove_range(mem, mfn_x(mfn), mfn_x(mfn));
>  }
>  
> +int xg_flush_tlbs(void)
> +{
> +    int rc;
> +
> +    do {
> +        rc = xen_hypercall_hvm_op(HVMOP_flush_tlbs, NULL);
> +    } while ( rc == -ERESTART );
> +
> +    return rc;
> +}
> +

Is it possible to make this a hook in the hypervisor_op?

I can foresee there will be something similar for Hyper-V.

>  static void ap_resume(void *unused)
>  {
>      map_vcpuinfo();
> diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> index 427c33db9d..a892db28c1 100644
> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -15,6 +15,7 @@
>  #include <xen/perfc.h>
>  #include <xen/spinlock.h>
>  #include <asm/current.h>
> +#include <asm/guest.h>
>  #include <asm/smp.h>
>  #include <asm/mc146818rtc.h>
>  #include <asm/flushtlb.h>
> @@ -235,6 +236,11 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
>      {
>          bool cpus_locked = false;
>  
> +        if ( xen_guest &&

Also it would be better to not expose xen_guest here. It is x86 generic
code after all.

I would probably introduce a function to tell if Xen is running
virtualised or not.

Wei.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs
  2019-12-24 13:26 ` [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs Roger Pau Monne
@ 2019-12-27 14:52   ` Andrew Cooper
  2019-12-31 12:10     ` Roger Pau Monné
  2020-01-03 15:17   ` Jan Beulich
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2019-12-27 14:52 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Jan Beulich

On 24/12/2019 13:26, Roger Pau Monne wrote:
> There's no need to call paging_update_cr3 unless CR3 trapping is
> enabled, and that's only the case when using shadow paging or when
> requested for introspection purposes, otherwise there's no need to
> pause all the vCPUs of the domain in order to perform the flush.
>
> Check whether CR3 trapping is currently in use in order to decide
> whether the vCPUs should be paused, otherwise just perform the flush.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I agree that the existing logic poor, but this direction looks to be
even more fragile.

Instead, I think it would be better to follow the EPT invalidation
example; mark all vcpus as needing a tlb flush, and IPI the domain dirty
mask, having the return-to-guest path do the flushing.

This avoids all vcpu pausing/unpausing activities, and the cost of the
flush is incurred by the target vcpu, rather than the vcpu making the
hypercall accumulate the cost for everything, as well as a large amount
of remote VMCS accesses.

It can probably also remove the need for the flush_vcpu() callback which
is going to be expensive due to retpoline, and whose contents are trivial.

~Andrew

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available
  2019-12-24 13:26 ` [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available Roger Pau Monne
  2019-12-25 16:13   ` Wei Liu
@ 2019-12-27 14:55   ` Andrew Cooper
  2020-01-03 15:23   ` Jan Beulich
  2 siblings, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2019-12-27 14:55 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Jan Beulich

On 24/12/2019 13:26, Roger Pau Monne wrote:
> Use Xen's L0 HVMOP_flush_tlbs hypercall when available in order to
> perform flushes. This greatly increases the performance of tlb flushes
> when running with a high amount of vCPUs as a Xen guest, and is
> specially important when running in shim mode.
>
> The following figures are from a PV guest running `make -j342 xen` in
> shim mode with 32 vCPUs.
>
> Using x2APIC and ALLBUT shorthand:
> real	4m35.973s
> user	4m35.110s
> sys	36m24.117s
>
> Using L0 assisted flush:
> real	1m17.391s
> user	4m42.413s
> sys	6m20.773s

Nice stats.

>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/guest/xen/xen.c    | 11 +++++++++++
>  xen/arch/x86/smp.c              |  6 ++++++
>  xen/include/asm-x86/guest/xen.h |  7 +++++++
>  3 files changed, 24 insertions(+)
>
> diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
> index 6dbc5f953f..e6493caecf 100644
> --- a/xen/arch/x86/guest/xen/xen.c
> +++ b/xen/arch/x86/guest/xen/xen.c
> @@ -281,6 +281,17 @@ int xg_free_unused_page(mfn_t mfn)
>      return rangeset_remove_range(mem, mfn_x(mfn), mfn_x(mfn));
>  }
>  
> +int xg_flush_tlbs(void)
> +{
> +    int rc;
> +
> +    do {
> +        rc = xen_hypercall_hvm_op(HVMOP_flush_tlbs, NULL);
> +    } while ( rc == -ERESTART );

ERESTART should never manifest like this, because it is taken care of
within the hypercall_page[] stub.  Anything else is a bug which needs
fixing at L0.

Have you actually seen one appearing?

~Andrew

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs
  2019-12-27 14:52   ` Andrew Cooper
@ 2019-12-31 12:10     ` Roger Pau Monné
  2020-01-03 11:18       ` Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Roger Pau Monné @ 2019-12-31 12:10 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Jan Beulich

On Fri, Dec 27, 2019 at 02:52:17PM +0000, Andrew Cooper wrote:
> On 24/12/2019 13:26, Roger Pau Monne wrote:
> > There's no need to call paging_update_cr3 unless CR3 trapping is
> > enabled, and that's only the case when using shadow paging or when
> > requested for introspection purposes, otherwise there's no need to
> > pause all the vCPUs of the domain in order to perform the flush.
> >
> > Check whether CR3 trapping is currently in use in order to decide
> > whether the vCPUs should be paused, otherwise just perform the flush.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I agree that the existing logic poor, but this direction looks to be
> even more fragile.
> 
> Instead, I think it would be better to follow the EPT invalidation
> example; mark all vcpus as needing a tlb flush, and IPI the domain dirty
> mask, having the return-to-guest path do the flushing.

AFAICT there's no need to call the tlb flush, the vmexit/vmentry
itself will perform the necessary flushes, so the only requirement is
to IPI the pCPUs in order to force a vmexit.

> This avoids all vcpu pausing/unpausing activities, and the cost of the
> flush is incurred by the target vcpu, rather than the vcpu making the
> hypercall accumulate the cost for everything, as well as a large amount
> of remote VMCS accesses.

Hm, then we would need a way to pin the vCPUs to the pCPUs they are
running on, or else in the introspection-enabled case you could end up
calling paging_update_cr3 on vCPUs of other domains (maybe that's
fine, but it could mess up with introspection I guess).

AFAICT the call to paging_update_cr3 needs to be done from
hvm_flush_vcpu_tlb or else we would have to freeze the scheduler so
that vCPUs don't move around pCPUs (or get de-scheduled), I think
we still need the pause in the introspection case, but the open coded
pause loop could be replaced with domain_pause_except_self.

> It can probably also remove the need for the flush_vcpu() callback which
> is going to be expensive due to retpoline, and whose contents are trivial.

I was planning to look into this, but wanted to send this version
first since it's already a big improvement in terms of performance.

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs
  2019-12-31 12:10     ` Roger Pau Monné
@ 2020-01-03 11:18       ` Jan Beulich
  0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2020-01-03 11:18 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: Andrew Cooper, Wei Liu, xen-devel

On 31.12.2019 13:10, Roger Pau Monné wrote:
> On Fri, Dec 27, 2019 at 02:52:17PM +0000, Andrew Cooper wrote:
>> On 24/12/2019 13:26, Roger Pau Monne wrote:
>>> There's no need to call paging_update_cr3 unless CR3 trapping is
>>> enabled, and that's only the case when using shadow paging or when
>>> requested for introspection purposes, otherwise there's no need to
>>> pause all the vCPUs of the domain in order to perform the flush.
>>>
>>> Check whether CR3 trapping is currently in use in order to decide
>>> whether the vCPUs should be paused, otherwise just perform the flush.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> I agree that the existing logic poor, but this direction looks to be
>> even more fragile.
>>
>> Instead, I think it would be better to follow the EPT invalidation
>> example; mark all vcpus as needing a tlb flush, and IPI the domain dirty
>> mask, having the return-to-guest path do the flushing.
> 
> AFAICT there's no need to call the tlb flush, the vmexit/vmentry
> itself will perform the necessary flushes, so the only requirement is
> to IPI the pCPUs in order to force a vmexit.

TLB flushing is at best conditional upon VM entry - see the callers
of hvm_asid_handle_vmenter().

Jan

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs
  2019-12-24 13:26 ` [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs Roger Pau Monne
  2019-12-27 14:52   ` Andrew Cooper
@ 2020-01-03 15:17   ` Jan Beulich
  2020-01-08 17:37     ` Roger Pau Monné
  1 sibling, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2020-01-03 15:17 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Andrew Cooper

On 24.12.2019 14:26, Roger Pau Monne wrote:
> There's no need to call paging_update_cr3 unless CR3 trapping is
> enabled, and that's only the case when using shadow paging or when
> requested for introspection purposes, otherwise there's no need to
> pause all the vCPUs of the domain in order to perform the flush.
> 
> Check whether CR3 trapping is currently in use in order to decide
> whether the vCPUs should be paused, otherwise just perform the flush.

First of all - with the commit introducing the pausing not saying
anything on the "why", you must have gained some understanding
there. Could you share this? I can't see why this was needed, and
sh_update_cr3() also doesn't look to have any respective ASSERT()
or alike. I'm having even more trouble seeing why in HAP mode the
pausing would be needed.

As a result I wonder whether, rather than determining whether
pausing is needed inside the function, this shouldn't be a flag
in struct paging_mode.

Next I seriously doubt introspection hooks should be called here.
Introspection should be about guest actions, and us calling
paging_update_cr3() is an implementation detail of Xen, not
something the guest controls. Even more, there not being any CR3
change here I wonder whether the call by the hooks to
hvm_update_guest_cr3() couldn't be suppressed altogether in this
case. Quite possibly in the shadow case there could be more
steps that aren't really needed, so perhaps a separate hook might
be on order.

Jan

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available
  2019-12-24 13:26 ` [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available Roger Pau Monne
  2019-12-25 16:13   ` Wei Liu
  2019-12-27 14:55   ` Andrew Cooper
@ 2020-01-03 15:23   ` Jan Beulich
  2 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2020-01-03 15:23 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Andrew Cooper

On 24.12.2019 14:26, Roger Pau Monne wrote:
> @@ -235,6 +236,11 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
>      {
>          bool cpus_locked = false;
>  
> +        if ( xen_guest &&
> +             !(flags & ~(FLUSH_TLB | FLUSH_TLB_GLOBAL | FLUSH_VA_VALID)) &&
> +             !xg_flush_tlbs() )
> +            return;

With the abstraction introduced recently by Wei I think this wants
to be a per-hypervisor hook, which would also get the linear address
passed, and which would then (rather than here) decide whether it
wants to also handle a single page flush a different way.

Jan

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs
  2020-01-03 15:17   ` Jan Beulich
@ 2020-01-08 17:37     ` Roger Pau Monné
  0 siblings, 0 replies; 11+ messages in thread
From: Roger Pau Monné @ 2020-01-08 17:37 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Wei Liu, Andrew Cooper

On Fri, Jan 03, 2020 at 04:17:14PM +0100, Jan Beulich wrote:
> On 24.12.2019 14:26, Roger Pau Monne wrote:
> > There's no need to call paging_update_cr3 unless CR3 trapping is
> > enabled, and that's only the case when using shadow paging or when
> > requested for introspection purposes, otherwise there's no need to
> > pause all the vCPUs of the domain in order to perform the flush.
> > 
> > Check whether CR3 trapping is currently in use in order to decide
> > whether the vCPUs should be paused, otherwise just perform the flush.
> 
> First of all - with the commit introducing the pausing not saying
> anything on the "why", you must have gained some understanding
> there. Could you share this?

hap_update_cr3 does a "v->arch.hvm.hw_cr[3] = v->arch.hvm.guest_cr[3]"
unconditionally, and such access would be racy if the vCPU is running
and also modifying cr3 at the same time AFAICT.

Just pausing each vCPU before calling paging_update_cr3 should be fine
and would have a smaller performance penalty.

> I can't see why this was needed, and
> sh_update_cr3() also doesn't look to have any respective ASSERT()
> or alike. I'm having even more trouble seeing why in HAP mode the
> pausing would be needed.
>
> As a result I wonder whether, rather than determining whether
> pausing is needed inside the function, this shouldn't be a flag
> in struct paging_mode.
>
> Next I seriously doubt introspection hooks should be called here.
> Introspection should be about guest actions, and us calling
> paging_update_cr3() is an implementation detail of Xen, not
> something the guest controls. Even more, there not being any CR3
> change here I wonder whether the call by the hooks to
> hvm_update_guest_cr3() couldn't be suppressed altogether in this
> case. Quite possibly in the shadow case there could be more
> steps that aren't really needed, so perhaps a separate hook might
> be on order.

Right, I guess just having a hook that does a flush would be enough.
Let me try to propose something slightly better.

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-01-08 17:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-24 13:26 [Xen-devel] [PATCH 0/2] x86: improve assisted tlb flush and use it in guest mode Roger Pau Monne
2019-12-24 13:26 ` [Xen-devel] [PATCH 1/2] x86/hvm: improve performance of HVMOP_flush_tlbs Roger Pau Monne
2019-12-27 14:52   ` Andrew Cooper
2019-12-31 12:10     ` Roger Pau Monné
2020-01-03 11:18       ` Jan Beulich
2020-01-03 15:17   ` Jan Beulich
2020-01-08 17:37     ` Roger Pau Monné
2019-12-24 13:26 ` [Xen-devel] [PATCH 2/2] x86/tlb: use Xen L0 assisted TLB flush when available Roger Pau Monne
2019-12-25 16:13   ` Wei Liu
2019-12-27 14:55   ` Andrew Cooper
2020-01-03 15:23   ` Jan Beulich

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.