[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 2018/3/27 16:52, Jan Beulich wrote:
On 27.03.18 at 06:52, <zhenzhong.duan@xxxxxxxxxx> 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?
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
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").

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).

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.
Ok, I'll wait Andrew's reply before go ahead.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.