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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 23 November 2016 16:01
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Xen-devel <xen-
> devel@xxxxxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>
> Subject: Re: [PATCH 02/15] x86/emul: Simplfy emulation state setup
> 
> 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.

D'oh, yes. Sorry, got confused by the field movements... my eyes were seeing 
'+' as '-' for some reason.

  Paul

> 
> >
> >>      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