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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/01/18 08:36, Jan Beulich wrote:
>>>>  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.

Actually, including them does do harm, in the case that a user wants to
compile without these modifications.  Excluding the symbols guarantees
the resulting binary wont link, if there is a mistake elsewhere which is
accidentally using them.

~Andrew

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