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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

?

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

Jan


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