[PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Tamas K Lengyel]

Sending the dr7 register during vm_events is useful for various applications,
but the current way the register value is gathered is incorrent. In this patch
we extend vmx_vmcs_save so that we get the correct value.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b9f340c..ae05794 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
     __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
     __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
     __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
+    __vmread(GUEST_DR7, &c->dr7);
 
     c->pending_event = 0;
     c->error_code = 0;
-- 
2.1.4

[PATCH v2 2/2] 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>
---
v2: get dr7 from hvm context
---
 xen/arch/x86/hvm/event.c       | 35 ++-----------------------
 xen/arch/x86/mm/p2m.c          | 57 +---------------------------------------
 xen/arch/x86/vm_event.c        | 59 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/vm_event.h |  2 ++
 4 files changed, 64 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..4e71948 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -122,6 +122,65 @@ 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;
+
+    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;
+
+    req->data.regs.x86.dr7 = ctxt.dr7;
+
+    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 v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Razvan Cojocaru]

On 02/12/2016 02:22 AM, Tamas K Lengyel wrote:
> Sending the dr7 register during vm_events is useful for various applications,
> but the current way the register value is gathered is incorrent. In this patch
> we extend vmx_vmcs_save so that we get the correct value.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Razvan Cojocaru]

On 02/12/2016 02:22 AM, Tamas K Lengyel wrote:
> 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>
> ---
> v2: get dr7 from hvm context
> ---
>  xen/arch/x86/hvm/event.c       | 35 ++-----------------------
>  xen/arch/x86/mm/p2m.c          | 57 +---------------------------------------
>  xen/arch/x86/vm_event.c        | 59 ++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/vm_event.h |  2 ++
>  4 files changed, 64 insertions(+), 89 deletions(-)

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Jan Beulich]

>>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> --- 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];
> -}

With this diff I suppose the patch here is meant to replace
"vm_event: Record FS_BASE/GS_BASE during events"? Such should
be made explicit, either by adding a note here (after the first ---
separator) or by explicitly withdrawing the other patch.

Jan

Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Jan Beulich]

>>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> Sending the dr7 register during vm_events is useful for various applications,
> but the current way the register value is gathered is incorrent. In this 
> patch
> we extend vmx_vmcs_save so that we get the correct value.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>

Iirc Andrew suggested ...

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
>      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
>      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
>      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
> +    __vmread(GUEST_DR7, &c->dr7);

... just when v == current.

Jan

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Jan Beulich]

>>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -122,6 +122,65 @@ 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;
> +
> +    if ( !is_hvm_domain(curr->domain) )
> +        return;

No such check existed in either of the two original functions. Why is
it needed all of the sudden? And if it is needed, why do the other
fields not get filled (as far as possible at least) for PV guests?

Jan

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Razvan Cojocaru]

On 02/12/2016 11:57 AM, Jan Beulich wrote:
>>>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -122,6 +122,65 @@ 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;
>> +
>> +    if ( !is_hvm_domain(curr->domain) )
>> +        return;
> 
> No such check existed in either of the two original functions. Why is
> it needed all of the sudden? And if it is needed, why do the other
> fields not get filled (as far as possible at least) for PV guests?

I can't speak for Tamas, but I suspect the check has been placed there
because calls to hvm_funcs.save_cpu_ctxt(curr, &ctxt) and
hvm_get_segment_register(curr, x86_seg_fs, &seg) follow, and he's put
vm_event_fill_regs() in xen/arch/x86/vm_event.c (a previous function was
called hvm_event_fill_regs(), in arch/x86/hvm/event.c, so no checking
for HVM was needed).

I don't think the check is needed for the current codepaths, but since
the code has been moved to xen/arch/x86/ the question about future PV
events is fair.


Thanks,
Razvan

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Jan Beulich]

