Re: [Xen-devel] [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/01/18 12:09, Jan Beulich wrote:
>>>> On 12.01.18 at 19:01, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -668,6 +668,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>      set_processor_id(0);
>>      set_current(INVALID_VCPU); /* debug sanity. */
>>      idle_vcpu[0] = current;
>> +    init_shadow_spec_ctrl_state();
> Considering the strict need to fill struct cpu_info fields early on (also
> in my SP3 band-aid) I wonder whether we wouldn't be better off
> uniformly memset()-ing the whole structure first thing here and in
> start_secondary(). But this is just a remark, not necessarily
> something to be done in this patch.

One thing I didn't quite get to in my KAISER series actually switched to
having BSP fill in the entire top-of-stack block for APs when the stack
was allocated.

I think that would be a good change in a future patch.

>
>> @@ -586,6 +611,10 @@ ENTRY(double_fault)
>>          movl  $TRAP_double_fault,4(%rsp)
>>          /* Set AC to reduce chance of further SMAP faults */
>>          SAVE_ALL STAC
>> +
>> +        SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
>> +        /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> Is it actually useful to do _anything_ in the double fault handler?

Typically no, but then again we hope never to execute this code.

OTOH, we would need to do this if we ever get around to doing espfix64.

>
>> +.macro DO_SPEC_CTRL_ENTRY maybexen:req ibrs_val:req
>> +/*
>> + * Requires %rsp=regs (also cpuinfo if !maybexen)
>> + * Clobbers %rax, %rcx, %rdx
>> + *
>> + * PV guests can't update MSR_SPEC_CTRL behind Xen's back, so no need to 
>> read
>> + * it back.  Entries from guest context need to clear SPEC_CTRL shadowing,
>> + * while entries from Xen must leave shadowing in its current state.
>> + */
>> +    mov $MSR_SPEC_CTRL, %ecx
>> +
>> +    .if \maybexen
>> +        cmpw $__HYPERVISOR_CS, UREGS_cs(%rsp)
> I see you've changed to cmpw here. To eliminate your length-
> changing-prefix concern, how about using testb instead to
> just evaluate RPL or the selector, as I'm doing in the SP3
> band-aid?

That would be better still.

~Andrew

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