Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-11 Thread Jan Beulich 
>>> On 10.02.16 at 21:56,  wrote:
> On 10/02/2016 17:11, Jan Beulich wrote:
> On 10.02.16 at 18:04,  wrote:
>>> On 2/10/2016 6:18 PM, Jan Beulich wrote:
>>> On 10.02.16 at 16:50,  wrote:
> --- 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,
> +};
 I don't see what good it does to put existing constants into an
 enum.
>>> As Andrew pointed out, an enum was requested in v1 instead of the 
>>> single_step param.
>>> One could use the already existing VM_EVENT_REASON_* constants, but 
>>> conceptually this
>>> function only involves a subset of those (i.e. *breakpoint vm-events*).
>> Re-using existing constants would seem fine to me.
>>
>> I only now realize that I've made a mistake while looking at the
>> above - the capitals made it implicitly "obvious" to me that they're
>> on the right side of an assignment. Please use capitals only for
>> #define-d constants, not enumerated ones.
> 
> Substantially more enums in the Xen codebase use caps than lowercase. 

Well, sadly this indeed seems to be the case.

> Given no specific direction in CODING_STYLE, this is an unreasonable
> request.

Hence I withdraw the request, despite continuing to be convinced
that all-caps enumerators are bad practice.

Jan


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

[Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Corneliu ZUZU 
This patch merges almost identical functions hvm_event_int3 and
hvm_event_single_step into a single function called hvm_event_breakpoint.

Signed-off-by: Corneliu ZUZU 
---
 xen/arch/x86/hvm/event.c| 76 +++--
 xen/arch/x86/hvm/vmx/vmx.c  | 15 
 xen/include/asm-x86/hvm/event.h | 11 --
 3 files changed, 52 insertions(+), 50 deletions(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index a3d4892..e3444db 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -1,9 +1,12 @@
 /*
-* event.c: Common hardware virtual machine event abstractions.
+* 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.
+* Copyright (c) 2016, Bitdefender S.R.L.
 *
 * This program is free software; you can redistribute it and/or modify it
 * under the terms and conditions of the GNU General Public License,
@@ -151,61 +154,52 @@ void hvm_event_guest_request(void)
 }
 }
 
-int hvm_event_int3(unsigned long rip)
+static inline
+uint64_t 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, );
-if ( sreg.attr.fields.dpl == 3 )
-pfec |= PFEC_user_mode;
+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.software_breakpoint.gfn = paging_gva_to_gfn(curr,
-  sreg.base + rip,
-  );
-
-rc = hvm_event_traps(1, );
-}
+hvm_get_segment_register(curr, x86_seg_cs, );
 
-return rc;
+return (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, );
 }
 
-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 = >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, );
-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, );
-req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
- );
+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, );
+default:
+return -EOPNOTSUPP;
 }
 
-return rc;
+req.vcpu_id = curr->vcpu_id;
+
+return hvm_event_traps(1, );
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b9f340c..cf0e642 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);

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Andrew Cooper 
On 10/02/16 16:18, Jan Beulich wrote:
>
>> --- 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,
>> +};
> I don't see what good it does to put existing constants into an
> enum.

An enum was requested during review of v1, because "bool
software_breakpoint" isn't a good function interface.

I don't mind if #defines were used instead, but the parameter should
have a semantic name used in calling code.

~Andrew

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Corneliu ZUZU 

On 2/10/2016 6:18 PM, Jan Beulich wrote:

On 10.02.16 at 16:50,  wrote:

@@ -151,61 +154,52 @@ void hvm_event_guest_request(void)
  }
  }
  
-int hvm_event_int3(unsigned long rip)

+static inline
+uint64_t gfn_of_rip(unsigned long rip)

This should be a single line and the return value should be
unsigned long.

Noted.

+return (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, );
  }

Pointless cast.

Noted.



--- 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,
+};

I don't see what good it does to put existing constants into an
enum.
As Andrew pointed out, an enum was requested in v1 instead of the 
single_step param.
One could use the already existing VM_EVENT_REASON_* constants, but 
conceptually this

function only involves a subset of those (i.e. *breakpoint vm-events*).



@@ -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);

I guess the comment was here for a reason, and this reason doesn't
go away with this code folding. But I'll leave it to the VM event code
maintainers to judge.

Jan


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



That comment seemed & still seems wrong to me, I don't see any code 
paths out of which that function would return -1.


Thank you,
Corneliu.

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Razvan Cojocaru 
On 02/10/2016 07:04 PM, Corneliu ZUZU wrote:
>>> @@ -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);
>> I guess the comment was here for a reason, and this reason doesn't
>> go away with this code folding. But I'll leave it to the VM event code
>> maintainers to judge.
>>
>> Jan
> 
> That comment seemed & still seems wrong to me, I don't see any code
> paths out of which that function would return -1.

That seems to be true. Those functions return whatever hvm_event_traps()
returns, which is 0 on success, 1 (maybe the minus is a typo?) if
there's no ring, or whatever value vm_event_claim_slot() returns.
Vm_event_claim_slot()'s documentation says that it can only return 0 (on
success), -ENOSYS or -EBUSY, none of which translate to -1 (and the code
seems to agree with that claim).

Maybe I'm missing some macro wizardry here, but I don't think so - it
looks like the comment is stale. Tamas, maybe you remember more, should
those functions return -1 if no listener is present?


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Jan Beulich 
>>> On 10.02.16 at 17:37,  wrote:
> On 10/02/16 16:18, Jan Beulich wrote:
>>
>>> --- 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,
>>> +};
>> I don't see what good it does to put existing constants into an
>> enum.
> 
> An enum was requested during review of v1, because "bool
> software_breakpoint" isn't a good function interface.
> 
> I don't mind if #defines were used instead, but the parameter should
> have a semantic name used in calling code.

To calling code there's no difference whether the parameter is of
an enum type or just plain or unsigned int.

Jan


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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Jan Beulich 
>>> On 10.02.16 at 16:50,  wrote:
> @@ -151,61 +154,52 @@ void hvm_event_guest_request(void)
>  }
>  }
>  
> -int hvm_event_int3(unsigned long rip)
> +static inline
> +uint64_t gfn_of_rip(unsigned long rip)

This should be a single line and the return value should be
unsigned long.

>  {
> -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, );
> -if ( sreg.attr.fields.dpl == 3 )
> -pfec |= PFEC_user_mode;
> +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.software_breakpoint.gfn = paging_gva_to_gfn(curr,
> -  sreg.base + rip,
> -  );
> -
> -rc = hvm_event_traps(1, );
> -}
> +hvm_get_segment_register(curr, x86_seg_cs, );
>  
> -return rc;
> +return (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, );
>  }

Pointless cast.

> --- 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,
> +};

I don't see what good it does to put existing constants into an
enum.

> @@ -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);

I guess the comment was here for a reason, and this reason doesn't
go away with this code folding. But I'll leave it to the VM event code
maintainers to judge.

Jan


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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Jan Beulich 
>>> On 10.02.16 at 18:04,  wrote:
> On 2/10/2016 6:18 PM, Jan Beulich wrote:
> On 10.02.16 at 16:50,  wrote:
>>> --- 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,
>>> +};
>> I don't see what good it does to put existing constants into an
>> enum.
> As Andrew pointed out, an enum was requested in v1 instead of the 
> single_step param.
> One could use the already existing VM_EVENT_REASON_* constants, but 
> conceptually this
> function only involves a subset of those (i.e. *breakpoint vm-events*).

Re-using existing constants would seem fine to me.

I only now realize that I've made a mistake while looking at the
above - the capitals made it implicitly "obvious" to me that they're
on the right side of an assignment. Please use capitals only for
#define-d constants, not enumerated ones.

Jan


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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Tamas K Lengyel 
On Wed, Feb 10, 2016 at 10:28 AM, Razvan Cojocaru  wrote:

> On 02/10/2016 07:04 PM, Corneliu ZUZU wrote:
> >>> @@ -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);
> >> I guess the comment was here for a reason, and this reason doesn't
> >> go away with this code folding. But I'll leave it to the VM event code
> >> maintainers to judge.
> >>
> >> Jan
> >
> > That comment seemed & still seems wrong to me, I don't see any code
> > paths out of which that function would return -1.
>
> That seems to be true. Those functions return whatever hvm_event_traps()
> returns, which is 0 on success, 1 (maybe the minus is a typo?) if
> there's no ring, or whatever value vm_event_claim_slot() returns.
> Vm_event_claim_slot()'s documentation says that it can only return 0 (on
> success), -ENOSYS or -EBUSY, none of which translate to -1 (and the code
> seems to agree with that claim).
>
> Maybe I'm missing some macro wizardry here, but I don't think so - it
> looks like the comment is stale. Tamas, maybe you remember more, should
> those functions return -1 if no listener is present?
>

It could very well be that it's just a comment that was forgotten and is
out-of-date. I don't see any issue removing it if it's actually misleading
(as it seems to be).

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Andrew Cooper 
On 10/02/2016 17:11, Jan Beulich wrote:
 On 10.02.16 at 18:04,  wrote:
>> On 2/10/2016 6:18 PM, Jan Beulich wrote:
>> On 10.02.16 at 16:50,  wrote:
 --- 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,
 +};
>>> I don't see what good it does to put existing constants into an
>>> enum.
>> As Andrew pointed out, an enum was requested in v1 instead of the 
>> single_step param.
>> One could use the already existing VM_EVENT_REASON_* constants, but 
>> conceptually this
>> function only involves a subset of those (i.e. *breakpoint vm-events*).
> Re-using existing constants would seem fine to me.
>
> I only now realize that I've made a mistake while looking at the
> above - the capitals made it implicitly "obvious" to me that they're
> on the right side of an assignment. Please use capitals only for
> #define-d constants, not enumerated ones.

Substantially more enums in the Xen codebase use caps than lowercase. 
Given no specific direction in CODING_STYLE, this is an unreasonable
request.

~Andrew

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Corneliu ZUZU 

On 2/10/2016 7:11 PM, Jan Beulich wrote:

On 10.02.16 at 18:04,  wrote:

On 2/10/2016 6:18 PM, Jan Beulich wrote:

On 10.02.16 at 16:50,  wrote:

--- 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,
+};

I don't see what good it does to put existing constants into an
enum.

As Andrew pointed out, an enum was requested in v1 instead of the
single_step param.
One could use the already existing VM_EVENT_REASON_* constants, but
conceptually this
function only involves a subset of those (i.e. *breakpoint vm-events*).

Re-using existing constants would seem fine to me.


Yes but then IMHO conceptually that would be wrong, since it would be 
like saying
"that param can be any type of vm-event", even though it can actually be 
only from a

subset of vm-event types and it will never be *any* type of vm-event.


I only now realize that I've made a mistake while looking at the
above - the capitals made it implicitly "obvious" to me that they're
on the right side of an assignment. Please use capitals only for
#define-d constants, not enumerated ones.

Jan


Most examples of existing enums I've looked at were capital-case, I thought
they're supposed to be like that. Could you please provide an example on how
enum values should look? (there isn't any in CODING_STYLE)

Let me know if you still want me to change this and how.

Corneliu.

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

2016-02-10 Thread Corneliu ZUZU 

On 2/10/2016 7:11 PM, Jan Beulich wrote:

On 10.02.16 at 18:04,  wrote:

On 2/10/2016 6:18 PM, Jan Beulich wrote:

On 10.02.16 at 16:50,  wrote:

--- 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,
+};

I don't see what good it does to put existing constants into an
enum.

As Andrew pointed out, an enum was requested in v1 instead of the
single_step param.
One could use the already existing VM_EVENT_REASON_* constants, but
conceptually this
function only involves a subset of those (i.e. *breakpoint vm-events*).

Re-using existing constants would seem fine to me.

I only now realize that I've made a mistake while looking at the
above - the capitals made it implicitly "obvious" to me that they're
on the right side of an assignment. Please use capitals only for
#define-d constants, not enumerated ones.

Jan




Also, why this bias towards #define vs enums? I usually always prefer 
the latter,
they make the code more readable, e.g. it's clearer what can or cannot 
be passed
to a corresponding function parameter. And from an IDE user's 
point-of-view, I don't

understand why I have to read a comment and search for a list of identifiers
rather than just clicking on a type name and getting there in a flash.
They also make debugging easier, etc.. :).

Corneliu.

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