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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.18 at 13:03, <zhenzhong.duan@xxxxxxxxxx> wrote:
> On 2018/3/27 16:52, Jan Beulich wrote:
>>>>> On 27.03.18 at 06:52, <zhenzhong.duan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel