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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/11/16 13:44, Jan Beulich wrote:
>>>> On 23.11.16 at 16:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5363,8 +5363,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long 
>> addr,
>>          goto bail;
>>      }
>>  
>> +    memset(&ptwr_ctxt, 0, sizeof(ptwr_ctxt));
>> +
>>      ptwr_ctxt.ctxt.regs = regs;
>> -    ptwr_ctxt.ctxt.force_writeback = 0;
>>      ptwr_ctxt.ctxt.addr_size = ptwr_ctxt.ctxt.sp_size =
>>          is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG;
>>      ptwr_ctxt.ctxt.swint_emulate = x86_swint_emulate_none;
> Mind using an explicit initializer instead, moving everything there that's
> already known at function start?

Done.

>
>> --- 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;
> I have to admit that I don't see the point of this.

There are early exit paths which leave it uninitialised.  An alternative
would be explicitly document that it is only valid for X86EMUL_OKAY?

>
>> --- 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;
>> +
>> +    /* 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 {
> Hmm, this separation looks to be correct for the current state of the
> emulator, but I don't think it is architecturally so (and hence it would
> become wrong in the course of us completing it): Both addr_size and
> sp_size are not necessarily inputs only. Also keeping regs in the
> input only group is slightly misleading - the pointer itself surely is input
> only, but the pointed to data isn't.
>
> So I'd suggest to have three groups: input, input/output, output,
> even if for your purpose here you only want to tell apart fields which
> need to be initialized before calling x86_emulate() / x86_decode()
> (the first two groups) from those which don't (the last group).

Right - proposed net result looks like:

struct x86_emulate_ctxt
{
    /*
     * Input-only state:
     */

    /* Software event injection support. */
    enum x86_swint_emulation swint_emulate;

    /* Set this if writes may have side effects. */
    bool force_writeback;

    /* Caller data that can be used by x86_emulate_ops' routines. */
    void *data;

    /*
     * Input/output state:
     */

    /* Register state before/after emulation. */
    struct cpu_user_regs *regs;

    /* Default address size in current execution mode (16, 32, or 64). */
    unsigned int addr_size;

    /* Stack pointer width in bits (16, 32 or 64). */
    unsigned int sp_size;

    /*
     * Output-only state:
     */

    /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */
    unsigned int opcode;

    /* Retirement state, set by the emulator (valid only on
X86EMUL_OKAY). */
    union {
        struct {
            uint8_t hlt:1;          /* Instruction HLTed. */
            uint8_t mov_ss:1;       /* Instruction sets MOV-SS irq
shadow. */
            uint8_t sti:1;          /* Instruction sets STI irq shadow. */
        } flags;
        uint8_t byte;
    } retire;
};

If that is ok?

~Andrew

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