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 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()?

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).


Thanks,
Razvan

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