>>> On 12.02.16 at 11:19, <rcojocaru@bitdefender.com> wrote:
> On 02/12/2016 11:57 AM, Jan Beulich wrote:
>>>>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -122,6 +122,65 @@ 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;
>>> +
>>> +    if ( !is_hvm_domain(curr->domain) )
>>> +        return;
>> 
>> No such check existed in either of the two original functions. Why is
>> it needed all of the sudden? And if it is needed, why do the other
>> fields not get filled (as far as possible at least) for PV guests?
> 
> I can't speak for Tamas, but I suspect the check has been placed there
> because calls to hvm_funcs.save_cpu_ctxt(curr, &ctxt) and
> hvm_get_segment_register(curr, x86_seg_fs, &seg) follow, and he's put
> vm_event_fill_regs() in xen/arch/x86/vm_event.c (a previous function was
> called hvm_event_fill_regs(), in arch/x86/hvm/event.c, so no checking
> for HVM was needed).
> 
> I don't think the check is needed for the current codepaths, but since
> the code has been moved to xen/arch/x86/ the question about future PV
> events is fair.

In which case ASSERT(is_hvm_vcpu(curr)) would be the common
way to document this (at once avoiding the open coding of
is_hvm_vcpu()).

Jan

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Lengyel, Tamas]

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

On Feb 12, 2016 03:41, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 12.02.16 at 11:19, <rcojocaru@bitdefender.com> wrote:
> > On 02/12/2016 11:57 AM, Jan Beulich wrote:
> >>>>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> >>> --- a/xen/arch/x86/vm_event.c
> >>> +++ b/xen/arch/x86/vm_event.c
> >>> @@ -122,6 +122,65 @@ 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;
> >>> +
> >>> +    if ( !is_hvm_domain(curr->domain) )
> >>> +        return;
> >>
> >> No such check existed in either of the two original functions. Why is
> >> it needed all of the sudden? And if it is needed, why do the other
> >> fields not get filled (as far as possible at least) for PV guests?
> >
> > I can't speak for Tamas, but I suspect the check has been placed there
> > because calls to hvm_funcs.save_cpu_ctxt(curr, &ctxt) and
> > hvm_get_segment_register(curr, x86_seg_fs, &seg) follow, and he's put
> > vm_event_fill_regs() in xen/arch/x86/vm_event.c (a previous function was
> > called hvm_event_fill_regs(), in arch/x86/hvm/event.c, so no checking
> > for HVM was needed).
> >
> > I don't think the check is needed for the current codepaths, but since
> > the code has been moved to xen/arch/x86/ the question about future PV
> > events is fair.

That was the idea.

>
> In which case ASSERT(is_hvm_vcpu(curr)) would be the common
> way to document this (at once avoiding the open coding of
> is_hvm_vcpu()).
>

I don't think we need an assert here. The function is fine for pv guests as
well up to that point. Filling in the rest of the registers for pv guests
can be done when pv events are implemented. Maybe a comment saying so is
warranted.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4229 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 v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Lengyel, Tamas]

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

On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> > Sending the dr7 register during vm_events is useful for various
applications,
> > but the current way the register value is gathered is incorrent. In this
> > patch
> > we extend vmx_vmcs_save so that we get the correct value.
> >
> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Iirc Andrew suggested ...
>
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct
hvm_hw_cpu *c)
> >      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
> >      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
> >      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
> > +    __vmread(GUEST_DR7, &c->dr7);
>
> ... just when v == current.
>

Would that check really be necessary? It would complicate the code not just
here but the caller would need to be aware too that in that case dr7 can be
aquired from someplace else. I don't see the harm in just saving dr7 here
in both cases.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1583 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 v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Jan Beulich]

>>> On 12.02.16 at 13:50, <tlengyel@novetta.com> wrote:
> On Feb 12, 2016 03:41, "Jan Beulich" <JBeulich@suse.com> wrote:
>> In which case ASSERT(is_hvm_vcpu(curr)) would be the common
>> way to document this (at once avoiding the open coding of
>> is_hvm_vcpu()).
>>
> 
> I don't think we need an assert here. The function is fine for pv guests as
> well up to that point. Filling in the rest of the registers for pv guests
> can be done when pv events are implemented. Maybe a comment saying so is
> warranted.

I disagree: Either you mean to support PV in the function, in which
case all fields should be filled, or you don't, in which case the
ASSERT() would at once document that PV is intended to not be
supported right now. With the conditional as in your patch any
future reader may either think "bug" or get confused as to the
actual intentions here.

