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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.01.18 at 19:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/01/18 09:23, Jan Beulich wrote:
>>>>> On 04.01.18 at 01:15, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- 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.

I don't understand - it's only a declaration. Without users, no
reference to the symbol will be put anywhere.

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

Oh, right, other than above here I agree in general. However, I
think I did suggest to unconditionally build the thunks as well - they
do no harm if unused.

Jan


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