Re: [Xen-devel] [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage

[Zhenzhong Duan]

On 2018/3/27 16:52, Jan Beulich wrote:

On 27.03.18 at 06:52,  wrote:

After reset, IBRS is disabled by processor, but a coming intr/nmi leave IBRS
enabled after their exit. It's not necessory for bootup code to run in low
performance with IBRS enabled.

On ORACLE X6-2(500GB/88 cpus, dom0 11GB/20 vcpus), we observed an 200s+ delay
in construct_dom0.

By initializing use_shadow_spec_ctrl with the result of (system_state <
SYS_STATE_active), IBRS is disabled in intr/nmi exit path at bootup stage.
Then delay in construct_dom0 is ~50s.

When hot-onlining a CPU, we initialize IBRS early and set use_shadow_spec_ctrl
to false to avoid Branch Target Injection from sibling threads.

v2: Use (system_state < SYS_STATE_active) to initialize use_shadow_spec_ctrl
instead of literal 1 per Jan.


Please place revision information below the first --- marker.


--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -32,8 +32,22 @@ extern uint8_t default_bti_ist_info;
  static inline void init_shadow_spec_ctrl_state(void)
  {
  struct cpu_info *info = get_cpu_info();
+uint32_t val = SPEC_CTRL_IBRS;


Why do you need this variable?


+/* Initialize IA32_SPEC_CTRL MSR for hotplugging cpu early */
+if ( system_state >= SYS_STATE_active )
+asm volatile (ALTERNATIVE(ASM_NOP3, "wrmsr", X86_FEATURE_XEN_IBRS_SET)
+  :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory");


I can see the point of doing this, but the title of the patch doesn't
cover it (I think this has been missing independent of your interrupt/
NMI paths consideration).

Further INIT# (unlike RESET#) doesn't clear the register, so you
may want/need to also clear the register in the
X86_FEATURE_XEN_IBRS_CLEAR case.

Also you don't need ASM_NOP3 here after 4008c71d7a ("x86/alt:
Support for automatic padding calculations").

Additionally I think it would be better to keep low and high parts
of the value next to each other in the constraints, rather than
putting the MSR index in the middle.


-info->shadow_spec_ctrl = info->use_shadow_spec_ctrl = 0;
+info->shadow_spec_ctrl = 0;
+/*
+ * We want to make sure we clear IBRS in interrupt exit path
+ * (DO_SPEC_CTRL_EXIT_TO_XEN) while dom0 is still booting to
+ * avoid unnecessary performance impact. As soon as dom0 has
+ * booted use_shadow_spec_ctrl will be cleared, for example,
+ * in idle routine.
+ */
+info->use_shadow_spec_ctrl = system_state < SYS_STATE_active;


I think the code overall would be more readable if you had just a
single condition (in if/else form).

And then there is the question of whether to use < / >= or
!= / == : In the resume case, not guest vCPU-s are active (yet),
so perhaps the latter would be better.

In any event please give Andrew a chance to reply before you
send another version, as he may have a different opinion and/or
other valuable input.


Hi Andrew,

May I have your comments? If there is no further suggestions from you, 
I'll prepare to make the new version.


Thanks
Zhenzhong

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

Re: [Xen-devel] [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage

[Jan Beulich]

>>> On 27.03.18 at 13:03,  wrote:
> On 2018/3/27 16:52, Jan Beulich wrote:
> On 27.03.18 at 06:52,  wrote:
>>> --- a/xen/include/asm-x86/spec_ctrl.h
>>> +++ b/xen/include/asm-x86/spec_ctrl.h
>>> @@ -32,8 +32,22 @@ extern uint8_t default_bti_ist_info;
>>>   static inline void init_shadow_spec_ctrl_state(void)
>>>   {
>>>   struct cpu_info *info = get_cpu_info();
>>> +uint32_t val = SPEC_CTRL_IBRS;
>> 
>> Why do you need this variable?
> This is a copy of the same code in spec_ctrl_enter_idle() and 
> spec_ctrl_exit_idle().

In the latter the variable looks pretty pointless too, but I assume
Andrew has put it there to be as symmetric as possible with the
former, where the variable is used twice.

>>> +/* Initialize IA32_SPEC_CTRL MSR for hotplugging cpu early */
>>> +if ( system_state >= SYS_STATE_active )
>>> +asm volatile (ALTERNATIVE(ASM_NOP3, "wrmsr", 
>>> X86_FEATURE_XEN_IBRS_SET)
>>> +  :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : 
>>> "memory");
>> 
>> I can see the point of doing this, but the title of the patch doesn't
>> cover it (I think this has been missing independent of your interrupt/
>> NMI paths consideration).
> Could I make a seperate patch for above four lines?
> Looks hard to describe all these in one title.

Well, addressing separate issues in separate patches would be
better anyway.

>> Further INIT# (unlike RESET#) doesn't clear the register, so you
>> may want/need to also clear the register in the
>> X86_FEATURE_XEN_IBRS_CLEAR case.
> I did consider using ALTERNATIVE_2 here, so dumped the IA32_SPEC_CTRL 
> msr's value just after the entry of init_shadow_spec_ctrl_state() for a 
> hot-onlining CPU and it's zero, this is the X86_FEATURE_XEN_IBRS_SET case.
> In X86_FEATURE_XEN_IBRS_CLEAR case, will IBRS ever be set?

Remember - we're possibly dealing with post-INIT# state here.
What has run on the CPU before the INIT# is simply unknown.

>> Additionally I think it would be better to keep low and high parts
>> of the value next to each other in the constraints, rather than
>> putting the MSR index in the middle.
> Same copy from spec_ctrl_enter_idle() and spec_ctrl_exit_idle(), maybe 
> it's better to have a seperate patch to fix all of them, including the 
> variable val.

Perhaps, yes. I didn't notice this ordering aspect when reviewing
the earlier patches.

Jan


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

Re: [Xen-devel] [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage

[Zhenzhong Duan]

On 2018/3/27 16:52, Jan Beulich wrote:

On 27.03.18 at 06:52,  wrote:

After reset, IBRS is disabled by processor, but a coming intr/nmi leave IBRS
enabled after their exit. It's not necessory for bootup code to run in low
performance with IBRS enabled.

On ORACLE X6-2(500GB/88 cpus, dom0 11GB/20 vcpus), we observed an 200s+ delay
in construct_dom0.

By initializing use_shadow_spec_ctrl with the result of (system_state <
SYS_STATE_active), IBRS is disabled in intr/nmi exit path at bootup stage.
Then delay in construct_dom0 is ~50s.

When hot-onlining a CPU, we initialize IBRS early and set use_shadow_spec_ctrl
to false to avoid Branch Target Injection from sibling threads.

v2: Use (system_state < SYS_STATE_active) to initialize use_shadow_spec_ctrl
instead of literal 1 per Jan.


Please place revision information below the first --- marker.

Ok




--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -32,8 +32,22 @@ extern uint8_t default_bti_ist_info;
  static inline void init_shadow_spec_ctrl_state(void)
  {
  struct cpu_info *info = get_cpu_info();
+uint32_t val = SPEC_CTRL_IBRS;


Why do you need this variable?
This is a copy of the same code in spec_ctrl_enter_idle() and 
spec_ctrl_exit_idle().



+/* Initialize IA32_SPEC_CTRL MSR for hotplugging cpu early */
+if ( system_state >= SYS_STATE_active )
+asm volatile (ALTERNATIVE(ASM_NOP3, "wrmsr", X86_FEATURE_XEN_IBRS_SET)
+  :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory");


I can see the point of doing this, but the title of the patch doesn't
cover it (I think this has been missing independent of your interrupt/
NMI paths consideration).

Could I make a seperate patch for above four lines?
Looks hard to describe all these in one title.


Further INIT# (unlike RESET#) doesn't clear the register, so you
may want/need to also clear the register in the
X86_FEATURE_XEN_IBRS_CLEAR case.
I did consider using ALTERNATIVE_2 here, so dumped the IA32_SPEC_CTRL 
msr's value just after the entry of init_shadow_spec_ctrl_state() for a 
hot-onlining CPU and it's zero, this is the X86_FEATURE_XEN_IBRS_SET case.

In X86_FEATURE_XEN_IBRS_CLEAR case, will IBRS ever be set?


Also you don't need ASM_NOP3 here after 4008c71d7a ("x86/alt:
Support for automatic padding calculations").

Yes


Additionally I think it would be better to keep low and high parts
of the value next to each other in the constraints, rather than
putting the MSR index in the middle.
Same copy from spec_ctrl_enter_idle() and spec_ctrl_exit_idle(), maybe 
it's better to have a seperate patch to fix all of them, including the 
variable val.



-info->shadow_spec_ctrl = info->use_shadow_spec_ctrl = 0;
+info->shadow_spec_ctrl = 0;
+/*
+ * We want to make sure we clear IBRS in interrupt exit path
+ * (DO_SPEC_CTRL_EXIT_TO_XEN) while dom0 is still booting to
+ * avoid unnecessary performance impact. As soon as dom0 has
+ * booted use_shadow_spec_ctrl will be cleared, for example,
+ * in idle routine.
+ */
+info->use_shadow_spec_ctrl = system_state < SYS_STATE_active;


I think the code overall would be more readable if you had just a
single condition (in if/else form).

Ok


And then there is the question of whether to use < / >= or
!= / == : In the resume case, not guest vCPU-s are active (yet),
so perhaps the latter would be better.

Ok


In any event please give Andrew a chance to reply before you
send another version, as he may have a different opinion and/or
other valuable input.

Ok, I'll wait Andrew's reply before go ahead.

Thanks
Zhenzhong

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

Re: [Xen-devel] [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage

[Jan Beulich]

>>> On 27.03.18 at 06:52,  wrote:
> After reset, IBRS is disabled by processor, but a coming intr/nmi leave IBRS
> enabled after their exit. It's not necessory for bootup code to run in low
> performance with IBRS enabled.
> 
> On ORACLE X6-2(500GB/88 cpus, dom0 11GB/20 vcpus), we observed an 200s+ delay
> in construct_dom0.
> 
> By initializing use_shadow_spec_ctrl with the result of (system_state <
> SYS_STATE_active), IBRS is disabled in intr/nmi exit path at bootup stage.
> Then delay in construct_dom0 is ~50s.
> 
> When hot-onlining a CPU, we initialize IBRS early and set use_shadow_spec_ctrl
> to false to avoid Branch Target Injection from sibling threads.
> 
> v2: Use (system_state < SYS_STATE_active) to initialize use_shadow_spec_ctrl
> instead of literal 1 per Jan.

Please place revision information below the first --- marker.

> --- a/xen/include/asm-x86/spec_ctrl.h
> +++ b/xen/include/asm-x86/spec_ctrl.h
> @@ -32,8 +32,22 @@ extern uint8_t default_bti_ist_info;
>  static inline void init_shadow_spec_ctrl_state(void)
>  {
>  struct cpu_info *info = get_cpu_info();
> +uint32_t val = SPEC_CTRL_IBRS;

Why do you need this variable?

> +/* Initialize IA32_SPEC_CTRL MSR for hotplugging cpu early */
> +if ( system_state >= SYS_STATE_active )
> +asm volatile (ALTERNATIVE(ASM_NOP3, "wrmsr", 
> X86_FEATURE_XEN_IBRS_SET)
> +  :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory");

I can see the point of doing this, but the title of the patch doesn't
cover it (I think this has been missing independent of your interrupt/
NMI paths consideration).

Further INIT# (unlike RESET#) doesn't clear the register, so you
may want/need to also clear the register in the
X86_FEATURE_XEN_IBRS_CLEAR case.

Also you don't need ASM_NOP3 here after 4008c71d7a ("x86/alt:
Support for automatic padding calculations").

Additionally I think it would be better to keep low and high parts
of the value next to each other in the constraints, rather than
putting the MSR index in the middle.

> -info->shadow_spec_ctrl = info->use_shadow_spec_ctrl = 0;
> +info->shadow_spec_ctrl = 0;
> +/*
> + * We want to make sure we clear IBRS in interrupt exit path
> + * (DO_SPEC_CTRL_EXIT_TO_XEN) while dom0 is still booting to
> + * avoid unnecessary performance impact. As soon as dom0 has
> + * booted use_shadow_spec_ctrl will be cleared, for example,
> + * in idle routine.
> + */
> +info->use_shadow_spec_ctrl = system_state < SYS_STATE_active;

I think the code overall would be more readable if you had just a
single condition (in if/else form).

And then there is the question of whether to use < / >= or
!= / == : In the resume case, not guest vCPU-s are active (yet),
so perhaps the latter would be better.

In any event please give Andrew a chance to reply before you
send another version, as he may have a different opinion and/or
other valuable input.

Jan


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

[Xen-devel] [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage

[Zhenzhong Duan]

After reset, IBRS is disabled by processor, but a coming intr/nmi leave IBRS
enabled after their exit. It's not necessory for bootup code to run in low
performance with IBRS enabled.

On ORACLE X6-2(500GB/88 cpus, dom0 11GB/20 vcpus), we observed an 200s+ delay
in construct_dom0.

By initializing use_shadow_spec_ctrl with the result of (system_state <
SYS_STATE_active), IBRS is disabled in intr/nmi exit path at bootup stage.
Then delay in construct_dom0 is ~50s.

When hot-onlining a CPU, we initialize IBRS early and set use_shadow_spec_ctrl
to false to avoid Branch Target Injection from sibling threads.

v2: Use (system_state < SYS_STATE_active) to initialize use_shadow_spec_ctrl
instead of literal 1 per Jan.

Signed-off-by: Zhenzhong Duan 
---
 xen/include/asm-x86/spec_ctrl.h |   16 +++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 5ab4ff3..1672317 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -32,8 +32,22 @@ extern uint8_t default_bti_ist_info;
 static inline void init_shadow_spec_ctrl_state(void)
 {
 struct cpu_info *info = get_cpu_info();
+uint32_t val = SPEC_CTRL_IBRS;
+
+/* Initialize IA32_SPEC_CTRL MSR for hotplugging cpu early */
+if ( system_state >= SYS_STATE_active )
+asm volatile (ALTERNATIVE(ASM_NOP3, "wrmsr", X86_FEATURE_XEN_IBRS_SET)
+  :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory");
 
-info->shadow_spec_ctrl = info->use_shadow_spec_ctrl = 0;
+info->shadow_spec_ctrl = 0;
+/*
+ * We want to make sure we clear IBRS in interrupt exit path
+ * (DO_SPEC_CTRL_EXIT_TO_XEN) while dom0 is still booting to
+ * avoid unnecessary performance impact. As soon as dom0 has
+ * booted use_shadow_spec_ctrl will be cleared, for example,
+ * in idle routine.
+ */
+info->use_shadow_spec_ctrl = system_state < SYS_STATE_active;
 info->bti_ist_info = default_bti_ist_info;
 }
 
-- 
1.7.3

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