Re: [Xen-devel] [PATCH v8 02/17] x86: Support indirect thunks from assembly code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.01.18 at 19:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> Introduce INDIRECT_CALL and INDIRECT_JMP which either degrade to a normal
> indirect branch, or dispatch to the __x86_indirect_thunk_* symbols.
> 
> Update all the manual indirect branches in to use the new thunks.  The
> indirect branches in the early boot and kexec path are left intact as we 
> can't
> use the compiled-in thunks at those points.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with a couple of cosmetic remarks:

> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -73,46 +73,63 @@ void (*pv_post_outb_hook)(unsigned int port, u8 value);
>  
>  typedef void io_emul_stub_t(struct cpu_user_regs *);
>  
> +void __x86_indirect_thunk_rcx(void);
> +
>  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;
>      bool use_quirk_stub = false;
>  
>      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] =
> +        (unsigned long)__x86_indirect_thunk_rcx - (stub_va + 11 + 4);

Given the context I think a cast to signed long would be more
appropriate here.

> +
> +#else
>      /* callq *%rcx */

Given the rest of the conditional you add, please either remove
the blank line above or add three more immediately inside the
preprocessor directives.

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

Perhaps better "P6_NOP3" in the comment? And perhaps
__stringify(P6_NOP3) in the memcpy() invocation, which may then
make unnecessary that part of the comment?

Jan


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