Re: [Xen-devel] [PATCH v6.5 11/26] x86: Support indirect thunks from assembly code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 01:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -153,8 +153,28 @@ trampoline_protmode_entry:
>          .code64
>  start64:
>          /* Jump to high mappings. */
> -        movabs  $__high_start,%rax
> -        jmpq    *%rax
> +        movabs  $__high_start, %rdi
> +
> +#ifdef CONFIG_INDIRECT_THUNK
> +        /*
> +         * If booting virtualised, or hot-onlining a CPU, sibling threads can
> +         * attempt Branch Target Injection against this jmp.
> +         *
> +         * We've got no usable stack so can't use a RETPOLINE thunk, and are
> +         * further than +- 2G from the high mappings so couldn't use 
> JUMP_THUNK
> +         * even if was a non-RETPOLINE thunk.  Futhermore, an LFENCE isn't
> +         * necesserily safe to use at this point.
> +         *
> +         * As this isn't a hotpath, use a fully serialising event to reduce
> +         * the speculation window as much as possible.  %ebx needs preserving
> +         * for __high_start.
> +         */
> +        mov     %ebx, %esi
> +        cpuid
> +        mov     %esi, %ebx
> +#endif
> +
> +        jmpq    *%rdi

Would there be anything wrong with omitting the #ifdef, slightly
improving readability?

> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -73,37 +73,58 @@ void (*pv_post_outb_hook)(unsigned int port, u8 value);
>  
>  typedef void io_emul_stub_t(struct cpu_user_regs *);
>  
> +#ifdef CONFIG_INDIRECT_THUNK
> +extern char ind_thunk_rcx[] asm ("__x86.indirect_thunk.rcx");
> +#endif

Again I don't see the value of the #ifdef.

>  static io_emul_stub_t *io_emul_stub_setup(struct priv_op_ctxt *ctxt, u8 
> opcode,
>                                            unsigned int port, unsigned int 
> bytes)
>  {
> +    struct stubs *this_stubs = &this_cpu(stubs);
> +    unsigned long stub_va = this_stubs->addr + STUB_BUF_SIZE / 2;
> +
>      if ( !ctxt->io_emul_stub )
> -        ctxt->io_emul_stub = map_domain_page(_mfn(this_cpu(stubs.mfn))) +
> -                                             (this_cpu(stubs.addr) &
> -                                              ~PAGE_MASK) +
> -                                             STUB_BUF_SIZE / 2;
> +        ctxt->io_emul_stub =
> +            map_domain_page(_mfn(this_stubs->mfn)) + (stub_va & ~PAGE_MASK);
>  
>      /* movq $host_to_guest_gpr_switch,%rcx */
>      ctxt->io_emul_stub[0] = 0x48;
>      ctxt->io_emul_stub[1] = 0xb9;
>      *(void **)&ctxt->io_emul_stub[2] = (void *)host_to_guest_gpr_switch;
> +
> +#ifdef CONFIG_INDIRECT_THUNK
> +    /* callq __x86.indirect_thunk.rcx */
> +    ctxt->io_emul_stub[10] = 0xe8;
> +    *(int32_t *)&ctxt->io_emul_stub[11] =
> +        _p(ind_thunk_rcx) - _p(stub_va + 11 + 4);

Why two (hidden) casts when one (clearly visible one) would do:

    *(int32_t *)&ctxt->io_emul_stub[11] =
        (unsigned long)ind_thunk_rcx - (stub_va + 11 + 4);

?

> +#else
>      /* callq *%rcx */
>      ctxt->io_emul_stub[10] = 0xff;
>      ctxt->io_emul_stub[11] = 0xd1;
> +
> +    /*
> +     * 3 bytes of P6_NOPS.
> +     * TODO: untangle ideal_nops from init/livepatch Kconfig options.
> +     */
> +    memcpy(&ctxt->io_emul_stub[12], "\x0f\x1f\x00", 3);
> +#endif

Same here - rather than making everything more complicated to
read/understand, why don't we avoid conditionals in places where
performance is of no concern. In the end it may well be that we
wouldn't need CONFIG_INDIRECT_THUNK at all, unless it became
a user-selectable option (as suggested in reply to patch 9).

> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -13,6 +13,14 @@
>  #include <asm/cpufeature.h>
>  #include <asm/alternative.h>
>  
> +#ifdef __ASSEMBLY__
> +# include <asm/indirect_thunk_asm.h>
> +#else
> +asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
> +      __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) );
> +asm ( "\t.include \"asm/indirect_thunk_asm.h\"" );
> +#endif

For this to work with all compilers, aren't you still missing the
addition of -Wa,-I$(BASEDIR)/include on top of the other
compiler option additions done in patch 9?

> --- /dev/null
> +++ b/xen/include/asm-x86/indirect_thunk_asm.h
> @@ -0,0 +1,41 @@
> +/*
> + * Warning!  This file is included at an assembler level for .c files, 
> causing
> + * usual #ifdef'ary to turn into comments.
> + */
> +
> +.macro IND_THUNK insn:req arg:req
> +/*
> + * Create an indirect branch.  insn is one of call/jmp, arg is a single
> + * register.
> + *
> + * With no compiler support, this degrated into a plain indirect call/jmp.

"degrades" or "degenerates" or yet something else?

> + * With compiler support, dispatch to the correct __x86.indirect_thunk.*
> + */
> +    .if CONFIG_INDIRECT_THUNK == 1

If we really want/need to keep this config option, why not without
the "== 1"?

> +        $done = 0
> +        .irp reg, rax, rbx, rcx, rdx, rsi, rdi, rbp, r8, r9, r10, r11, r12, 
> r13, r14, r15

Same cosmetic remark as earlier regarding the ordering and the
possible omission of the r-s here.

Jan

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