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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Sep 26, 2016 08:29, "Razvan Cojocaru" <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>
> On 09/26/2016 01:33 PM, Jan Beulich wrote:
> >>>> On 22.09.16 at 20:54, <tamas.lengyel@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >> Acked-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> >
> > Non-VM-event specific code:
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel