Re: [Xen-devel] [PATCH 02/15] x86/emul: Simplfy emulation state setup

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/11/16 15:58, Paul Durrant wrote:
>> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
>> b/xen/arch/x86/x86_emulate/x86_emulate.c
>> index 04f0dac..c5d9664 100644
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -1904,6 +1904,8 @@ x86_decode(
>>      state->regs = ctxt->regs;
>>      state->eip = ctxt->regs->eip;
>>
>> +    /* Initialise output state in x86_emulate_ctxt */
>> +    ctxt->opcode = ~0u;
>>      ctxt->retire.byte = 0;
> In the commit message you state that x86_decode() will "explicitly initalise 
> all output state at its start". This doesn't seem to be all the output state. 
> In fact you appear to be removing some initialization.

There are only two fields of output state, as delineated by the extra
comments in x86_emulate_ctxt.  Most of x86_emulate_ctxt is input state.

>
>>      op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt-
>>> addr_size/8;
>> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h
>> b/xen/arch/x86/x86_emulate/x86_emulate.h
>> index 993c576..93b268e 100644
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -412,6 +412,10 @@ struct cpu_user_regs;
>>
>>  struct x86_emulate_ctxt
>>  {
>> +    /*
>> +     * Input state:
>> +     */
>> +
>>      /* Register state before/after emulation. */
>>      struct cpu_user_regs *regs;
>>
>> @@ -421,14 +425,21 @@ struct x86_emulate_ctxt
>>      /* Stack pointer width in bits (16, 32 or 64). */
>>      unsigned int sp_size;
>>
>> -    /* Canonical opcode (see below). */
>> -    unsigned int opcode;
>> -
>>      /* Software event injection support. */
>>      enum x86_swint_emulation swint_emulate;
>>
>>      /* Set this if writes may have side effects. */
>> -    uint8_t force_writeback;
>> +    bool force_writeback;
> Is this type change intentional? I assume it is, but you didn't call it out.

Yes.  I thought I had it in the commit message, but will update for v2.

~Andrew

>
>   Paul
>
>> +
>> +    /* Caller data that can be used by x86_emulate_ops' routines. */
>> +    void *data;
>> +
>> +    /*
>> +     * Output state:
>> +     */
>> +
>> +    /* Canonical opcode (see below). */
>> +    unsigned int opcode;
>>
>>      /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY).
>> */
>>      union {
>> @@ -439,9 +450,6 @@ struct x86_emulate_ctxt
>>          } flags;
>>          uint8_t byte;
>>      } retire;
>> -
>> -    /* Caller data that can be used by x86_emulate_ops' routines. */
>> -    void *data;
>>  };
>>
>>  /*
>> --
>> 2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel