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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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

It might help dispel future confusion, but the commit text should
suffice for now - I won't hold the patch hostage because of this.


Thanks,
Razvan

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