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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 23 November 2016 15:39
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <JBeulich@xxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Subject: [PATCH 02/15] x86/emul: Simplfy emulation state setup
> 
> The current code to set up emulation state is ad-hoc and error prone.
> 
>  * Consistently zero all emulation state structures.
>  * Avoid explicitly initialising some state to 0.
>  * Explicitly identify all input and output state in x86_emulate_ctxt.  This
>    involves rearanging some fields.
>  * Have x86_decode() explicitly initalise all output state at its start.
> 
> In addition, move the calculation of hvmemul_ctxt->ctxt.swint_emulate
> from
> _hvm_emulate_one() to hvm_emulate_init_once(), as it doesn't need
> recalculating for each instruction.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/emulate.c             | 28 +++++++++++++++-------------
>  xen/arch/x86/mm.c                      |  3 ++-
>  xen/arch/x86/mm/shadow/common.c        |  4 ++--
>  xen/arch/x86/x86_emulate/x86_emulate.c |  2 ++
>  xen/arch/x86/x86_emulate/x86_emulate.h | 22 +++++++++++++++-------
>  5 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 3ab0e8e..f24e211 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1769,13 +1769,6 @@ static int _hvm_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt,
> 
>      vio->mmio_retry = 0;
> 
> -    if ( cpu_has_vmx )
> -        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
> -    else if ( cpu_has_svm_nrips )
> -        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
> -    else
> -        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
> -
>      rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
> 
>      if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> @@ -1946,14 +1939,23 @@ void hvm_emulate_init_once(
>      struct hvm_emulate_ctxt *hvmemul_ctxt,
>      struct cpu_user_regs *regs)
>  {
> -    hvmemul_ctxt->intr_shadow =
> hvm_funcs.get_interrupt_shadow(current);
> -    hvmemul_ctxt->ctxt.regs = regs;
> -    hvmemul_ctxt->ctxt.force_writeback = 1;
> -    hvmemul_ctxt->seg_reg_accessed = 0;
> -    hvmemul_ctxt->seg_reg_dirty = 0;
> -    hvmemul_ctxt->set_context = 0;
> +    struct vcpu *curr = current;
> +
> +    memset(hvmemul_ctxt, 0, sizeof(*hvmemul_ctxt));
> +
> +    hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(curr);
>      hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>      hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
> +
> +    hvmemul_ctxt->ctxt.regs = regs;
> +    hvmemul_ctxt->ctxt.force_writeback = true;
> +
> +    if ( cpu_has_vmx )
> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
> +    else if ( cpu_has_svm_nrips )
> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
> +    else
> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
>  }
> 
>  void hvm_emulate_init_per_insn(
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 03dcd71..9901f6f 100644
> --- 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;
> diff --git a/xen/arch/x86/mm/shadow/common.c
> b/xen/arch/x86/mm/shadow/common.c
> index ced2313..6f6668d 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -385,8 +385,9 @@ const struct x86_emulate_ops
> *shadow_init_emulation(
>      struct vcpu *v = current;
>      unsigned long addr;
> 
> +    memset(sh_ctxt, 0, sizeof(*sh_ctxt));
> +
>      sh_ctxt->ctxt.regs = regs;
> -    sh_ctxt->ctxt.force_writeback = 0;
>      sh_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
> 
>      if ( is_pv_vcpu(v) )
> @@ -396,7 +397,6 @@ const struct x86_emulate_ops
> *shadow_init_emulation(
>      }
> 
>      /* Segment cache initialisation. Primed with CS. */
> -    sh_ctxt->valid_seg_regs = 0;
>      creg = hvm_get_seg_reg(x86_seg_cs, sh_ctxt);
> 
>      /* Work out the emulation mode. */
> 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.

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

  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