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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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