| Message ID | 1454950682-9459-3-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/02/16 16:57, Corneliu ZUZU wrote: > diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h > index 11eb1fe..7c2252b 100644 > --- a/xen/include/asm-x86/hvm/event.h > +++ b/xen/include/asm-x86/hvm/event.h > @@ -27,9 +27,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_software_breakpoint(unsigned long rip, > + bool_t single_step); Are we liable to ever gain any other type of software breakpoint? Might it be more sense to pass an enum rather than a bool here? Otherwise, the changes look sensible. ~Andrew
On Mon, Feb 8, 2016 at 10:15 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 08/02/16 16:57, Corneliu ZUZU wrote: > > diff --git a/xen/include/asm-x86/hvm/event.h > b/xen/include/asm-x86/hvm/event.h > > index 11eb1fe..7c2252b 100644 > > --- a/xen/include/asm-x86/hvm/event.h > > +++ b/xen/include/asm-x86/hvm/event.h > > @@ -27,9 +27,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_software_breakpoint(unsigned long rip, > > + bool_t single_step); > > Are we liable to ever gain any other type of software breakpoint? Might > it be more sense to pass an enum rather than a bool here? > Exactly what I was thinking, that would make the code somewhat more readable. Tamas
On 2/8/2016 7:15 PM, Andrew Cooper wrote: > On 08/02/16 16:57, Corneliu ZUZU wrote: >> diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h >> index 11eb1fe..7c2252b 100644 >> --- a/xen/include/asm-x86/hvm/event.h >> +++ b/xen/include/asm-x86/hvm/event.h >> @@ -27,9 +27,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_software_breakpoint(unsigned long rip, >> + bool_t single_step); > Are we liable to ever gain any other type of software breakpoint? Might > it be more sense to pass an enum rather than a bool here? > > Otherwise, the changes look sensible. > > ~Andrew > Well, IMHO, no. Besides breakpointing/trapping after each instruction (i.e. VM_EVENT_REASON_SINGLESTEP) and on arbitrary instructions (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I don't see what other types of breakpointing one can define (at least from the hypervisor's point of view), and if in the future there will be other types, that could also be changed into an enum then. But making that param an enum now would also be fine by me. Since I noticed Tamas also prefers this option, I will make that change. Corneliu.
On Mon, Feb 8, 2016 at 10:49 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > On 2/8/2016 7:15 PM, Andrew Cooper wrote: > >> On 08/02/16 16:57, Corneliu ZUZU wrote: >> >>> diff --git a/xen/include/asm-x86/hvm/event.h >>> b/xen/include/asm-x86/hvm/event.h >>> index 11eb1fe..7c2252b 100644 >>> --- a/xen/include/asm-x86/hvm/event.h >>> +++ b/xen/include/asm-x86/hvm/event.h >>> @@ -27,9 +27,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_software_breakpoint(unsigned long rip, >>> + bool_t single_step); >>> >> Are we liable to ever gain any other type of software breakpoint? Might >> it be more sense to pass an enum rather than a bool here? >> >> Otherwise, the changes look sensible. >> >> ~Andrew >> >> > Well, IMHO, no. Besides breakpointing/trapping after each instruction > (i.e. VM_EVENT_REASON_SINGLESTEP) > and on arbitrary instructions (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I > don't see what other > types of breakpointing one can define (at least from the hypervisor's > point of view), and if in the future > there will be other types, that could also be changed into an enum then. > But making that param an enum now would also be fine by me. > Since I noticed Tamas also prefers this option, I will make that change. > It's just about code readability. Functionally it would be the same as it is right now, two options in the enum will likely be represented by 0/1. But when you read the code you don't have to keep that boolean state in mind, you explicitly have the path spelled out. Tamas
On 2/8/2016 8:17 PM, Tamas K Lengyel wrote: > > > On Mon, Feb 8, 2016 at 10:49 AM, Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> wrote: > > On 2/8/2016 7:15 PM, Andrew Cooper wrote: > > On 08/02/16 16:57, Corneliu ZUZU wrote: > > diff --git a/xen/include/asm-x86/hvm/event.h > b/xen/include/asm-x86/hvm/event.h > index 11eb1fe..7c2252b 100644 > --- a/xen/include/asm-x86/hvm/event.h > +++ b/xen/include/asm-x86/hvm/event.h > @@ -27,9 +27,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_software_breakpoint(unsigned long rip, > + bool_t single_step); > > Are we liable to ever gain any other type of software > breakpoint? Might > it be more sense to pass an enum rather than a bool here? > > Otherwise, the changes look sensible. > > ~Andrew > > > Well, IMHO, no. Besides breakpointing/trapping after each > instruction (i.e. VM_EVENT_REASON_SINGLESTEP) > and on arbitrary instructions > (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I don't see what other > types of breakpointing one can define (at least from the > hypervisor's point of view), and if in the future > there will be other types, that could also be changed into an enum > then. > But making that param an enum now would also be fine by me. > Since I noticed Tamas also prefers this option, I will make that > change. > > > It's just about code readability. Functionally it would be the same as > it is right now, two options in the enum will likely be represented by > 0/1. But when you read the code you don't have to keep that boolean > state in mind, you explicitly have the path spelled out. > > Tamas > That makes sense, and probably that enum value will be handled in a switch. Will do. Corneliu.
>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote: > This patch merges almost identical functions hvm_event_int3 > and hvm_event_single_step into a single function called > hvm_event_software_breakpoint. Except that "software breakpoint" is rather questionable a name here, considering that on x86 this is basically an alias for "int3". If it was "breakpoint", one might argue (see the other responses you've got) that breakpoint event resulting from debug register settings might then be candidates to come here too. Jan
On 2/9/2016 1:19 PM, Jan Beulich wrote: >>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote: >> This patch merges almost identical functions hvm_event_int3 >> and hvm_event_single_step into a single function called >> hvm_event_software_breakpoint. > Except that "software breakpoint" is rather questionable a name > here, considering that on x86 this is basically an alias for "int3". > If it was "breakpoint", one might argue (see the other responses > you've got) that breakpoint event resulting from debug register > settings might then be candidates to come here too. > > Jan Yeah..should I then: * keep both functions and only rename hvm_event_int3 to hvm_event_software_breakpoint * separate the code that gets the GFN of the instruction pointer in a static inline function ? Corneliu.
>>> On 09.02.16 at 12:52, <czuzu@bitdefender.com> wrote: > On 2/9/2016 1:19 PM, Jan Beulich wrote: >>>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote: >>> This patch merges almost identical functions hvm_event_int3 >>> and hvm_event_single_step into a single function called >>> hvm_event_software_breakpoint. >> Except that "software breakpoint" is rather questionable a name >> here, considering that on x86 this is basically an alias for "int3". >> If it was "breakpoint", one might argue (see the other responses >> you've got) that breakpoint event resulting from debug register >> settings might then be candidates to come here too. > > Yeah..should I then: > * keep both functions and only rename hvm_event_int3 to > hvm_event_software_breakpoint I actually think that the intention of folding two almost identical functions is a good one. I'm merely suggesting to think of a better name - perhaps just "breakpoint" or "debug event"?
On 2/9/2016 2:12 PM, Jan Beulich wrote: >>>> On 09.02.16 at 12:52, <czuzu@bitdefender.com> wrote: >> On 2/9/2016 1:19 PM, Jan Beulich wrote: >>>>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote: >>>> This patch merges almost identical functions hvm_event_int3 >>>> and hvm_event_single_step into a single function called >>>> hvm_event_software_breakpoint. >>> Except that "software breakpoint" is rather questionable a name >>> here, considering that on x86 this is basically an alias for "int3". >>> If it was "breakpoint", one might argue (see the other responses >>> you've got) that breakpoint event resulting from debug register >>> settings might then be candidates to come here too. >> Yeah..should I then: >> * keep both functions and only rename hvm_event_int3 to >> hvm_event_software_breakpoint > I actually think that the intention of folding two almost identical > functions is a good one. I'm merely suggesting to think of a > better name - perhaps just "breakpoint" or "debug event"? > > SGTM. I'll change it to hvm_event_breakpoint then. Corneliu.
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index a3d4892..9dc533b 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -151,17 +151,20 @@ void hvm_event_guest_request(void) } } -int hvm_event_int3(unsigned long rip) +int hvm_event_software_breakpoint(unsigned long rip, bool_t single_step) { int rc = 0; struct vcpu *curr = current; + struct arch_domain *ad = &curr->domain->arch; + bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled + : ad->monitor.software_breakpoint_enabled ); - if ( curr->domain->arch.monitor.software_breakpoint_enabled ) + if ( enabled ) { + uint64_t gfn; 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, }; @@ -170,37 +173,18 @@ int hvm_event_int3(unsigned long rip) 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); - } - - return rc; -} - -int hvm_event_single_step(unsigned long rip) -{ - int rc = 0; - struct vcpu *curr = current; - - if ( curr->domain->arch.monitor.singlestep_enabled ) - { - 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; - - hvm_get_segment_register(curr, x86_seg_cs, &sreg); - req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip, - &pfec); + gfn = paging_gva_to_gfn(curr, sreg.base + rip, &pfec); + + if ( single_step ) + { + req.reason = VM_EVENT_REASON_SINGLESTEP; + req.u.singlestep.gfn = gfn; + } + else + { + req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT; + req.u.software_breakpoint.gfn = gfn; + } rc = hvm_event_traps(1, &req); } diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index b9f340c..1a24788 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3192,7 +3192,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) break; } else { - int handled = hvm_event_int3(regs->eip); + int handled = hvm_event_software_breakpoint(regs->eip, 0); if ( handled < 0 ) { @@ -3517,10 +3517,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_software_breakpoint(regs->eip, 1); + 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..7c2252b 100644 --- a/xen/include/asm-x86/hvm/event.h +++ b/xen/include/asm-x86/hvm/event.h @@ -27,9 +27,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_software_breakpoint(unsigned long rip, + bool_t single_step); 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_software_breakpoint. Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/x86/hvm/event.c | 52 ++++++++++++++--------------------------- xen/arch/x86/hvm/vmx/vmx.c | 11 +++++---- xen/include/asm-x86/hvm/event.h | 5 ++-- 3 files changed, 26 insertions(+), 42 deletions(-)