Re: [Xen-devel] [PATCH v4 8/8] x86: avoid double CR3 reload when switching to guest user mode

[Juergen Gross]

On 19/03/18 14:41, Jan Beulich wrote:
> When XPTI is active, the CR3 load in restore_all_guest is sufficient
> when switching to user mode, improving in particular system call and
> page fault exit paths for the guest.
> 
> Signed-off-by: Jan Beulich 
> Tested-by: Juergen Gross 
> Reviewed-by: Juergen Gross 

I've done some more simple performance tests with some of the patches
with xpti=false and xpti=true. Data is always elapsed time, system time
and user time in seconds for a make -j 4 in dom0 with 4 vcpus, stddev
in braces, based on 5 runs each (I tried 30 runs, but the result didn't
really change):

xpti=false
no patch:  89.96 ( 2.84)   97.05 ( 2.39)  178.64 ( 1.55)
Jan p1:90.65 ( 2.57)   99.50 ( 3.85)  180.35 ( 2.44)
Jan p5:91.33 ( 0.89)   99.72 ( 2.56)  180.97 ( 1.71)
Jan p6:90.86 ( 2.59)   97.09 ( 2.59)  177.85 ( 2.35)
Jan p7:90.72 ( 2.84)  100.10 ( 4.60)  179.85 ( 2.61)
Jan p8:88.59 ( 0.71)   96.31 ( 2.14)  178.47 ( 0.86)

xpti=true
no patch: 113.42 ( 3.13)  165.99 ( 3.89)  180.99 ( 1.63)
Jan p1:   111.69 ( 3.15)  163.63 ( 3.61)  181.22 ( 1.93)
Jan p5:   114.76 ( 2.28)  167.15 ( 4.67)  181.13 ( 1.75)
Jan p6:   116.85 ( 2.35)  168.73 ( 3.68)  181.27 ( 1.98)
Jan p7:   115.37 ( 2.71)  166.96 ( 4.41)  180.82 ( 1.98)
Jan p8:   114.85 ( 2.83)  167.08 ( 5.00)  181.27 ( 1.85)

Summing it up: performance isn't really changing for any of the patches.


Juergen

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

Re: [Xen-devel] [PATCH v4 8/8] x86: avoid double CR3 reload when switching to guest user mode

[Jan Beulich]

