| Message ID | 20160915165120.29753-2-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/15/16 19:51, Tamas K Lengyel wrote: > When emulating instructions Xen's emulator maintains a small i-cache fetched > from the guest memory. This patch extends the vm_event interface to allow > overwriting this i-cache via a buffer returned in the vm_event response. > > When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber > normally has to remove the INT3 from memory - singlestep - place back INT3 > to allow the guest to continue execution. This routine however is susceptible > to a race-condition on multi-vCPU guests. By allowing the subscriber to return > the i-cache to be used for emulation it can side-step the problem by returning > a clean buffer without the INT3 present. > > As part of this patch we rename hvm_mem_access_emulate_one to > hvm_emulate_one_vm_event to better reflect that it is used in various vm_event > scenarios now, not just in response to mem_access events. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@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> > > v2: rework hvm_mem_access_emulate_one switch statement > add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match > > Note: this patch has now been fully tested and works as intended Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> On a side note, I see that you're using an email address that's different from the one in MAINTAINERS. Should we update the MAINTAINERS file? Thanks, Razvan
On Sat, Sep 17, 2016 at 10:40 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 09/15/16 19:51, Tamas K Lengyel wrote: >> When emulating instructions Xen's emulator maintains a small i-cache fetched >> from the guest memory. This patch extends the vm_event interface to allow >> overwriting this i-cache via a buffer returned in the vm_event response. >> >> When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber >> normally has to remove the INT3 from memory - singlestep - place back INT3 >> to allow the guest to continue execution. This routine however is susceptible >> to a race-condition on multi-vCPU guests. By allowing the subscriber to return >> the i-cache to be used for emulation it can side-step the problem by returning >> a clean buffer without the INT3 present. >> >> As part of this patch we rename hvm_mem_access_emulate_one to >> hvm_emulate_one_vm_event to better reflect that it is used in various vm_event >> scenarios now, not just in response to mem_access events. >> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@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> >> >> v2: rework hvm_mem_access_emulate_one switch statement >> add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match >> >> Note: this patch has now been fully tested and works as intended > > Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > On a side note, I see that you're using an email address that's > different from the one in MAINTAINERS. Should we update the MAINTAINERS > file? It's fine for now (both go to the same place at the end anyway). Tamas
>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: > @@ -1793,7 +1793,17 @@ 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 ) > + { > + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == > + sizeof(curr->arch.vm_event->emul.insn)); This should quite clearly be !=, and I think it builds only because you use the wrong operand in the first sizeof(). > + 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); This memcpy()s between dissimilar types. Please omit the & and properly add .data on the second argument (and this .data addition should then also be mirrored in the BUILD_BUG_ON()). > + } > + else if ( !vio->mmio_insn_bytes ) And then - I'm sorry for not having thought of this before - I think this would better not live here, or have an effect more explicitly only when coming here through hvm_emulate_one_vm_event(). Since the former seems impractical, I think giving _hvm_emulate_one() one or two extra parameters would be the most straightforward approach. > @@ -1944,11 +1954,11 @@ 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: > + ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA); > + ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN); > rc = hvm_emulate_one(&ctx); > + break; > } Thanks - this is a lot better readable now! > @@ -1983,7 +1993,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; I think you can almost guess the comment here and on the declaration of these fields: Please use false here and plain bool there. > --- 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; The header talking about incompatibilities between these flags - wouldn't it be a good idea to actually -EINVAL such combinations? > --- 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> I have to admit that looking through the header changes you do I can't figure why this adjustment is necessary. > --- 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: Blank lines between non-fall-through case statements please (part of me thinks I've said so before). > --- 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 */ Stray change. Jan
On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >> @@ -1793,7 +1793,17 @@ 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 ) >> + { >> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >> + sizeof(curr->arch.vm_event->emul.insn)); > > This should quite clearly be !=, and I think it builds only because you > use the wrong operand in the first sizeof(). Yea.. I don't know what I was thinking =) > >> + 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); > > This memcpy()s between dissimilar types. Please omit the & and > properly add .data on the second argument (and this .data > addition should then also be mirrored in the BUILD_BUG_ON()). Ack. > >> + } >> + else if ( !vio->mmio_insn_bytes ) > > And then - I'm sorry for not having thought of this before - I think > this would better not live here, or have an effect more explicitly > only when coming here through hvm_emulate_one_vm_event(). > Since the former seems impractical, I think giving _hvm_emulate_one() > one or two extra parameters would be the most straightforward > approach. > >> @@ -1944,11 +1954,11 @@ 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: >> + ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA); >> + ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN); >> rc = hvm_emulate_one(&ctx); >> + break; >> } > > Thanks - this is a lot better readable now! > >> @@ -1983,7 +1993,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; > > I think you can almost guess the comment here and on the declaration > of these fields: Please use false here and plain bool there. > >> --- 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; > > The header talking about incompatibilities between these flags - > wouldn't it be a good idea to actually -EINVAL such combinations? The header just points out that setting all these flags at the same time won't have a "combined" effect - evident from the if-else treatment above. There is really no point to do an error, the error would never reach the user. Setting response flags through vm_event doesn't have an error-path back. >> --- 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> > > I have to admit that looking through the header changes you do I > can't figure why this adjustment is necessary. Yea, it's just a residue (the patch was first made when this header was still included). > >> --- 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: > > Blank lines between non-fall-through case statements please (part > of me thinks I've said so before). Ack. > >> --- 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 */ > > Stray change. > > Jan Thanks! Tamas
On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >> @@ -1793,7 +1793,17 @@ 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 ) >> + { >> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >> + sizeof(curr->arch.vm_event->emul.insn)); > > This should quite clearly be !=, and I think it builds only because you > use the wrong operand in the first sizeof(). > >> + 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); > > This memcpy()s between dissimilar types. Please omit the & and > properly add .data on the second argument (and this .data > addition should then also be mirrored in the BUILD_BUG_ON()). > >> + } >> + else if ( !vio->mmio_insn_bytes ) > > And then - I'm sorry for not having thought of this before - I think > this would better not live here, or have an effect more explicitly > only when coming here through hvm_emulate_one_vm_event(). > Since the former seems impractical, I think giving _hvm_emulate_one() > one or two extra parameters would be the most straightforward > approach. So this is the spot where the mmio insn buffer is getting copied as well instead of fetching the instructions from the guest memory. So having the vm_event buffer getting copied here too makes the most sense. Having the vm_event insn buffer getting copied in somewhere else, while the mmio insn buffer getting copied here, IMHO just fragments the flow even more making it harder to see what is actually happening. How about adjusting the if-else here to be: if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) ... else if ( vio->mmio_insn_bytes ) ... else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) ? Tamas
> From: Tamas K Lengyel > Sent: Tuesday, September 20, 2016 12:40 AM > > >> --- 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; > > > > The header talking about incompatibilities between these flags - > > wouldn't it be a good idea to actually -EINVAL such combinations? > > The header just points out that setting all these flags at the same > time won't have a "combined" effect - evident from the if-else > treatment above. There is really no point to do an error, the error > would never reach the user. Setting response flags through vm_event > doesn't have an error-path back. > Maybe you can have an explicit comment on priority of those flags, so consistent behavior can be expected moving forward. e.g. above code implies read_data>nowrite>insn_data. w/o a proper guidance here, future code changes by others may break that sequence... Thanks Kevin
>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: > On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>> @@ -1793,7 +1793,17 @@ 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 ) >>> + { >>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>> + sizeof(curr->arch.vm_event->emul.insn)); >> >> This should quite clearly be !=, and I think it builds only because you >> use the wrong operand in the first sizeof(). >> >>> + 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); >> >> This memcpy()s between dissimilar types. Please omit the & and >> properly add .data on the second argument (and this .data >> addition should then also be mirrored in the BUILD_BUG_ON()). >> >>> + } >>> + else if ( !vio->mmio_insn_bytes ) >> >> And then - I'm sorry for not having thought of this before - I think >> this would better not live here, or have an effect more explicitly >> only when coming here through hvm_emulate_one_vm_event(). >> Since the former seems impractical, I think giving _hvm_emulate_one() >> one or two extra parameters would be the most straightforward >> approach. > > So this is the spot where the mmio insn buffer is getting copied as > well instead of fetching the instructions from the guest memory. So > having the vm_event buffer getting copied here too makes the most > sense. Having the vm_event insn buffer getting copied in somewhere > else, while the mmio insn buffer getting copied here, IMHO just > fragments the flow even more making it harder to see what is actually > happening. And I didn't unconditionally ask to move the copying elsewhere. The alternative - passing the override in as function argument(s), which would then be NULL/zero for all cases except the VM event one, would be as suitable. It is in particular ... > How about adjusting the if-else here to be: > > if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) > ... > else if ( vio->mmio_insn_bytes ) > ... > else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) ... this curr->arch.vm_event reference which I'd like to see gone from this specific code path. The ordering in your original patch, otoh, would then be fine (check for the override first with unlikely(), else do what is being done today). Such a code structure would then also ease a possible second way of overriding the insn by some other party, without having to touch the code here again. Jan
On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>> @@ -1793,7 +1793,17 @@ 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 ) >>>> + { >>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>> + sizeof(curr->arch.vm_event->emul.insn)); >>> >>> This should quite clearly be !=, and I think it builds only because you >>> use the wrong operand in the first sizeof(). >>> >>>> + 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); >>> >>> This memcpy()s between dissimilar types. Please omit the & and >>> properly add .data on the second argument (and this .data >>> addition should then also be mirrored in the BUILD_BUG_ON()). >>> >>>> + } >>>> + else if ( !vio->mmio_insn_bytes ) >>> >>> And then - I'm sorry for not having thought of this before - I think >>> this would better not live here, or have an effect more explicitly >>> only when coming here through hvm_emulate_one_vm_event(). >>> Since the former seems impractical, I think giving _hvm_emulate_one() >>> one or two extra parameters would be the most straightforward >>> approach. >> >> So this is the spot where the mmio insn buffer is getting copied as >> well instead of fetching the instructions from the guest memory. So >> having the vm_event buffer getting copied here too makes the most >> sense. Having the vm_event insn buffer getting copied in somewhere >> else, while the mmio insn buffer getting copied here, IMHO just >> fragments the flow even more making it harder to see what is actually >> happening. > > And I didn't unconditionally ask to move the copying elsewhere. > The alternative - passing the override in as function argument(s), > which would then be NULL/zero for all cases except the VM event > one, would be as suitable. It is in particular ... > >> How about adjusting the if-else here to be: >> >> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >> ... >> else if ( vio->mmio_insn_bytes ) >> ... >> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) > > ... this curr->arch.vm_event reference which I'd like to see gone > from this specific code path. The ordering in your original patch, > otoh, would then be fine (check for the override first with unlikely(), > else do what is being done today). Such a code structure would > then also ease a possible second way of overriding the insn by > some other party, without having to touch the code here again. > So that check is one that Razvan asked to be added. I think it is necessary too as there seems to be a race-condition if vm_event gets shutdown after the response flag is set but before this emulation path takes place. Effectively set_context_insn may be set but the arch.vm_event already gotten freed. Razvan, is that correct? Tamas
On 09/20/2016 05:56 PM, Tamas K Lengyel wrote: > On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>>> @@ -1793,7 +1793,17 @@ 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 ) >>>>> + { >>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>>> + sizeof(curr->arch.vm_event->emul.insn)); >>>> >>>> This should quite clearly be !=, and I think it builds only because you >>>> use the wrong operand in the first sizeof(). >>>> >>>>> + 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); >>>> >>>> This memcpy()s between dissimilar types. Please omit the & and >>>> properly add .data on the second argument (and this .data >>>> addition should then also be mirrored in the BUILD_BUG_ON()). >>>> >>>>> + } >>>>> + else if ( !vio->mmio_insn_bytes ) >>>> >>>> And then - I'm sorry for not having thought of this before - I think >>>> this would better not live here, or have an effect more explicitly >>>> only when coming here through hvm_emulate_one_vm_event(). >>>> Since the former seems impractical, I think giving _hvm_emulate_one() >>>> one or two extra parameters would be the most straightforward >>>> approach. >>> >>> So this is the spot where the mmio insn buffer is getting copied as >>> well instead of fetching the instructions from the guest memory. So >>> having the vm_event buffer getting copied here too makes the most >>> sense. Having the vm_event insn buffer getting copied in somewhere >>> else, while the mmio insn buffer getting copied here, IMHO just >>> fragments the flow even more making it harder to see what is actually >>> happening. >> >> And I didn't unconditionally ask to move the copying elsewhere. >> The alternative - passing the override in as function argument(s), >> which would then be NULL/zero for all cases except the VM event >> one, would be as suitable. It is in particular ... >> >>> How about adjusting the if-else here to be: >>> >>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >>> ... >>> else if ( vio->mmio_insn_bytes ) >>> ... >>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) >> >> ... this curr->arch.vm_event reference which I'd like to see gone >> from this specific code path. The ordering in your original patch, >> otoh, would then be fine (check for the override first with unlikely(), >> else do what is being done today). Such a code structure would >> then also ease a possible second way of overriding the insn by >> some other party, without having to touch the code here again. >> > > So that check is one that Razvan asked to be added. I think it is > necessary too as there seems to be a race-condition if vm_event gets > shutdown after the response flag is set but before this emulation path > takes place. Effectively set_context_insn may be set but the > arch.vm_event already gotten freed. Razvan, is that correct? Yes, that's the general idea, but there's also already a check in hvm_do_resume() right before calling hvm_mem_access_emulate_one(), so as long as that's the only code path leading here it should be alright to remove the check. The check adds extra safety but it's not strictly necessary here, so if Jan prefers it taken out it should, theoretically, be fine for now. Thanks, Razvan
>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote: > On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>>> @@ -1793,7 +1793,17 @@ 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 ) >>>>> + { >>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>>> + sizeof(curr->arch.vm_event->emul.insn)); >>>> >>>> This should quite clearly be !=, and I think it builds only because you >>>> use the wrong operand in the first sizeof(). >>>> >>>>> + 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); >>>> >>>> This memcpy()s between dissimilar types. Please omit the & and >>>> properly add .data on the second argument (and this .data >>>> addition should then also be mirrored in the BUILD_BUG_ON()). >>>> >>>>> + } >>>>> + else if ( !vio->mmio_insn_bytes ) >>>> >>>> And then - I'm sorry for not having thought of this before - I think >>>> this would better not live here, or have an effect more explicitly >>>> only when coming here through hvm_emulate_one_vm_event(). >>>> Since the former seems impractical, I think giving _hvm_emulate_one() >>>> one or two extra parameters would be the most straightforward >>>> approach. >>> >>> So this is the spot where the mmio insn buffer is getting copied as >>> well instead of fetching the instructions from the guest memory. So >>> having the vm_event buffer getting copied here too makes the most >>> sense. Having the vm_event insn buffer getting copied in somewhere >>> else, while the mmio insn buffer getting copied here, IMHO just >>> fragments the flow even more making it harder to see what is actually >>> happening. >> >> And I didn't unconditionally ask to move the copying elsewhere. >> The alternative - passing the override in as function argument(s), >> which would then be NULL/zero for all cases except the VM event >> one, would be as suitable. It is in particular ... >> >>> How about adjusting the if-else here to be: >>> >>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >>> ... >>> else if ( vio->mmio_insn_bytes ) >>> ... >>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) >> >> ... this curr->arch.vm_event reference which I'd like to see gone >> from this specific code path. The ordering in your original patch, >> otoh, would then be fine (check for the override first with unlikely(), >> else do what is being done today). Such a code structure would >> then also ease a possible second way of overriding the insn by >> some other party, without having to touch the code here again. > > So that check is one that Razvan asked to be added. I think it is > necessary too as there seems to be a race-condition if vm_event gets > shutdown after the response flag is set but before this emulation path > takes place. Effectively set_context_insn may be set but the > arch.vm_event already gotten freed. Razvan, is that correct? Well, in case you misunderstood: I didn't ask for the check to be _removed_, but for it to be _moved elsewhere_. Jan
On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote: >> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>>>> @@ -1793,7 +1793,17 @@ 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 ) >>>>>> + { >>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>>>> + sizeof(curr->arch.vm_event->emul.insn)); >>>>> >>>>> This should quite clearly be !=, and I think it builds only because you >>>>> use the wrong operand in the first sizeof(). >>>>> >>>>>> + 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); >>>>> >>>>> This memcpy()s between dissimilar types. Please omit the & and >>>>> properly add .data on the second argument (and this .data >>>>> addition should then also be mirrored in the BUILD_BUG_ON()). >>>>> >>>>>> + } >>>>>> + else if ( !vio->mmio_insn_bytes ) >>>>> >>>>> And then - I'm sorry for not having thought of this before - I think >>>>> this would better not live here, or have an effect more explicitly >>>>> only when coming here through hvm_emulate_one_vm_event(). >>>>> Since the former seems impractical, I think giving _hvm_emulate_one() >>>>> one or two extra parameters would be the most straightforward >>>>> approach. >>>> >>>> So this is the spot where the mmio insn buffer is getting copied as >>>> well instead of fetching the instructions from the guest memory. So >>>> having the vm_event buffer getting copied here too makes the most >>>> sense. Having the vm_event insn buffer getting copied in somewhere >>>> else, while the mmio insn buffer getting copied here, IMHO just >>>> fragments the flow even more making it harder to see what is actually >>>> happening. >>> >>> And I didn't unconditionally ask to move the copying elsewhere. >>> The alternative - passing the override in as function argument(s), >>> which would then be NULL/zero for all cases except the VM event >>> one, would be as suitable. It is in particular ... >>> >>>> How about adjusting the if-else here to be: >>>> >>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >>>> ... >>>> else if ( vio->mmio_insn_bytes ) >>>> ... >>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) >>> >>> ... this curr->arch.vm_event reference which I'd like to see gone >>> from this specific code path. The ordering in your original patch, >>> otoh, would then be fine (check for the override first with unlikely(), >>> else do what is being done today). Such a code structure would >>> then also ease a possible second way of overriding the insn by >>> some other party, without having to touch the code here again. >> >> So that check is one that Razvan asked to be added. I think it is >> necessary too as there seems to be a race-condition if vm_event gets >> shutdown after the response flag is set but before this emulation path >> takes place. Effectively set_context_insn may be set but the >> arch.vm_event already gotten freed. Razvan, is that correct? > > Well, in case you misunderstood: I didn't ask for the check to be > _removed_, but for it to be _moved elsewhere_. > So as Razvan pointed out, there is a check already in hvm_do_resume for exactly the same effect, so then what you are asking for is already done. Tamas
On Mon, Sep 19, 2016 at 6:36 PM, Tian, Kevin <kevin.tian@intel.com> wrote: >> From: Tamas K Lengyel >> Sent: Tuesday, September 20, 2016 12:40 AM >> >> >> --- 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; >> > >> > The header talking about incompatibilities between these flags - >> > wouldn't it be a good idea to actually -EINVAL such combinations? >> >> The header just points out that setting all these flags at the same >> time won't have a "combined" effect - evident from the if-else >> treatment above. There is really no point to do an error, the error >> would never reach the user. Setting response flags through vm_event >> doesn't have an error-path back. >> > > Maybe you can have an explicit comment on priority of those flags, > so consistent behavior can be expected moving forward. e.g. above > code implies read_data>nowrite>insn_data. w/o a proper guidance > here, future code changes by others may break that sequence... > Fair point, will do that. Thanks, Tamas
>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote: > On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote: >>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>>>>> @@ -1793,7 +1793,17 @@ 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 ) >>>>>>> + { >>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>>>>> + sizeof(curr->arch.vm_event->emul.insn)); >>>>>> >>>>>> This should quite clearly be !=, and I think it builds only because you >>>>>> use the wrong operand in the first sizeof(). >>>>>> >>>>>>> + 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); >>>>>> >>>>>> This memcpy()s between dissimilar types. Please omit the & and >>>>>> properly add .data on the second argument (and this .data >>>>>> addition should then also be mirrored in the BUILD_BUG_ON()). >>>>>> >>>>>>> + } >>>>>>> + else if ( !vio->mmio_insn_bytes ) >>>>>> >>>>>> And then - I'm sorry for not having thought of this before - I think >>>>>> this would better not live here, or have an effect more explicitly >>>>>> only when coming here through hvm_emulate_one_vm_event(). >>>>>> Since the former seems impractical, I think giving _hvm_emulate_one() >>>>>> one or two extra parameters would be the most straightforward >>>>>> approach. >>>>> >>>>> So this is the spot where the mmio insn buffer is getting copied as >>>>> well instead of fetching the instructions from the guest memory. So >>>>> having the vm_event buffer getting copied here too makes the most >>>>> sense. Having the vm_event insn buffer getting copied in somewhere >>>>> else, while the mmio insn buffer getting copied here, IMHO just >>>>> fragments the flow even more making it harder to see what is actually >>>>> happening. >>>> >>>> And I didn't unconditionally ask to move the copying elsewhere. >>>> The alternative - passing the override in as function argument(s), >>>> which would then be NULL/zero for all cases except the VM event >>>> one, would be as suitable. It is in particular ... >>>> >>>>> How about adjusting the if-else here to be: >>>>> >>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >>>>> ... >>>>> else if ( vio->mmio_insn_bytes ) >>>>> ... >>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) >>>> >>>> ... this curr->arch.vm_event reference which I'd like to see gone >>>> from this specific code path. The ordering in your original patch, >>>> otoh, would then be fine (check for the override first with unlikely(), >>>> else do what is being done today). Such a code structure would >>>> then also ease a possible second way of overriding the insn by >>>> some other party, without having to touch the code here again. >>> >>> So that check is one that Razvan asked to be added. I think it is >>> necessary too as there seems to be a race-condition if vm_event gets >>> shutdown after the response flag is set but before this emulation path >>> takes place. Effectively set_context_insn may be set but the >>> arch.vm_event already gotten freed. Razvan, is that correct? >> >> Well, in case you misunderstood: I didn't ask for the check to be >> _removed_, but for it to be _moved elsewhere_. >> > > So as Razvan pointed out, there is a check already in hvm_do_resume > for exactly the same effect, so then what you are asking for is > already done. Partly - I really meant all curr->arch.vm_event uses to go away from that path. The other part (passing in the override buffer instead of special casing vm-event handling here) still would need to be done. Jan
On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote: >> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote: >>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >>>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>>>>>> @@ -1793,7 +1793,17 @@ 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 ) >>>>>>>> + { >>>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>>>>>> + sizeof(curr->arch.vm_event->emul.insn)); >>>>>>> >>>>>>> This should quite clearly be !=, and I think it builds only because you >>>>>>> use the wrong operand in the first sizeof(). >>>>>>> >>>>>>>> + 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); >>>>>>> >>>>>>> This memcpy()s between dissimilar types. Please omit the & and >>>>>>> properly add .data on the second argument (and this .data >>>>>>> addition should then also be mirrored in the BUILD_BUG_ON()). >>>>>>> >>>>>>>> + } >>>>>>>> + else if ( !vio->mmio_insn_bytes ) >>>>>>> >>>>>>> And then - I'm sorry for not having thought of this before - I think >>>>>>> this would better not live here, or have an effect more explicitly >>>>>>> only when coming here through hvm_emulate_one_vm_event(). >>>>>>> Since the former seems impractical, I think giving _hvm_emulate_one() >>>>>>> one or two extra parameters would be the most straightforward >>>>>>> approach. >>>>>> >>>>>> So this is the spot where the mmio insn buffer is getting copied as >>>>>> well instead of fetching the instructions from the guest memory. So >>>>>> having the vm_event buffer getting copied here too makes the most >>>>>> sense. Having the vm_event insn buffer getting copied in somewhere >>>>>> else, while the mmio insn buffer getting copied here, IMHO just >>>>>> fragments the flow even more making it harder to see what is actually >>>>>> happening. >>>>> >>>>> And I didn't unconditionally ask to move the copying elsewhere. >>>>> The alternative - passing the override in as function argument(s), >>>>> which would then be NULL/zero for all cases except the VM event >>>>> one, would be as suitable. It is in particular ... >>>>> >>>>>> How about adjusting the if-else here to be: >>>>>> >>>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >>>>>> ... >>>>>> else if ( vio->mmio_insn_bytes ) >>>>>> ... >>>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) >>>>> >>>>> ... this curr->arch.vm_event reference which I'd like to see gone >>>>> from this specific code path. The ordering in your original patch, >>>>> otoh, would then be fine (check for the override first with unlikely(), >>>>> else do what is being done today). Such a code structure would >>>>> then also ease a possible second way of overriding the insn by >>>>> some other party, without having to touch the code here again. >>>> >>>> So that check is one that Razvan asked to be added. I think it is >>>> necessary too as there seems to be a race-condition if vm_event gets >>>> shutdown after the response flag is set but before this emulation path >>>> takes place. Effectively set_context_insn may be set but the >>>> arch.vm_event already gotten freed. Razvan, is that correct? >>> >>> Well, in case you misunderstood: I didn't ask for the check to be >>> _removed_, but for it to be _moved elsewhere_. >>> >> >> So as Razvan pointed out, there is a check already in hvm_do_resume >> for exactly the same effect, so then what you are asking for is >> already done. > > Partly - I really meant all curr->arch.vm_event uses to go away from > that path. The other part (passing in the override buffer instead of > special casing vm-event handling here) still would need to be done. > I don't really follow what exactly you are looking for. You want the buffer to be sent in as an input? We can do that but I mean the mmio case doesn't do that either.. And what do you mean not "special casing vm_event handling"? We need to handle it in an if-statement because by default the buffer is fetched from memory. We don't want to do that, just as the mmio case doesn't want that either. So I think if we want to be consistent we do what the mmio case is doing, fetching the buffer from curr->arch.hvm_vcpu.hvm_io, only we fetch it from curr->arch.vm_event. Tamas
>>> On 20.09.16 at 17:54, <tamas.lengyel@zentific.com> wrote: > On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote: >>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote: >>>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >>>>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>>>>>>> @@ -1793,7 +1793,17 @@ 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 ) >>>>>>>>> + { >>>>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>>>>>>> + sizeof(curr->arch.vm_event->emul.insn)); >>>>>>>> >>>>>>>> This should quite clearly be !=, and I think it builds only because you >>>>>>>> use the wrong operand in the first sizeof(). >>>>>>>> >>>>>>>>> + 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); >>>>>>>> >>>>>>>> This memcpy()s between dissimilar types. Please omit the & and >>>>>>>> properly add .data on the second argument (and this .data >>>>>>>> addition should then also be mirrored in the BUILD_BUG_ON()). >>>>>>>> >>>>>>>>> + } >>>>>>>>> + else if ( !vio->mmio_insn_bytes ) >>>>>>>> >>>>>>>> And then - I'm sorry for not having thought of this before - I think >>>>>>>> this would better not live here, or have an effect more explicitly >>>>>>>> only when coming here through hvm_emulate_one_vm_event(). >>>>>>>> Since the former seems impractical, I think giving _hvm_emulate_one() >>>>>>>> one or two extra parameters would be the most straightforward >>>>>>>> approach. >>>>>>> >>>>>>> So this is the spot where the mmio insn buffer is getting copied as >>>>>>> well instead of fetching the instructions from the guest memory. So >>>>>>> having the vm_event buffer getting copied here too makes the most >>>>>>> sense. Having the vm_event insn buffer getting copied in somewhere >>>>>>> else, while the mmio insn buffer getting copied here, IMHO just >>>>>>> fragments the flow even more making it harder to see what is actually >>>>>>> happening. >>>>>> >>>>>> And I didn't unconditionally ask to move the copying elsewhere. >>>>>> The alternative - passing the override in as function argument(s), >>>>>> which would then be NULL/zero for all cases except the VM event >>>>>> one, would be as suitable. It is in particular ... >>>>>> >>>>>>> How about adjusting the if-else here to be: >>>>>>> >>>>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >>>>>>> ... >>>>>>> else if ( vio->mmio_insn_bytes ) >>>>>>> ... >>>>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) >>>>>> >>>>>> ... this curr->arch.vm_event reference which I'd like to see gone >>>>>> from this specific code path. The ordering in your original patch, >>>>>> otoh, would then be fine (check for the override first with unlikely(), >>>>>> else do what is being done today). Such a code structure would >>>>>> then also ease a possible second way of overriding the insn by >>>>>> some other party, without having to touch the code here again. >>>>> >>>>> So that check is one that Razvan asked to be added. I think it is >>>>> necessary too as there seems to be a race-condition if vm_event gets >>>>> shutdown after the response flag is set but before this emulation path >>>>> takes place. Effectively set_context_insn may be set but the >>>>> arch.vm_event already gotten freed. Razvan, is that correct? >>>> >>>> Well, in case you misunderstood: I didn't ask for the check to be >>>> _removed_, but for it to be _moved elsewhere_. >>>> >>> >>> So as Razvan pointed out, there is a check already in hvm_do_resume >>> for exactly the same effect, so then what you are asking for is >>> already done. >> >> Partly - I really meant all curr->arch.vm_event uses to go away from >> that path. The other part (passing in the override buffer instead of >> special casing vm-event handling here) still would need to be done. >> > > I don't really follow what exactly you are looking for. You want the > buffer to be sent in as an input? We can do that but I mean the mmio > case doesn't do that either.. And what do you mean not "special casing > vm_event handling"? We need to handle it in an if-statement because by > default the buffer is fetched from memory. We don't want to do that, > just as the mmio case doesn't want that either. So I think if we want > to be consistent we do what the mmio case is doing, fetching the > buffer from curr->arch.hvm_vcpu.hvm_io, only we fetch it from > curr->arch.vm_event. No. Please look back at my original reply (still visible in context above). You're comparing apples and oranges - the existing override is an integral part of the emulation logic, while yours is an add-on. And btw., see how https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00897.html even factors out that part. It might even be an option to simply copy your override data right into vio->mmio_insn{,_bytes}, in the vm-event specific function, allowing all other code to remain untouched. Jan
On Wed, Sep 21, 2016 at 3:03 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 20.09.16 at 17:54, <tamas.lengyel@zentific.com> wrote: >> On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote: >>>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote: >>>>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote: >>>>>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote: >>>>>>>>>> @@ -1793,7 +1793,17 @@ 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 ) >>>>>>>>>> + { >>>>>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == >>>>>>>>>> + sizeof(curr->arch.vm_event->emul.insn)); >>>>>>>>> >>>>>>>>> This should quite clearly be !=, and I think it builds only because you >>>>>>>>> use the wrong operand in the first sizeof(). >>>>>>>>> >>>>>>>>>> + 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); >>>>>>>>> >>>>>>>>> This memcpy()s between dissimilar types. Please omit the & and >>>>>>>>> properly add .data on the second argument (and this .data >>>>>>>>> addition should then also be mirrored in the BUILD_BUG_ON()). >>>>>>>>> >>>>>>>>>> + } >>>>>>>>>> + else if ( !vio->mmio_insn_bytes ) >>>>>>>>> >>>>>>>>> And then - I'm sorry for not having thought of this before - I think >>>>>>>>> this would better not live here, or have an effect more explicitly >>>>>>>>> only when coming here through hvm_emulate_one_vm_event(). >>>>>>>>> Since the former seems impractical, I think giving _hvm_emulate_one() >>>>>>>>> one or two extra parameters would be the most straightforward >>>>>>>>> approach. >>>>>>>> >>>>>>>> So this is the spot where the mmio insn buffer is getting copied as >>>>>>>> well instead of fetching the instructions from the guest memory. So >>>>>>>> having the vm_event buffer getting copied here too makes the most >>>>>>>> sense. Having the vm_event insn buffer getting copied in somewhere >>>>>>>> else, while the mmio insn buffer getting copied here, IMHO just >>>>>>>> fragments the flow even more making it harder to see what is actually >>>>>>>> happening. >>>>>>> >>>>>>> And I didn't unconditionally ask to move the copying elsewhere. >>>>>>> The alternative - passing the override in as function argument(s), >>>>>>> which would then be NULL/zero for all cases except the VM event >>>>>>> one, would be as suitable. It is in particular ... >>>>>>> >>>>>>>> How about adjusting the if-else here to be: >>>>>>>> >>>>>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn ) >>>>>>>> ... >>>>>>>> else if ( vio->mmio_insn_bytes ) >>>>>>>> ... >>>>>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event ) >>>>>>> >>>>>>> ... this curr->arch.vm_event reference which I'd like to see gone >>>>>>> from this specific code path. The ordering in your original patch, >>>>>>> otoh, would then be fine (check for the override first with unlikely(), >>>>>>> else do what is being done today). Such a code structure would >>>>>>> then also ease a possible second way of overriding the insn by >>>>>>> some other party, without having to touch the code here again. >>>>>> >>>>>> So that check is one that Razvan asked to be added. I think it is >>>>>> necessary too as there seems to be a race-condition if vm_event gets >>>>>> shutdown after the response flag is set but before this emulation path >>>>>> takes place. Effectively set_context_insn may be set but the >>>>>> arch.vm_event already gotten freed. Razvan, is that correct? >>>>> >>>>> Well, in case you misunderstood: I didn't ask for the check to be >>>>> _removed_, but for it to be _moved elsewhere_. >>>>> >>>> >>>> So as Razvan pointed out, there is a check already in hvm_do_resume >>>> for exactly the same effect, so then what you are asking for is >>>> already done. >>> >>> Partly - I really meant all curr->arch.vm_event uses to go away from >>> that path. The other part (passing in the override buffer instead of >>> special casing vm-event handling here) still would need to be done. >>> >> >> I don't really follow what exactly you are looking for. You want the >> buffer to be sent in as an input? We can do that but I mean the mmio >> case doesn't do that either.. And what do you mean not "special casing >> vm_event handling"? We need to handle it in an if-statement because by >> default the buffer is fetched from memory. We don't want to do that, >> just as the mmio case doesn't want that either. So I think if we want >> to be consistent we do what the mmio case is doing, fetching the >> buffer from curr->arch.hvm_vcpu.hvm_io, only we fetch it from >> curr->arch.vm_event. > > No. Please look back at my original reply (still visible in context > above). You're comparing apples and oranges - the existing override > is an integral part of the emulation logic, while yours is an add-on. > And btw., see how > https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00897.html > even factors out that part. > > It might even be an option to simply copy your override data right > into vio->mmio_insn{,_bytes}, in the vm-event specific function, > allowing all other code to remain untouched. We can do that, though that seems to be a bit hack-ish. OTOH it would not require any other code-changes, as you say, so if that's really an option, I'm fine with it. Tamas
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index cc25676..acae998 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,17 @@ 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 ) + { + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) == + sizeof(curr->arch.vm_event->emul.insn)); + + 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 +1941,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 +1954,11 @@ 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: + ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA); + ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN); rc = hvm_emulate_one(&ctx); + break; } switch ( rc ) @@ -1983,7 +1993,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..ca5d515 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..ca73f99 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -27,7 +27,10 @@ */ struct arch_vm_event { uint32_t emulate_flags; - struct vm_event_emul_read_data emul_read_data; + union { + struct vm_event_emul_read_data read; + struct vm_event_emul_insn_data insn; + } emul; struct monitor_write_data write_data; }; diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index f756126..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 Xen's emulator maintains a small i-cache fetched from the guest memory. This patch extends the vm_event interface to allow overwriting this i-cache via a buffer returned in the vm_event response. When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber normally has to remove the INT3 from memory - singlestep - place back INT3 to allow the guest to continue execution. This routine however is susceptible to a race-condition on multi-vCPU guests. By allowing the subscriber to return the i-cache to be used for emulation it can side-step the problem by returning a clean buffer without the INT3 present. As part of this patch we rename hvm_mem_access_emulate_one to hvm_emulate_one_vm_event to better reflect that it is used in various vm_event scenarios now, not just in response to mem_access events. Signed-off-by: Tamas K Lengyel <tamas.lengyel@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> v2: rework hvm_mem_access_emulate_one switch statement add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match Note: this patch has now been fully tested and works as intended --- xen/arch/x86/hvm/emulate.c | 37 ++++++++++++++++++++++++------------- 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 | 5 ++++- xen/include/public/vm_event.h | 16 +++++++++++++++- 8 files changed, 63 insertions(+), 23 deletions(-)