Re: [Xen-devel] [PATCH] x86/PV: avoid indirect call/thunk in I/O emulation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/02/18 16:03, Jan Beulich wrote:
> The stub is within reach from the .text section, so there's no point
> using an indirect call here. This has the added benefit of there no
> longer being two sufficiently different approaches, breaking one of
> which people may not even notice.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Wow - after all effort I've spent changing (and breaking) this, it
hadn't even occurred to me that a plain jmp was fine.

>
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -73,55 +73,42 @@ void (*pv_post_outb_hook)(unsigned int p
>  
>  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;
> +    signed long disp;

I'm not in favour of sprinkling 'signed' all over the code base.

long is already unambiguous, and a far more common construct to encounter. 

>      bool use_quirk_stub = false;
>  
>      if ( !ctxt->io_emul_stub )
>          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] =
> -        (long)__x86_indirect_thunk_rcx - (stub_va + 11 + 4);
> -#else
> -    /* callq *%rcx */
> -    ctxt->io_emul_stub[10] = 0xff;
> -    ctxt->io_emul_stub[11] = 0xd1;
> -    /* TODO: untangle ideal_nops from init/livepatch Kconfig options. */
> -    memcpy(&ctxt->io_emul_stub[12], "\x0f\x1f\x00", 3); /* P6_NOP3 */
> -#endif
> +    /* call host_to_guest_gpr_switch */
> +    ctxt->io_emul_stub[0] = 0xe8;
> +    disp = (long)host_to_guest_gpr_switch - (stub_va + 1 + 4);

The 1 in the middle here only aids clarity in the context above, where
it was part of a direct assignment to offset 11 in io_emul_stub[].

It is marginal, but in this case, I think stub_va + 5 would be slightly
clearer, as call opcode is obviously at 0, and the length of a call
instruction is 5.

~Andrew

> +    BUG_ON((int32_t)disp != disp);
> +    *(int32_t *)&ctxt->io_emul_stub[1] = disp;
>  
>      if ( unlikely(ioemul_handle_quirk) )
> -        use_quirk_stub = ioemul_handle_quirk(opcode, &ctxt->io_emul_stub[15],
> +        use_quirk_stub = ioemul_handle_quirk(opcode, &ctxt->io_emul_stub[5],
>                                               ctxt->ctxt.regs);
>  
>      if ( !use_quirk_stub )
>      {
>          /* data16 or nop */
> -        ctxt->io_emul_stub[15] = (bytes != 2) ? 0x90 : 0x66;
> +        ctxt->io_emul_stub[5] = (bytes != 2) ? 0x90 : 0x66;
>          /* <io-access opcode> */
> -        ctxt->io_emul_stub[16] = opcode;
> +        ctxt->io_emul_stub[6] = opcode;
>          /* imm8 or nop */
> -        ctxt->io_emul_stub[17] = !(opcode & 8) ? port : 0x90;
> +        ctxt->io_emul_stub[7] = !(opcode & 8) ? port : 0x90;
>          /* ret (jumps to guest_to_host_gpr_switch) */
> -        ctxt->io_emul_stub[18] = 0xc3;
> +        ctxt->io_emul_stub[8] = 0xc3;
>      }
>  
> -    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX(19, /* Default emul stub */
> -                                         15 + IOEMUL_QUIRK_STUB_BYTES));
> +    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX(9, /* Default emul stub */
> +                                         5 + IOEMUL_QUIRK_STUB_BYTES));
>  
>      /* Handy function-typed pointer to the stub. */
>      return (void *)stub_va;
>
>
>


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