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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/01/18 15:14, Jan Beulich wrote:
>>>> 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?

The problem is that execution needs to jmp to after the wrmsr, rather
than before the testb $3, UREGS_cs(%rsp), which is where UNLIKELY_END()
would currently leave it.

We do not have suitable UNLIKELY() infrastructure to express a non-empty
likely basic block.

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

Thanks,

~Andrew

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