Re: [Xen-devel] [PATCH v10 05/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/01/18 13:08, Jan Beulich wrote:
>>>> On 24.01.18 at 14:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>> We need to be able to either set or clear IBRS in Xen context, as well as
>> restore appropriate guest values in guest context.  See the documentation in
>> asm-x86/spec_ctrl_asm.h for details.
>>
>> With the contemporary microcode, writes to %cr3 are slower when 
>> SPEC_CTRL.IBRS
>> is set.  Therefore, the positioning of SPEC_CTRL_{ENTRY/EXIT}* is important.
>>
>> Ideally, the IBRS_SET/IBRS_CLEAR hunks might be positioned either side of the
>> %cr3 change, but that is rather more complicated to arrange, and could still
>> result in a guest controlled value in SPEC_CTRL during the %cr3 change,
>> negating the saving if the guest chose to have IBRS set.
>>
>> Therefore, we optimise for the pre-Skylake case (being far more common in the
>> field than Skylake and later, at the moment), where we have a Xen-preferred
>> value of IBRS clear when switching %cr3.
>>
>> There is a semi-unrelated bugfix, where various asm_defn.h macros have a
>> hidden dependency on PAGE_SIZE, which results in an assembler error if used 
>> in
>> a .macro definition.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> with a spelling observation and a question / perhaps implied
> suggestion:
>
>> @@ -99,6 +109,15 @@ UNLIKELY_END(realmode)
>>  .Lvmx_vmentry_fail:
>>          sti
>>          SAVE_ALL
>> +
>> +        /*
>> +         * PV variant needed here as no guest code has executed (so
>> +         * MSR_SPEC_CTRL can't have changed value), and NMIs/MCEs are liable
>> +         * to hit (in which case the HVM varient might corrupt things).
> variant

Fixed.

>
>> +.macro DO_SPEC_CTRL_ENTRY maybexen:req ibrs_val:req
>> +/*
>> + * Requires %rsp=regs (also cpuinfo if !maybexen)
>> + * Requires %r14=stack_end (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
>> +        testb $3, UREGS_cs(%rsp)
>> +        jz .L\@_entry_from_xen
>> +    .endif
>> +
>> +    /*
>> +     * Clear SPEC_CTRL shadowing *before* loading Xen's value.  If entering
>> +     * from a possibly-xen context, %rsp doesn't necessarily alias the 
>> cpuinfo
>> +     * block so calculate the position directly.
>> +     */
>> +    .if \maybexen
>> +        movb $0, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14)
>> +    .else
>> +        movb $0, CPUINFO_use_shadow_spec_ctrl(%rsp)
>> +    .endif
>> +
>> +.L\@_entry_from_xen:
>> +    /* Load Xen's intended value. */
>> +    mov $\ibrs_val, %eax
>> +    xor %edx, %edx
>> +    wrmsr
>> +.endm
> Did you consider avoiding the conditional branch here altogether,
> by doing something like
>
>     .if \maybexen
>         testb $3, UREGS_cs(%rsp)
>         setz %al
>         neg %al
>         and %al, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14)
>     .else
>
> ?

That is quite subtle, although if you are looking to optimise further,
use_shadow_spec_ctrl being a bool means you can drop the neg.  An and
with the result of setz is good enough not to change the boolean value.

> It may also be worthwhile again to pull up the zeroing of %edx,
> using %dl instead of $0 in the movb (and maybe then also
> similarly in DO_SPEC_CTRL_EXIT_TO_XEN, but there I'm less
> certain it couldn't have a negative effect).

What negative effects are you worried about?  These macros are self
contained.

~Andrew

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