Jan

Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Jan Beulich]

>>> On 12.02.16 at 13:57, <tlengyel@novetta.com> wrote:
> On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
>> > Sending the dr7 register during vm_events is useful for various
> applications,
>> > but the current way the register value is gathered is incorrent. In this
>> > patch
>> > we extend vmx_vmcs_save so that we get the correct value.
>> >
>> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> Iirc Andrew suggested ...
>>
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
>> >      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
>> >      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
>> >      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
>> > +    __vmread(GUEST_DR7, &c->dr7);
>>
>> ... just when v == current.
>>
> 
> Would that check really be necessary? It would complicate the code not just
> here but the caller would need to be aware too that in that case dr7 can be
> aquired from someplace else. I don't see the harm in just saving dr7 here
> in both cases.

Maybe the solution then is for the suggested if() to have an "else"?
While, as someone said elsewhere, a few more cycles may not be
noticable, why make things slower than they need to be. Plus - what
guarantees that the VMCS field isn't stale while the guest isn't running
(perhaps it got updated but not sync-ed back yet in anticipation for
this to happen during vCPU resume)?

Jan

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[George Dunlap]

On 12/02/16 00:22, Tamas K Lengyel wrote:
> 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>

This isn't p2m code per se; but insofar as it's necessary, this patch or
any future version of the patch which is just removing
p2m_vm_event_fill_regs() elsewhere and renaming it can have:

Acked-by: George Dunlap <george.dunlap@citrix.com>

for the p2m.c change

Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Lengyel, Tamas]

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

On Fri, Feb 12, 2016 at 7:57 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 12.02.16 at 13:50, <tlengyel@novetta.com> wrote:
> > On Feb 12, 2016 03:41, "Jan Beulich" <JBeulich@suse.com> wrote:
> >> In which case ASSERT(is_hvm_vcpu(curr)) would be the common
> >> way to document this (at once avoiding the open coding of
> >> is_hvm_vcpu()).
> >>
> >
> > I don't think we need an assert here. The function is fine for pv guests
> as
> > well up to that point. Filling in the rest of the registers for pv guests
> > can be done when pv events are implemented. Maybe a comment saying so is
> > warranted.
>
> I disagree: Either you mean to support PV in the function, in which
> case all fields should be filled, or you don't, in which case the
> ASSERT() would at once document that PV is intended to not be
> supported right now. With the conditional as in your patch any
> future reader may either think "bug" or get confused as to the
> actual intentions here.
>

Alright, sounds good to me.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1673 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 v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Lengyel, Tamas]

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

On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 12.02.16 at 13:57, <tlengyel@novetta.com> wrote:
> > On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
> >>
> >> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> >> > Sending the dr7 register during vm_events is useful for various
> > applications,
> >> > but the current way the register value is gathered is incorrent. In
> this
> >> > patch
> >> > we extend vmx_vmcs_save so that we get the correct value.
> >> >
> >> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>
> >> Iirc Andrew suggested ...
> >>
> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct
> hvm_hw_cpu *c)
> >> >      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
> >> >      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
> >> >      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
> >> > +    __vmread(GUEST_DR7, &c->dr7);
> >>
> >> ... just when v == current.
> >>
> >
> > Would that check really be necessary? It would complicate the code not
> just
> > here but the caller would need to be aware too that in that case dr7 can
> be
> > aquired from someplace else. I don't see the harm in just saving dr7 here
> > in both cases.
>
> Maybe the solution then is for the suggested if() to have an "else"?
> While, as someone said elsewhere, a few more cycles may not be
> noticable, why make things slower than they need to be. Plus - what
> guarantees that the VMCS field isn't stale while the guest isn't running
> (perhaps it got updated but not sync-ed back yet in anticipation for
> this to happen during vCPU resume)?
>

I would say the caller is better suited to make this choice then this
function. This function is intended to save vmcs values, so it should do so
regardless whether the value in it is stale or not. Then the caller can
selectively choose to use the values it knows not to be stale. As for it
adding cycles, the if/else check here would also add some cycles. I would
guess that the performance difference between the if/else check and
__vmread would be unnoticeable so I don't really see any value in doing
this check here.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 3245 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 v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Jan Beulich]

