* [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage
@ 2018-03-27 4:52 Zhenzhong Duan
2018-03-27 8:52 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Zhenzhong Duan @ 2018-03-27 4:52 UTC (permalink / raw)
To: Xen-Devel; +Cc: Andrew Cooper3, boris.ostrovsky, srinivas.eeda, JBeulich
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 <zhenzhong.duan@oracle.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage
2018-03-27 4:52 [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage Zhenzhong Duan
@ 2018-03-27 8:52 ` Jan Beulich
2018-03-27 11:03 ` Zhenzhong Duan
2018-04-02 9:40 ` Zhenzhong Duan
0 siblings, 2 replies; 5+ messages in thread
From: Jan Beulich @ 2018-03-27 8:52 UTC (permalink / raw)
To: Zhenzhong Duan; +Cc: srinivas.eeda, Andrew Cooper3, boris.ostrovsky, Xen-Devel
>>> On 27.03.18 at 06:52, <zhenzhong.duan@oracle.com> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage
2018-03-27 8:52 ` Jan Beulich
@ 2018-03-27 11:03 ` Zhenzhong Duan
2018-03-27 11:43 ` Jan Beulich
2018-04-02 9:40 ` Zhenzhong Duan
1 sibling, 1 reply; 5+ messages in thread
From: Zhenzhong Duan @ 2018-03-27 11:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: srinivas.eeda, Andrew Cooper3, boris.ostrovsky, Xen-Devel
On 2018/3/27 16:52, Jan Beulich wrote:
>>>> On 27.03.18 at 06:52, <zhenzhong.duan@oracle.com> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage
2018-03-27 11:03 ` Zhenzhong Duan
@ 2018-03-27 11:43 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2018-03-27 11:43 UTC (permalink / raw)
To: zhenzhong.duan; +Cc: srinivas.eeda, Andrew Cooper3, boris.ostrovsky, Xen-Devel
>>> On 27.03.18 at 13:03, <zhenzhong.duan@oracle.com> wrote:
> On 2018/3/27 16:52, Jan Beulich wrote:
>>>>> On 27.03.18 at 06:52, <zhenzhong.duan@oracle.com> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage
2018-03-27 8:52 ` Jan Beulich
2018-03-27 11:03 ` Zhenzhong Duan
@ 2018-04-02 9:40 ` Zhenzhong Duan
1 sibling, 0 replies; 5+ messages in thread
From: Zhenzhong Duan @ 2018-04-02 9:40 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper3; +Cc: srinivas.eeda, boris.ostrovsky, Xen-Devel
On 2018/3/27 16:52, Jan Beulich wrote:
>>>> On 27.03.18 at 06:52, <zhenzhong.duan@oracle.com> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-02 9:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-27 4:52 [PATCH v2] x86/boot: Disable IBRS in intr/nmi exit path at bootup stage Zhenzhong Duan
2018-03-27 8:52 ` Jan Beulich
2018-03-27 11:03 ` Zhenzhong Duan
2018-03-27 11:43 ` Jan Beulich
2018-04-02 9:40 ` Zhenzhong Duan
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.