Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Jan Beulich]

>>> On 08.02.16 at 17:57,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/9/2016 1:19 PM, Jan Beulich wrote:

On 08.02.16 at 17:57,  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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Jan Beulich]

>>> On 09.02.16 at 12:52,  wrote:
> On 2/9/2016 1:19 PM, Jan Beulich wrote:
> On 08.02.16 at 17:57,  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"?


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/9/2016 2:12 PM, Jan Beulich wrote:

On 09.02.16 at 12:52,  wrote:

On 2/9/2016 1:19 PM, Jan Beulich wrote:

On 08.02.16 at 17:57,  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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

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 
---
 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(-)

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 = >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, );
-req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
-  sreg.base + rip,
-  );
-
-rc = hvm_event_traps(1, );
-}
-
-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, );
-if ( sreg.attr.fields.dpl == 3 )
-pfec |= PFEC_user_mode;
-
-hvm_get_segment_register(curr, x86_seg_cs, );
-req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
- );
+gfn = paging_gva_to_gfn(curr, sreg.base + rip, );
+
+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, );
 }
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__ */
-- 
2.5.0


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/8/2016 8:17 PM, Tamas K Lengyel wrote:



On Mon, Feb 8, 2016 at 10:49 AM, Corneliu ZUZU > 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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Tamas K Lengyel]

On Mon, Feb 8, 2016 at 10:15 AM, 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?
>

Exactly what I was thinking, that would make the code somewhat more
readable.

Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Andrew Cooper]

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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Tamas K Lengyel]

On Mon, Feb 8, 2016 at 10:49 AM, Corneliu ZUZU 
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel