Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/02/2016 21:49, Razvan Cojocaru wrote:
> On 02/11/2016 11:35 PM, Andrew Cooper wrote:
>> On 11/02/2016 21:05, Tamas K Lengyel wrote:
>>
>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>> index 08d678a..fa5d154 100644
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -122,6 +122,64 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>>>      v->arch.user_regs.eip = rsp->data.regs.x86.rip;
>>>  }
>>>  
>>> +void vm_event_fill_regs(vm_event_request_t *req)
>>> +{
>>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>> +    struct segment_register seg;
>>> +    struct hvm_hw_cpu ctxt;
>>> +    struct vcpu *curr = current;
>>> +
>>> +    req->data.regs.x86.rax = regs->eax;
>>> +    req->data.regs.x86.rcx = regs->ecx;
>>> +    req->data.regs.x86.rdx = regs->edx;
>>> +    req->data.regs.x86.rbx = regs->ebx;
>>> +    req->data.regs.x86.rsp = regs->esp;
>>> +    req->data.regs.x86.rbp = regs->ebp;
>>> +    req->data.regs.x86.rsi = regs->esi;
>>> +    req->data.regs.x86.rdi = regs->edi;
>>> +
>>> +    req->data.regs.x86.r8  = regs->r8;
>>> +    req->data.regs.x86.r9  = regs->r9;
>>> +    req->data.regs.x86.r10 = regs->r10;
>>> +    req->data.regs.x86.r11 = regs->r11;
>>> +    req->data.regs.x86.r12 = regs->r12;
>>> +    req->data.regs.x86.r13 = regs->r13;
>>> +    req->data.regs.x86.r14 = regs->r14;
>>> +    req->data.regs.x86.r15 = regs->r15;
>>> +
>>> +    req->data.regs.x86.rflags = regs->eflags;
>>> +    req->data.regs.x86.rip    = regs->eip;
>>> +    req->data.regs.x86.dr7    = curr->arch.debugreg[7];
>> I think there is a %dr7 handling issue here.  For an HVM guests, this
>> field is only valid when you are not in the context of the guest, as it
>> lives in the vmcs/vmcs.  (PV guests keep it synchronously up to date)
> Would this make it OK to use in p2m_vm_event_fill_regs() but not in
> hvm_event_fill_regs(), as it currently is? Maybe this is the issue I'm
> remembering.

Its use in p2m_mem_access_check() looks similarly buggy.  That is also
in the context of 'current'.

I would have thought that the use of hardware debugging facilities would
be rare in the general case, which probably means that by chance, the
value is right most of the time (as it gets synchronised when a vcpu is
scheduled on a new pcpu).

~Andrew