[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/2] x86/vm_event: Allow returning i-cache for emulation



On 09/15/16 19:36, Tamas K Lengyel wrote:
> On Wed, Sep 14, 2016 at 1:58 AM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 09/13/2016 09:12 PM, Tamas K Lengyel wrote:
>>> When emulating instructions the emulator maintains a small i-cache fetched
>>> from the guest memory. This patch extends the vm_event interface to allow
>>> returning this i-cache via the vm_event response instead.
>>>
>>> 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>
>>> ---
>>> Cc: 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: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>>
>>> Note: This patch only has been compile-tested.
>>> ---
>>>  xen/arch/x86/hvm/emulate.c        | 44 
>>> ++++++++++++++++++++++++++-------------
>>>  xen/arch/x86/hvm/hvm.c            |  9 +++++---
>>>  xen/arch/x86/hvm/vmx/vmx.c        |  1 +
>>>  xen/arch/x86/vm_event.c           |  9 +++++++-
>>>  xen/common/vm_event.c             |  1 -
>>>  xen/include/asm-x86/hvm/emulate.h |  8 ++++---
>>>  xen/include/asm-x86/vm_event.h    |  3 ++-
>>>  xen/include/public/vm_event.h     | 16 +++++++++++++-
>>>  8 files changed, 67 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>> index cc25676..504ed35 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;
>>>      }
>>> @@ -827,7 +827,7 @@ static int hvmemul_read(
>>>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>>>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>
>>> -    if ( unlikely(hvmemul_ctxt->set_context) )
>>> +    if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>          return set_context_data(p_data, bytes);
>>>
>>>      return __hvmemul_read(
>>> @@ -1029,7 +1029,7 @@ static int hvmemul_cmpxchg(
>>>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>>>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>
>>> -    if ( unlikely(hvmemul_ctxt->set_context) )
>>> +    if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>      {
>>>          int rc = set_context_data(p_new, bytes);
>>>
>>> @@ -1122,7 +1122,7 @@ static int hvmemul_rep_outs(
>>>      p2m_type_t p2mt;
>>>      int rc;
>>>
>>> -    if ( unlikely(hvmemul_ctxt->set_context) )
>>> +    if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>          return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
>>>                                              bytes_per_rep, reps, ctxt);
>>>
>>> @@ -1264,7 +1264,7 @@ static int hvmemul_rep_movs(
>>>      if ( buf == NULL )
>>>          return X86EMUL_UNHANDLEABLE;
>>>
>>> -    if ( unlikely(hvmemul_ctxt->set_context) )
>>> +    if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>      {
>>>          rc = set_context_data(buf, bytes);
>>>
>>> @@ -1470,7 +1470,7 @@ static int hvmemul_read_io(
>>>
>>>      *val = 0;
>>>
>>> -    if ( unlikely(hvmemul_ctxt->set_context) )
>>> +    if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>          return set_context_data(val, bytes);
>>>
>>>      return hvmemul_do_pio_buffer(port, bytes, IOREQ_READ, val);
>>> @@ -1793,7 +1793,14 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
>>> *hvmemul_ctxt,
>>>          pfec |= PFEC_user_mode;
>>>
>>>      hvmemul_ctxt->insn_buf_eip = regs->eip;
>>> -    if ( !vio->mmio_insn_bytes )
>>> +
>>> +    if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>> +    {
>>> +        hvmemul_ctxt->insn_buf_bytes = 
>>> sizeof(curr->arch.vm_event->emul_insn);
>>> +        memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul_insn,
>>> +               hvmemul_ctxt->insn_buf_bytes);
>>> +    }
>>> +    else if ( !vio->mmio_insn_bytes )
>>>      {
>>>          hvmemul_ctxt->insn_buf_bytes =
>>>              hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?:
>>> @@ -1931,7 +1938,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,11 +1951,19 @@ 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. */
>>> -    default:
>>> +    case EMUL_KIND_SET_CONTEXT_DATA:
>>> +        ctx.set_context_data = 1;
>>> +        rc = hvm_emulate_one(&ctx);
>>> +        break;
>>> +    case EMUL_KIND_SET_CONTEXT_INSN:
>>> +        ctx.set_context_insn = 1;
>>>          rc = hvm_emulate_one(&ctx);
>>> +        break;
>>> +    case EMUL_KIND_NORMAL:
>>> +        rc = hvm_emulate_one(&ctx);
>>> +        break;
>>> +    default:
>>> +        return;
>>>      }
>>>
>>>      switch ( rc )
>>> @@ -1983,7 +1998,8 @@ void hvm_emulate_prepare(
>>>      hvmemul_ctxt->ctxt.force_writeback = 1;
>>>      hvmemul_ctxt->seg_reg_accessed = 0;
>>>      hvmemul_ctxt->seg_reg_dirty = 0;
>>> -    hvmemul_ctxt->set_context = 0;
>>> +    hvmemul_ctxt->set_context_data = 0;
>>> +    hvmemul_ctxt->set_context_insn = 0;
>>>      hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>>>      hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
>>>  }
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index ca96643..7462794 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>>>
>>>              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/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 2759e6f..d214716 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -57,6 +57,7 @@
>>>  #include <asm/altp2m.h>
>>>  #include <asm/event.h>
>>>  #include <asm/monitor.h>
>>> +#include <asm/vm_event.h>
>>>  #include <public/arch-x86/cpuid.h>
>>>
>>>  static bool_t __initdata opt_force_ept;
>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>> index 343b9c8..03beed3 100644
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -209,11 +209,18 @@ 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/common/vm_event.c b/xen/common/vm_event.c
>>> index 907ab40..d8ee7f3 100644
>>> --- a/xen/common/vm_event.c
>>> +++ b/xen/common/vm_event.c
>>> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct 
>>> vm_event_domain *ved)
>>>           * In some cases the response type needs extra handling, so here
>>>           * we call the appropriate handlers.
>>>           */
>>> -
>>>          /* Check flags which apply only when the vCPU is paused */
>>>          if ( atomic_read(&v->vm_event_pause_count) )
>>>          {
>>> diff --git a/xen/include/asm-x86/hvm/emulate.h 
>>> b/xen/include/asm-x86/hvm/emulate.h
>>> index 3aabcbe..b52f99e 100644
>>> --- a/xen/include/asm-x86/hvm/emulate.h
>>> +++ b/xen/include/asm-x86/hvm/emulate.h
>>> @@ -34,20 +34,22 @@ struct hvm_emulate_ctxt {
>>>
>>>      uint32_t intr_shadow;
>>>
>>> -    bool_t set_context;
>>> +    bool_t set_context_data;
>>> +    bool_t set_context_insn;
>>>  };
>>>
>>>  enum emul_kind {
>>>      EMUL_KIND_NORMAL,
>>>      EMUL_KIND_NOWRITE,
>>> -    EMUL_KIND_SET_CONTEXT
>>> +    EMUL_KIND_SET_CONTEXT_DATA,
>>> +    EMUL_KIND_SET_CONTEXT_INSN
>>
>> Since this breaks compilation of existing clients, I think we should
>> increment some macro to alow for compile-time switching (maybe
>> VM_EVENT_INTERFACE_VERSION?).
> 
> I'm not sure I follow - this is a Xen internal-only enumeration. What
> kind-of clients are you referring to?

Nevermind, I thought these changes propagate to the toolstack headers.
Sorry for the confusion.


Thanks,
Razvan


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.