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

[Zhenzhong Duan <zhenzhong.duan@xxxxxxxxxx>]

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?
>
> > +    /* 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel