Re: [Xen-devel] [PATCH v6.5 21/26] x86/entry: Use MSR_SPEC_CTRL at each entry/exit point

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 01:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> Set or clear IBRS in Xen context, and appropriate guest values in guest
> context.  See the documentation in asm-x86/spec_ctrl_asm.h for details.

I think this is misleading - there is no setting or clearing in Xen context
here, as the controlling features (X86_FEATURE_XEN_IBRS_{SET,CLEAR})
get set only in the next patch.

> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -7,6 +7,7 @@
>  #include <asm/asm-offsets.h>
>  #endif
>  #include <asm/bug.h>
> +#include <asm/page.h>
>  #include <asm/processor.h>
>  #include <asm/percpu.h>
>  #include <xen/stringify.h>
> @@ -344,4 +345,6 @@ static always_inline void stac(void)
>  #define REX64_PREFIX "rex64/"
>  #endif
>  
> +#include <asm/spec_ctrl_asm.h>

Why do all consumers of asm_defns.h need to also see the
definitions that other header holds? If there are include ordering
issues, can't interested .c and/or .S files include that other
header first? In fact I have a patch pending (in a yet to be
finished series) which does that for processor.h: It shouldn't
really be included by this header, as nothing in here needs
anything out of there (the things needed live in x86-defns.h).

> --- a/xen/include/asm-x86/nops.h
> +++ b/xen/include/asm-x86/nops.h
> @@ -50,7 +50,7 @@
>  #define P6_NOP9 0x66,0x0f,0x1f,0x84,0x00,0,0,0,0
>  
>  #ifdef __ASSEMBLY__
> -#define _ASM_MK_NOP(x) .byte x
> +#define _ASM_MK_NOP(x) .byte x;

Imo the semicolon doesn't belong here, but ...

> @@ -65,6 +65,12 @@
>  #define ASM_NOP8 _ASM_MK_NOP(P6_NOP8)
>  #define ASM_NOP9 _ASM_MK_NOP(P6_NOP9)
>  
> +#define ASM_NOP22 ASM_NOP8 ASM_NOP8 ASM_NOP6
> +#define ASM_NOP26 ASM_NOP8 ASM_NOP8 ASM_NOP8 ASM_NOP2
> +#define ASM_NOP32 ASM_NOP8 ASM_NOP8 ASM_NOP8 ASM_NOP8
> +#define ASM_NOP33 ASM_NOP8 ASM_NOP8 ASM_NOP8 ASM_NOP7 ASM_NOP2
> +#define ASM_NOP39 ASM_NOP8 ASM_NOP8 ASM_NOP8 ASM_NOP8 ASM_NOP7

... here (between the elements, matching e.g. ASM_AC() and
CR4_PV32_RESTORE).

If you really want to keep introducing these (instead of the - imo -
much more clean use of suitable .skip in the macros, as is being
done in the 32-bit PV SMEP/SMAP handling), please at least use
ASM_NOP9 here as far as possible.

> +.macro DO_SPEC_CTRL_ENTRY_FROM_VMEXIT ibrs_val:req
> +/*
> + * Requires %rbx=current, %rsp=regs/cpuinfo
> + * Clobbers %rax, %rcx, %rdx
> + *
> + * The common case is that a guest has direct access to MSR_SPEC_CTRL, at
> + * which point we need to save the guest value before setting IBRS for Xen.
> + * Unilaterally saving the guest value is shorter and faster than checking.
> + */
> +    mov $MSR_SPEC_CTRL, %ecx
> +    rdmsr
> +
> +    /* Stash the value from hardware. */
> +    mov VCPU_arch_msr(%rbx), %rdx
> +    mov %al, VCPUMSR_spec_ctrl_host(%rdx)
> +    xor %edx, %edx
> +
> +    /* Clear SPEC_CTRL shadowing *before* loading Xen's value. */
> +    movb %dl, CPUINFO_use_shadow_spec_ctrl(%rsp)
> +
> +    /* Load Xen's indented value. */

"intended" (also another time below)?

> +.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
> +        cmpl $__HYPERVISOR_CS, UREGS_cs(%rsp)

Perhaps better "cmpw", and if you're afraid of the length changing
prefix decode stall, then use an intermediate register.

Jan


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