[PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Tamas K Lengyel]

Currently the registers saved in the request depend on which type of event
is filling in the registers. In this patch we consolidate the two versions
of register filling function as to return a fix set of registers irrespective
of the underlying event.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/hvm/event.c       | 35 ++-----------------------
 xen/arch/x86/mm/p2m.c          | 57 +----------------------------------------
 xen/arch/x86/vm_event.c        | 58 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/vm_event.h |  2 ++
 4 files changed, 63 insertions(+), 89 deletions(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index a3d4892..e84d44b 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -23,40 +23,9 @@
 #include <asm/hvm/event.h>
 #include <asm/monitor.h>
 #include <asm/altp2m.h>
+#include <asm/vm_event.h>
 #include <public/vm_event.h>
 
-static void hvm_event_fill_regs(vm_event_request_t *req)
-{
-    const struct cpu_user_regs *regs = guest_cpu_user_regs();
-    const 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.msr_efer = curr->arch.hvm_vcpu.guest_efer;
-    req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0];
-    req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3];
-    req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4];
-}
-
 static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
 {
     int rc;
@@ -90,7 +59,7 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
         req->altp2m_idx = vcpu_altp2m(curr).p2midx;
     }
 
-    hvm_event_fill_regs(req);
+    vm_event_fill_regs(req);
     vm_event_put_request(currd, &currd->vm_event->monitor, req);
 
     return 1;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a45ee35..45c6caa 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1507,61 +1507,6 @@ void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
     }
 }
 
-static void p2m_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;
-
-    /* Architecture-specific vmcs/vmcb bits */
-    hvm_funcs.save_cpu_ctxt(curr, &ctxt);
-
-    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];
-    req->data.regs.x86.cr0 = ctxt.cr0;
-    req->data.regs.x86.cr2 = ctxt.cr2;
-    req->data.regs.x86.cr3 = ctxt.cr3;
-    req->data.regs.x86.cr4 = ctxt.cr4;
-
-    req->data.regs.x86.sysenter_cs = ctxt.sysenter_cs;
-    req->data.regs.x86.sysenter_esp = ctxt.sysenter_esp;
-    req->data.regs.x86.sysenter_eip = ctxt.sysenter_eip;
-
-    req->data.regs.x86.msr_efer = ctxt.msr_efer;
-    req->data.regs.x86.msr_star = ctxt.msr_star;
-    req->data.regs.x86.msr_lstar = ctxt.msr_lstar;
-
-    hvm_get_segment_register(curr, x86_seg_fs, &seg);
-    req->data.regs.x86.fs_base = seg.base;
-
-    hvm_get_segment_register(curr, x86_seg_gs, &seg);
-    req->data.regs.x86.gs_base = seg.base;
-
-    hvm_get_segment_register(curr, x86_seg_cs, &seg);
-    req->data.regs.x86.cs_arbytes = seg.attr.bytes;
-}
-
 void p2m_mem_access_emulate_check(struct vcpu *v,
                                   const vm_event_response_t *rsp)
 {
@@ -1760,7 +1705,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
         req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
         req->vcpu_id = v->vcpu_id;
 
-        p2m_vm_event_fill_regs(req);
+        vm_event_fill_regs(req);
 
         if ( altp2m_active(v->domain) )
         {
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];
+
+    if ( !is_hvm_domain(curr->domain) )
+        return;
+
+    /* Architecture-specific vmcs/vmcb bits */
+    hvm_funcs.save_cpu_ctxt(curr, &ctxt);
+
+    req->data.regs.x86.cr0 = ctxt.cr0;
+    req->data.regs.x86.cr2 = ctxt.cr2;
+    req->data.regs.x86.cr3 = ctxt.cr3;
+    req->data.regs.x86.cr4 = ctxt.cr4;
+
+    req->data.regs.x86.sysenter_cs = ctxt.sysenter_cs;
+    req->data.regs.x86.sysenter_esp = ctxt.sysenter_esp;
+    req->data.regs.x86.sysenter_eip = ctxt.sysenter_eip;
+
+    req->data.regs.x86.msr_efer = ctxt.msr_efer;
+    req->data.regs.x86.msr_star = ctxt.msr_star;
+    req->data.regs.x86.msr_lstar = ctxt.msr_lstar;
+
+    hvm_get_segment_register(curr, x86_seg_fs, &seg);
+    req->data.regs.x86.fs_base = seg.base;
+
+    hvm_get_segment_register(curr, x86_seg_gs, &seg);
+    req->data.regs.x86.gs_base = seg.base;
+
+    hvm_get_segment_register(curr, x86_seg_cs, &seg);
+    req->data.regs.x86.cs_arbytes = seg.attr.bytes;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 5aff834..e81148d 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -44,4 +44,6 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
 void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
 
+void vm_event_fill_regs(vm_event_request_t *req);
+
 #endif /* __ASM_X86_VM_EVENT_H__ */
-- 
2.1.4

Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Andrew Cooper]

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)

Normally, the vcpu context switch code takes care of calling
{vmx,svm}_{save,restore}_dr(), which is why migration works fine (not
permitted in the context of the guest), you are now introducing a new path.

I think you need to modify {vmx,svm}_{vmcb,vmcs}_save() to do a
selective {vmx,svm}_save_dr() if v == current.

~Andrew

Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Razvan Cojocaru]

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.


Thanks,
Razvan

Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Andrew Cooper]

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

Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Lengyel, Tamas]

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

On Thu, Feb 11, 2016 at 2:58 PM, Andrew Cooper <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. It does look
like dr7 will need a separate hvm function we can call to do a __vmread for
us on GUEST_DR7.

Tamas

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

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

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

Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Andrew Cooper]

[-- 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

Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Lengyel, Tamas]

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

On Thu, Feb 11, 2016 at 3:30 PM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 11/02/2016 22:25, Lengyel, Tamas wrote:
>
>
>
> On Thu, Feb 11, 2016 at 2:58 PM, Andrew Cooper <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.
>

Sure, adding an extra vmread in there would be simple enough.

Tamas

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

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

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