>>> On 15.02.16 at 17:27, <tlengyel@novetta.com> wrote:
> On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 12.02.16 at 13:57, <tlengyel@novetta.com> wrote:
>> > On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
>> >>
>> >> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
>> >> > Sending the dr7 register during vm_events is useful for various
>> > applications,
>> >> > but the current way the register value is gathered is incorrent. In
>> this
>> >> > patch
>> >> > we extend vmx_vmcs_save so that we get the correct value.
>> >> >
>> >> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> >>
>> >> Iirc Andrew suggested ...
>> >>
>> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> >> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct
>> hvm_hw_cpu *c)
>> >> >      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
>> >> >      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
>> >> >      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
>> >> > +    __vmread(GUEST_DR7, &c->dr7);
>> >>
>> >> ... just when v == current.
>> >>
>> >
>> > Would that check really be necessary? It would complicate the code not
>> just
>> > here but the caller would need to be aware too that in that case dr7 can
>> be
>> > aquired from someplace else. I don't see the harm in just saving dr7 here
>> > in both cases.
>>
>> Maybe the solution then is for the suggested if() to have an "else"?
>> While, as someone said elsewhere, a few more cycles may not be
>> noticable, why make things slower than they need to be. Plus - what
>> guarantees that the VMCS field isn't stale while the guest isn't running
>> (perhaps it got updated but not sync-ed back yet in anticipation for
>> this to happen during vCPU resume)?
>>
> 
> I would say the caller is better suited to make this choice then this
> function. This function is intended to save vmcs values, so it should do so
> regardless whether the value in it is stale or not.

That's a valid point, but while I agree it nevertheless only makes
me ...

> Then the caller can
> selectively choose to use the values it knows not to be stale. As for it
> adding cycles, the if/else check here would also add some cycles. I would
> guess that the performance difference between the if/else check and
> __vmread would be unnoticeable so I don't really see any value in doing
> this check here.

... ask to then tweak the caller to overwrite the DR7 value with the
known non-stale one in the v != current case.

Jan

Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Lengyel, Tamas]

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

On Mon, Feb 15, 2016 at 9:48 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 15.02.16 at 17:27, <tlengyel@novetta.com> wrote:
> > On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >
> >> >>> On 12.02.16 at 13:57, <tlengyel@novetta.com> wrote:
> >> > On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >>
> >> >> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> >> >> > Sending the dr7 register during vm_events is useful for various
> >> > applications,
> >> >> > but the current way the register value is gathered is incorrent. In
> >> this
> >> >> > patch
> >> >> > we extend vmx_vmcs_save so that we get the correct value.
> >> >> >
> >> >> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> >>
> >> >> Iirc Andrew suggested ...
> >> >>
> >> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> >> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v,
> struct
> >> hvm_hw_cpu *c)
> >> >> >      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
> >> >> >      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
> >> >> >      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
> >> >> > +    __vmread(GUEST_DR7, &c->dr7);
> >> >>
> >> >> ... just when v == current.
> >> >>
> >> >
> >> > Would that check really be necessary? It would complicate the code not
> >> just
> >> > here but the caller would need to be aware too that in that case dr7
> can
> >> be
> >> > aquired from someplace else. I don't see the harm in just saving dr7
> here
> >> > in both cases.
> >>
> >> Maybe the solution then is for the suggested if() to have an "else"?
> >> While, as someone said elsewhere, a few more cycles may not be
> >> noticable, why make things slower than they need to be. Plus - what
> >> guarantees that the VMCS field isn't stale while the guest isn't running
> >> (perhaps it got updated but not sync-ed back yet in anticipation for
> >> this to happen during vCPU resume)?
> >>
> >
> > I would say the caller is better suited to make this choice then this
> > function. This function is intended to save vmcs values, so it should do
> so
> > regardless whether the value in it is stale or not.
>
> That's a valid point, but while I agree it nevertheless only makes
> me ...
>
> > Then the caller can
> > selectively choose to use the values it knows not to be stale. As for it
> > adding cycles, the if/else check here would also add some cycles. I would
> > guess that the performance difference between the if/else check and
> > __vmread would be unnoticeable so I don't really see any value in doing
> > this check here.
>
> ... ask to then tweak the caller to overwrite the DR7 value with the
> known non-stale one in the v != current case.
>

