| Message ID | 1455518118-414-2-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/15/2016 08:35 AM, Corneliu ZUZU wrote: > This patch merges almost identical functions hvm_event_int3 and > hvm_event_single_step into a single function called hvm_event_breakpoint. > Also fixes event.c file header comment in the process. > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > --- > xen/arch/x86/hvm/event.c | 108 +++++++++++++++++++--------------------- > xen/arch/x86/hvm/vmx/vmx.c | 15 +++--- > xen/include/asm-x86/hvm/event.h | 11 ++-- > 3 files changed, 67 insertions(+), 67 deletions(-) Looks good to me. Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Thanks, Razvan
On 2/15/2016 10:30 AM, Razvan Cojocaru wrote: > On 02/15/2016 08:35 AM, Corneliu ZUZU wrote: >> This patch merges almost identical functions hvm_event_int3 and >> hvm_event_single_step into a single function called hvm_event_breakpoint. >> Also fixes event.c file header comment in the process. >> >> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >> --- >> xen/arch/x86/hvm/event.c | 108 +++++++++++++++++++--------------------- >> xen/arch/x86/hvm/vmx/vmx.c | 15 +++--- >> xen/include/asm-x86/hvm/event.h | 11 ++-- >> 3 files changed, 67 insertions(+), 67 deletions(-) > Looks good to me. > > Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > > Thanks, > Razvan > I forgot to ask: when getting an Acked-By response, should I include that patch in the next patch-series or ommit it? I've done that w/ the first patch in the last patch-series, but IDK if I was correct to do so. Thanks, Corneliu.
On Mon, Feb 15, 2016 at 10:40 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > On 2/15/2016 10:30 AM, Razvan Cojocaru wrote: > >> On 02/15/2016 08:35 AM, Corneliu ZUZU wrote: >> >>> This patch merges almost identical functions hvm_event_int3 and >>> hvm_event_single_step into a single function called hvm_event_breakpoint. >>> Also fixes event.c file header comment in the process. >>> >>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >>> --- >>> xen/arch/x86/hvm/event.c | 108 >>> +++++++++++++++++++--------------------- >>> xen/arch/x86/hvm/vmx/vmx.c | 15 +++--- >>> xen/include/asm-x86/hvm/event.h | 11 ++-- >>> 3 files changed, 67 insertions(+), 67 deletions(-) >>> >> Looks good to me. >> >> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >> >> >> Thanks, >> Razvan >> >> If the patch hasn't been merged to staging yet then include it. If it's a longer series the cover page can indicate which patches are still in need of review and which ones have been already acked. Furthermore, for each patch under the Signed-off-by tag you can say what changed in each revision. If nothing changed, you can say no change or just don't put anything for that revision. See http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html for an example. Also keep in mind that if the changes in a revision are significant enough you can't keep the Acked-by tag on the patch, it will need to be re-acked. Tamas
On 2/15/2016 7:47 PM, Tamas K Lengyel wrote: > > > On Mon, Feb 15, 2016 at 10:40 AM, Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> wrote: > > On 2/15/2016 10:30 AM, Razvan Cojocaru wrote: > > On 02/15/2016 08:35 AM, Corneliu ZUZU wrote: > > This patch merges almost identical functions > hvm_event_int3 and > hvm_event_single_step into a single function called > hvm_event_breakpoint. > Also fixes event.c file header comment in the process. > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> > --- > xen/arch/x86/hvm/event.c | 108 > +++++++++++++++++++--------------------- > xen/arch/x86/hvm/vmx/vmx.c | 15 +++--- > xen/include/asm-x86/hvm/event.h | 11 ++-- > 3 files changed, 67 insertions(+), 67 deletions(-) > > Looks good to me. > > Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com > <mailto:rcojocaru@bitdefender.com>> > > > Thanks, > Razvan > > > If the patch hasn't been merged to staging yet then include it. If > it's a longer series the cover page can indicate which patches are > still in need of review and which ones have been already acked. > Furthermore, for each patch under the Signed-off-by tag you can say > what changed in each revision. If nothing changed, you can say no > change or just don't put anything for that revision. See > http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html for > an example. Also keep in mind that if the changes in a revision are > significant enough you can't keep the Acked-by tag on the patch, it > will need to be re-acked. > > Tamas Got it, thanks. Corneliu.
On 02/15/2016 07:54 PM, Corneliu ZUZU wrote: > On 2/15/2016 7:47 PM, Tamas K Lengyel wrote: >> >> >> On Mon, Feb 15, 2016 at 10:40 AM, Corneliu ZUZU <czuzu@bitdefender.com >> <mailto:czuzu@bitdefender.com>> wrote: >> >> On 2/15/2016 10:30 AM, Razvan Cojocaru wrote: >> >> On 02/15/2016 08:35 AM, Corneliu ZUZU wrote: >> >> This patch merges almost identical functions >> hvm_event_int3 and >> hvm_event_single_step into a single function called >> hvm_event_breakpoint. >> Also fixes event.c file header comment in the process. >> >> Signed-off-by: Corneliu ZUZU >> <<mailto:czuzu@bitdefender.com>czuzu@bitdefender.com> >> --- >> xen/arch/x86/hvm/event.c | 108 >> +++++++++++++++++++--------------------- >> xen/arch/x86/hvm/vmx/vmx.c | 15 +++--- >> xen/include/asm-x86/hvm/event.h | 11 ++-- >> 3 files changed, 67 insertions(+), 67 deletions(-) >> >> Looks good to me. >> >> Acked-by: Razvan Cojocaru >> <<mailto:rcojocaru@bitdefender.com>rcojocaru@bitdefender.com> >> >> >> Thanks, >> Razvan >> >> >> If the patch hasn't been merged to staging yet then include it. If >> it's a longer series the cover page can indicate which patches are >> still in need of review and which ones have been already acked. >> Furthermore, for each patch under the Signed-off-by tag you can say >> what changed in each revision. If nothing changed, you can say no >> change or just don't put anything for that revision. See >> <http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html>http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html >> for an example. Also keep in mind that if the changes in a revision >> are significant enough you can't keep the Acked-by tag on the patch, >> it will need to be re-acked. >> >> Tamas > > Got it, thanks. This particular patch did make it into staging. It's 557c7873f35aa39bd84977b28948457b1b342f92.
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index a3d4892..874a36c 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -1,22 +1,24 @@ /* -* event.c: Common hardware virtual machine event abstractions. -* -* Copyright (c) 2004, Intel Corporation. -* Copyright (c) 2005, International Business Machines Corporation. -* Copyright (c) 2008, Citrix Systems, Inc. -* -* This program is free software; you can redistribute it and/or modify it -* under the terms and conditions of the GNU General Public License, -* version 2, as published by the Free Software Foundation. -* -* This program is distributed in the hope it will be useful, but WITHOUT -* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or -* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for -* more details. -* -* You should have received a copy of the GNU General Public License along with -* this program; If not, see <http://www.gnu.org/licenses/>. -*/ + * arch/x86/hvm/event.c + * + * Arch-specific hardware virtual machine event abstractions. + * + * Copyright (c) 2004, Intel Corporation. + * Copyright (c) 2005, International Business Machines Corporation. + * Copyright (c) 2008, Citrix Systems, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ #include <xen/vm_event.h> #include <xen/paging.h> @@ -151,61 +153,51 @@ void hvm_event_guest_request(void) } } -int hvm_event_int3(unsigned long rip) +static inline unsigned long gfn_of_rip(unsigned long rip) { - int rc = 0; struct vcpu *curr = current; + struct segment_register sreg; + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch; - if ( curr->domain->arch.monitor.software_breakpoint_enabled ) - { - struct segment_register sreg; - uint32_t pfec = PFEC_page_present | PFEC_insn_fetch; - vm_event_request_t req = { - .reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT, - .vcpu_id = curr->vcpu_id, - }; - - hvm_get_segment_register(curr, x86_seg_ss, &sreg); - if ( sreg.attr.fields.dpl == 3 ) - pfec |= PFEC_user_mode; + hvm_get_segment_register(curr, x86_seg_ss, &sreg); + if ( sreg.attr.fields.dpl == 3 ) + pfec |= PFEC_user_mode; - hvm_get_segment_register(curr, x86_seg_cs, &sreg); - req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr, - sreg.base + rip, - &pfec); - - rc = hvm_event_traps(1, &req); - } + hvm_get_segment_register(curr, x86_seg_cs, &sreg); - return rc; + return paging_gva_to_gfn(curr, sreg.base + rip, &pfec); } -int hvm_event_single_step(unsigned long rip) +int hvm_event_breakpoint(unsigned long rip, + enum hvm_event_breakpoint_type type) { - int rc = 0; struct vcpu *curr = current; + struct arch_domain *ad = &curr->domain->arch; + vm_event_request_t req; - if ( curr->domain->arch.monitor.singlestep_enabled ) + switch ( type ) { - struct segment_register sreg; - uint32_t pfec = PFEC_page_present | PFEC_insn_fetch; - vm_event_request_t req = { - .reason = VM_EVENT_REASON_SINGLESTEP, - .vcpu_id = curr->vcpu_id, - }; - - hvm_get_segment_register(curr, x86_seg_ss, &sreg); - if ( sreg.attr.fields.dpl == 3 ) - pfec |= PFEC_user_mode; + case HVM_EVENT_SOFTWARE_BREAKPOINT: + if ( !ad->monitor.software_breakpoint_enabled ) + return 0; + req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT; + req.u.software_breakpoint.gfn = gfn_of_rip(rip); + break; - hvm_get_segment_register(curr, x86_seg_cs, &sreg); - req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip, - &pfec); + case HVM_EVENT_SINGLESTEP_BREAKPOINT: + if ( !ad->monitor.singlestep_enabled ) + return 0; + req.reason = VM_EVENT_REASON_SINGLESTEP; + req.u.singlestep.gfn = gfn_of_rip(rip); + break; - rc = hvm_event_traps(1, &req); + default: + return -EOPNOTSUPP; } - return rc; + req.vcpu_id = curr->vcpu_id; + + return hvm_event_traps(1, &req); } /* diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 5e99226..44e778f 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3192,8 +3192,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) break; } else { - int handled = hvm_event_int3(regs->eip); - + int handled = + hvm_event_breakpoint(regs->eip, + HVM_EVENT_SOFTWARE_BREAKPOINT); + if ( handled < 0 ) { struct hvm_trap trap = { @@ -3517,10 +3519,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) case EXIT_REASON_MONITOR_TRAP_FLAG: v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MONITOR_TRAP_FLAG; vmx_update_cpu_exec_control(v); - if ( v->arch.hvm_vcpu.single_step ) { - hvm_event_single_step(regs->eip); - if ( v->domain->debugger_attached ) - domain_pause_for_debugger(); + if ( v->arch.hvm_vcpu.single_step ) + { + hvm_event_breakpoint(regs->eip, HVM_EVENT_SINGLESTEP_BREAKPOINT); + if ( v->domain->debugger_attached ) + domain_pause_for_debugger(); } break; diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h index 11eb1fe..7a83b45 100644 --- a/xen/include/asm-x86/hvm/event.h +++ b/xen/include/asm-x86/hvm/event.h @@ -17,6 +17,12 @@ #ifndef __ASM_X86_HVM_EVENT_H__ #define __ASM_X86_HVM_EVENT_H__ +enum hvm_event_breakpoint_type +{ + HVM_EVENT_SOFTWARE_BREAKPOINT, + HVM_EVENT_SINGLESTEP_BREAKPOINT, +}; + /* * Called for current VCPU on crX/MSR changes by guest. * The event might not fire if the client has subscribed to it in onchangeonly @@ -27,9 +33,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, #define hvm_event_crX(what, new, old) \ hvm_event_cr(VM_EVENT_X86_##what, new, old) void hvm_event_msr(unsigned int msr, uint64_t value); -/* Called for current VCPU: returns -1 if no listener */ -int hvm_event_int3(unsigned long rip); -int hvm_event_single_step(unsigned long rip); +int hvm_event_breakpoint(unsigned long rip, + enum hvm_event_breakpoint_type type); void hvm_event_guest_request(void); #endif /* __ASM_X86_HVM_EVENT_H__ */
This patch merges almost identical functions hvm_event_int3 and hvm_event_single_step into a single function called hvm_event_breakpoint. Also fixes event.c file header comment in the process. Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/x86/hvm/event.c | 108 +++++++++++++++++++--------------------- xen/arch/x86/hvm/vmx/vmx.c | 15 +++--- xen/include/asm-x86/hvm/event.h | 11 ++-- 3 files changed, 67 insertions(+), 67 deletions(-)