Re: [Xen-devel] [PATCH v10 07/11] x86/entry: Avoid using alternatives in NMI/#MC paths

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.01.18 at 16:04, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 25/01/18 13:43, Jan Beulich wrote:
>>>>> On 24.01.18 at 14:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> @@ -256,6 +261,69 @@
>>>          DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_SET,           \
>>>          DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_CLEAR
>>>  
>>> +/* TODO: Drop these when the alternatives infrastructure is NMI/#MC safe. 
>>> */
>>> +.macro SPEC_CTRL_ENTRY_FROM_INTR_IST
>>> +/*
>>> + * Requires %rsp=regs, %r14=stack_end
>>> + * Clobbers %rax, %rcx, %rdx
>>> + *
>>> + * This is logical merge of DO_OVERWRITE_RSB and DO_SPEC_CTRL_ENTRY
>>> + * maybexen=1, but with conditionals rather than alternatives.
>>> + */
>>> +    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %eax
>>> +
>>> +    testb $BTI_IST_RSB, %al
>>> +    jz .L\@_skip_rsb
>>> +
>>> +    DO_OVERWRITE_RSB
>>> +
>>> +.L\@_skip_rsb:
>>> +
>>> +    testb $BTI_IST_WRMSR, %al
>>> +    jz .L\@_skip_wrmsr
>>> +
>>> +    testb $3, UREGS_cs(%rsp)
>>> +    jz .L\@_entry_from_xen
>>> +
>>> +    movb $0, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14)
>>> +
>>> +.L\@_entry_from_xen:
>>> +    /*
>>> +     * Load Xen's intended value.  SPEC_CTRL_IBRS vs 0 is encoded in the
>>> +     * bottom bit of bti_ist_info, via a deliberate alias with 
>>> BTI_IST_IBRS.
>>> +     */
>>> +    mov $MSR_SPEC_CTRL, %ecx
>>> +    and $BTI_IST_IBRS, %eax
>>> +    xor %edx, %edx
>>> +    wrmsr
>>> +
>>> +    /* Opencoded UNLIKELY_START() with no condition. */
>>> +.Ldispatch.\@_serialise:
>>> +    .subsection 1
>> How about adding .ifnb to UNLIKELY_START(), instead of open coding?
>> Or wait - why can't you use it as-is above (instead of the
>> "jz .L\@_skip_wrmsr")?
> 
> Because then the end label is wrong.  One way of another, we either need
> to opencode something, or implement UNLIKELY_ELSE() which is actually
> the likely case, and this is a substantially larger can of worms.

I'm willing to trust you for now (until I get around to play with this
myself), but I don't follow: In an UNLIKELY_START() /
UNLIKELY_END() pair, how could the labels be wrong if both specify
the same tag?

>> And then, having reached the end of the patch - where is the
>> new struct cpu_info field written? Or is this again happening only in
>> later patches, with the one here just setting the stage? If so,
>> shouldn't you at least zero the field in init_shadow_spec_ctrl_state()?
> 
> Hmm - I should set 0 in init_shadow_spec_ctrl_state().
> 
> All the other calculation logic needs to be deferred to the subsequent
> patch for bisectiblaty.

Of course. With this initialization fixed and the connection made
between BTI_IST_IBRS and the MSR bit at the definition point of
the former,
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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