All paths that end up using this dr7 value in vm_event have v==current, so
right now there is no caller to this function using dr7 where v!=current.
Future callers where v!=current could do so indeed.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4471 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 v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Jan Beulich]

>>> On 15.02.16 at 17:55, <tlengyel@novetta.com> wrote:
> On Mon, Feb 15, 2016 at 9:48 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 15.02.16 at 17:27, <tlengyel@novetta.com> wrote:
>> > On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >
>> >> >>> On 12.02.16 at 13:57, <tlengyel@novetta.com> wrote:
>> >> > On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
>> >> >>
>> >> >> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
>> >> >> > Sending the dr7 register during vm_events is useful for various
>> >> > applications,
>> >> >> > but the current way the register value is gathered is incorrent. In
>> >> this
>> >> >> > patch
>> >> >> > we extend vmx_vmcs_save so that we get the correct value.
>> >> >> >
>> >> >> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> >> >>
>> >> >> Iirc Andrew suggested ...
>> >> >>
>> >> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> >> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> >> >> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v,
>> struct
>> >> hvm_hw_cpu *c)
>> >> >> >      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
>> >> >> >      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
>> >> >> >      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
>> >> >> > +    __vmread(GUEST_DR7, &c->dr7);
>> >> >>
>> >> >> ... just when v == current.
>> >> >>
>> >> >
>> >> > Would that check really be necessary? It would complicate the code not
>> >> just
>> >> > here but the caller would need to be aware too that in that case dr7
>> can
>> >> be
>> >> > aquired from someplace else. I don't see the harm in just saving dr7
>> here
>> >> > in both cases.
>> >>
>> >> Maybe the solution then is for the suggested if() to have an "else"?
>> >> While, as someone said elsewhere, a few more cycles may not be
>> >> noticable, why make things slower than they need to be. Plus - what
>> >> guarantees that the VMCS field isn't stale while the guest isn't running
>> >> (perhaps it got updated but not sync-ed back yet in anticipation for
>> >> this to happen during vCPU resume)?
>> >>
>> >
>> > I would say the caller is better suited to make this choice then this
>> > function. This function is intended to save vmcs values, so it should do
>> so
>> > regardless whether the value in it is stale or not.
>>
>> That's a valid point, but while I agree it nevertheless only makes
>> me ...
>>
>> > Then the caller can
>> > selectively choose to use the values it knows not to be stale. As for it
>> > adding cycles, the if/else check here would also add some cycles. I would
>> > guess that the performance difference between the if/else check and
>> > __vmread would be unnoticeable so I don't really see any value in doing
>> > this check here.
>>
>> ... ask to then tweak the caller to overwrite the DR7 value with the
>> known non-stale one in the v != current case.
> 
> All paths that end up using this dr7 value in vm_event have v==current, so
> right now there is no caller to this function using dr7 where v!=current.
> Future callers where v!=current could do so indeed.

Well, first of all the vm_event consumers of this data are secondary.
The primary consumer is the VM save logic, which runs with
v != current. It just so happens that hvm_save_cpu_ctxt() already
ignores that field and uses v->arch.debugreg[7] instead. Hence
we're back to square one: How much of an overhead is the extra
VMREAD (the data gathered by which the primary consumer has no
use for)?

Jan

Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Lengyel, Tamas]

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

