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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.18 at 17:53, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/02/18 16:03, Jan Beulich wrote:
>> --- 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. 

It's this "common" which made me use "signed" here (and in similar
places elsewhere) - we still have far too many cases left where plain
short/int/long are used when the unsigned variant would actually be
better. Hence, _at least_ until those issues are all gone, I very much
prefer making explicit when signed quantities are meant.

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

Yeah, I wasn't sure which way would be better. Done.

Jan


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