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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/01/18 09:23, Jan Beulich wrote:
>>>> 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?

Functionally, not, but I'm not convinced it would make anything better
either.

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

This ifdef is strictly required.  asm ("__x86.indirect_thunk.rcx");
doesn't exist when building with an incapable toolchain.

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

Again, these are strictly required, because of ind_thunk_rcx[].

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

Ah yes - I will fold that in.

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

Degrades.

~Andrew

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