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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.01.18 at 12:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Hmm, yes, I can see your point. Yet the harm done is minimal -
unintended/accidental use of the stubs is not going to result in
misbehavior, just in somewhat reduced performance.

Jan


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