Re: [Xen-devel] [PATCH v4] x86/vm_event: Allow overwriting Xen's i-cache used for emulation

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Tamas K Lengyel [mailto:tamas.lengyel@xxxxxxxxxxxx]
> Sent: 22 September 2016 19:54
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew
> Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jun Nakajima
> <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> Subject: [PATCH v4] x86/vm_event: Allow overwriting Xen's i-cache used for
> emulation
> 
> When emulating instructions Xen's emulator maintains a small i-cache
> fetched from the guest memory. This patch extends the vm_event interface
> to allow overwriting this i-cache via a buffer returned in the vm_event
> response.
> 
> When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor
> subscriber normally has to remove the INT3 from memory - singlestep - place
> back INT3 to allow the guest to continue execution. This routine however is
> susceptible to a race-condition on multi-vCPU guests. By allowing the
> subscriber to return the i-cache to be used for emulation it can side-step the
> problem by returning a clean buffer without the INT3 present.
> 
> As part of this patch we rename hvm_mem_access_emulate_one to
> hvm_emulate_one_vm_event to better reflect that it is used in various
> vm_event scenarios now, not just in response to mem_access events.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> Acked-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> ---
> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>

I/O emulation code:
Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> 
> v4: Copy insn buffer into mmio buffer to avoid more login in
> hvm_emulate_one
>     Add comment in hvm_do_resume to preserve order as described in
> vm_event.h
> ---
>  xen/arch/x86/hvm/emulate.c        | 27 +++++++++++++++++++++------
>  xen/arch/x86/hvm/hvm.c            | 13 ++++++++++---
>  xen/arch/x86/vm_event.c           | 11 ++++++++++-
>  xen/include/asm-x86/hvm/emulate.h |  5 +++--
>  xen/include/asm-x86/vm_event.h    |  5 ++++-
>  xen/include/public/vm_event.h     | 17 ++++++++++++++++-
>  6 files changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index cc25676..17f7f0d 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -76,9 +76,9 @@ static int set_context_data(void *buffer, unsigned int
> size)
>      if ( curr->arch.vm_event )
>      {
>          unsigned int safe_size =
> -            min(size, curr->arch.vm_event->emul_read_data.size);
> +            min(size, curr->arch.vm_event->emul.read.size);
> 
> -        memcpy(buffer, curr->arch.vm_event->emul_read_data.data,
> safe_size);
> +        memcpy(buffer, curr->arch.vm_event->emul.read.data, safe_size);
>          memset(buffer + safe_size, 0, size - safe_size);
>          return X86EMUL_OKAY;
>      }
> @@ -1931,7 +1931,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
> unsigned long gla)
>      return rc;
>  }
> 
> -void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int
> trapnr,
> +void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int
> trapnr,
>      unsigned int errcode)
>  {
>      struct hvm_emulate_ctxt ctx = {{ 0 }}; @@ -1944,10 +1944,25 @@ void
> hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
>      case EMUL_KIND_NOWRITE:
>          rc = hvm_emulate_one_no_write(&ctx);
>          break;
> -    case EMUL_KIND_SET_CONTEXT:
> -        ctx.set_context = 1;
> -        /* Intentional fall-through. */
> +    case EMUL_KIND_SET_CONTEXT_INSN: {
> +        struct vcpu *curr = current;
> +        struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
> +
> +        BUILD_BUG_ON(sizeof(vio->mmio_insn) !=
> +                     sizeof(curr->arch.vm_event->emul.insn.data));
> +        ASSERT(!vio->mmio_insn_bytes);
> +
> +        /*
> +         * Stash insn buffer into mmio buffer here instead of ctx
> +         * to avoid having to add more logic to hvm_emulate_one.
> +         */
> +        vio->mmio_insn_bytes = sizeof(vio->mmio_insn);
> +        memcpy(vio->mmio_insn, curr->arch.vm_event->emul.insn.data,
> +               vio->mmio_insn_bytes);
> +    }
> +    /* Fall-through */
>      default:
> +        ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA);
>          rc = hvm_emulate_one(&ctx);
>      }
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index
> 7bad845..b06e4d5 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -487,15 +487,22 @@ void hvm_do_resume(struct vcpu *v)
>          {
>              enum emul_kind kind = EMUL_KIND_NORMAL;
> 
> +            /*
> +             * Please observ the order here to match the flag descriptions
> +             * provided in public/vm_event.h
> +             */
>              if ( v->arch.vm_event->emulate_flags &
>                   VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -                kind = EMUL_KIND_SET_CONTEXT;
> +                kind = EMUL_KIND_SET_CONTEXT_DATA;
>              else if ( v->arch.vm_event->emulate_flags &
>                        VM_EVENT_FLAG_EMULATE_NOWRITE )
>                  kind = EMUL_KIND_NOWRITE;
> +            else if ( v->arch.vm_event->emulate_flags &
> +                      VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> +                kind = EMUL_KIND_SET_CONTEXT_INSN;
> 
> -            hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
> -                                       HVM_DELIVER_NO_ERROR_CODE);
> +            hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
> +                                     HVM_DELIVER_NO_ERROR_CODE);
> 
>              v->arch.vm_event->emulate_flags = 0;
>          }
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index
> 343b9c8..1e88d67 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -209,11 +209,20 @@ void vm_event_emulate_check(struct vcpu *v,
> vm_event_response_t *rsp)
>          if ( p2m_mem_access_emulate_check(v, rsp) )
>          {
>              if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -                v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +                v->arch.vm_event->emul.read = rsp->data.emul.read;
> 
>              v->arch.vm_event->emulate_flags = rsp->flags;
>          }
>          break;
> +
> +    case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> +        if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> +        {
> +            v->arch.vm_event->emul.insn = rsp->data.emul.insn;
> +            v->arch.vm_event->emulate_flags = rsp->flags;
> +        }
> +        break;
> +
>      default:
>          break;
>      };
> diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-
> x86/hvm/emulate.h
> index 3aabcbe..96d8f0b 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -40,14 +40,15 @@ struct hvm_emulate_ctxt {  enum emul_kind {
>      EMUL_KIND_NORMAL,
>      EMUL_KIND_NOWRITE,
> -    EMUL_KIND_SET_CONTEXT
> +    EMUL_KIND_SET_CONTEXT_DATA,
> +    EMUL_KIND_SET_CONTEXT_INSN
>  };
> 
>  int hvm_emulate_one(
>      struct hvm_emulate_ctxt *hvmemul_ctxt);  int
> hvm_emulate_one_no_write(
>      struct hvm_emulate_ctxt *hvmemul_ctxt); -void
> hvm_mem_access_emulate_one(enum emul_kind kind,
> +void hvm_emulate_one_vm_event(enum emul_kind kind,
>      unsigned int trapnr,
>      unsigned int errcode);
>  void hvm_emulate_prepare(
> diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-
> x86/vm_event.h index ebb5d88..ca73f99 100644
> --- a/xen/include/asm-x86/vm_event.h
> +++ b/xen/include/asm-x86/vm_event.h
> @@ -27,7 +27,10 @@
>   */
>  struct arch_vm_event {
>      uint32_t emulate_flags;
> -    struct vm_event_emul_read_data emul_read_data;
> +    union {
> +        struct vm_event_emul_read_data read;
> +        struct vm_event_emul_insn_data insn;
> +    } emul;
>      struct monitor_write_data write_data;  };
> 
> diff --git a/xen/include/public/vm_event.h
> b/xen/include/public/vm_event.h index f756126..ba8e387 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -97,6 +97,14 @@
>   * Requires the vCPU to be paused already (synchronous events only).
>   */
>  #define VM_EVENT_FLAG_SET_REGISTERS      (1 << 8)
> +/*
> + * Instruction cache is being sent back to the hypervisor in the event
> +response
> + * to be used by the emulator. This flag is only useful when combined
> +with
> + * VM_EVENT_FLAG_EMULATE and does not take presedence if combined
> with
> + * VM_EVENT_FLAG_EMULATE_NOWRITE or
> VM_EVENT_FLAG_SET_EMUL_READ_DATA, (i.e.
> + * if any of those flags are set, only those will be honored).
> + */
> +#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9)
> 
>  /*
>   * Reasons for the vm event request
> @@ -265,6 +273,10 @@ struct vm_event_emul_read_data {
>      uint8_t  data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];  };
> 
> +struct vm_event_emul_insn_data {
> +    uint8_t data[16]; /* Has to be completely filled */ };
> +
>  typedef struct vm_event_st {
>      uint32_t version;   /* VM_EVENT_INTERFACE_VERSION */
>      uint32_t flags;     /* VM_EVENT_FLAG_* */
> @@ -291,7 +303,10 @@ typedef struct vm_event_st {
>              struct vm_event_regs_arm arm;
>          } regs;
> 
> -        struct vm_event_emul_read_data emul_read_data;
> +        union {
> +            struct vm_event_emul_read_data read;
> +            struct vm_event_emul_insn_data insn;
> +        } emul;
>      } data;
>  } vm_event_request_t, vm_event_response_t;
> 
> --
> 2.9.3


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