* [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save
@ 2016-02-12 0:22 Tamas K Lengyel
2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Tamas K Lengyel @ 2016-02-12 0:22 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Keir Fraser, Jun Nakajima, Andrew Cooper, Jan Beulich,
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 0:22 [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tamas K Lengyel @ 2016-02-12 0:22 ` Tamas K Lengyel 2016-02-12 6:57 ` Razvan Cojocaru ` (3 more replies) 2016-02-12 6:56 ` [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Razvan Cojocaru 2016-02-12 9:12 ` Jan Beulich 2 siblings, 4 replies; 21+ messages in thread From: Tamas K Lengyel @ 2016-02-12 0:22 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Razvan Cojocaru, George Dunlap, Andrew Cooper, Jan Beulich, 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel @ 2016-02-12 6:57 ` Razvan Cojocaru 2016-02-12 9:07 ` Jan Beulich ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Razvan Cojocaru @ 2016-02-12 6:57 UTC (permalink / raw) To: Tamas K Lengyel, xen-devel Cc: George Dunlap, Andrew Cooper, Keir Fraser, Jan Beulich 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel 2016-02-12 6:57 ` Razvan Cojocaru @ 2016-02-12 9:07 ` Jan Beulich 2016-02-12 9:57 ` Jan Beulich 2016-02-15 14:17 ` George Dunlap 3 siblings, 0 replies; 21+ messages in thread From: Jan Beulich @ 2016-02-12 9:07 UTC (permalink / raw) To: Tamas K Lengyel Cc: George Dunlap, Andrew Cooper, Keir Fraser, Razvan Cojocaru, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel 2016-02-12 6:57 ` Razvan Cojocaru 2016-02-12 9:07 ` Jan Beulich @ 2016-02-12 9:57 ` Jan Beulich 2016-02-12 10:19 ` Razvan Cojocaru 2016-02-15 14:17 ` George Dunlap 3 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2016-02-12 9:57 UTC (permalink / raw) To: Tamas K Lengyel Cc: George Dunlap, Andrew Cooper, Keir Fraser, Razvan Cojocaru, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 9:57 ` Jan Beulich @ 2016-02-12 10:19 ` Razvan Cojocaru 2016-02-12 10:41 ` Jan Beulich 0 siblings, 1 reply; 21+ messages in thread From: Razvan Cojocaru @ 2016-02-12 10:19 UTC (permalink / raw) To: Jan Beulich, Tamas K Lengyel Cc: George Dunlap, Andrew Cooper, Keir Fraser, xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 10:19 ` Razvan Cojocaru @ 2016-02-12 10:41 ` Jan Beulich 2016-02-12 12:50 ` Lengyel, Tamas 0 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2016-02-12 10:41 UTC (permalink / raw) To: Razvan Cojocaru, Tamas K Lengyel Cc: George Dunlap, Andrew Cooper, Keir Fraser, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 10:41 ` Jan Beulich @ 2016-02-12 12:50 ` Lengyel, Tamas 2016-02-12 14:57 ` Jan Beulich 0 siblings, 1 reply; 21+ messages in thread From: Lengyel, Tamas @ 2016-02-12 12:50 UTC (permalink / raw) To: Jan Beulich Cc: George Dunlap, Andrew Cooper, Keir Fraser, Razvan Cojocaru, xen-devel [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 12:50 ` Lengyel, Tamas @ 2016-02-12 14:57 ` Jan Beulich 2016-02-15 16:19 ` Lengyel, Tamas 0 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2016-02-12 14:57 UTC (permalink / raw) To: Tamas Lengyel Cc: George Dunlap, Andrew Cooper, Keir Fraser, Razvan Cojocaru, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 14:57 ` Jan Beulich @ 2016-02-15 16:19 ` Lengyel, Tamas 0 siblings, 0 replies; 21+ messages in thread From: Lengyel, Tamas @ 2016-02-15 16:19 UTC (permalink / raw) To: Jan Beulich Cc: George Dunlap, Andrew Cooper, Keir Fraser, Razvan Cojocaru, xen-devel [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel ` (2 preceding siblings ...) 2016-02-12 9:57 ` Jan Beulich @ 2016-02-15 14:17 ` George Dunlap 3 siblings, 0 replies; 21+ messages in thread From: George Dunlap @ 2016-02-15 14:17 UTC (permalink / raw) To: Tamas K Lengyel, xen-devel Cc: George Dunlap, Andrew Cooper, Keir Fraser, Jan Beulich, Razvan Cojocaru 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-12 0:22 [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tamas K Lengyel 2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel @ 2016-02-12 6:56 ` Razvan Cojocaru 2016-02-12 9:12 ` Jan Beulich 2 siblings, 0 replies; 21+ messages in thread From: Razvan Cojocaru @ 2016-02-12 6:56 UTC (permalink / raw) To: Tamas K Lengyel, xen-devel Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jan Beulich, Jun Nakajima 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> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-12 0:22 [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tamas K Lengyel 2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel 2016-02-12 6:56 ` [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Razvan Cojocaru @ 2016-02-12 9:12 ` Jan Beulich 2016-02-12 12:57 ` Lengyel, Tamas 2 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2016-02-12 9:12 UTC (permalink / raw) To: Tamas K Lengyel Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-12 9:12 ` Jan Beulich @ 2016-02-12 12:57 ` Lengyel, Tamas 2016-02-12 15:00 ` Jan Beulich 0 siblings, 1 reply; 21+ messages in thread From: Lengyel, Tamas @ 2016-02-12 12:57 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-12 12:57 ` Lengyel, Tamas @ 2016-02-12 15:00 ` Jan Beulich 2016-02-15 16:27 ` Lengyel, Tamas 0 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2016-02-12 15:00 UTC (permalink / raw) To: Tamas Lengyel Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-12 15:00 ` Jan Beulich @ 2016-02-15 16:27 ` Lengyel, Tamas 2016-02-15 16:48 ` Jan Beulich 0 siblings, 1 reply; 21+ messages in thread From: Lengyel, Tamas @ 2016-02-15 16:27 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-15 16:27 ` Lengyel, Tamas @ 2016-02-15 16:48 ` Jan Beulich 2016-02-15 16:55 ` Lengyel, Tamas 0 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2016-02-15 16:48 UTC (permalink / raw) To: Tamas Lengyel Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-15 16:48 ` Jan Beulich @ 2016-02-15 16:55 ` Lengyel, Tamas 2016-02-15 17:06 ` Jan Beulich 0 siblings, 1 reply; 21+ messages in thread From: Lengyel, Tamas @ 2016-02-15 16:55 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-15 16:55 ` Lengyel, Tamas @ 2016-02-15 17:06 ` Jan Beulich 2016-02-15 17:17 ` Lengyel, Tamas 0 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2016-02-15 17:06 UTC (permalink / raw) To: Tamas Lengyel Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-15 17:06 ` Jan Beulich @ 2016-02-15 17:17 ` Lengyel, Tamas 2016-02-16 9:17 ` Jan Beulich 0 siblings, 1 reply; 21+ messages in thread From: Lengyel, Tamas @ 2016-02-15 17:17 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-15 17:17 ` Lengyel, Tamas @ 2016-02-16 9:17 ` Jan Beulich 0 siblings, 0 replies; 21+ messages in thread From: Jan Beulich @ 2016-02-16 9:17 UTC (permalink / raw) To: Andrew Cooper, Tamas Lengyel Cc: xen-devel, Kevin Tian, Keir Fraser, Jun Nakajima >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-02-16 9:17 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-12 0:22 [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tamas K Lengyel 2016-02-12 0:22 ` [PATCH v2 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel 2016-02-12 6:57 ` Razvan Cojocaru 2016-02-12 9:07 ` Jan Beulich 2016-02-12 9:57 ` Jan Beulich 2016-02-12 10:19 ` Razvan Cojocaru 2016-02-12 10:41 ` Jan Beulich 2016-02-12 12:50 ` Lengyel, Tamas 2016-02-12 14:57 ` Jan Beulich 2016-02-15 16:19 ` Lengyel, Tamas 2016-02-15 14:17 ` George Dunlap 2016-02-12 6:56 ` [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Razvan Cojocaru 2016-02-12 9:12 ` Jan Beulich 2016-02-12 12:57 ` Lengyel, Tamas 2016-02-12 15:00 ` Jan Beulich 2016-02-15 16:27 ` Lengyel, Tamas 2016-02-15 16:48 ` Jan Beulich 2016-02-15 16:55 ` Lengyel, Tamas 2016-02-15 17:06 ` Jan Beulich 2016-02-15 17:17 ` Lengyel, Tamas 2016-02-16 9:17 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).