Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

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

[-- Attachment #1.1: Type: text/plain, Size: 3253 bytes --]

On 11/02/2016 22:25, Lengyel, Tamas wrote:
>
>
> On Thu, Feb 11, 2016 at 2:58 PM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     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).
>
>
> This is an issue that should be addressed in a separate patch.

Agreed.

> It does look like dr7 will need a separate hvm function we can call to
> do a __vmread for us on GUEST_DR7.

It would be better to modify the existing function to do the right
thing, rather than to introduce a brand new one.  In some copious free
time, I already want to cull some of the redundant hvm_funcs.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 6768 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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