On Sep 26, 2016 08:29, "Razvan Cojocaru" <rcojoc...@bitdefender.com> wrote:
>
> On 09/26/2016 01:33 PM, Jan Beulich wrote:
> >>>> On 22.09.16 at 20:54, <tamas.leng...@zentific.com> 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.leng...@zentific.com>
> >> Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>
> >
> > Non-VM-event specific code:
> > Reviewed-by: Jan Beulich <jbeul...@suse.com>
> >
> > One question though:
> >
> >> --- a/xen/arch/x86/vm_event.c
> >> +++ b/xen/arch/x86/vm_event.c
> >> @@ -209,11 +209,20 @@ void vm_event_emulate_check(struct vcpu *v,
vm_event_response_t *rsp)
> >> if ( p2m_mem_access_emulate_check(v, rsp) )
> >> {
> >> if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> >> - v->arch.vm_event->emul_read_data =
rsp->data.emul_read_data;
> >> + v->arch.vm_event->emul.read = rsp->data.emul.read;
> >>
> >> v->arch.vm_event->emulate_flags = rsp->flags;
> >> }
> >> break;
> >> +
> >> + case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> >> + if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> >> + {
> >> + v->arch.vm_event->emul.insn = rsp->data.emul.insn;
> >> + v->arch.vm_event->emulate_flags = rsp->flags;
> >> + }
> >> + break;
> >
> > Is this intentionally different from the case above (where the setting
> > of ->emulate_flags is outside the inner if()?
Yes, it's intentional.
> Good point. The case below should follow suit of the one above unless
> there's a corner case Tamas is aware of that I'm missing. Otherwise, a
> comment would be nice to explain the difference (perhaps for
> VM_EVENT_REASON_SOFTWARE_BREAKPOINT only
> VM_EVENT_FLAG_SET_EMUL_INSN_DATA ever makes sense - never a simple
> emulation).
>
That's exactly the case here as the commit text explains. Emulating in
response to an int3 event only makes sense if you return the insn buffer. I
can add a comment to that effect if that helps, though I think it's pretty
straight forward.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
