Re: [PATCH RFC V2 1/6] xen: Emulate with no writes

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/07/14 16:43, Razvan Cojocaru wrote:
> Added support for emulating an instruction with no memory writes.
> Additionally, introduced hvm_emulate_one_full(), which acts upon all
> possible return values from the hvm_emulate_one() functions (RETRY,
> EXCEPTION, UNHANDLEABLE).
>
> Changes since V1:
>  - Removed the Linux code that computes the length of an instruction.
>  - Unused function parameters are no longer marked.
>  - Refactored the code to eliminate redundancy.
>  - Made the exception passed on to the guest by hvm_emulate_one_full()
>    configurable.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/arch/x86/hvm/emulate.c        |   73 +++++++++++++++++++++++++++++++++++--
>  xen/include/asm-x86/hvm/emulate.h |    5 +++
>  2 files changed, 75 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index eac159f..114a77a 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -688,6 +688,17 @@ static int hvmemul_write(
>      return X86EMUL_OKAY;
>  }
>  
> +static int hvmemul_write_dummy(

hvmemul_write_discard() would make its purpose more clear.

> +    enum x86_segment seg,
> +    unsigned long offset,
> +    void *p_data,
> +    unsigned int bytes,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    /* discarding the write */

Full stop please.

> +    return X86EMUL_OKAY;
> +}
> +
>  static int hvmemul_cmpxchg(
>      enum x86_segment seg,
>      unsigned long offset,
> @@ -1138,8 +1149,8 @@ static const struct x86_emulate_ops hvm_emulate_ops = {
>      .invlpg        = hvmemul_invlpg
>  };
>  
> -int hvm_emulate_one(
> -    struct hvm_emulate_ctxt *hvmemul_ctxt)
> +static int hvm_emulate_one_with_ops(struct hvm_emulate_ctxt *hvmemul_ctxt,
> +    const struct x86_emulate_ops *ops)

You could get away with a renaming this to _hvm_emulate_one() to help
identify that it is basically identical to hvm_emulate_one(), just with
slightly different parameters.

>  {
>      struct cpu_user_regs *regs = hvmemul_ctxt->ctxt.regs;
>      struct vcpu *curr = current;
> @@ -1191,7 +1202,7 @@ int hvm_emulate_one(
>      vio->mmio_retrying = vio->mmio_retry;
>      vio->mmio_retry = 0;
>  
> -    rc = x86_emulate(&hvmemul_ctxt->ctxt, &hvm_emulate_ops);
> +    rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>  
>      if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>          rc = X86EMUL_RETRY;
> @@ -1239,6 +1250,62 @@ int hvm_emulate_one(
>      return X86EMUL_OKAY;
>  }
>  
> +int hvm_emulate_one(
> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
> +    return hvm_emulate_one_with_ops(hvmemul_ctxt, &hvm_emulate_ops);
> +}
> +
> +int hvm_emulate_one_no_write(
> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
> +    struct x86_emulate_ops local_ops = hvm_emulate_ops;
> +    local_ops.write = hvmemul_write_dummy;

This is runtime constant and somwhat expensive to set up for each
function call.

You could make a static const struct x86_emulate_ops
hvm_emulate_ops_no_write and use that each time.

> +
> +    return hvm_emulate_one_with_ops(hvmemul_ctxt, &local_ops);
> +}
> +
> +void hvm_emulate_one_full(bool_t nowrite,
> +    unsigned int unhandleable_trapnr,
> +    int unhandleable_errcode)
> +{
> +    struct hvm_emulate_ctxt ctx[1] = {};

This construct looks suspect.  What is wrong with

struct hvm_emulate_ctxt ctx = { 0 }; and using &ctx below ?

> +    int rc = X86EMUL_RETRY;
> +
> +    hvm_emulate_prepare(ctx, guest_cpu_user_regs());
> +
> +    while ( rc == X86EMUL_RETRY )
> +    {
> +        if ( nowrite )
> +            rc = hvm_emulate_one_no_write(ctx);
> +        else
> +            rc = hvm_emulate_one(ctx);
> +    }

This however is not appropriate.  X86EMUL_RETRY can include "talk to
dom0 and get the paging subsystem to page in part of the VM", which
given some current work-in-progress can be to the other end of a network
connection on the far side of an optimistic migration.

You can't cause a Xen pcpu to spin like this for that length of time. 
Anything longer than a second and all other pcpus will block against the
time calibration rendezvous, and longer than 5 will panic on the NMI
watchdog.

> +
> +    switch ( rc )
> +    {
> +    case X86EMUL_UNHANDLEABLE:
> +        gdprintk(XENLOG_DEBUG, "Emulation failed @ %04x:%lx: "
> +               "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
> +               hvmemul_get_seg_reg(x86_seg_cs, ctx)->sel,
> +               ctx->insn_buf_eip,
> +               ctx->insn_buf[0], ctx->insn_buf[1],
> +               ctx->insn_buf[2], ctx->insn_buf[3],
> +               ctx->insn_buf[4], ctx->insn_buf[5],
> +               ctx->insn_buf[6], ctx->insn_buf[7],
> +               ctx->insn_buf[8], ctx->insn_buf[9]);

Hmm - I keep meaning to add a new %p custom format to print out hex
buffers like this.  It is sadly quite a long way down my todo list.

> +        hvm_inject_hw_exception(unhandleable_trapnr, unhandleable_errcode);
> +        break;
> +    case X86EMUL_EXCEPTION:
> +        if ( ctx->exn_pending )
> +            hvm_inject_hw_exception(ctx->exn_vector, ctx->exn_error_code);
> +        /* fall through */
> +    default:
> +        hvm_emulate_writeback(ctx);
> +        break;
> +    }
> +}
> +
>  void hvm_emulate_prepare(
>      struct hvm_emulate_ctxt *hvmemul_ctxt,
>      struct cpu_user_regs *regs)
> diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
> index 00a06cc..d68b485 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -37,6 +37,11 @@ struct hvm_emulate_ctxt {
>  
>  int hvm_emulate_one(
>      struct hvm_emulate_ctxt *hvmemul_ctxt);
> +int hvm_emulate_one_no_write(
> +    struct hvm_emulate_ctxt *hvmemul_ctxt);
> +void hvm_emulate_one_full(bool_t nowrite,
> +    unsigned int unhandleable_trapnr,
> +    int unhandleable_errcode);

Prevailing Xen nomenclature would be 'trapnr' and 'error_code'.  The
error code should be unsigned as well.

~Andrew

>  void hvm_emulate_prepare(
>      struct hvm_emulate_ctxt *hvmemul_ctxt,
>      struct cpu_user_regs *regs);