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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

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