| Message ID | 20160913181223.1459-2-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@zentific.com> > --- > Cc: Paul Durrant <paul.durrant@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > > 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?). Thanks, Razvan
>>> On 13.09.16 at 20:12, <tamas.lengyel@zentific.com> 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. I guess I'm sightly confused: Isn't the purpose to have the monitoring app _write_ to the cache maintained in Xen? Or else, who is "emulator" here? If that's the app, then I think subject and description for hypervisor patches would better be written taking the perspective of the hypervisor, especially when using potentially ambiguous terms. > Note: This patch only has been compile-tested. This certainly needs to change before this can be committed. > @@ -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) ?: > @@ -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; One of the former two surely can be made fall through into the latter? > + default: > + return; Why don't you retain the previous default handling? > --- 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; As you have been told by others on patch 1 already - please take the opportunity to switch to plain bool. > --- a/xen/include/asm-x86/vm_event.h > +++ b/xen/include/asm-x86/vm_event.h > @@ -27,7 +27,8 @@ > */ > struct arch_vm_event { > uint32_t emulate_flags; > - struct vm_event_emul_read_data emul_read_data; > + struct vm_event_emul_read_data emul_read; > + struct vm_event_emul_insn_data emul_insn; Why don't these get put in a union, when ... > --- a/xen/include/public/vm_event.h > +++ b/xen/include/public/vm_event.h > @@ -97,6 +97,13 @@ > * 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 is incompatible with also setting > + * VM_EVENT_FLAG_EMULATE_NOWRITE or VM_EVENT_FLAG_SET_EMUL_READ_DATA. > + */ > +#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9) ... place these restrictions and use a union in in the public header? > @@ -265,6 +272,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 */ > +}; Any reason for the 16 (rather than the architectural 15) here? Jan
On Wed, Sep 14, 2016 at 9:55 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 13.09.16 at 20:12, <tamas.lengyel@zentific.com> 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. > > I guess I'm sightly confused: Isn't the purpose to have the monitoring > app _write_ to the cache maintained in Xen? Or else, who is > "emulator" here? If that's the app, then I think subject and description > for hypervisor patches would better be written taking the perspective > of the hypervisor, especially when using potentially ambiguous terms. Well, I thought it was obvious that the built-in emulator in Xen is what this patch is referring to. Otherwise none of this makes sense. > >> Note: This patch only has been compile-tested. > > This certainly needs to change before this can be committed. I know, it's in process. That's why I put this note here to not get too hasty about merging it. >> @@ -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) ?: > > > >> @@ -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; > > One of the former two surely can be made fall through into the latter? That's what I did before by turning this into if-else's and you requested to go back to a switch. With a switch though I'll rather make the cases explicit as a fall-through just makes it harder to read for no real benefit. > >> + default: >> + return; > > Why don't you retain the previous default handling? The default label is unused and this makes it more apparent when reading the code. > >> --- 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; > > As you have been told by others on patch 1 already - please take > the opportunity to switch to plain bool. Ack. > >> --- a/xen/include/asm-x86/vm_event.h >> +++ b/xen/include/asm-x86/vm_event.h >> @@ -27,7 +27,8 @@ >> */ >> struct arch_vm_event { >> uint32_t emulate_flags; >> - struct vm_event_emul_read_data emul_read_data; >> + struct vm_event_emul_read_data emul_read; >> + struct vm_event_emul_insn_data emul_insn; > > Why don't these get put in a union, when ... > >> --- a/xen/include/public/vm_event.h >> +++ b/xen/include/public/vm_event.h >> @@ -97,6 +97,13 @@ >> * 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 is incompatible with also setting >> + * VM_EVENT_FLAG_EMULATE_NOWRITE or VM_EVENT_FLAG_SET_EMUL_READ_DATA. >> + */ >> +#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9) > > ... place these restrictions and use a union in in the public header? True, they can be put into a union there as well. > >> @@ -265,6 +272,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 */ >> +}; > > Any reason for the 16 (rather than the architectural 15) here? Yes, see the definition in include/asm-x86/hvm/emulate.h: struct hvm_emulate_ctxt { struct x86_emulate_ctxt ctxt; /* Cache of 16 bytes of instruction. */ uint8_t insn_buf[16]; Tamas
>>> On 14.09.16 at 18:20, <tamas.lengyel@zentific.com> wrote: > On Wed, Sep 14, 2016 at 9:55 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 13.09.16 at 20:12, <tamas.lengyel@zentific.com> 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. >> >> I guess I'm sightly confused: Isn't the purpose to have the monitoring >> app _write_ to the cache maintained in Xen? Or else, who is >> "emulator" here? If that's the app, then I think subject and description >> for hypervisor patches would better be written taking the perspective >> of the hypervisor, especially when using potentially ambiguous terms. > > Well, I thought it was obvious that the built-in emulator in Xen is > what this patch is referring to. Otherwise none of this makes sense. It would have been if the sentence didn't say "returning". The internal emulator's cache gets effectively overwritten, not returned to anything (unless I'm still misunderstanding something). >>> @@ -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; >> >> One of the former two surely can be made fall through into the latter? > > That's what I did before by turning this into if-else's and you > requested to go back to a switch. With a switch though I'll rather > make the cases explicit as a fall-through just makes it harder to read > for no real benefit. I disagree. >>> + default: >>> + return; >> >> Why don't you retain the previous default handling? > > The default label is unused and this makes it more apparent when > reading the code. Just like before, imo there shouldn't be any EMUL_KIND_NORMAL case; that should be the default label instead. >>> @@ -265,6 +272,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 */ >>> +}; >> >> Any reason for the 16 (rather than the architectural 15) here? > > Yes, see the definition in include/asm-x86/hvm/emulate.h: > > struct hvm_emulate_ctxt { > struct x86_emulate_ctxt ctxt; > > /* Cache of 16 bytes of instruction. */ > uint8_t insn_buf[16]; Ah, I didn't recall we have an oversized cache there too. But such a connection would better be documented by a BUILD_BUG_ON() comparing the two sizeof()s. Jan
On Wed, Sep 14, 2016 at 11:58 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 14.09.16 at 18:20, <tamas.lengyel@zentific.com> wrote: >> On Wed, Sep 14, 2016 at 9:55 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 13.09.16 at 20:12, <tamas.lengyel@zentific.com> 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. >>> >>> I guess I'm sightly confused: Isn't the purpose to have the monitoring >>> app _write_ to the cache maintained in Xen? Or else, who is >>> "emulator" here? If that's the app, then I think subject and description >>> for hypervisor patches would better be written taking the perspective >>> of the hypervisor, especially when using potentially ambiguous terms. >> >> Well, I thought it was obvious that the built-in emulator in Xen is >> what this patch is referring to. Otherwise none of this makes sense. > > It would have been if the sentence didn't say "returning". The > internal emulator's cache gets effectively overwritten, not > returned to anything (unless I'm still misunderstanding something). It's "returning" the i-cache in the sense that it's part of a vm_event response. > >>>> @@ -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; >>> >>> One of the former two surely can be made fall through into the latter? >> >> That's what I did before by turning this into if-else's and you >> requested to go back to a switch. With a switch though I'll rather >> make the cases explicit as a fall-through just makes it harder to read >> for no real benefit. > > I disagree. OK, I don't really care about it too much so if you feel that strongly about it then fine. > >>>> + default: >>>> + return; >>> >>> Why don't you retain the previous default handling? >> >> The default label is unused and this makes it more apparent when >> reading the code. > > Just like before, imo there shouldn't be any EMUL_KIND_NORMAL > case; that should be the default label instead. But there is EMUL_KIND_NORMAL case. All calls to this function must specify a pre-defined kind. There is no calling this function with non-defined kinds so the default label is really just EMUL_KIND_NORMAL. Why is it better to keep it under the default label then instead of explicitly showing that it's actually just that one case? > >>>> @@ -265,6 +272,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 */ >>>> +}; >>> >>> Any reason for the 16 (rather than the architectural 15) here? >> >> Yes, see the definition in include/asm-x86/hvm/emulate.h: >> >> struct hvm_emulate_ctxt { >> struct x86_emulate_ctxt ctxt; >> >> /* Cache of 16 bytes of instruction. */ >> uint8_t insn_buf[16]; > > Ah, I didn't recall we have an oversized cache there too. But such a > connection would better be documented by a BUILD_BUG_ON() > comparing the two sizeof()s. Sure. Thanks, Tamas
>>> On 15.09.16 at 17:27, <tamas.lengyel@zentific.com> wrote: > On Wed, Sep 14, 2016 at 11:58 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 14.09.16 at 18:20, <tamas.lengyel@zentific.com> wrote: >>> On Wed, Sep 14, 2016 at 9:55 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 13.09.16 at 20:12, <tamas.lengyel@zentific.com> 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. >>>> >>>> I guess I'm sightly confused: Isn't the purpose to have the monitoring >>>> app _write_ to the cache maintained in Xen? Or else, who is >>>> "emulator" here? If that's the app, then I think subject and description >>>> for hypervisor patches would better be written taking the perspective >>>> of the hypervisor, especially when using potentially ambiguous terms. >>> >>> Well, I thought it was obvious that the built-in emulator in Xen is >>> what this patch is referring to. Otherwise none of this makes sense. >> >> It would have been if the sentence didn't say "returning". The >> internal emulator's cache gets effectively overwritten, not >> returned to anything (unless I'm still misunderstanding something). > > It's "returning" the i-cache in the sense that it's part of a vm_event > response. Well, I can only express my desire for this to get expressed in a less confusing way. Maybe it's just me ... >>>>> @@ -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; >>>> >>>> Why don't you retain the previous default handling? >>> >>> The default label is unused and this makes it more apparent when >>> reading the code. >> >> Just like before, imo there shouldn't be any EMUL_KIND_NORMAL >> case; that should be the default label instead. > > But there is EMUL_KIND_NORMAL case. All calls to this function must > specify a pre-defined kind. There is no calling this function with > non-defined kinds so the default label is really just > EMUL_KIND_NORMAL. Why is it better to keep it under the default label > then instead of explicitly showing that it's actually just that one > case? Because changing that aspect is not the subject of your patch. And btw - blank lines between non-fall-through cases please. Jan
On Wed, Sep 14, 2016 at 1:58 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> 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@zentific.com> >> --- >> Cc: Paul Durrant <paul.durrant@citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: Jun Nakajima <jun.nakajima@intel.com> >> Cc: Kevin Tian <kevin.tian@intel.com> >> Cc: George Dunlap <george.dunlap@eu.citrix.com> >> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> >> 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? Tamas
On 09/15/16 19:36, Tamas K Lengyel wrote: > On Wed, Sep 14, 2016 at 1:58 AM, Razvan Cojocaru > <rcojocaru@bitdefender.com> 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@zentific.com> >>> --- >>> Cc: Paul Durrant <paul.durrant@citrix.com> >>> Cc: Jan Beulich <jbeulich@suse.com> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>> Cc: Jun Nakajima <jun.nakajima@intel.com> >>> Cc: Kevin Tian <kevin.tian@intel.com> >>> Cc: George Dunlap <george.dunlap@eu.citrix.com> >>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Julien Grall <julien.grall@arm.com> >>> >>> 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
On 09/15/16 20:08, Razvan Cojocaru wrote: > On 09/15/16 19:36, Tamas K Lengyel wrote: >> On Wed, Sep 14, 2016 at 1:58 AM, Razvan Cojocaru >> <rcojocaru@bitdefender.com> 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@zentific.com> >>>> --- >>>> Cc: Paul Durrant <paul.durrant@citrix.com> >>>> Cc: Jan Beulich <jbeulich@suse.com> >>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Cc: Jun Nakajima <jun.nakajima@intel.com> >>>> Cc: Kevin Tian <kevin.tian@intel.com> >>>> Cc: George Dunlap <george.dunlap@eu.citrix.com> >>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>> Cc: Julien Grall <julien.grall@arm.com> >>>> >>>> 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. On second thought (and a night's rest), the problem is real. I've made it a point to test the patches today before re-reviewing them, and this happened: bdvmixeneventmanager.cpp:359:46: error: ‘union vm_event_st::<anonymous>’ has no member named ‘emul_read_data’ uint32_t rspDataSize = sizeof( rsp.data.emul_read_data.data ); ^ bdvmixeneventmanager.cpp:386:44: error: ‘union vm_event_st::<anonymous>’ has no member named ‘emul_read_data’ action, rsp.data.emul_read_data.data, rspDataSize, ^ bdvmixeneventmanager.cpp:389:16: error: ‘union vm_event_st::<anonymous>’ has no member named ‘emul_read_data’ rsp.data.emul_read_data.size = rspDataSize; ^ make: *** [bdvmixeneventmanager.o] Error 1 So, as you can see, existing clients really don't compile without modification. Thanks, Razvan
On Fri, Sep 16, 2016 at 1:21 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 09/15/16 20:08, Razvan Cojocaru wrote: >> On 09/15/16 19:36, Tamas K Lengyel wrote: >>> On Wed, Sep 14, 2016 at 1:58 AM, Razvan Cojocaru >>> <rcojocaru@bitdefender.com> 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@zentific.com> >>>>> --- >>>>> Cc: Paul Durrant <paul.durrant@citrix.com> >>>>> Cc: Jan Beulich <jbeulich@suse.com> >>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Cc: Jun Nakajima <jun.nakajima@intel.com> >>>>> Cc: Kevin Tian <kevin.tian@intel.com> >>>>> Cc: George Dunlap <george.dunlap@eu.citrix.com> >>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >>>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>>> Cc: Julien Grall <julien.grall@arm.com> >>>>> >>>>> 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. > > On second thought (and a night's rest), the problem is real. I've made > it a point to test the patches today before re-reviewing them, and this > happened: > > bdvmixeneventmanager.cpp:359:46: error: ‘union vm_event_st::<anonymous>’ > has no member named ‘emul_read_data’ > uint32_t rspDataSize = sizeof( rsp.data.emul_read_data.data ); > ^ > bdvmixeneventmanager.cpp:386:44: error: ‘union vm_event_st::<anonymous>’ > has no member named ‘emul_read_data’ > action, rsp.data.emul_read_data.data, > rspDataSize, > ^ > bdvmixeneventmanager.cpp:389:16: error: ‘union vm_event_st::<anonymous>’ > has no member named ‘emul_read_data’ > rsp.data.emul_read_data.size = rspDataSize; > ^ > make: *** [bdvmixeneventmanager.o] Error 1 > > So, as you can see, existing clients really don't compile without > modification. > Hi Razvan, yes, for Xen 4.8 we already bumped the VM_EVENT_INTERFACE_VERSION, so any client building on it need to adapt accordingly. That's why we have the versioning in place. The way we handle this in LibVMI is by defining a local copy of all supported Xen events ABI versions and build with that rather then with xen/vm_event.h so we have backwards compatibility. See https://github.com/libvmi/libvmi/blob/master/libvmi/driver/xen/xen_events_abi.h. Cheers, Tamas
On 09/16/16 18:37, Tamas K Lengyel wrote: >>>>> 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. >> > >> > On second thought (and a night's rest), the problem is real. I've made >> > it a point to test the patches today before re-reviewing them, and this >> > happened: >> > >> > bdvmixeneventmanager.cpp:359:46: error: ‘union vm_event_st::<anonymous>’ >> > has no member named ‘emul_read_data’ >> > uint32_t rspDataSize = sizeof( rsp.data.emul_read_data.data ); >> > ^ >> > bdvmixeneventmanager.cpp:386:44: error: ‘union vm_event_st::<anonymous>’ >> > has no member named ‘emul_read_data’ >> > action, rsp.data.emul_read_data.data, >> > rspDataSize, >> > ^ >> > bdvmixeneventmanager.cpp:389:16: error: ‘union vm_event_st::<anonymous>’ >> > has no member named ‘emul_read_data’ >> > rsp.data.emul_read_data.size = rspDataSize; >> > ^ >> > make: *** [bdvmixeneventmanager.o] Error 1 >> > >> > So, as you can see, existing clients really don't compile without >> > modification. >> > > Hi Razvan, > yes, for Xen 4.8 we already bumped the VM_EVENT_INTERFACE_VERSION, so > any client building on it need to adapt accordingly. Fair enough. That's all I was asking for / about: that we should make sure that VM_EVENT_INTERFACE_VERSION reflects these changes. If it's already been incremented for this pending release, that's perfect. Thanks, Razvan
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 }; 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..a672784 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -27,7 +27,8 @@ */ struct arch_vm_event { uint32_t emulate_flags; - struct vm_event_emul_read_data emul_read_data; + struct vm_event_emul_read_data emul_read; + struct vm_event_emul_insn_data emul_insn; struct monitor_write_data write_data; }; diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index f756126..ef62932 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -97,6 +97,13 @@ * 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 is incompatible with also setting + * VM_EVENT_FLAG_EMULATE_NOWRITE or VM_EVENT_FLAG_SET_EMUL_READ_DATA. + */ +#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9) /* * Reasons for the vm event request @@ -265,6 +272,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 +302,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;
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@zentific.com> --- Cc: Paul Durrant <paul.durrant@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> 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(-)