Re: [Xen-devel] [PATCH] x86/ioemul: Account for ioemul_handle_quirk() in stub length check

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/01/2018 09:52, Jan Beulich wrote:
>>>> On 09.01.18 at 17:47, <andrew.cooper3@xxxxxxxxxx> wrote:
>> The opcode potentially written into ctxt->io_emul_stub[] in the case
>> that ioemul_handle_quirk() is overriding the default logic isnt
>> accounted for in the build-time check that the stubs are large enough.
>>
>> Introduce IOEMUL_QUIRK_STUB_BYTES and use for both the main and quirk
>> stub cases.  As a slim optimisation, avoid writing out the default stub
>> when we know we are going to overwrite it.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with one suggestion:
>
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -89,19 +89,24 @@ static io_emul_stub_t *io_emul_stub_setup(struct 
>> priv_op_ctxt *ctxt, u8 opcode,
>>      /* callq *%rcx */
>>      ctxt->io_emul_stub[10] = 0xff;
>>      ctxt->io_emul_stub[11] = 0xd1;
>> -    /* data16 or nop */
>> -    ctxt->io_emul_stub[12] = (bytes != 2) ? 0x90 : 0x66;
>> -    /* <io-access opcode> */
>> -    ctxt->io_emul_stub[13] = opcode;
>> -    /* imm8 or nop */
>> -    ctxt->io_emul_stub[14] = !(opcode & 8) ? port : 0x90;
>> -    /* ret (jumps to guest_to_host_gpr_switch) */
>> -    ctxt->io_emul_stub[15] = 0xc3;
>> -    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < 16);
>> -
>> -    if ( ioemul_handle_quirk )
>> +
>> +    if ( likely(!ioemul_handle_quirk) )
>> +    {
>> +        /* data16 or nop */
>> +        ctxt->io_emul_stub[12] = (bytes != 2) ? 0x90 : 0x66;
>> +        /* <io-access opcode> */
>> +        ctxt->io_emul_stub[13] = opcode;
>> +        /* imm8 or nop */
>> +        ctxt->io_emul_stub[14] = !(opcode & 8) ? port : 0x90;
>> +        /* ret (jumps to guest_to_host_gpr_switch) */
>> +        ctxt->io_emul_stub[15] = 0xc3;
>> +    }
>> +    else
>>          ioemul_handle_quirk(opcode, &ctxt->io_emul_stub[12], 
>> ctxt->ctxt.regs);
>>  
>> +    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX(16, /* Regular stubs */
> Now that we have it available globally, would you mind using
> MAX_INST_LEN + 1 here instead of the literal 16?

This raw 16 is because we write 16 bytes into ctxt->io_emul_stub[], and
this jumps to 19 with the INDIRECT_THUNK work, not because it is related
to MAX_INST_LEN.

~Andrew

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