On Mon, Feb 15, 2016 at 10:06 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 15.02.16 at 17:55, <tlengyel@novetta.com> wrote:
> > On Mon, Feb 15, 2016 at 9:48 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >
> >> >>> On 15.02.16 at 17:27, <tlengyel@novetta.com> wrote:
> >> > On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <JBeulich@suse.com>
> wrote:
> >> >
> >> >> >>> On 12.02.16 at 13:57, <tlengyel@novetta.com> wrote:
> >> >> > On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >> >>
> >> >> >> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
> >> >> >> > Sending the dr7 register during vm_events is useful for various
> >> >> > applications,
> >> >> >> > but the current way the register value is gathered is
> incorrent. In
> >> >> this
> >> >> >> > patch
> >> >> >> > we extend vmx_vmcs_save so that we get the correct value.
> >> >> >> >
> >> >> >> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> >> >>
> >> >> >> Iirc Andrew suggested ...
> >> >> >>
> >> >> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> >> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> >> >> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v,
> >> struct
> >> >> hvm_hw_cpu *c)
> >> >> >> >      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
> >> >> >> >      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
> >> >> >> >      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
> >> >> >> > +    __vmread(GUEST_DR7, &c->dr7);
> >> >> >>
> >> >> >> ... just when v == current.
> >> >> >>
> >> >> >
> >> >> > Would that check really be necessary? It would complicate the code
> not
> >> >> just
> >> >> > here but the caller would need to be aware too that in that case
> dr7
> >> can
> >> >> be
> >> >> > aquired from someplace else. I don't see the harm in just saving
> dr7
> >> here
> >> >> > in both cases.
> >> >>
> >> >> Maybe the solution then is for the suggested if() to have an "else"?
> >> >> While, as someone said elsewhere, a few more cycles may not be
> >> >> noticable, why make things slower than they need to be. Plus - what
> >> >> guarantees that the VMCS field isn't stale while the guest isn't
> running
> >> >> (perhaps it got updated but not sync-ed back yet in anticipation for
> >> >> this to happen during vCPU resume)?
> >> >>
> >> >
> >> > I would say the caller is better suited to make this choice then this
> >> > function. This function is intended to save vmcs values, so it should
> do
> >> so
> >> > regardless whether the value in it is stale or not.
> >>
> >> That's a valid point, but while I agree it nevertheless only makes
> >> me ...
> >>
> >> > Then the caller can
> >> > selectively choose to use the values it knows not to be stale. As for
> it
> >> > adding cycles, the if/else check here would also add some cycles. I
> would
> >> > guess that the performance difference between the if/else check and
> >> > __vmread would be unnoticeable so I don't really see any value in
> doing
> >> > this check here.
> >>
> >> ... ask to then tweak the caller to overwrite the DR7 value with the
> >> known non-stale one in the v != current case.
> >
> > All paths that end up using this dr7 value in vm_event have v==current,
> so
> > right now there is no caller to this function using dr7 where v!=current.
> > Future callers where v!=current could do so indeed.
>
> Well, first of all the vm_event consumers of this data are secondary.
> The primary consumer is the VM save logic, which runs with
> v != current. It just so happens that hvm_save_cpu_ctxt() already
> ignores that field and uses v->arch.debugreg[7] instead. Hence
> we're back to square one: How much of an overhead is the extra
> VMREAD (the data gathered by which the primary consumer has no
> use for)?


I don't have an answer to that but I don't think it's very significant. My
original intention was to introduce a separate hvm function to do this
saving of dr7 which was voted against by Andrew. I personally don't have a
use for dr7 at the moment either way.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 5999 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 v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Jan Beulich]

>>> On 15.02.16 at 18:17, <tlengyel@novetta.com> wrote:
> On Mon, Feb 15, 2016 at 10:06 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 15.02.16 at 17:55, <tlengyel@novetta.com> wrote:
>> > All paths that end up using this dr7 value in vm_event have v==current,
>> so
>> > right now there is no caller to this function using dr7 where v!=current.
>> > Future callers where v!=current could do so indeed.
>>
>> Well, first of all the vm_event consumers of this data are secondary.
>> The primary consumer is the VM save logic, which runs with
>> v != current. It just so happens that hvm_save_cpu_ctxt() already
>> ignores that field and uses v->arch.debugreg[7] instead. Hence
>> we're back to square one: How much of an overhead is the extra
>> VMREAD (the data gathered by which the primary consumer has no
>> use for)?
> 
> 
> I don't have an answer to that but I don't think it's very significant. My
> original intention was to introduce a separate hvm function to do this
> saving of dr7 which was voted against by Andrew. I personally don't have a
> use for dr7 at the moment either way.

Andrew, thoughts? (As usual, a single such unnecessary addition
may not be noticable, but once we set a precedent, more might
follow, until we get into the noticable range.)

Jan