* [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save @ 2016-02-15 19:04 Tamas K Lengyel 2016-02-15 19:04 ` [PATCH v3 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel 2016-02-16 6:58 ` [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tian, Kevin 0 siblings, 2 replies; 11+ messages in thread From: Tamas K Lengyel @ 2016-02-15 19:04 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] 11+ messages in thread
* [PATCH v3 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs 2016-02-15 19:04 [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tamas K Lengyel @ 2016-02-15 19:04 ` Tamas K Lengyel 2016-02-16 6:58 ` [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tian, Kevin 1 sibling, 0 replies; 11+ messages in thread From: Tamas K Lengyel @ 2016-02-15 19:04 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> Acked-by: George Dunlap <george.dunlap@citrix.com> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.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> v3: add ASSERT check for hvm vcpu 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 | 58 ++++++++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/vm_event.h | 2 ++ 4 files changed, 63 insertions(+), 89 deletions(-) diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index a3d4892..e84d44b 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -23,40 +23,9 @@ #include <asm/hvm/event.h> #include <asm/monitor.h> #include <asm/altp2m.h> +#include <asm/vm_event.h> #include <public/vm_event.h> -static void hvm_event_fill_regs(vm_event_request_t *req) -{ - const struct cpu_user_regs *regs = guest_cpu_user_regs(); - const struct vcpu *curr = current; - - req->data.regs.x86.rax = regs->eax; - req->data.regs.x86.rcx = regs->ecx; - req->data.regs.x86.rdx = regs->edx; - req->data.regs.x86.rbx = regs->ebx; - req->data.regs.x86.rsp = regs->esp; - req->data.regs.x86.rbp = regs->ebp; - req->data.regs.x86.rsi = regs->esi; - req->data.regs.x86.rdi = regs->edi; - - req->data.regs.x86.r8 = regs->r8; - req->data.regs.x86.r9 = regs->r9; - req->data.regs.x86.r10 = regs->r10; - req->data.regs.x86.r11 = regs->r11; - req->data.regs.x86.r12 = regs->r12; - req->data.regs.x86.r13 = regs->r13; - req->data.regs.x86.r14 = regs->r14; - req->data.regs.x86.r15 = regs->r15; - - req->data.regs.x86.rflags = regs->eflags; - req->data.regs.x86.rip = regs->eip; - - req->data.regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer; - req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0]; - req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3]; - req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4]; -} - static int hvm_event_traps(uint8_t sync, vm_event_request_t *req) { int rc; @@ -90,7 +59,7 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req) req->altp2m_idx = vcpu_altp2m(curr).p2midx; } - hvm_event_fill_regs(req); + vm_event_fill_regs(req); vm_event_put_request(currd, &currd->vm_event->monitor, req); return 1; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index a45ee35..45c6caa 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1507,61 +1507,6 @@ void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp) } } -static void p2m_vm_event_fill_regs(vm_event_request_t *req) -{ - const struct cpu_user_regs *regs = guest_cpu_user_regs(); - struct segment_register seg; - struct hvm_hw_cpu ctxt; - struct vcpu *curr = current; - - /* Architecture-specific vmcs/vmcb bits */ - hvm_funcs.save_cpu_ctxt(curr, &ctxt); - - req->data.regs.x86.rax = regs->eax; - req->data.regs.x86.rcx = regs->ecx; - req->data.regs.x86.rdx = regs->edx; - req->data.regs.x86.rbx = regs->ebx; - req->data.regs.x86.rsp = regs->esp; - req->data.regs.x86.rbp = regs->ebp; - req->data.regs.x86.rsi = regs->esi; - req->data.regs.x86.rdi = regs->edi; - - req->data.regs.x86.r8 = regs->r8; - req->data.regs.x86.r9 = regs->r9; - req->data.regs.x86.r10 = regs->r10; - req->data.regs.x86.r11 = regs->r11; - req->data.regs.x86.r12 = regs->r12; - req->data.regs.x86.r13 = regs->r13; - req->data.regs.x86.r14 = regs->r14; - req->data.regs.x86.r15 = regs->r15; - - req->data.regs.x86.rflags = regs->eflags; - req->data.regs.x86.rip = regs->eip; - - req->data.regs.x86.dr7 = curr->arch.debugreg[7]; - req->data.regs.x86.cr0 = ctxt.cr0; - req->data.regs.x86.cr2 = ctxt.cr2; - req->data.regs.x86.cr3 = ctxt.cr3; - req->data.regs.x86.cr4 = ctxt.cr4; - - req->data.regs.x86.sysenter_cs = ctxt.sysenter_cs; - req->data.regs.x86.sysenter_esp = ctxt.sysenter_esp; - req->data.regs.x86.sysenter_eip = ctxt.sysenter_eip; - - req->data.regs.x86.msr_efer = ctxt.msr_efer; - req->data.regs.x86.msr_star = ctxt.msr_star; - req->data.regs.x86.msr_lstar = ctxt.msr_lstar; - - hvm_get_segment_register(curr, x86_seg_fs, &seg); - req->data.regs.x86.fs_base = seg.base; - - hvm_get_segment_register(curr, x86_seg_gs, &seg); - req->data.regs.x86.gs_base = seg.base; - - hvm_get_segment_register(curr, x86_seg_cs, &seg); - req->data.regs.x86.cs_arbytes = seg.attr.bytes; -} - void p2m_mem_access_emulate_check(struct vcpu *v, const vm_event_response_t *rsp) { @@ -1760,7 +1705,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; req->vcpu_id = v->vcpu_id; - p2m_vm_event_fill_regs(req); + vm_event_fill_regs(req); if ( altp2m_active(v->domain) ) { diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 08d678a..4671577 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -122,6 +122,64 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp) v->arch.user_regs.eip = rsp->data.regs.x86.rip; } +void vm_event_fill_regs(vm_event_request_t *req) +{ + const struct cpu_user_regs *regs = guest_cpu_user_regs(); + struct segment_register seg; + struct hvm_hw_cpu ctxt; + struct vcpu *curr = current; + + ASSERT(is_hvm_vcpu(curr)); + + 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; + + /* 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] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-15 19:04 [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tamas K Lengyel 2016-02-15 19:04 ` [PATCH v3 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel @ 2016-02-16 6:58 ` Tian, Kevin 2016-02-16 10:47 ` Jan Beulich 1 sibling, 1 reply; 11+ messages in thread From: Tian, Kevin @ 2016-02-16 6:58 UTC (permalink / raw) To: Tamas K Lengyel, xen-devel Cc: Andrew Cooper, Keir Fraser, Jan Beulich, Nakajima, Jun > From: Tamas K Lengyel [mailto:tlengyel@novetta.com] > Sent: Tuesday, February 16, 2016 3:04 AM > > 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); > Hi, Tamas, I didn't see the open closed around "v != current", if I'm not missing some mails... Have you confirmed with Jan that he is OK with it? Thanks Kevin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-16 6:58 ` [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tian, Kevin @ 2016-02-16 10:47 ` Jan Beulich 2016-02-19 17:06 ` Lengyel, Tamas 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2016-02-16 10:47 UTC (permalink / raw) To: Kevin Tian, Tamas K Lengyel Cc: Andrew Cooper, Keir Fraser, Jun Nakajima, xen-devel >>> On 16.02.16 at 07:58, <kevin.tian@intel.com> wrote: >> --- 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); >> > > Hi, Tamas, I didn't see the open closed around "v != current", if > I'm not missing some mails... Have you confirmed with Jan that > he is OK with it? We didn't really settle on this yet. I'm not heavily opposed to it remaining unconditional for now, but as said in the other replay my fear is that this might later lead to further additions which may then also be of no interest to the main (save/migration) user of this code. Jan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-16 10:47 ` Jan Beulich @ 2016-02-19 17:06 ` Lengyel, Tamas 2016-02-19 17:18 ` Andrew Cooper 0 siblings, 1 reply; 11+ messages in thread From: Lengyel, Tamas @ 2016-02-19 17:06 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1047 bytes --] On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 16.02.16 at 07:58, <kevin.tian@intel.com> wrote: > >> --- 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); > >> > > > > Hi, Tamas, I didn't see the open closed around "v != current", if > > I'm not missing some mails... Have you confirmed with Jan that > > he is OK with it? > > We didn't really settle on this yet. I'm not heavily opposed to it > remaining unconditional for now, but as said in the other replay > my fear is that this might later lead to further additions which > may then also be of no interest to the main (save/migration) > user of this code. > Andrew, any comment if this is OK from your perspective? Thanks, Tamas [-- Attachment #1.2: Type: text/html, Size: 1727 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] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-19 17:06 ` Lengyel, Tamas @ 2016-02-19 17:18 ` Andrew Cooper 2016-02-19 17:26 ` Lengyel, Tamas 0 siblings, 1 reply; 11+ messages in thread From: Andrew Cooper @ 2016-02-19 17:18 UTC (permalink / raw) To: Lengyel, Tamas, Jan Beulich Cc: xen-devel, Kevin Tian, Keir Fraser, Jun Nakajima [-- Attachment #1.1: Type: text/plain, Size: 1487 bytes --] On 19/02/16 17:06, Lengyel, Tamas wrote: > > > On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com > <mailto:JBeulich@suse.com>> wrote: > > >>> On 16.02.16 at 07:58, <kevin.tian@intel.com <mailto:kevin.tian@intel.com>> wrote: > >> --- 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); > >> > > > > Hi, Tamas, I didn't see the open closed around "v != current", if > > I'm not missing some mails... Have you confirmed with Jan that > > he is OK with it? > > We didn't really settle on this yet. I'm not heavily opposed to it > remaining unconditional for now, but as said in the other replay > my fear is that this might later lead to further additions which > may then also be of no interest to the main (save/migration) > user of this code. > > > Andrew, any comment if this is OK from your perspective? I specifically suggested the use of vmx_save_dr() to make all debug state consistent. I don't see much purpose in being able to introspect just %dr7. If any debug related activities are going on, all debug registers are relevant. Is this not the case? ~Andrew [-- Attachment #1.2: Type: text/html, Size: 3382 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] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-19 17:18 ` Andrew Cooper @ 2016-02-19 17:26 ` Lengyel, Tamas 2016-02-19 18:04 ` Lengyel, Tamas 2016-02-22 9:25 ` Razvan Cojocaru 0 siblings, 2 replies; 11+ messages in thread From: Lengyel, Tamas @ 2016-02-19 17:26 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel, Kevin Tian, Keir Fraser, Jun Nakajima, Jan Beulich [-- Attachment #1.1: Type: text/plain, Size: 1954 bytes --] On Fri, Feb 19, 2016 at 10:18 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 19/02/16 17:06, Lengyel, Tamas wrote: > > > > On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 16.02.16 at 07:58, < <kevin.tian@intel.com>kevin.tian@intel.com> >> wrote: >> >> --- 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); >> >> >> > >> > Hi, Tamas, I didn't see the open closed around "v != current", if >> > I'm not missing some mails... Have you confirmed with Jan that >> > he is OK with it? >> >> We didn't really settle on this yet. I'm not heavily opposed to it >> remaining unconditional for now, but as said in the other replay >> my fear is that this might later lead to further additions which >> may then also be of no interest to the main (save/migration) >> user of this code. >> > > Andrew, any comment if this is OK from your perspective? > > > I specifically suggested the use of vmx_save_dr() to make all debug state > consistent. > I might have missed that comment. > > I don't see much purpose in being able to introspect just %dr7. If any > debug related activities are going on, all debug registers are relevant. > > Is this not the case? > Right now only dr7 is included in the automatic register snapshot sent with each vm_event. I personally don't use any of them so I can't comment on how it would be useful by itself (Razvan?). From my perspective the only issue at hand has been that the current way dr7 was gathered was incorrect. IMHO if someone needs the other debug registers for each vm_event, that change can be introduced in a separate patch. Tamas [-- Attachment #1.2: Type: text/html, Size: 4090 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] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-19 17:26 ` Lengyel, Tamas @ 2016-02-19 18:04 ` Lengyel, Tamas 2016-02-22 9:25 ` Razvan Cojocaru 1 sibling, 0 replies; 11+ messages in thread From: Lengyel, Tamas @ 2016-02-19 18:04 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel, Kevin Tian, Keir Fraser, Jun Nakajima, Jan Beulich [-- Attachment #1.1: Type: text/plain, Size: 2317 bytes --] On Fri, Feb 19, 2016 at 10:26 AM, Lengyel, Tamas <tlengyel@novetta.com> wrote: > > > > On Fri, Feb 19, 2016 at 10:18 AM, Andrew Cooper <andrew.cooper3@citrix.com > > wrote: > >> On 19/02/16 17:06, Lengyel, Tamas wrote: >> >> >> >> On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> >>> On 16.02.16 at 07:58, < <kevin.tian@intel.com>kevin.tian@intel.com> >>> wrote: >>> >> --- 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); >>> >> >>> > >>> > Hi, Tamas, I didn't see the open closed around "v != current", if >>> > I'm not missing some mails... Have you confirmed with Jan that >>> > he is OK with it? >>> >>> We didn't really settle on this yet. I'm not heavily opposed to it >>> remaining unconditional for now, but as said in the other replay >>> my fear is that this might later lead to further additions which >>> may then also be of no interest to the main (save/migration) >>> user of this code. >>> >> >> Andrew, any comment if this is OK from your perspective? >> >> >> I specifically suggested the use of vmx_save_dr() to make all debug state >> consistent. >> > > I might have missed that comment. > > >> >> I don't see much purpose in being able to introspect just %dr7. If any >> debug related activities are going on, all debug registers are relevant. >> >> Is this not the case? >> > > Right now only dr7 is included in the automatic register snapshot sent > with each vm_event. I personally don't use any of them so I can't comment > on how it would be useful by itself (Razvan?). From my perspective the only > issue at hand has been that the current way dr7 was gathered was incorrect. > IMHO if someone needs the other debug registers for each vm_event, that > change can be introduced in a separate patch. > In case this needs any more discussion I will split the dr7 fix parts from the consolidation patch as there is no need to have it be held up. It has already been acked and is unrelated to this issue. Thanks, Tamas [-- Attachment #1.2: Type: text/html, Size: 4814 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] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-19 17:26 ` Lengyel, Tamas 2016-02-19 18:04 ` Lengyel, Tamas @ 2016-02-22 9:25 ` Razvan Cojocaru 2016-02-22 13:51 ` Lengyel, Tamas 1 sibling, 1 reply; 11+ messages in thread From: Razvan Cojocaru @ 2016-02-22 9:25 UTC (permalink / raw) To: Lengyel, Tamas, Andrew Cooper Cc: xen-devel, Kevin Tian, Keir Fraser, Jan Beulich, Jun Nakajima On 02/19/2016 07:26 PM, Lengyel, Tamas wrote: > > > On Fri, Feb 19, 2016 at 10:18 AM, Andrew Cooper > <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote: > > On 19/02/16 17:06, Lengyel, Tamas wrote: >> >> >> On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com >> <mailto:JBeulich@suse.com>> wrote: >> >> >>> On 16.02.16 at 07:58, <<mailto:kevin.tian@intel.com>kevin.tian@intel.com >> <mailto:kevin.tian@intel.com>> wrote: >> >> --- 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); >> >> >> > >> > Hi, Tamas, I didn't see the open closed around "v != >> current", if >> > I'm not missing some mails... Have you confirmed with Jan that >> > he is OK with it? >> >> We didn't really settle on this yet. I'm not heavily opposed to it >> remaining unconditional for now, but as said in the other replay >> my fear is that this might later lead to further additions which >> may then also be of no interest to the main (save/migration) >> user of this code. >> >> >> Andrew, any comment if this is OK from your perspective? > > I specifically suggested the use of vmx_save_dr() to make all debug > state consistent. > > > I might have missed that comment. > > > > I don't see much purpose in being able to introspect just %dr7. If > any debug related activities are going on, all debug registers are > relevant. > > Is this not the case? > > > Right now only dr7 is included in the automatic register snapshot sent > with each vm_event. I personally don't use any of them so I can't > comment on how it would be useful by itself (Razvan?). From my > perspective the only issue at hand has been that the current way dr7 was > gathered was incorrect. IMHO if someone needs the other debug registers > for each vm_event, that change can be introduced in a separate patch. Andrew is right, all debug registers are relevant for debug activities. In fact, I've checked with the introspection engine team, and they no longer use DR7 at the moment (I don't recall exactly why it has been requested when I first wrote the patch a few years ago). So if nobody minds - and I find it unlikely that anyone would - we can, for the moment, simply remove DR7 altogether from the registers sent with the vm_event. Should they become necessary, we should indeed include all of them in a future patch. Thanks, Razvan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-22 9:25 ` Razvan Cojocaru @ 2016-02-22 13:51 ` Lengyel, Tamas 2016-02-22 13:53 ` Razvan Cojocaru 0 siblings, 1 reply; 11+ messages in thread From: Lengyel, Tamas @ 2016-02-22 13:51 UTC (permalink / raw) To: Razvan Cojocaru Cc: Kevin Tian, Keir Fraser, Jan Beulich, Andrew Cooper, Jun Nakajima, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 3394 bytes --] On Feb 22, 2016 04:23, "Razvan Cojocaru" <rcojocaru@bitdefender.com> wrote: > > On 02/19/2016 07:26 PM, Lengyel, Tamas wrote: > > > > > > On Fri, Feb 19, 2016 at 10:18 AM, Andrew Cooper > > <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote: > > > > On 19/02/16 17:06, Lengyel, Tamas wrote: > >> > >> > >> On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com > >> <mailto:JBeulich@suse.com>> wrote: > >> > >> >>> On 16.02.16 at 07:58, <<mailto:kevin.tian@intel.com> kevin.tian@intel.com > >> <mailto:kevin.tian@intel.com>> wrote: > >> >> --- 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); > >> >> > >> > > >> > Hi, Tamas, I didn't see the open closed around "v != > >> current", if > >> > I'm not missing some mails... Have you confirmed with Jan that > >> > he is OK with it? > >> > >> We didn't really settle on this yet. I'm not heavily opposed to it > >> remaining unconditional for now, but as said in the other replay > >> my fear is that this might later lead to further additions which > >> may then also be of no interest to the main (save/migration) > >> user of this code. > >> > >> > >> Andrew, any comment if this is OK from your perspective? > > > > I specifically suggested the use of vmx_save_dr() to make all debug > > state consistent. > > > > > > I might have missed that comment. > > > > > > > > I don't see much purpose in being able to introspect just %dr7. If > > any debug related activities are going on, all debug registers are > > relevant. > > > > Is this not the case? > > > > > > Right now only dr7 is included in the automatic register snapshot sent > > with each vm_event. I personally don't use any of them so I can't > > comment on how it would be useful by itself (Razvan?). From my > > perspective the only issue at hand has been that the current way dr7 was > > gathered was incorrect. IMHO if someone needs the other debug registers > > for each vm_event, that change can be introduced in a separate patch. > > Andrew is right, all debug registers are relevant for debug activities. > In fact, I've checked with the introspection engine team, and they no > longer use DR7 at the moment (I don't recall exactly why it has been > requested when I first wrote the patch a few years ago). > > So if nobody minds - and I find it unlikely that anyone would - we can, > for the moment, simply remove DR7 altogether from the registers sent > with the vm_event. Should they become necessary, we should indeed > include all of them in a future patch. > I would rather not remove it if it's not necessary as it's a change in the vm_event interface. If we do, we would have to bump the version, the user needs to be aware of the change, etc. So while the whole vm_event rework was done partly to allow us to do such changes, I would rather just keep it for now. Tamas [-- Attachment #1.2: Type: text/html, Size: 5015 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] 11+ messages in thread
* Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save 2016-02-22 13:51 ` Lengyel, Tamas @ 2016-02-22 13:53 ` Razvan Cojocaru 0 siblings, 0 replies; 11+ messages in thread From: Razvan Cojocaru @ 2016-02-22 13:53 UTC (permalink / raw) To: Lengyel, Tamas Cc: Kevin Tian, Keir Fraser, Jan Beulich, Andrew Cooper, Jun Nakajima, xen-devel On 02/22/2016 03:51 PM, Lengyel, Tamas wrote: > > On Feb 22, 2016 04:23, "Razvan Cojocaru" <rcojocaru@bitdefender.com > <mailto:rcojocaru@bitdefender.com>> wrote: >> >> On 02/19/2016 07:26 PM, Lengyel, Tamas wrote: >> > >> > >> > On Fri, Feb 19, 2016 at 10:18 AM, Andrew Cooper >> > <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com> > <mailto:andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>>> > wrote: >> > >> > On 19/02/16 17:06, Lengyel, Tamas wrote: >> >> >> >> >> >> On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com > <mailto:JBeulich@suse.com> >> >> <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>> wrote: >> >> >> >> >>> On 16.02.16 at 07:58, <<mailto:kevin.tian@intel.com > <mailto:kevin.tian@intel.com>>kevin.tian@intel.com > <mailto:kevin.tian@intel.com> >> >> <mailto:kevin.tian@intel.com > <mailto:kevin.tian@intel.com>>> wrote: >> >> >> --- 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); >> >> >> >> >> > >> >> > Hi, Tamas, I didn't see the open closed around "v != >> >> current", if >> >> > I'm not missing some mails... Have you confirmed with Jan > that >> >> > he is OK with it? >> >> >> >> We didn't really settle on this yet. I'm not heavily > opposed to it >> >> remaining unconditional for now, but as said in the other > replay >> >> my fear is that this might later lead to further additions > which >> >> may then also be of no interest to the main (save/migration) >> >> user of this code. >> >> >> >> >> >> Andrew, any comment if this is OK from your perspective? >> > >> > I specifically suggested the use of vmx_save_dr() to make all debug >> > state consistent. >> > >> > >> > I might have missed that comment. >> > >> > >> > >> > I don't see much purpose in being able to introspect just %dr7. If >> > any debug related activities are going on, all debug registers are >> > relevant. >> > >> > Is this not the case? >> > >> > >> > Right now only dr7 is included in the automatic register snapshot sent >> > with each vm_event. I personally don't use any of them so I can't >> > comment on how it would be useful by itself (Razvan?). From my >> > perspective the only issue at hand has been that the current way dr7 was >> > gathered was incorrect. IMHO if someone needs the other debug registers >> > for each vm_event, that change can be introduced in a separate patch. >> >> Andrew is right, all debug registers are relevant for debug activities. >> In fact, I've checked with the introspection engine team, and they no >> longer use DR7 at the moment (I don't recall exactly why it has been >> requested when I first wrote the patch a few years ago). >> >> So if nobody minds - and I find it unlikely that anyone would - we can, >> for the moment, simply remove DR7 altogether from the registers sent >> with the vm_event. Should they become necessary, we should indeed >> include all of them in a future patch. >> > > I would rather not remove it if it's not necessary as it's a change in > the vm_event interface. If we do, we would have to bump the version, the > user needs to be aware of the change, etc. So while the whole vm_event > rework was done partly to allow us to do such changes, I would rather > just keep it for now. Fine with me. Thanks, Razvan ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-02-22 13:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-15 19:04 [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tamas K Lengyel 2016-02-15 19:04 ` [PATCH v3 2/2] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Tamas K Lengyel 2016-02-16 6:58 ` [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Tian, Kevin 2016-02-16 10:47 ` Jan Beulich 2016-02-19 17:06 ` Lengyel, Tamas 2016-02-19 17:18 ` Andrew Cooper 2016-02-19 17:26 ` Lengyel, Tamas 2016-02-19 18:04 ` Lengyel, Tamas 2016-02-22 9:25 ` Razvan Cojocaru 2016-02-22 13:51 ` Lengyel, Tamas 2016-02-22 13:53 ` Razvan Cojocaru
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.