>>> On 22.03.18 at 14:20,  wrote:
> On Mon, Mar 19, 2018 at 07:41:42AM -0600, Jan Beulich wrote:
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -219,10 +219,22 @@ int pv_domain_initialise(struct domain *
>>  return rc;
>>  }
>>  
>> -static void _toggle_guest_pt(struct vcpu *v)
>> +static void _toggle_guest_pt(struct vcpu *v, bool force_cr3)
>>  {
>> +ASSERT(!in_irq());
>> +
>>  v->arch.flags ^= TF_kernel_mode;
>>  update_cr3(v);
>> +
>> +/*
>> + * There's no need to load CR3 here when it is going to be loaded on the
>> + * way out to guest mode again anyway, and when the page tables we're
>> + * currently on are the kernel ones (whereas when switching to kernel
>> + * mode we need to be able to write a bounce frame onto the kernel 
>> stack).
>> + */
> 
> Not sure I follow the comment. If you're talking about
> create_bounce_frame, it wouldn't call this function in the first place,
> right?

Right. The comment is talking about what may happen after we
return from here.

>> +if ( !force_cr3 && !(v->arch.flags & TF_kernel_mode) )
> 
> Also, it takes a bit of mental power to see !(v->arch.flags &
> TF_kernel_mode) means the mode Xen is using. Can you maybe just use a
> variable at the beginning like
> 
>bool kernel_mode = v->arch.flags & TF_kernel_mode;
> 
> and then use it here?

Except for the (how I would say) clutter by the extra local variable
I don't see much of a difference.

Jan


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

Re: [Xen-devel] [PATCH v4 8/8] x86: avoid double CR3 reload when switching to guest user mode

[Wei Liu]

On Mon, Mar 19, 2018 at 07:41:42AM -0600, Jan Beulich wrote:
> When XPTI is active, the CR3 load in restore_all_guest is sufficient
> when switching to user mode, improving in particular system call and
> page fault exit paths for the guest.
> 
> Signed-off-by: Jan Beulich 
> Tested-by: Juergen Gross 
> Reviewed-by: Juergen Gross 
> ---
> v2: Add ASSERT(!in_irq()).
> 
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -219,10 +219,22 @@ int pv_domain_initialise(struct domain *
>  return rc;
>  }
>  
> -static void _toggle_guest_pt(struct vcpu *v)
> +static void _toggle_guest_pt(struct vcpu *v, bool force_cr3)
>  {
> +ASSERT(!in_irq());
> +
>  v->arch.flags ^= TF_kernel_mode;
>  update_cr3(v);
> +
> +/*
> + * There's no need to load CR3 here when it is going to be loaded on the
> + * way out to guest mode again anyway, and when the page tables we're
> + * currently on are the kernel ones (whereas when switching to kernel
> + * mode we need to be able to write a bounce frame onto the kernel 
> stack).
> + */

Not sure I follow the comment. If you're talking about
create_bounce_frame, it wouldn't call this function in the first place,
right?

> +if ( !force_cr3 && !(v->arch.flags & TF_kernel_mode) )

Also, it takes a bit of mental power to see !(v->arch.flags &
TF_kernel_mode) means the mode Xen is using. Can you maybe just use a
variable at the beginning like

   bool kernel_mode = v->arch.flags & TF_kernel_mode;

and then use it here?

> +return;
> +
>  /* Don't flush user global mappings from the TLB. Don't tick TLB clock. 
> */
>  asm volatile ( "mov %0, %%cr3" : : "r" (v->arch.cr3) : "memory" );
>  
> @@ -252,13 +264,13 @@ void toggle_guest_mode(struct vcpu *v)
>  }
>  asm volatile ( "swapgs" );
>  
> -_toggle_guest_pt(v);
> +_toggle_guest_pt(v, cpu_has_no_xpti);
>  }
>  
>  void toggle_guest_pt(struct vcpu *v)
>  {
>  if ( !is_pv_32bit_vcpu(v) )
> -_toggle_guest_pt(v);
> +_toggle_guest_pt(v, true);
>  }
>  
>  /*
> 
> 
> 
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Xen-devel] [PATCH v4 8/8] x86: avoid double CR3 reload when switching to guest user mode

[Jan Beulich]

When XPTI is active, the CR3 load in restore_all_guest is sufficient
when switching to user mode, improving in particular system call and
page fault exit paths for the guest.

Signed-off-by: Jan Beulich 
Tested-by: Juergen Gross 
Reviewed-by: Juergen Gross 
---
v2: Add ASSERT(!in_irq()).

--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -219,10 +219,22 @@ int pv_domain_initialise(struct domain *
 return rc;
 }
 
-static void _toggle_guest_pt(struct vcpu *v)
+static void _toggle_guest_pt(struct vcpu *v, bool force_cr3)
 {
+ASSERT(!in_irq());
+
 v->arch.flags ^= TF_kernel_mode;
 update_cr3(v);
+
+/*
+ * There's no need to load CR3 here when it is going to be loaded on the
+ * way out to guest mode again anyway, and when the page tables we're
+ * currently on are the kernel ones (whereas when switching to kernel
+ * mode we need to be able to write a bounce frame onto the kernel stack).
+ */
+if ( !force_cr3 && !(v->arch.flags & TF_kernel_mode) )
+return;
+
 /* Don't flush user global mappings from the TLB. Don't tick TLB clock. */
 asm volatile ( "mov %0, %%cr3" : : "r" (v->arch.cr3) : "memory" );
 
@@ -252,13 +264,13 @@ void toggle_guest_mode(struct vcpu *v)
 }
 asm volatile ( "swapgs" );
 
-_toggle_guest_pt(v);
+_toggle_guest_pt(v, cpu_has_no_xpti);
 }
 
 void toggle_guest_pt(struct vcpu *v)
 {
 if ( !is_pv_32bit_vcpu(v) )
-_toggle_guest_pt(v);
+_toggle_guest_pt(v, true);
 }
 
 /*




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