[PATCH v2 0/7] Vm-events: move monitor vm-events code to common-side.

[PATCH v2 0/7] Vm-events: move monitor vm-events code to common-side.

[Corneliu ZUZU]

This patch series is an attempt to move (most) of the monitor vm-events code to
the common-side.

Patches summary:

1. Minor file-comment fix

2. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
hvm_event_breakpoint.

3. Add Kconfigs:
    HAS_VM_EVENT_WRITE_CTRLREG, HAS_VM_EVENT_SINGLESTEP,
    HAS_VM_EVENT_SOFTWARE_BREAKPOINT, HAS_VM_EVENT_GUEST_REQUEST
and move monitor_domctl to common-side. Used the Kconfigs to selectively
activate implemented monitor vm-events code for each architecture.

4. Minor file renames. Read (*) below.

5. Move hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
hvm_event_breakpoint functions to common-side. (note: arch_hvm_event_fill_regs
on ARM-side will be implemented in a separate patch)

6. Minor file renames + comment fixes. Read (*) below.

7. Move arch_domain.monitor bitfield members to struct domain (common-side).
   Moved bits: single-step, software-breakpoint and guest-request.
(note: write_ctrlreg_* were left on the arch-side, since control-registers
number can vary across architectures)

(*) was only necessary to avoid git seeing a file as being modified, rather than
moved (would have made the diff unnecessarily bulky).

---
Changed since v1:
  Major:
    * not moving arch/arm/hvm.c to arch/arm/hvm/hvm.c anymore
    * hvm_event_software_breakpoint renamed to hvm_event_breakpoint
    * hvm_event_breakpoint single_step param replaced by enum
  Minor:
    * formatting & comment fixes: e.g. removed blank lines around #if/#endif
    * improved code readability (used switch/case instead of if in some places)

[PATCH v2 1/7] xen/arm: fix file comments

[Corneliu ZUZU]

Add file header comment and local variable block @ EOF
of xen/arch/arm/hvm.c.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/arm/hvm.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 5fd0753..056db1a 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -1,3 +1,21 @@
+/*
+ * arch/arm/hvm.c
+ *
+ * Arch-specific hardware virtual machine abstractions.
+ *
+ * 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/config.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -14,7 +32,6 @@
 #include <asm/hypercall.h>
 
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
 {
     long rc = 0;
 
@@ -65,3 +82,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     return rc;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.5.0

Re: [PATCH v2 1/7] xen/arm: fix file comments

[Stefano Stabellini]

On Wed, 10 Feb 2016, Corneliu ZUZU wrote:
> Add file header comment and local variable block @ EOF
> of xen/arch/arm/hvm.c.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/hvm.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 5fd0753..056db1a 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -1,3 +1,21 @@
> +/*
> + * arch/arm/hvm.c
> + *
> + * Arch-specific hardware virtual machine abstractions.
> + *
> + * 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/config.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> @@ -14,7 +32,6 @@
>  #include <asm/hypercall.h>
>  
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> -
>  {
>      long rc = 0;
>  
> @@ -65,3 +82,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      return rc;
>  }
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.5.0
>

[PATCH v2 2/7] xen/x86: merge 2 hvm_event_... 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_breakpoint.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 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, &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 (uint64_t) 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 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);
-          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__ */
-- 
2.5.0

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

[Jan Beulich]

>>> On 10.02.16 at 16:50, <czuzu@bitdefender.com> 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, &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 (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
>  }

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

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

[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

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

[Jan Beulich]

>>> On 10.02.16 at 17:37, <andrew.cooper3@citrix.com> 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

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

[Corneliu ZUZU]

On 2/10/2016 6:18 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 16:50, <czuzu@bitdefender.com> 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, &pfec);
>>   }
> 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.

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

[Jan Beulich]

>>> On 10.02.16 at 18:04, <czuzu@bitdefender.com> wrote:
> On 2/10/2016 6:18 PM, Jan Beulich wrote:
>>>>> On 10.02.16 at 16:50, <czuzu@bitdefender.com> 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

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

[Corneliu ZUZU]

On 2/10/2016 7:11 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 18:04, <czuzu@bitdefender.com> wrote:
>> On 2/10/2016 6:18 PM, Jan Beulich wrote:
>>>>>> On 10.02.16 at 16:50, <czuzu@bitdefender.com> 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.

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

[Andrew Cooper]

On 10/02/2016 17:11, Jan Beulich wrote:
>>>> On 10.02.16 at 18:04, <czuzu@bitdefender.com> wrote:
>> On 2/10/2016 6:18 PM, Jan Beulich wrote:
>>>>>> On 10.02.16 at 16:50, <czuzu@bitdefender.com> 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

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

[Jan Beulich]

>>> On 10.02.16 at 21:56, <andrew.cooper3@citrix.com> wrote:
> On 10/02/2016 17:11, Jan Beulich wrote:
>>>>> On 10.02.16 at 18:04, <czuzu@bitdefender.com> wrote:
>>> On 2/10/2016 6:18 PM, Jan Beulich wrote:
>>>>>>> On 10.02.16 at 16:50, <czuzu@bitdefender.com> 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

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

[Corneliu ZUZU]

On 2/10/2016 7:11 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 18:04, <czuzu@bitdefender.com> wrote:
>> On 2/10/2016 6:18 PM, Jan Beulich wrote:
>>>>>> On 10.02.16 at 16:50, <czuzu@bitdefender.com> 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.

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

[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

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1774 bytes --]

On Wed, Feb 10, 2016 at 10:28 AM, Razvan Cojocaru <rcojocaru@bitdefender.com
> 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

[-- Attachment #1.2: Type: text/html, Size: 2450 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

1. Kconfig:
  * Added Kconfigs for common monitor vm-events:
  # see files: common/Kconfig, x86/Kconfig
    HAS_VM_EVENT_WRITE_CTRLREG
    HAS_VM_EVENT_SINGLESTEP
    HAS_VM_EVENT_SOFTWARE_BREAKPOINT
    HAS_VM_EVENT_GUEST_REQUEST

2. Moved monitor_domctl from arch-side to common-side
  2.1. Moved arch/x86/monitor.c to common/monitor.c
    # see files: arch/x86/Makefile, xen/common/Makefile, xen/common/monitor.c
    # changes:
        - removed status_check (we would have had to duplicate it in X86
            arch_monitor_domctl_event otherwise)
        - moved get_capabilities to arch-side (arch_monitor_get_capabilities)
        - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see
            arch_monitor_domctl_op)
        - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
            arch_monitor_domctl_event)
        - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*

  2.2. Moved asm-x86/monitor.h to xen/monitor.h
    # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
                 arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c

  2.3. Removed asm-arm/monitor.h (no longer needed)

3. Added x86/monitor_x86.c => will rename in next commit to monitor.c (not done
in this commit to avoid git seeing this as being the modified old monitor.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
    # see files: arch/x86/Makefile
    # implements X86-side arch_monitor_domctl_event

4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
monitor.h in next commit, reason is the same as @ (3.).
    # define/implement: arch_monitor_get_capabilities, arch_monitor_domctl_op
        and arch_monitor_domctl_event

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/Kconfig                              |   4 +
 xen/arch/x86/Makefile                             |   2 +-
 xen/arch/x86/hvm/event.c                          |   2 +-
 xen/arch/x86/hvm/hvm.c                            |   2 +-
 xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
 xen/arch/x86/monitor_x86.c                        |  72 ++++++++
 xen/common/Kconfig                                |  20 +++
 xen/common/Makefile                               |   1 +
 xen/common/domctl.c                               |   2 +-
 xen/{arch/x86 => common}/monitor.c                | 195 +++++++++-------------
 xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
 xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
 xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
 13 files changed, 293 insertions(+), 134 deletions(-)
 create mode 100644 xen/arch/x86/monitor_x86.c
 rename xen/{arch/x86 => common}/monitor.c (44%)
 rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
 create mode 100644 xen/include/asm-x86/monitor_arch.h
 rename xen/include/{asm-x86 => xen}/monitor.h (74%)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 3a90f47..e46be1b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -14,6 +14,10 @@ config X86
 	select HAS_MEM_ACCESS
 	select HAS_MEM_PAGING
 	select HAS_MEM_SHARING
+	select HAS_VM_EVENT_WRITE_CTRLREG
+	select HAS_VM_EVENT_SINGLESTEP
+	select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+	select HAS_VM_EVENT_GUEST_REQUEST
 	select HAS_NS16550
 	select HAS_PASSTHROUGH
 	select HAS_PCI
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 8e6e901..6e80cf0 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,7 +36,7 @@ obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
 obj-y += mm.o x86_64/mm.o
-obj-y += monitor.o
+obj-y += monitor_x86.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index e3444db..04faa72 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -23,8 +23,8 @@
 
 #include <xen/vm_event.h>
 #include <xen/paging.h>
+#include <xen/monitor.h>
 #include <asm/hvm/event.h>
-#include <asm/monitor.h>
 #include <asm/altp2m.h>
 #include <public/vm_event.h>
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 930d0e3..e93a648 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -32,6 +32,7 @@
 #include <xen/guest_access.h>
 #include <xen/event.h>
 #include <xen/paging.h>
+#include <xen/monitor.h>
 #include <xen/cpu.h>
 #include <xen/wait.h>
 #include <xen/mem_access.h>
@@ -51,7 +52,6 @@
 #include <asm/traps.h>
 #include <asm/mc146818rtc.h>
 #include <asm/mce.h>
-#include <asm/monitor.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/vpt.h>
 #include <asm/hvm/support.h>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index cf0e642..be67b60 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -25,6 +25,7 @@
 #include <xen/domain_page.h>
 #include <xen/hypercall.h>
 #include <xen/perfc.h>
+#include <xen/monitor.h>
 #include <asm/current.h>
 #include <asm/io.h>
 #include <asm/iocap.h>
@@ -57,7 +58,6 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/altp2m.h>
 #include <asm/event.h>
-#include <asm/monitor.h>
 #include <public/arch-x86/cpuid.h>
 
 static bool_t __initdata opt_force_ept;
diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c
new file mode 100644
index 0000000..d19fd15
--- /dev/null
+++ b/xen/arch/x86/monitor_x86.c
@@ -0,0 +1,72 @@
+/*
+ * arch/x86/monitor_x86.c
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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 <asm/monitor_arch.h>
+
+bool_t arch_monitor_domctl_event(struct domain *d,
+                                 struct xen_domctl_monitor_op *mop,
+                                 int *rc)
+{
+    struct arch_domain *ad = &d->arch;
+    bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+    switch ( mop->event )
+    {
+        case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
+        {
+            bool_t old_status = ad->monitor.mov_to_msr_enabled;
+
+            if ( unlikely(old_status == requested_status) )
+                return -EEXIST;
+
+            if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
+                 mop->u.mov_to_msr.extended_capture &&
+                 !hvm_enable_msr_exit_interception(d) )
+                return -EOPNOTSUPP;
+
+            domain_pause(d);
+
+            if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
+                 mop->u.mov_to_msr.extended_capture )
+                ad->monitor.mov_to_msr_extended = 1;
+            else
+                ad->monitor.mov_to_msr_extended = 0;
+
+            ad->monitor.mov_to_msr_enabled = !old_status;
+            domain_unpause(d);
+            break;
+        }
+
+        default:
+            return 0;
+    }
+
+    return 1;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6f404b4..172da13 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -36,6 +36,26 @@ config HAS_MEM_PAGING
 config HAS_MEM_SHARING
 	bool
 
+config HAS_VM_EVENT_WRITE_CTRLREG
+	bool
+	---help---
+	  Select if ctrl-reg write monitor vm-events are supported
+
+config HAS_VM_EVENT_SINGLESTEP
+	bool
+	---help---
+	  Select if single-step monitor vm-events are supported
+
+config HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+	bool
+	---help---
+	  Select if software-breakpoint monitor vm-events are supported
+
+config HAS_VM_EVENT_GUEST_REQUEST
+	bool
+	---help---
+	  Select if guest-request monitor vm-events are supported
+
 # Select HAS_PDX if PDX is supported
 config HAS_PDX
 	bool
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 6e82b33..0d76efe 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -20,6 +20,7 @@ obj-y += lib.o
 obj-y += lzo.o
 obj-$(CONFIG_HAS_MEM_ACCESS) += mem_access.o
 obj-y += memory.o
+obj-y += monitor.o
 obj-y += multicall.o
 obj-y += notifier.o
 obj-y += page_alloc.o
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 121a34a..4b1dec1 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -25,11 +25,11 @@
 #include <xen/paging.h>
 #include <xen/hypercall.h>
 #include <xen/vm_event.h>
+#include <xen/monitor.h>
 #include <asm/current.h>
 #include <asm/irq.h>
 #include <asm/page.h>
 #include <asm/p2m.h>
-#include <asm/monitor.h>
 #include <public/domctl.h>
 #include <xsm/xsm.h>
 
diff --git a/xen/arch/x86/monitor.c b/xen/common/monitor.c
similarity index 44%
rename from xen/arch/x86/monitor.c
rename to xen/common/monitor.c
index 1d43880..a4899c3 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/common/monitor.c
@@ -1,9 +1,10 @@
 /*
- * arch/x86/monitor.c
+ * xen/common/monitor.c
  *
- * Architecture-specific monitor_op domctl handler.
+ * Common monitor_op domctl handler.
  *
  * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public
@@ -18,101 +19,66 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/config.h>
-#include <xen/sched.h>
-#include <xen/mm.h>
-#include <asm/domain.h>
-#include <asm/monitor.h>
-#include <public/domctl.h>
+#include <xen/monitor.h>
+#include <xen/sched.h>              /* for domain_pause, ... */
+#include <xen/config.h>             /* for XENLOG_WARNING */
 #include <xsm/xsm.h>
+#include <public/domctl.h>
 
-/*
- * Sanity check whether option is already enabled/disabled
- */
-static inline
-int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
-{
-    bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
-
-    if ( status == requested_status )
-        return -EEXIST;
-
-    return 0;
-}
-
-static inline uint32_t get_capabilities(struct domain *d)
-{
-    uint32_t capabilities = 0;
-
-    /*
-     * At the moment only Intel HVM domains are supported. However, event
-     * delivery could be extended to AMD and PV domains.
-     */
-    if ( !is_hvm_domain(d) || !cpu_has_vmx )
-        return capabilities;
-
-    capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-                   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
-                   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-                   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-    /* Since we know this is on VMX, we can just call the hvm func */
-    if ( hvm_is_singlestep_supported() )
-        capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
-    return capabilities;
-}
+#include <asm/monitor_arch.h>       /* for monitor_arch_# */
+#if CONFIG_X86
+#include <public/vm_event.h>        /* for VM_EVENT_X86_CR3 */
+#include <asm/hvm/hvm.h>            /* for hvm_update_guest_cr, ... */
+#endif
 
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 {
     int rc;
-    struct arch_domain *ad = &d->arch;
-    uint32_t capabilities = get_capabilities(d);
+    bool_t requested_status = 0;
 
-    if ( current->domain == d ) /* no domain_pause() */
+    if ( unlikely(current->domain == d) ) /* no domain_pause() */
         return -EPERM;
 
     rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
-    if ( rc )
+    if ( unlikely(rc) )
         return rc;
 
     switch ( mop->op )
     {
-    case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
-        mop->event = capabilities;
-        return 0;
+    case XEN_DOMCTL_MONITOR_OP_ENABLE:
+        requested_status = 1;
+        /* fallthrough */
+    case XEN_DOMCTL_MONITOR_OP_DISABLE:
+        /* Check if event type is available. */
+        if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) )
+            return -EOPNOTSUPP;
+        break;
 
-    case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
-        domain_pause(d);
-        ad->mem_access_emulate_each_rep = !!mop->event;
-        domain_unpause(d);
+    case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
+        mop->event = arch_monitor_get_capabilities(d);
         return 0;
-    }
-
-    /*
-     * Sanity check
-     */
-    if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
-         mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
-        return -EOPNOTSUPP;
 
-    /* Check if event type is available. */
-    if ( !(capabilities & (1 << mop->event)) )
+    default:
+        /* The monitor op is probably handled on the arch-side. */
+        if ( likely(arch_monitor_domctl_op(d, mop, &rc)) )
+            return rc;
+        /* unrecognized op */
         return -EOPNOTSUPP;
+    }
 
     switch ( mop->event )
     {
+#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG
     case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
     {
+        struct arch_domain *ad = &d->arch;
         unsigned int ctrlreg_bitmask =
             monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
-        bool_t status =
+        bool_t old_status =
             !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
-        struct vcpu *v;
 
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
 
         domain_pause(d);
 
@@ -126,93 +92,92 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         else
             ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
 
-        if ( !status )
+        if ( !old_status )
             ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
         else
             ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
 
-        if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 )
-            /* Latches new CR3 mask through CR0 code */
+#if CONFIG_X86
+        if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
+        {
+            struct vcpu *v;
+            /* Latches new CR3 mask through CR0 code. */
             for_each_vcpu ( d, v )
                 hvm_update_guest_cr(v, 0);
+        }
+#endif
 
         domain_unpause(d);
 
         break;
     }
+#endif // HAS_VM_EVENT_WRITE_CTRLREG
 
-    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
-    {
-        bool_t status = ad->monitor.mov_to_msr_enabled;
-
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
-
-        if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
-             mop->u.mov_to_msr.extended_capture &&
-             !hvm_enable_msr_exit_interception(d) )
-            return -EOPNOTSUPP;
-
-        domain_pause(d);
-
-        if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
-             mop->u.mov_to_msr.extended_capture )
-                ad->monitor.mov_to_msr_extended = 1;
-        else
-            ad->monitor.mov_to_msr_extended = 0;
-
-        ad->monitor.mov_to_msr_enabled = !status;
-        domain_unpause(d);
-        break;
-    }
-
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP
     case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
     {
-        bool_t status = ad->monitor.singlestep_enabled;
+        struct arch_domain *ad = &d->arch;
+        bool_t old_status = ad->monitor.singlestep_enabled;
 
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.singlestep_enabled = !status;
+        ad->monitor.singlestep_enabled = !old_status;
         domain_unpause(d);
         break;
     }
+#endif // HAS_VM_EVENT_SINGLESTEP
 
+#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
     case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
     {
-        bool_t status = ad->monitor.software_breakpoint_enabled;
+        struct arch_domain *ad = &d->arch;
+        bool_t old_status = ad->monitor.software_breakpoint_enabled;
 
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.software_breakpoint_enabled = !status;
+        ad->monitor.software_breakpoint_enabled = !old_status;
         domain_unpause(d);
         break;
     }
+#endif // HAS_VM_EVENT_SOFTWARE_BREAKPOINT
 
+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
     case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
     {
-        bool_t status = ad->monitor.guest_request_enabled;
+        struct arch_domain *ad = &d->arch;
+        bool_t old_status = ad->monitor.guest_request_enabled;
 
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
 
         domain_pause(d);
         ad->monitor.guest_request_sync = mop->u.guest_request.sync;
-        ad->monitor.guest_request_enabled = !status;
+        ad->monitor.guest_request_enabled = !old_status;
         domain_unpause(d);
         break;
     }
+#endif // HAS_VM_EVENT_GUEST_REQUEST
 
     default:
-        return -EOPNOTSUPP;
+        /* Give arch-side the chance to handle this event */
+        if ( likely(arch_monitor_domctl_event(d, mop, &rc)) )
+            return rc;
+
+        /*
+         * Should not be reached unless arch_monitor_get_capabilities() is not
+         * properly implemented. In that case, since reaching this point does
+         * not really break anything, don't crash the hypervisor, issue a
+         * warning instead of BUG().
+         */
+        printk(XENLOG_WARNING
+                "WARNING, BUG: arch_monitor_get_capabilities() not implemented"
+                "properly.\n");
 
+        return -EOPNOTSUPP;
     };
 
     return 0;
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor_arch.h
similarity index 46%
rename from xen/include/asm-arm/monitor.h
rename to xen/include/asm-arm/monitor_arch.h
index a3a9703..d0df66c 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor_arch.h
@@ -1,9 +1,10 @@
 /*
- * include/asm-arm/monitor.h
+ * include/asm-arm/monitor_arch.h
  *
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
  *
  * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public
@@ -18,16 +19,35 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_ARM_MONITOR_H__
-#define __ASM_ARM_MONITOR_H__
+#ifndef __ASM_ARM_MONITOR_ARCH_H__
+#define __ASM_ARM_MONITOR_ARCH_H__
 
 #include <xen/sched.h>
 #include <public/domctl.h>
 
 static inline
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op)
+uint32_t arch_monitor_get_capabilities(struct domain *d)
 {
-    return -ENOSYS;
+    /* No monitor vm-events implemented on ARM. */
+    return 0;
 }
 
-#endif /* __ASM_X86_MONITOR_H__ */
+static inline
+bool_t arch_monitor_domctl_op(struct domain *d,
+                              struct xen_domctl_monitor_op *mop,
+                              int *rc)
+{
+    /* No arch-specific monitor ops on ARM. */
+    return 0;
+}
+
+static inline
+bool_t arch_monitor_domctl_event(struct domain *d,
+                                 struct xen_domctl_monitor_op *mop,
+                                 int *rc)
+{
+    /* No arch-specific monitor vm-events on ARM. */
+    return 0;
+}
+
+#endif /* __ASM_ARM_MONITOR_ARCH_H__ */
diff --git a/xen/include/asm-x86/monitor_arch.h b/xen/include/asm-x86/monitor_arch.h
new file mode 100644
index 0000000..d9daf65
--- /dev/null
+++ b/xen/include/asm-x86/monitor_arch.h
@@ -0,0 +1,74 @@
+/*
+ * include/asm-x86/monitor_arch.h
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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/>.
+ */
+
+#ifndef __ASM_X86_MONITOR_ARCH_H__
+#define __ASM_X86_MONITOR_ARCH_H__
+
+#include <xen/sched.h>              /* for struct domain, is_hvm_domain, ... */
+#include <public/domctl.h>          /* for XEN_DOMCTL_MONITOR_#, ... */
+#include <asm/cpufeature.h>         /* for cpu_has_vmx */
+#include <asm/hvm/hvm.h>            /* for hvm_is_singlestep_supported */
+
+static inline
+uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+    uint32_t capabilities = 0;
+
+    /*
+     * At the moment only Intel HVM domains are supported. However, event
+     * delivery could be extended to AMD and PV domains.
+     */
+    if ( !is_hvm_domain(d) || !cpu_has_vmx )
+        return capabilities;
+
+    capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
+                   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
+                   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
+                   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+
+    /* Since we know this is on VMX, we can just call the hvm func */
+    if ( hvm_is_singlestep_supported() )
+        capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+
+    return capabilities;
+}
+
+static inline
+bool_t arch_monitor_domctl_op(struct domain *d,
+                              struct xen_domctl_monitor_op *mop,
+                              int *rc)
+{
+    if( likely(XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP == mop->op) )
+    {
+        domain_pause(d);
+        d->arch.mem_access_emulate_each_rep = !!mop->event;
+        domain_unpause(d);
+        *rc = 0;
+        return 1;
+    }
+    return 0;
+}
+
+bool_t arch_monitor_domctl_event(struct domain *d,
+                                 struct xen_domctl_monitor_op *mop,
+                                 int *rc);
+
+#endif /* __ASM_X86_MONITOR_ARCH_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/xen/monitor.h
similarity index 74%
rename from xen/include/asm-x86/monitor.h
rename to xen/include/xen/monitor.h
index 7c8280b..edeff78 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/xen/monitor.h
@@ -1,9 +1,10 @@
 /*
- * include/asm-x86/monitor.h
+ * include/xen/monitor.h
  *
- * Architecture-specific monitor_op domctl handler.
+ * Common monitor_op domctl handler.
  *
  * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public
@@ -18,14 +19,16 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_X86_MONITOR_H__
-#define __ASM_X86_MONITOR_H__
+#ifndef __MONITOR_H__
+#define __MONITOR_H__
 
-struct domain;
-struct xen_domctl_monitor_op;
+#include <xen/sched.h>
+#include <public/domctl.h>
 
+#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG
 #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
+#endif
 
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
 
-#endif /* __ASM_X86_MONITOR_H__ */
+#endif /* __MONITOR_H__ */
-- 
2.5.0

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Jan Beulich]

>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote:
>  xen/arch/x86/Kconfig                              |   4 +
>  xen/arch/x86/Makefile                             |   2 +-
>  xen/arch/x86/hvm/event.c                          |   2 +-
>  xen/arch/x86/hvm/hvm.c                            |   2 +-
>  xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>  xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>  xen/common/Kconfig                                |  20 +++
>  xen/common/Makefile                               |   1 +
>  xen/common/domctl.c                               |   2 +-
>  xen/{arch/x86 => common}/monitor.c                | 195 +++++++++-------------
>  xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
>  xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>  xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
>  13 files changed, 293 insertions(+), 134 deletions(-)
>  create mode 100644 xen/arch/x86/monitor_x86.c
>  rename xen/{arch/x86 => common}/monitor.c (44%)
>  rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>  create mode 100644 xen/include/asm-x86/monitor_arch.h
>  rename xen/include/{asm-x86 => xen}/monitor.h (74%)

With percentages as low as 44 I'm not sure all this strange
renaming and introduction of oddly named new files is actually a
good idea.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -14,6 +14,10 @@ config X86
>  	select HAS_MEM_ACCESS
>  	select HAS_MEM_PAGING
>  	select HAS_MEM_SHARING
> +	select HAS_VM_EVENT_WRITE_CTRLREG
> +	select HAS_VM_EVENT_SINGLESTEP
> +	select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
> +	select HAS_VM_EVENT_GUEST_REQUEST
>  	select HAS_NS16550
>  	select HAS_PASSTHROUGH
>  	select HAS_PCI

Please don't break the alphabetic ordering.

> --- a/xen/arch/x86/monitor.c
> +++ b/xen/common/monitor.c
> @@ -1,9 +1,10 @@
>  /*
> - * arch/x86/monitor.c
> + * xen/common/monitor.c
>   *
> - * Architecture-specific monitor_op domctl handler.
> + * Common monitor_op domctl handler.
>   *
>   * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public
> @@ -18,101 +19,66 @@
>   * License along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <xen/config.h>
> -#include <xen/sched.h>
> -#include <xen/mm.h>
> -#include <asm/domain.h>
> -#include <asm/monitor.h>
> -#include <public/domctl.h>
> +#include <xen/monitor.h>
> +#include <xen/sched.h>              /* for domain_pause, ... */
> +#include <xen/config.h>             /* for XENLOG_WARNING */

Please simply drop this include, it's being automatically included via
compiler command line option. Also please avoid comments like this
unless they explain an otherwise unexpected or unreasonable
inclusion.

Jan

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Jan Beulich]

>>> On 10.02.16 at 17:26, <JBeulich@suse.com> wrote:
>>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote:
>>  xen/arch/x86/Kconfig                              |   4 +
>>  xen/arch/x86/Makefile                             |   2 +-
>>  xen/arch/x86/hvm/event.c                          |   2 +-
>>  xen/arch/x86/hvm/hvm.c                            |   2 +-
>>  xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>>  xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>>  xen/common/Kconfig                                |  20 +++
>>  xen/common/Makefile                               |   1 +
>>  xen/common/domctl.c                               |   2 +-
>>  xen/{arch/x86 => common}/monitor.c                | 195 +++++++++-------------
>>  xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
>>  xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>>  xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
>>  13 files changed, 293 insertions(+), 134 deletions(-)
>>  create mode 100644 xen/arch/x86/monitor_x86.c
>>  rename xen/{arch/x86 => common}/monitor.c (44%)
>>  rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>>  create mode 100644 xen/include/asm-x86/monitor_arch.h
>>  rename xen/include/{asm-x86 => xen}/monitor.h (74%)
> 
> With percentages as low as 44 I'm not sure all this strange
> renaming and introduction of oddly named new files is actually a
> good idea.

Also it looks like this moving around of files needs to be
accompanied by adjustments to ./MAINTAINERS.

Jan

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 6:30 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 17:26, <JBeulich@suse.com> wrote:
>>>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote:
>>>   xen/arch/x86/Kconfig                              |   4 +
>>>   xen/arch/x86/Makefile                             |   2 +-
>>>   xen/arch/x86/hvm/event.c                          |   2 +-
>>>   xen/arch/x86/hvm/hvm.c                            |   2 +-
>>>   xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>>>   xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>>>   xen/common/Kconfig                                |  20 +++
>>>   xen/common/Makefile                               |   1 +
>>>   xen/common/domctl.c                               |   2 +-
>>>   xen/{arch/x86 => common}/monitor.c                | 195 +++++++++-------------
>>>   xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
>>>   xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>>>   xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
>>>   13 files changed, 293 insertions(+), 134 deletions(-)
>>>   create mode 100644 xen/arch/x86/monitor_x86.c
>>>   rename xen/{arch/x86 => common}/monitor.c (44%)
>>>   rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>>>   create mode 100644 xen/include/asm-x86/monitor_arch.h
>>>   rename xen/include/{asm-x86 => xen}/monitor.h (74%)
>> With percentages as low as 44 I'm not sure all this strange
>> renaming and introduction of oddly named new files is actually a
>> good idea.
> Also it looks like this moving around of files needs to be
> accompanied by adjustments to ./MAINTAINERS.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Will check that, indeed these might have broken something there.

Corneliu.

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 6:26 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote:
>>   xen/arch/x86/Kconfig                              |   4 +
>>   xen/arch/x86/Makefile                             |   2 +-
>>   xen/arch/x86/hvm/event.c                          |   2 +-
>>   xen/arch/x86/hvm/hvm.c                            |   2 +-
>>   xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>>   xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>>   xen/common/Kconfig                                |  20 +++
>>   xen/common/Makefile                               |   1 +
>>   xen/common/domctl.c                               |   2 +-
>>   xen/{arch/x86 => common}/monitor.c                | 195 +++++++++-------------
>>   xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
>>   xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>>   xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
>>   13 files changed, 293 insertions(+), 134 deletions(-)
>>   create mode 100644 xen/arch/x86/monitor_x86.c
>>   rename xen/{arch/x86 => common}/monitor.c (44%)
>>   rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>>   create mode 100644 xen/include/asm-x86/monitor_arch.h
>>   rename xen/include/{asm-x86 => xen}/monitor.h (74%)
> With percentages as low as 44 I'm not sure all this strange
> renaming and introduction of oddly named new files is actually a
> good idea.
The diff would have actually looked a lot worse if I didn't do that, at 
least IMO.
The "strange renaming and ..." was an effort I made to make reviewing these
patches easier :). The reason is explained in the introductory message 
and in this commit
message.
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -14,6 +14,10 @@ config X86
>>   	select HAS_MEM_ACCESS
>>   	select HAS_MEM_PAGING
>>   	select HAS_MEM_SHARING
>> +	select HAS_VM_EVENT_WRITE_CTRLREG
>> +	select HAS_VM_EVENT_SINGLESTEP
>> +	select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
>> +	select HAS_VM_EVENT_GUEST_REQUEST
>>   	select HAS_NS16550
>>   	select HAS_PASSTHROUGH
>>   	select HAS_PCI
> Please don't break the alphabetic ordering.

Noted. Didn't realise they were in alphabetic order.

> +#include <xen/config.h> /* for XENLOG_WARNING */
> Please simply drop this include, it's being automatically included via
> compiler command line option. Also please avoid comments like this
> unless they explain an otherwise unexpected or unreasonable
> inclusion.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Noted, noted.

That you,
Corneliu.

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 9136 bytes --]

On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:

> 1. Kconfig:
>   * Added Kconfigs for common monitor vm-events:
>   # see files: common/Kconfig, x86/Kconfig
>     HAS_VM_EVENT_WRITE_CTRLREG
>     HAS_VM_EVENT_SINGLESTEP
>     HAS_VM_EVENT_SOFTWARE_BREAKPOINT
>     HAS_VM_EVENT_GUEST_REQUEST
>
> 2. Moved monitor_domctl from arch-side to common-side
>   2.1. Moved arch/x86/monitor.c to common/monitor.c
>     # see files: arch/x86/Makefile, xen/common/Makefile,
> xen/common/monitor.c
>     # changes:
>         - removed status_check (we would have had to duplicate it in X86
>             arch_monitor_domctl_event otherwise)
>         - moved get_capabilities to arch-side
> (arch_monitor_get_capabilities)
>         - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see
>             arch_monitor_domctl_op)
>         - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
>             arch_monitor_domctl_event)
>         - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*
>
>   2.2. Moved asm-x86/monitor.h to xen/monitor.h
>     # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
>                  arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c
>
>   2.3. Removed asm-arm/monitor.h (no longer needed)
>
> 3. Added x86/monitor_x86.c => will rename in next commit to monitor.c (not
> done
> in this commit to avoid git seeing this as being the modified old
> monitor.c =>
> keeping the same name would have rendered an unnecessarily bulky diff)
>     # see files: arch/x86/Makefile
>     # implements X86-side arch_monitor_domctl_event
>
> 4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
> monitor.h in next commit, reason is the same as @ (3.).
>     # define/implement: arch_monitor_get_capabilities,
> arch_monitor_domctl_op
>         and arch_monitor_domctl_event
>

So these commit messages are not very good IMHO. Rather then just
summarizing what the patch does you should describe why the patch is needed
in the first place. Usually for longer series having a cover page is also
helpful to outline how the patches in the series fit together.


>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>  xen/arch/x86/Kconfig                              |   4 +
>  xen/arch/x86/Makefile                             |   2 +-
>  xen/arch/x86/hvm/event.c                          |   2 +-
>  xen/arch/x86/hvm/hvm.c                            |   2 +-
>  xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>  xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>  xen/common/Kconfig                                |  20 +++
>  xen/common/Makefile                               |   1 +
>  xen/common/domctl.c                               |   2 +-
>  xen/{arch/x86 => common}/monitor.c                | 195
> +++++++++-------------
>  xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
>  xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>  xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
>  13 files changed, 293 insertions(+), 134 deletions(-)
>  create mode 100644 xen/arch/x86/monitor_x86.c
>  rename xen/{arch/x86 => common}/monitor.c (44%)
>  rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>  create mode 100644 xen/include/asm-x86/monitor_arch.h
>  rename xen/include/{asm-x86 => xen}/monitor.h (74%)
>
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 3a90f47..e46be1b 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -14,6 +14,10 @@ config X86
>         select HAS_MEM_ACCESS
>         select HAS_MEM_PAGING
>         select HAS_MEM_SHARING
> +       select HAS_VM_EVENT_WRITE_CTRLREG
>

You mentioned in the previous revision of this series that currently you
only have plans to implement write_ctrleg and guest_request events for ARM.
I think singlestep and software_breakpoint should not be moved to common
without a clear plan to have those implemented. Now, if you were to include
the implementation of write_ctrlreg and guest_request in this series and
leave the others in x86 as they are now, I don't think there would be any
reason to have these Kconfig options present at all.


> +       select HAS_VM_EVENT_SINGLESTEP
> +       select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
> +       select HAS_VM_EVENT_GUEST_REQUEST
>         select HAS_NS16550
>         select HAS_PASSTHROUGH
>         select HAS_PCI
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 8e6e901..6e80cf0 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -36,7 +36,7 @@ obj-y += microcode_intel.o
>  # This must come after the vendor specific files.
>  obj-y += microcode.o
>  obj-y += mm.o x86_64/mm.o
> -obj-y += monitor.o
> +obj-y += monitor_x86.o
>  obj-y += mpparse.o
>  obj-y += nmi.o
>  obj-y += numa.o
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index e3444db..04faa72 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -23,8 +23,8 @@
>
>  #include <xen/vm_event.h>
>  #include <xen/paging.h>
> +#include <xen/monitor.h>
>  #include <asm/hvm/event.h>
> -#include <asm/monitor.h>
>  #include <asm/altp2m.h>
>  #include <public/vm_event.h>
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 930d0e3..e93a648 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -32,6 +32,7 @@
>  #include <xen/guest_access.h>
>  #include <xen/event.h>
>  #include <xen/paging.h>
> +#include <xen/monitor.h>
>  #include <xen/cpu.h>
>  #include <xen/wait.h>
>  #include <xen/mem_access.h>
> @@ -51,7 +52,6 @@
>  #include <asm/traps.h>
>  #include <asm/mc146818rtc.h>
>  #include <asm/mce.h>
> -#include <asm/monitor.h>
>  #include <asm/hvm/hvm.h>
>  #include <asm/hvm/vpt.h>
>  #include <asm/hvm/support.h>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index cf0e642..be67b60 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -25,6 +25,7 @@
>  #include <xen/domain_page.h>
>  #include <xen/hypercall.h>
>  #include <xen/perfc.h>
> +#include <xen/monitor.h>
>  #include <asm/current.h>
>  #include <asm/io.h>
>  #include <asm/iocap.h>
> @@ -57,7 +58,6 @@
>  #include <asm/hvm/nestedhvm.h>
>  #include <asm/altp2m.h>
>  #include <asm/event.h>
> -#include <asm/monitor.h>
>  #include <public/arch-x86/cpuid.h>
>
>  static bool_t __initdata opt_force_ept;
> diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c
> new file mode 100644
> index 0000000..d19fd15
> --- /dev/null
> +++ b/xen/arch/x86/monitor_x86.c
> @@ -0,0 +1,72 @@
> +/*
> + * arch/x86/monitor_x86.c
> + *
> + * Arch-specific monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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 <asm/monitor_arch.h>
> +
> +bool_t arch_monitor_domctl_event(struct domain *d,
> +                                 struct xen_domctl_monitor_op *mop,
> +                                 int *rc)
> +{
> +    struct arch_domain *ad = &d->arch;
> +    bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
> +
> +    switch ( mop->event )
> +    {
> +        case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
> +        {
> +            bool_t old_status = ad->monitor.mov_to_msr_enabled;
> +
> +            if ( unlikely(old_status == requested_status) )
> +                return -EEXIST;
>

This function is defined to return bool_t and yet you are returning
non-boolean error codes. I think it would be better if this function just
had a single rc instead of two (not passing one rc as a pointer on input).


> +
> +            if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
> +                 mop->u.mov_to_msr.extended_capture &&
> +                 !hvm_enable_msr_exit_interception(d) )
> +                return -EOPNOTSUPP;
> +
> +            domain_pause(d);
> +
> +            if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
> +                 mop->u.mov_to_msr.extended_capture )
> +                ad->monitor.mov_to_msr_extended = 1;
> +            else
> +                ad->monitor.mov_to_msr_extended = 0;
> +
> +            ad->monitor.mov_to_msr_enabled = !old_status;
> +            domain_unpause(d);
> +            break;
> +        }
> +
> +        default:
> +            return 0;
> +    }
> +
> +    return 1;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>

[-- Attachment #1.2: Type: text/html, Size: 11601 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 6801 bytes --]

On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:
>
>
> On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     1. Kconfig:
>       * Added Kconfigs for common monitor vm-events:
>       # see files: common/Kconfig, x86/Kconfig
>         HAS_VM_EVENT_WRITE_CTRLREG
>         HAS_VM_EVENT_SINGLESTEP
>         HAS_VM_EVENT_SOFTWARE_BREAKPOINT
>         HAS_VM_EVENT_GUEST_REQUEST
>
>     2. Moved monitor_domctl from arch-side to common-side
>       2.1. Moved arch/x86/monitor.c to common/monitor.c
>         # see files: arch/x86/Makefile, xen/common/Makefile,
>     xen/common/monitor.c
>         # changes:
>             - removed status_check (we would have had to duplicate it
>     in X86
>                 arch_monitor_domctl_event otherwise)
>             - moved get_capabilities to arch-side
>     (arch_monitor_get_capabilities)
>             - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to
>     arch-side (see
>                 arch_monitor_domctl_op)
>             - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
>                 arch_monitor_domctl_event)
>             - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*
>
>       2.2. Moved asm-x86/monitor.h to xen/monitor.h
>         # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
>                      arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c
>
>       2.3. Removed asm-arm/monitor.h (no longer needed)
>
>     3. Added x86/monitor_x86.c => will rename in next commit to
>     monitor.c (not done
>     in this commit to avoid git seeing this as being the modified old
>     monitor.c =>
>     keeping the same name would have rendered an unnecessarily bulky diff)
>         # see files: arch/x86/Makefile
>         # implements X86-side arch_monitor_domctl_event
>
>     4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
>     monitor.h in next commit, reason is the same as @ (3.).
>         # define/implement: arch_monitor_get_capabilities,
>     arch_monitor_domctl_op
>             and arch_monitor_domctl_event
>
>
> So these commit messages are not very good IMHO. Rather then just 
> summarizing what the patch does you should describe why the patch is 
> needed in the first place. Usually for longer series having a cover 
> page is also helpful to outline how the patches in the series fit 
> together.

The intention was to make review easier (following the changes in 
parallel w/
the commit message). But indeed I was maybe too focused on that, should have
started w/ stating the purpose of these changes rather than jumping 
directly to detailing
them. Will try to compact these commit messages next time (maybe move 
details to the
introductory section instead, as you suggest).

>
>     Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
>     <mailto:czuzu@bitdefender.com>>
>     ---
>      xen/arch/x86/Kconfig                              |   4 +
>      xen/arch/x86/Makefile                             |   2 +-
>      xen/arch/x86/hvm/event.c                          |   2 +-
>      xen/arch/x86/hvm/hvm.c                            |   2 +-
>      xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>      xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>      xen/common/Kconfig                                |  20 +++
>      xen/common/Makefile                               |   1 +
>      xen/common/domctl.c                               |   2 +-
>      xen/{arch/x86 => common}/monitor.c                | 195
>     +++++++++-------------
>      xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++-
>      xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>      xen/include/{asm-x86 => xen}/monitor.h            | 17 +-
>      13 files changed, 293 insertions(+), 134 deletions(-)
>      create mode 100644 xen/arch/x86/monitor_x86.c
>      rename xen/{arch/x86 => common}/monitor.c (44%)
>      rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>      create mode 100644 xen/include/asm-x86/monitor_arch.h
>      rename xen/include/{asm-x86 => xen}/monitor.h (74%)
>
>     diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>     index 3a90f47..e46be1b 100644
>     --- a/xen/arch/x86/Kconfig
>     +++ b/xen/arch/x86/Kconfig
>     @@ -14,6 +14,10 @@ config X86
>             select HAS_MEM_ACCESS
>             select HAS_MEM_PAGING
>             select HAS_MEM_SHARING
>     +       select HAS_VM_EVENT_WRITE_CTRLREG
>
>
> You mentioned in the previous revision of this series that currently 
> you only have plans to implement write_ctrleg and guest_request events 
> for ARM. I think singlestep and software_breakpoint should not be 
> moved to common without a clear plan to have those implemented. Now, 
> if you were to include the implementation of write_ctrlreg and 
> guest_request in this series and leave the others in x86 as they are 
> now, I don't think there would be any reason to have these Kconfig 
> options present at all.

Moving what made sense to be moved to common was a suggestion of Ian's.
The purpose of this patch is to --avoid-- having to go through this 
process again
when an implementation of feature X for architecture A != X86 comes into 
place.
IMHO what is common should stay in common and I don't see any issues w/
having them there, only advantages (future implementations of these 
features will
be easier).
Maybe Ian can chime in on this.

Regarding single-step & software-breakpoint monitor vm-events for ARM, that
should be very feasible IMO, as I detailed in an email I sent to you in 
v1, in which
I was pointing how we could use the debugging architecture.

>
>     +bool_t arch_monitor_domctl_event(struct domain *d,
>     +                                 struct xen_domctl_monitor_op *mop,
>     +                                 int *rc)
>     +{
>     +    struct arch_domain *ad = &d->arch;
>     +    bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE ==
>     mop->op);
>     +
>     +    switch ( mop->event )
>     +    {
>     +        case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
>     +        {
>     +            bool_t old_status = ad->monitor.mov_to_msr_enabled;
>     +
>     +            if ( unlikely(old_status == requested_status) )
>     +                return -EEXIST;
>
>
> This function is defined to return bool_t and yet you are returning 
> non-boolean error codes. I think it would be better if this function 
> just had a single rc instead of two (not passing one rc as a pointer 
> on input).

That's a baad mistake, good catch. It should be "*rc = -EEXIST; return 
1", will change in v3.

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

Thank you,
Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 11227 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 5798 bytes --]

On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:

>
> On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:
>
>
>
> On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <czuzu@bitdefender.com>
> wrote:
>
>> 1. Kconfig:
>>   * Added Kconfigs for common monitor vm-events:
>>   # see files: common/Kconfig, x86/Kconfig
>>     HAS_VM_EVENT_WRITE_CTRLREG
>>     HAS_VM_EVENT_SINGLESTEP
>>     HAS_VM_EVENT_SOFTWARE_BREAKPOINT
>>     HAS_VM_EVENT_GUEST_REQUEST
>>
>> 2. Moved monitor_domctl from arch-side to common-side
>>   2.1. Moved arch/x86/monitor.c to common/monitor.c
>>     # see files: arch/x86/Makefile, xen/common/Makefile,
>> xen/common/monitor.c
>>     # changes:
>>         - removed status_check (we would have had to duplicate it in X86
>>             arch_monitor_domctl_event otherwise)
>>         - moved get_capabilities to arch-side
>> (arch_monitor_get_capabilities)
>>         - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see
>>             arch_monitor_domctl_op)
>>         - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
>>             arch_monitor_domctl_event)
>>         - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*
>>
>>   2.2. Moved asm-x86/monitor.h to xen/monitor.h
>>     # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
>>                  arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c
>>
>>   2.3. Removed asm-arm/monitor.h (no longer needed)
>>
>> 3. Added x86/monitor_x86.c => will rename in next commit to monitor.c
>> (not done
>> in this commit to avoid git seeing this as being the modified old
>> monitor.c =>
>> keeping the same name would have rendered an unnecessarily bulky diff)
>>     # see files: arch/x86/Makefile
>>     # implements X86-side arch_monitor_domctl_event
>>
>> 4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
>> monitor.h in next commit, reason is the same as @ (3.).
>>     # define/implement: arch_monitor_get_capabilities,
>> arch_monitor_domctl_op
>>         and arch_monitor_domctl_event
>>
>
> So these commit messages are not very good IMHO. Rather then just
> summarizing what the patch does you should describe why the patch is needed
> in the first place. Usually for longer series having a cover page is also
> helpful to outline how the patches in the series fit together.
>
>
> The intention was to make review easier (following the changes in parallel
> w/
> the commit message). But indeed I was maybe too focused on that, should
> have
> started w/ stating the purpose of these changes rather than jumping
> directly to detailing
> them. Will try to compact these commit messages next time (maybe move
> details to the
> introductory section instead, as you suggest).
>
>
>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>> ---
>>  xen/arch/x86/Kconfig                              |   4 +
>>  xen/arch/x86/Makefile                             |   2 +-
>>  xen/arch/x86/hvm/event.c                          |   2 +-
>>  xen/arch/x86/hvm/hvm.c                            |   2 +-
>>  xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>>  xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>>  xen/common/Kconfig                                |  20 +++
>>  xen/common/Makefile                               |   1 +
>>  xen/common/domctl.c                               |   2 +-
>>  xen/{arch/x86 => common}/monitor.c                | 195
>> +++++++++-------------
>>  xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
>>  xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>>  xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
>>  13 files changed, 293 insertions(+), 134 deletions(-)
>>  create mode 100644 xen/arch/x86/monitor_x86.c
>>  rename xen/{arch/x86 => common}/monitor.c (44%)
>>  rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>>  create mode 100644 xen/include/asm-x86/monitor_arch.h
>>  rename xen/include/{asm-x86 => xen}/monitor.h (74%)
>>
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 3a90f47..e46be1b 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -14,6 +14,10 @@ config X86
>>         select HAS_MEM_ACCESS
>>         select HAS_MEM_PAGING
>>         select HAS_MEM_SHARING
>> +       select HAS_VM_EVENT_WRITE_CTRLREG
>>
>
> You mentioned in the previous revision of this series that currently you
> only have plans to implement write_ctrleg and guest_request events for ARM.
> I think singlestep and software_breakpoint should not be moved to common
> without a clear plan to have those implemented. Now, if you were to include
> the implementation of write_ctrlreg and guest_request in this series and
> leave the others in x86 as they are now, I don't think there would be any
> reason to have these Kconfig options present at all.
>
>
> Moving what made sense to be moved to common was a suggestion of Ian's.
> The purpose of this patch is to --avoid-- having to go through this
> process again
> when an implementation of feature X for architecture A != X86 comes into
> place.
> IMHO what is common should stay in common and I don't see any issues w/
> having them there, only advantages (future implementations of these
> features will
> be easier).
> Maybe Ian can chime in on this.
>

That's the upside. The downside is that in the interim, while those
features are not implemented, we need to add a bunch of Kconfig variables
to decide under what build they are available. If it was moved to common
only when the feature is available for all architectures, we wouldn't need
that many ifdefs and the code would be clearer. So I do see why it would be
beneficial having these moved to common now, but I still rather have it
happen when it's necessary and without the Kconfig settings.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 9398 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 4076 bytes --]

On 2/10/2016 7:56 PM, Tamas K Lengyel wrote:
>
>
> On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>
>     On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:
>>
>>
>>         diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>>         index 3a90f47..e46be1b 100644
>>         --- a/xen/arch/x86/Kconfig
>>         +++ b/xen/arch/x86/Kconfig
>>         @@ -14,6 +14,10 @@ config X86
>>                 select HAS_MEM_ACCESS
>>                 select HAS_MEM_PAGING
>>                 select HAS_MEM_SHARING
>>         +       select HAS_VM_EVENT_WRITE_CTRLREG
>>
>>
>>     You mentioned in the previous revision of this series that
>>     currently you only have plans to implement write_ctrleg and
>>     guest_request events for ARM. I think singlestep and
>>     software_breakpoint should not be moved to common without a clear
>>     plan to have those implemented. Now, if you were to include the
>>     implementation of write_ctrlreg and guest_request in this series
>>     and leave the others in x86 as they are now, I don't think there
>>     would be any reason to have these Kconfig options present at all.
>
>     Moving what made sense to be moved to common was a suggestion of
>     Ian's.
>     The purpose of this patch is to --avoid-- having to go through
>     this process again
>     when an implementation of feature X for architecture A != X86
>     comes into place.
>     IMHO what is common should stay in common and I don't see any
>     issues w/
>     having them there, only advantages (future implementations of
>     these features will
>     be easier).
>     Maybe Ian can chime in on this.
>
>
> That's the upside. The downside is that in the interim, while those 
> features are not implemented, we need to add a bunch of Kconfig 
> variables to decide under what build they are available.

Technically, you don't need to add anything unless you implement that 
feature.
E.g. the ARM Kconfig currently contains no mention of these options, 
since they're not implemented there @ all.
And when implemented they're only added once and it's one line "select 
HAS_....", it's not like you have to specify a command-line
parameter when building Xen or something like that, so IMO they don't 
add considerable complexity.
And after all, these kind of situations (i.e. activating/deactivating 
code based on architecture) are why arch-specific Kconfigs exist, right? 
Why not use them? :)

> If it was moved to common only when the feature is available for all 
> architectures, we wouldn't need that many ifdefs and the code would be 
> clearer. So I do see why it would be beneficial having these moved to 
> common now, but I still rather have it happen when it's necessary and 
> without the Kconfig settings.

What if a 3rd architecture comes into place, you'd have to move them 
back from common to the arch-side and get back to the code duplication 
we just got rid of?
And if you then also implement it for the 3rd architecture, you move 
them back to common from the arch-side?
It seems uncomfortable having to keep track of all architectures when 
wanting to add such a feature implementation for just one of them.
I don't know what & if such plans exist for Xen, but why make that 
future process of adding in support for other architectures 
unnecessarily complicated,
even if it won't happen soon?

Also, IMO the code is clearer like this:
* you know what parts interest you when implementing parts of these 
features/when debugging/when simply looking @ the code
* the #ifdefs make it possible for that code to be put in common => that 
makes it *clear* that those code parts are NOT
architecture specific and their implementation can be safely used for 
all architectures.
* code duplication is avoided => avoid having to reflect a modification 
when it happens in more places than it would be necessary

There are disadvantages, everything has a downside but IMHO they are 
minor compared to the alternative.

Ian, any comments on this? :)

Thank you,
Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 7344 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 583 bytes --]

>
> * the #ifdefs make it possible for that code to be put in common => that
> makes it *clear* that those code parts are NOT
> architecture specific and their implementation can be safely used for all
> architectures.
>

The current practice has been to put empty static inline functions into
architecture-specific headers if the part is not handled/implemented for
the specific architecture. This has already been the case for
monitor_domctl (there is already separate asm-arm/monitor.h and
asm-x86/monitor.h) so it should be followed as more of the code moves into
common.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 906 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 1372 bytes --]

On 2/11/2016 5:44 PM, Tamas K Lengyel wrote:
>
>     * the #ifdefs make it possible for that code to be put in common
>     => that makes it *clear* that those code parts are NOT
>     architecture specific and their implementation can be safely used
>     for all architectures.
>
>
> The current practice has been to put empty static inline functions 
> into architecture-specific headers if the part is not 
> handled/implemented for the specific architecture. This has already 
> been the case for monitor_domctl (there is already separate 
> asm-arm/monitor.h and asm-x86/monitor.h) so it should be followed as 
> more of the code moves into common.
>
> Tamas

Point is, they *are* implemented, because that's *common* code, it 
doesn't make sense to be moved to the arch-side
when you know that their implementation will be *the same* from 
arch-to-arch.
Not *everything* needs to stay on the arch-side, just what is 
architecture-specific - that's why e.g. arch_hvm_event_fill_regs,
arch_hvm_event_gfn_of_ip are not in common and are static inline 
functions as you say, because they have *different*
implementations *depending on the architecture*.

Finally, if Ian or any other ARM maintainer feels the same with moving 
code that can be moved and has been moved to common
back on the arch-side (effectively undoing 50% of my efforts),  I will 
do so.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 2518 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 1785 bytes --]

On 2/12/2016 8:05 AM, Corneliu ZUZU wrote:
> On 2/11/2016 5:44 PM, Tamas K Lengyel wrote:
>>
>>     * the #ifdefs make it possible for that code to be put in common
>>     => that makes it *clear* that those code parts are NOT
>>     architecture specific and their implementation can be safely used
>>     for all architectures.
>>
>>
>> The current practice has been to put empty static inline functions 
>> into architecture-specific headers if the part is not 
>> handled/implemented for the specific architecture. This has already 
>> been the case for monitor_domctl (there is already separate 
>> asm-arm/monitor.h and asm-x86/monitor.h) so it should be followed as 
>> more of the code moves into common.
>>
>> Tamas
>
> Point is, they *are* implemented, because that's *common* code, it 
> doesn't make sense to be moved to the arch-side
> when you know that their implementation will be *the same* from 
> arch-to-arch.
> Not *everything* needs to stay on the arch-side, just what is 
> architecture-specific - that's why e.g. arch_hvm_event_fill_regs,
> arch_hvm_event_gfn_of_ip are not in common and are static inline 
> functions as you say, because they have *different*
> implementations *depending on the architecture*.
>
> Finally, if Ian or any other ARM maintainer feels the same with moving 
> code that can be moved and has been moved to common
> back on the arch-side (effectively undoing 50% of my efforts),  I will 
> do so.
>
> Corneliu.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Actually, noted, will move single-step and software-breakpoint back on 
the arch-side as you suggest in v3.
If someone else disagrees, I suppose it will be discussed then.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 3563 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:
> I think it would be better if this function just had a single rc 
> instead of two (not passing one rc as a pointer on input).

Good point. Would it be ok if:
* I remove the rc param
* make return type int
* rc = 0 if arch-side didn't handle the event, but no errors occurred
* rc = 1 if arch-side handled the event and no errors occurred
* rc < 0 if errors occurred
* return rc
?

Didn't cross my mind why error rcs are < 0, I only now realize that
probably just these kind of situations are the reason for that.

Corneliu.

[PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

Rename:
    - arch/x86/monitor_x86.c -> arch/x86/monitor.c
    - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h

(previous commit explains why these renames were necessary)

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/Makefile                             | 2 +-
 xen/arch/x86/{monitor_x86.c => monitor.c}         | 4 ++--
 xen/common/monitor.c                              | 2 +-
 xen/include/asm-arm/{monitor_arch.h => monitor.h} | 8 ++++----
 xen/include/asm-x86/{monitor_arch.h => monitor.h} | 8 ++++----
 5 files changed, 12 insertions(+), 12 deletions(-)
 rename xen/arch/x86/{monitor_x86.c => monitor.c} (97%)
 rename xen/include/asm-arm/{monitor_arch.h => monitor.h} (90%)
 rename xen/include/asm-x86/{monitor_arch.h => monitor.h} (94%)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 6e80cf0..8e6e901 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,7 +36,7 @@ obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
 obj-y += mm.o x86_64/mm.o
-obj-y += monitor_x86.o
+obj-y += monitor.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor.c
similarity index 97%
rename from xen/arch/x86/monitor_x86.c
rename to xen/arch/x86/monitor.c
index d19fd15..568def2 100644
--- a/xen/arch/x86/monitor_x86.c
+++ b/xen/arch/x86/monitor.c
@@ -1,5 +1,5 @@
 /*
- * arch/x86/monitor_x86.c
+ * arch/x86/monitor.c
  *
  * Arch-specific monitor_op domctl handler.
  *
@@ -19,7 +19,7 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <asm/monitor_arch.h>
+#include <asm/monitor.h>
 
 bool_t arch_monitor_domctl_event(struct domain *d,
                                  struct xen_domctl_monitor_op *mop,
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index a4899c3..03063bb 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -25,7 +25,7 @@
 #include <xsm/xsm.h>
 #include <public/domctl.h>
 
-#include <asm/monitor_arch.h>       /* for monitor_arch_# */
+#include <asm/monitor.h>            /* for monitor_arch_# */
 #if CONFIG_X86
 #include <public/vm_event.h>        /* for VM_EVENT_X86_CR3 */
 #include <asm/hvm/hvm.h>            /* for hvm_update_guest_cr, ... */
diff --git a/xen/include/asm-arm/monitor_arch.h b/xen/include/asm-arm/monitor.h
similarity index 90%
rename from xen/include/asm-arm/monitor_arch.h
rename to xen/include/asm-arm/monitor.h
index d0df66c..eb770da 100644
--- a/xen/include/asm-arm/monitor_arch.h
+++ b/xen/include/asm-arm/monitor.h
@@ -1,5 +1,5 @@
 /*
- * include/asm-arm/monitor_arch.h
+ * include/asm-arm/monitor.h
  *
  * Arch-specific monitor_op domctl handler.
  *
@@ -19,8 +19,8 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_ARM_MONITOR_ARCH_H__
-#define __ASM_ARM_MONITOR_ARCH_H__
+#ifndef __ASM_ARM_MONITOR_H__
+#define __ASM_ARM_MONITOR_H__
 
 #include <xen/sched.h>
 #include <public/domctl.h>
@@ -50,4 +50,4 @@ bool_t arch_monitor_domctl_event(struct domain *d,
     return 0;
 }
 
-#endif /* __ASM_ARM_MONITOR_ARCH_H__ */
+#endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-x86/monitor_arch.h b/xen/include/asm-x86/monitor.h
similarity index 94%
rename from xen/include/asm-x86/monitor_arch.h
rename to xen/include/asm-x86/monitor.h
index d9daf65..b12823c 100644
--- a/xen/include/asm-x86/monitor_arch.h
+++ b/xen/include/asm-x86/monitor.h
@@ -1,5 +1,5 @@
 /*
- * include/asm-x86/monitor_arch.h
+ * include/asm-x86/monitor.h
  *
  * Arch-specific monitor_op domctl handler.
  *
@@ -19,8 +19,8 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_X86_MONITOR_ARCH_H__
-#define __ASM_X86_MONITOR_ARCH_H__
+#ifndef __ASM_X86_MONITOR_H__
+#define __ASM_X86_MONITOR_H__
 
 #include <xen/sched.h>              /* for struct domain, is_hvm_domain, ... */
 #include <public/domctl.h>          /* for XEN_DOMCTL_MONITOR_#, ... */
@@ -71,4 +71,4 @@ bool_t arch_monitor_domctl_event(struct domain *d,
                                  struct xen_domctl_monitor_op *mop,
                                  int *rc);
 
-#endif /* __ASM_X86_MONITOR_ARCH_H__ */
+#endif /* __ASM_X86_MONITOR_H__ */
-- 
2.5.0

Re: [PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 694 bytes --]

On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:

> Rename:
>     - arch/x86/monitor_x86.c -> arch/x86/monitor.c
>     - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h
>
> (previous commit explains why these renames were necessary)
>

Referencing a "previous commit" like this is not acceptable as you don't
know how these patches will get applied and there might be other patches
that get committed in-between yours. In each patch explain clearly why the
patch is needed. I still find it odd that you need to rename x86/monitor.c
-> x86/monitor_x86.c -> x86/monitor.c in the same series.


>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>

[-- Attachment #1.2: Type: text/html, Size: 1308 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Jan Beulich]

>>> On 10.02.16 at 17:44, <tamas@tklengyel.com> wrote:
> On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com>
> wrote:
> 
>> Rename:
>>     - arch/x86/monitor_x86.c -> arch/x86/monitor.c
>>     - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h
>>
>> (previous commit explains why these renames were necessary)
>>
> 
> Referencing a "previous commit" like this is not acceptable as you don't
> know how these patches will get applied and there might be other patches
> that get committed in-between yours. In each patch explain clearly why the
> patch is needed. I still find it odd that you need to rename x86/monitor.c
> -> x86/monitor_x86.c -> x86/monitor.c in the same series.

Indeed. Thinking about it again perhaps the operations should be
"move to common/ (mostly) verbatim" followed by "break out x86
specific code" (if that's the smaller portion compared to what is to
stay).

Jan

Re: [PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

On 2/10/2016 7:16 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 17:44, <tamas@tklengyel.com> wrote:
>> On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com>
>> wrote:
>>
>>> Rename:
>>>      - arch/x86/monitor_x86.c -> arch/x86/monitor.c
>>>      - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h
>>>
>>> (previous commit explains why these renames were necessary)
>>>
>> Referencing a "previous commit" like this is not acceptable as you don't
>> know how these patches will get applied and there might be other patches
>> that get committed in-between yours. In each patch explain clearly why the
>> patch is needed. I still find it odd that you need to rename x86/monitor.c
>> -> x86/monitor_x86.c -> x86/monitor.c in the same series.
> Indeed. Thinking about it again perhaps the operations should be
> "move to common/ (mostly) verbatim" followed by "break out x86
> specific code" (if that's the smaller portion compared to what is to
> stay).
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

That's a great alternative! Will do so in v3.

Corneliu.

Re: [PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 2460 bytes --]

On 2/10/2016 6:44 PM, Tamas K Lengyel wrote:
>
>
> On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     Rename:
>         - arch/x86/monitor_x86.c -> arch/x86/monitor.c
>         - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h
>
>     (previous commit explains why these renames were necessary)
>
>
> Referencing a "previous commit" like this is not acceptable as you 
> don't know how these patches will get applied and there might be other 
> patches that get committed in-between yours. In each patch explain 
> clearly why the patch is needed. I still find it odd that you need to 
> rename x86/monitor.c -> x86/monitor_x86.c -> x86/monitor.c in the same 
> series.
>
>
>     Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
>     <mailto:czuzu@bitdefender.com>>
>     ---
>

As I stated earlier, those commits (4 & 6) are the result of a git diff
limitation (also see my responses to these patches in v1).
The following simple experiment illustrates my point precisely:
1) create a local git repo, add file ./a.txt there with many lines (e.g. 
100) and make that commit 1
2) now create a dir ./d, move a.txt -> ./d/a.txt and add another ./a.txt 
w/ few lines, e.g. 2, make that commit 2

Git diff (even w/ the -M, -C options) of commit 2 would then report that:

     # ./a.txt has been modified => diff w/ 100 deletions & 2 additions
     # ./d/a.txt has been added => diff w/ 100 additions
     => 202 additions & deletions total, *even though* similarity 
between ./a.txt in commit 1 and
         ./d/a.txt in commit 2 is 100% ! (also even if you use -M, -C 
diff options)

*instead of* reporting

     # ./a.txt moved to ./d/a.txt, similarity index 100% => 0 deletions 
& 0 additions
     # added ./a.txt => 2 additions
     => 2 additions total

You could see why this limitation would have made the diffs somewhat bulky,
especially when those files also get to be modified in the process.
This is also the reason why accepting patch 3 without 4 or 5 without 6 
wouldn't
make sense, 3+4 should be treated as a single patch, 5+6 similarly.

As a proposed resolution: since I took these measures *only to ease code 
review*,
patches 4 & 6 can be safely squashed onto their previous commits (3 & 5) 
when and if acking them.
Or I could just worry less about this next time and simply squash them 
myself before sending
them for review, less to do on my part :).

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 4251 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH v2 5/7] xen/vm-events: Move hvm_event_* functions to common-side.

[Corneliu ZUZU]

1. Moved hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
    hvm_event_breakpoint from arch-side to common-side

  1.1. Moved arch/x86/hvm/event.c to common/hvm/event.c
        # see files: arch/x86/hvm/Makefile, xen/common/Makefile,
                     xen/common/hvm/Makefile, xen/common/hvm/event.c
        # changes:
            - moved hvm_event_fill_regs to arch-side (arch_hvm_event_fill_regs)
            - added vcpu parameter to hvm_event_traps
            - surrounded common hvm_event_* implementations w/
               CONFIG_HAS_VM_EVENT_*
            - moved hvm_event_msr to arch-side (see x86/hvm/event_x86.c)
            - moved rip->gfn code in hvm_event_breakpoint to arch-side
               (see arch_hvm_event_gfn_of_ip) and renamed rip param to
               ip (i.e. now the parameter signifies a 'generic' instruction
               pointer, rather than the X86_64 RIP register)

  1.2. Moved asm-x86/hvm/event.h to xen/hvm/event.h
        # see files: arch/x86/hvm/hvm.c, arch/x86/hvm/vmx/vmx.c

2. Added x86/hvm/event_x86.c => will rename in next commit to event.c (not done
in this commit to avoid git seeing this as being the modified old event.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
    # see files: arch/x86/hvm/Makefile
    # implements X86-specific hvm_event_msr

3. Added asm-x86/hvm/event_arch.h, asm-arm/hvm/event_arch.h (renamed to
event.h in next commit, reason is the same as @ (2.).
    # define/implement: arch_hvm_event_fill_regs, arch_hvm_event_gfn_of_ip and
                        hvm_event_msr (X86 side only)

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/hvm/Makefile                |   2 +-
 xen/arch/x86/hvm/event_x86.c             |  51 ++++++++++++
 xen/arch/x86/hvm/hvm.c                   |   3 +-
 xen/arch/x86/hvm/vmx/vmx.c               |   2 +-
 xen/common/Makefile                      |   2 +-
 xen/common/hvm/Makefile                  |   3 +-
 xen/{arch/x86 => common}/hvm/event.c     | 139 +++++++++++--------------------
 xen/include/asm-arm/hvm/event_arch.h     |  40 +++++++++
 xen/include/asm-x86/hvm/event_arch.h     |  93 +++++++++++++++++++++
 xen/include/{asm-x86 => xen}/hvm/event.h |  46 +++++++---
 10 files changed, 273 insertions(+), 108 deletions(-)
 create mode 100644 xen/arch/x86/hvm/event_x86.c
 rename xen/{arch/x86 => common}/hvm/event.c (44%)
 create mode 100644 xen/include/asm-arm/hvm/event_arch.h
 create mode 100644 xen/include/asm-x86/hvm/event_arch.h
 rename xen/include/{asm-x86 => xen}/hvm/event.h (59%)

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 794e793..15daa09 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,7 @@ subdir-y += vmx
 
 obj-y += asid.o
 obj-y += emulate.o
-obj-y += event.o
+obj-y += event_x86.o
 obj-y += hpet.o
 obj-y += hvm.o
 obj-y += i8254.o
diff --git a/xen/arch/x86/hvm/event_x86.c b/xen/arch/x86/hvm/event_x86.c
new file mode 100644
index 0000000..7b54f18
--- /dev/null
+++ b/xen/arch/x86/hvm/event_x86.c
@@ -0,0 +1,51 @@
+/*
+ * arch/x86/hvm/event_x86.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,
+ * 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/hvm/event.h>
+
+void hvm_event_msr(unsigned int msr, uint64_t value)
+{
+    struct vcpu *curr = current;
+
+    if ( curr->domain->arch.monitor.mov_to_msr_enabled )
+    {
+        vm_event_request_t req = {
+            .reason = VM_EVENT_REASON_MOV_TO_MSR,
+            .vcpu_id = curr->vcpu_id,
+            .u.mov_to_msr.msr = msr,
+            .u.mov_to_msr.value = value,
+        };
+
+        hvm_event_traps(curr, 1, &req);
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e93a648..2dec80e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -33,6 +33,7 @@
 #include <xen/event.h>
 #include <xen/paging.h>
 #include <xen/monitor.h>
+#include <xen/hvm/event.h>
 #include <xen/cpu.h>
 #include <xen/wait.h>
 #include <xen/mem_access.h>
@@ -58,7 +59,7 @@
 #include <asm/hvm/cacheattr.h>
 #include <asm/hvm/trace.h>
 #include <asm/hvm/nestedhvm.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/event_arch.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/svm/svm.h> /* for cpu_has_tsc_ratio */
 #include <asm/altp2m.h>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index be67b60..3055106 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -26,6 +26,7 @@
 #include <xen/hypercall.h>
 #include <xen/perfc.h>
 #include <xen/monitor.h>
+#include <xen/hvm/event.h>
 #include <asm/current.h>
 #include <asm/io.h>
 #include <asm/iocap.h>
@@ -51,7 +52,6 @@
 #include <asm/hvm/vpt.h>
 #include <public/hvm/save.h>
 #include <asm/hvm/trace.h>
-#include <asm/hvm/event.h>
 #include <asm/xenoprof.h>
 #include <asm/debugger.h>
 #include <asm/apic.h>
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 0d76efe..703faa5 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -67,7 +67,7 @@ obj-$(xenoprof)    += xenoprof.o
 
 obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o tmem_xen.o xlat.o)
 
-subdir-$(CONFIG_X86) += hvm
+subdir-y += hvm
 
 subdir-$(coverage) += gcov
 
diff --git a/xen/common/hvm/Makefile b/xen/common/hvm/Makefile
index a464a57..11e109d 100644
--- a/xen/common/hvm/Makefile
+++ b/xen/common/hvm/Makefile
@@ -1 +1,2 @@
-obj-y += save.o
+obj-$(CONFIG_X86) += save.o
+obj-y += event.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/common/hvm/event.c
similarity index 44%
rename from xen/arch/x86/hvm/event.c
rename to xen/common/hvm/event.c
index 04faa72..604e902 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -1,7 +1,7 @@
 /*
-* arch/x86/hvm/event.c
+* xen/common/hvm/event.c
 *
-* Arch-specific hardware virtual machine event abstractions.
+* Common hardware virtual machine event abstractions.
 *
 * Copyright (c) 2004, Intel Corporation.
 * Copyright (c) 2005, International Business Machines Corporation.
@@ -21,52 +21,22 @@
 * this program; If not, see <http://www.gnu.org/licenses/>.
 */
 
-#include <xen/vm_event.h>
-#include <xen/paging.h>
+#include <xen/hvm/event.h>
+#include <xen/vm_event.h>           /* for vm_event_# calls */
 #include <xen/monitor.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/event_arch.h>     /* for hvm_event_arch_# calls */
+#if CONFIG_X86
 #include <asm/altp2m.h>
-#include <public/vm_event.h>
+#endif
 
-static void hvm_event_fill_regs(vm_event_request_t *req)
-{
-    const struct cpu_user_regs *regs = guest_cpu_user_regs();
-    const struct vcpu *curr = current;
-
-    req->data.regs.x86.rax = regs->eax;
-    req->data.regs.x86.rcx = regs->ecx;
-    req->data.regs.x86.rdx = regs->edx;
-    req->data.regs.x86.rbx = regs->ebx;
-    req->data.regs.x86.rsp = regs->esp;
-    req->data.regs.x86.rbp = regs->ebp;
-    req->data.regs.x86.rsi = regs->esi;
-    req->data.regs.x86.rdi = regs->edi;
-
-    req->data.regs.x86.r8  = regs->r8;
-    req->data.regs.x86.r9  = regs->r9;
-    req->data.regs.x86.r10 = regs->r10;
-    req->data.regs.x86.r11 = regs->r11;
-    req->data.regs.x86.r12 = regs->r12;
-    req->data.regs.x86.r13 = regs->r13;
-    req->data.regs.x86.r14 = regs->r14;
-    req->data.regs.x86.r15 = regs->r15;
-
-    req->data.regs.x86.rflags = regs->eflags;
-    req->data.regs.x86.rip    = regs->eip;
-
-    req->data.regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer;
-    req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0];
-    req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3];
-    req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4];
-}
-
-static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
+int hvm_event_traps(struct vcpu *v,
+                    uint8_t sync,
+                    vm_event_request_t *req)
 {
     int rc;
-    struct vcpu *curr = current;
-    struct domain *currd = curr->domain;
+    struct domain *d = v->domain;
 
-    rc = vm_event_claim_slot(currd, &currd->vm_event->monitor);
+    rc = vm_event_claim_slot(d, &d->vm_event->monitor);
     switch ( rc )
     {
     case 0:
@@ -84,93 +54,75 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
     if ( sync )
     {
         req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
-        vm_event_vcpu_pause(curr);
+        vm_event_vcpu_pause(v);
     }
 
-    if ( altp2m_active(currd) )
+#if CONFIG_X86
+    if ( altp2m_active(d) )
     {
         req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
-        req->altp2m_idx = vcpu_altp2m(curr).p2midx;
+        req->altp2m_idx = vcpu_altp2m(v).p2midx;
     }
+#endif
+
+    arch_hvm_event_fill_regs(req);
 
-    hvm_event_fill_regs(req);
-    vm_event_put_request(currd, &currd->vm_event->monitor, req);
+    vm_event_put_request(d, &d->vm_event->monitor, req);
 
     return 1;
 }
 
-bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
+#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG
+bool_t hvm_event_cr(unsigned int index,
+                    unsigned long value,
+                    unsigned long old)
 {
-    struct arch_domain *currad = &current->domain->arch;
+    struct vcpu *curr = current;
+    struct arch_domain *ad = &curr->domain->arch;
     unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
 
-    if ( (currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) &&
-         (!(currad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
+    if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) &&
+         (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
           value != old) )
     {
+        bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask);
+
         vm_event_request_t req = {
             .reason = VM_EVENT_REASON_WRITE_CTRLREG,
-            .vcpu_id = current->vcpu_id,
+            .vcpu_id = curr->vcpu_id,
             .u.write_ctrlreg.index = index,
             .u.write_ctrlreg.new_value = value,
             .u.write_ctrlreg.old_value = old
         };
 
-        hvm_event_traps(currad->monitor.write_ctrlreg_sync & ctrlreg_bitmask,
-                        &req);
+        hvm_event_traps(curr, sync, &req);
         return 1;
     }
 
     return 0;
 }
+#endif // HAS_VM_EVENT_WRITE_CTRLREG
 
-void hvm_event_msr(unsigned int msr, uint64_t value)
-{
-    struct vcpu *curr = current;
-    vm_event_request_t req = {
-        .reason = VM_EVENT_REASON_MOV_TO_MSR,
-        .vcpu_id = curr->vcpu_id,
-        .u.mov_to_msr.msr = msr,
-        .u.mov_to_msr.value = value,
-    };
-
-    if ( curr->domain->arch.monitor.mov_to_msr_enabled )
-        hvm_event_traps(1, &req);
-}
-
+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
 void hvm_event_guest_request(void)
 {
     struct vcpu *curr = current;
-    struct arch_domain *currad = &curr->domain->arch;
+    struct arch_domain *ad = &curr->domain->arch;
 
-    if ( currad->monitor.guest_request_enabled )
+    if ( ad->monitor.guest_request_enabled )
     {
         vm_event_request_t req = {
             .reason = VM_EVENT_REASON_GUEST_REQUEST,
             .vcpu_id = curr->vcpu_id,
         };
 
-        hvm_event_traps(currad->monitor.guest_request_sync, &req);
+        hvm_event_traps(curr, ad->monitor.guest_request_sync, &req);
     }
 }
+#endif // HAS_VM_EVENT_GUEST_REQUEST
 
-static inline
-uint64_t gfn_of_rip(unsigned long rip)
-{
-    struct vcpu *curr = current;
-    struct segment_register sreg;
-    uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-
-    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);
-
-    return (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
-}
-
-int hvm_event_breakpoint(unsigned long rip,
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP || CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+int hvm_event_breakpoint(unsigned long ip,
                          enum hvm_event_breakpoint_type type)
 {
     struct vcpu *curr = current;
@@ -179,19 +131,23 @@ int hvm_event_breakpoint(unsigned long rip,
 
     switch ( type )
     {
+#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
     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);
+        req.u.software_breakpoint.gfn = arch_hvm_event_gfn_of_ip(ip);
         break;
+#endif
 
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP
     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);
+        req.u.singlestep.gfn = arch_hvm_event_gfn_of_ip(ip);
         break;
+#endif
 
     default:
         return -EOPNOTSUPP;
@@ -199,8 +155,9 @@ int hvm_event_breakpoint(unsigned long rip,
 
     req.vcpu_id = curr->vcpu_id;
 
-    return hvm_event_traps(1, &req);
+    return hvm_event_traps(curr, 1, &req);
 }
+#endif // HAS_VM_EVENT_SINGLESTEP || HAS_VM_EVENT_SOFTWARE_BREAKPOINT
 
 /*
  * Local variables:
diff --git a/xen/include/asm-arm/hvm/event_arch.h b/xen/include/asm-arm/hvm/event_arch.h
new file mode 100644
index 0000000..beebca2
--- /dev/null
+++ b/xen/include/asm-arm/hvm/event_arch.h
@@ -0,0 +1,40 @@
+/*
+ * include/asm-arm/hvm/event_arch.h
+ *
+ * Arch-specific hardware virtual machine event abstractions.
+ *
+ * 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,
+ * 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/>.
+ */
+
+#ifndef __ASM_ARM_HVM_EVENT_ARCH_H__
+#define __ASM_ARM_HVM_EVENT_ARCH_H__
+
+static inline
+void arch_hvm_event_fill_regs(vm_event_request_t *req)
+{
+    /* Not supported on ARM. */
+}
+
+#endif /* __ASM_ARM_HVM_EVENT_ARCH_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/hvm/event_arch.h b/xen/include/asm-x86/hvm/event_arch.h
new file mode 100644
index 0000000..b9fb559
--- /dev/null
+++ b/xen/include/asm-x86/hvm/event_arch.h
@@ -0,0 +1,93 @@
+/*
+ * include/asm-x86/hvm/event_arch.h
+ * 
+ * Arch-specific hardware virtual machine event abstractions.
+ * 
+ * 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,
+ * 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/>.
+ */
+
+#ifndef __ASM_X86_HVM_EVENT_ARCH_H__
+#define __ASM_X86_HVM_EVENT_ARCH_H__
+
+#include <xen/sched.h>
+#include <xen/paging.h>
+#include <public/vm_event.h>
+
+static inline
+void arch_hvm_event_fill_regs(vm_event_request_t *req)
+{
+    const struct cpu_user_regs *regs = guest_cpu_user_regs();
+    const struct vcpu *curr = current;
+
+    req->data.regs.x86.rax = regs->eax;
+    req->data.regs.x86.rcx = regs->ecx;
+    req->data.regs.x86.rdx = regs->edx;
+    req->data.regs.x86.rbx = regs->ebx;
+    req->data.regs.x86.rsp = regs->esp;
+    req->data.regs.x86.rbp = regs->ebp;
+    req->data.regs.x86.rsi = regs->esi;
+    req->data.regs.x86.rdi = regs->edi;
+
+    req->data.regs.x86.r8  = regs->r8;
+    req->data.regs.x86.r9  = regs->r9;
+    req->data.regs.x86.r10 = regs->r10;
+    req->data.regs.x86.r11 = regs->r11;
+    req->data.regs.x86.r12 = regs->r12;
+    req->data.regs.x86.r13 = regs->r13;
+    req->data.regs.x86.r14 = regs->r14;
+    req->data.regs.x86.r15 = regs->r15;
+
+    req->data.regs.x86.rflags = regs->eflags;
+    req->data.regs.x86.rip    = regs->eip;
+
+    req->data.regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer;
+    req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0];
+    req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3];
+   req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4];
+}
+
+/*
+ * Returns the GFN of the given instruction pointer.
+ * Needed by hvm_event_software_breakpoint.
+ */
+static inline
+uint64_t arch_hvm_event_gfn_of_ip(unsigned long ip)
+{
+    struct vcpu *curr = current;
+    struct segment_register sreg;
+    uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
+
+    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
+    if ( 3 == sreg.attr.fields.dpl )
+        pfec |= PFEC_user_mode;
+
+    hvm_get_segment_register(curr, x86_seg_cs, &sreg);
+
+    return (uint64_t) paging_gva_to_gfn(curr, sreg.base + ip, &pfec);
+}
+
+void hvm_event_msr(unsigned int msr, uint64_t value);
+
+#endif /* __ASM_X86_HVM_EVENT_ARCH_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/xen/hvm/event.h
similarity index 59%
rename from xen/include/asm-x86/hvm/event.h
rename to xen/include/xen/hvm/event.h
index 7a83b45..662278e 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/xen/hvm/event.h
@@ -1,5 +1,9 @@
 /*
- * event.h: Hardware virtual machine assist events.
+ * include/xen/hvm/event.h
+ *
+ * Common hardware virtual machine event abstractions.
+ *
+ * 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,
@@ -14,30 +18,48 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_X86_HVM_EVENT_H__
-#define __ASM_X86_HVM_EVENT_H__
+#ifndef __HVM_EVENT_H__
+#define __HVM_EVENT_H__
 
-enum hvm_event_breakpoint_type
-{
-    HVM_EVENT_SOFTWARE_BREAKPOINT,
-    HVM_EVENT_SINGLESTEP_BREAKPOINT,
-};
+#include <xen/sched.h>              /* for struct vcpu */
+#include <public/vm_event.h>        /* for vm_event_request_t */
 
+#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG
 /*
  * Called for current VCPU on crX/MSR changes by guest.
  * The event might not fire if the client has subscribed to it in onchangeonly
  * mode, hence the bool_t return type for control register write events.
  */
-bool_t hvm_event_cr(unsigned int index, unsigned long value,
+bool_t hvm_event_cr(unsigned int index,
+                    unsigned long value,
                     unsigned long old);
+
+#if CONFIG_X86
 #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);
-int hvm_event_breakpoint(unsigned long rip,
+#endif
+#endif // HAS_VM_EVENT_WRITE_CTRLREG
+
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP || CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+enum hvm_event_breakpoint_type
+{
+    HVM_EVENT_SOFTWARE_BREAKPOINT,
+    HVM_EVENT_SINGLESTEP_BREAKPOINT,
+};
+
+int hvm_event_breakpoint(unsigned long ip,
                          enum hvm_event_breakpoint_type type);
+#endif // HAS_VM_EVENT_SINGLESTEP || HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+
+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
 void hvm_event_guest_request(void);
+#endif // HAS_VM_EVENT_GUEST_REQUEST
+
+int hvm_event_traps(struct vcpu *v,
+                    uint8_t sync,
+                    vm_event_request_t *req);
 
-#endif /* __ASM_X86_HVM_EVENT_H__ */
+#endif /* __HVM_EVENT_H__ */
 
 /*
  * Local variables:
-- 
2.5.0

[PATCH v2 6/7] Rename event_x86.c to event.c and event_arch.h to event.h + minor fixes

[Corneliu ZUZU]

Rename:
    - arch/x86/hvm/event_x86.c -> arch/x86/hvm/event.c
    - asm-{x86,arm}/hvm/event_arch.h -> asm-{x86/arm}/hvm/event.h

(previous commit explains why these renames were necessary)

Minor fixes:
    * xen/common/hvm/event.c: fix malformed file header comment
    * xen/hvm/event.h: fix comment & change hvm_event_crX first param name to a more
      descriptive one

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/hvm/Makefile                         |  2 +-
 xen/arch/x86/hvm/{event_x86.c => event.c}         |  2 +-
 xen/arch/x86/hvm/hvm.c                            |  2 +-
 xen/common/hvm/event.c                            | 44 +++++++++++------------
 xen/include/asm-arm/hvm/{event_arch.h => event.h} |  8 ++---
 xen/include/asm-x86/hvm/{event_arch.h => event.h} |  8 ++---
 xen/include/xen/hvm/event.h                       |  6 ++--
 7 files changed, 36 insertions(+), 36 deletions(-)
 rename xen/arch/x86/hvm/{event_x86.c => event.c} (98%)
 rename xen/include/asm-arm/hvm/{event_arch.h => event.h} (86%)
 rename xen/include/asm-x86/hvm/{event_arch.h => event.h} (94%)

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 15daa09..794e793 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,7 @@ subdir-y += vmx
 
 obj-y += asid.o
 obj-y += emulate.o
-obj-y += event_x86.o
+obj-y += event.o
 obj-y += hpet.o
 obj-y += hvm.o
 obj-y += i8254.o
diff --git a/xen/arch/x86/hvm/event_x86.c b/xen/arch/x86/hvm/event.c
similarity index 98%
rename from xen/arch/x86/hvm/event_x86.c
rename to xen/arch/x86/hvm/event.c
index 7b54f18..3349c34 100644
--- a/xen/arch/x86/hvm/event_x86.c
+++ b/xen/arch/x86/hvm/event.c
@@ -1,5 +1,5 @@
 /*
- * arch/x86/hvm/event_x86.c
+ * arch/x86/hvm/event.c
  *
  * Arch-specific hardware virtual machine event abstractions.
  *
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2dec80e..69e2854 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -59,7 +59,7 @@
 #include <asm/hvm/cacheattr.h>
 #include <asm/hvm/trace.h>
 #include <asm/hvm/nestedhvm.h>
-#include <asm/hvm/event_arch.h>
+#include <asm/hvm/event.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/svm/svm.h> /* for cpu_has_tsc_ratio */
 #include <asm/altp2m.h>
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
index 604e902..0b532d5 100644
--- a/xen/common/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -1,30 +1,30 @@
 /*
-* xen/common/hvm/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.
-* 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,
-* 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/>.
-*/
+ * xen/common/hvm/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.
+ * 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,
+ * 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/hvm/event.h>
 #include <xen/vm_event.h>           /* for vm_event_# calls */
 #include <xen/monitor.h>
-#include <asm/hvm/event_arch.h>     /* for hvm_event_arch_# calls */
+#include <asm/hvm/event.h>          /* for hvm_event_arch_# calls */
 #if CONFIG_X86
 #include <asm/altp2m.h>
 #endif
diff --git a/xen/include/asm-arm/hvm/event_arch.h b/xen/include/asm-arm/hvm/event.h
similarity index 86%
rename from xen/include/asm-arm/hvm/event_arch.h
rename to xen/include/asm-arm/hvm/event.h
index beebca2..4aa797b 100644
--- a/xen/include/asm-arm/hvm/event_arch.h
+++ b/xen/include/asm-arm/hvm/event.h
@@ -1,5 +1,5 @@
 /*
- * include/asm-arm/hvm/event_arch.h
+ * include/asm-arm/hvm/event.h
  *
  * Arch-specific hardware virtual machine event abstractions.
  *
@@ -18,8 +18,8 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_ARM_HVM_EVENT_ARCH_H__
-#define __ASM_ARM_HVM_EVENT_ARCH_H__
+#ifndef __ASM_ARM_HVM_EVENT_H__
+#define __ASM_ARM_HVM_EVENT_H__
 
 static inline
 void arch_hvm_event_fill_regs(vm_event_request_t *req)
@@ -27,7 +27,7 @@ void arch_hvm_event_fill_regs(vm_event_request_t *req)
     /* Not supported on ARM. */
 }
 
-#endif /* __ASM_ARM_HVM_EVENT_ARCH_H__ */
+#endif /* __ASM_ARM_HVM_EVENT_H__ */
 
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/hvm/event_arch.h b/xen/include/asm-x86/hvm/event.h
similarity index 94%
rename from xen/include/asm-x86/hvm/event_arch.h
rename to xen/include/asm-x86/hvm/event.h
index b9fb559..88a81f0 100644
--- a/xen/include/asm-x86/hvm/event_arch.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -1,5 +1,5 @@
 /*
- * include/asm-x86/hvm/event_arch.h
+ * include/asm-x86/hvm/event.h
  * 
  * Arch-specific hardware virtual machine event abstractions.
  * 
@@ -18,8 +18,8 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_X86_HVM_EVENT_ARCH_H__
-#define __ASM_X86_HVM_EVENT_ARCH_H__
+#ifndef __ASM_X86_HVM_EVENT_H__
+#define __ASM_X86_HVM_EVENT_H__
 
 #include <xen/sched.h>
 #include <xen/paging.h>
@@ -80,7 +80,7 @@ uint64_t arch_hvm_event_gfn_of_ip(unsigned long ip)
 
 void hvm_event_msr(unsigned int msr, uint64_t value);
 
-#endif /* __ASM_X86_HVM_EVENT_ARCH_H__ */
+#endif /* __ASM_X86_HVM_EVENT_H__ */
 
 /*
  * Local variables:
diff --git a/xen/include/xen/hvm/event.h b/xen/include/xen/hvm/event.h
index 662278e..4ab42a0 100644
--- a/xen/include/xen/hvm/event.h
+++ b/xen/include/xen/hvm/event.h
@@ -26,7 +26,7 @@
 
 #if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG
 /*
- * Called for current VCPU on crX/MSR changes by guest.
+ * Called for current VCPU on control-register changes by guest.
  * The event might not fire if the client has subscribed to it in onchangeonly
  * mode, hence the bool_t return type for control register write events.
  */
@@ -35,8 +35,8 @@ bool_t hvm_event_cr(unsigned int index,
                     unsigned long old);
 
 #if CONFIG_X86
-#define hvm_event_crX(what, new, old) \
-    hvm_event_cr(VM_EVENT_X86_##what, new, old)
+#define hvm_event_crX(cr, new, old) \
+    hvm_event_cr(VM_EVENT_X86_##cr, new, old)
 #endif
 #endif // HAS_VM_EVENT_WRITE_CTRLREG
 
-- 
2.5.0

[PATCH v2 7/7] xen/vm-events: move arch_domain.monitor bits to common

[Corneliu ZUZU]

This patch moves bitfield members for single-step, software-breakpoint and
guest-request monitor vm-events from the arch-side (arch_domain.monitor) to
the common-side (domain.monitor).
Ctrl-reg bits (i.e. write_ctrlreg_* members) are left on the arch-side, because
control-registers number can vary across architectures.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c  |  4 ++--
 xen/common/hvm/event.c       | 12 ++++++------
 xen/common/monitor.c         | 17 +++++++----------
 xen/include/asm-x86/domain.h | 16 ++++++----------
 xen/include/xen/sched.h      | 16 ++++++++++++++++
 5 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 5bc3c74..07acbf2 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1706,8 +1706,8 @@ void vmx_do_resume(struct vcpu *v)
     }
 
     debug_state = v->domain->debugger_attached
-                  || v->domain->arch.monitor.software_breakpoint_enabled
-                  || v->domain->arch.monitor.singlestep_enabled;
+                  || v->domain->monitor.software_breakpoint_enabled
+                  || v->domain->monitor.singlestep_enabled;
 
     if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
     {
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
index 0b532d5..0e5f30e 100644
--- a/xen/common/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -107,16 +107,16 @@ bool_t hvm_event_cr(unsigned int index,
 void hvm_event_guest_request(void)
 {
     struct vcpu *curr = current;
-    struct arch_domain *ad = &curr->domain->arch;
+    struct domain *d = curr->domain;
 
-    if ( ad->monitor.guest_request_enabled )
+    if ( d->monitor.guest_request_enabled )
     {
         vm_event_request_t req = {
             .reason = VM_EVENT_REASON_GUEST_REQUEST,
             .vcpu_id = curr->vcpu_id,
         };
 
-        hvm_event_traps(curr, ad->monitor.guest_request_sync, &req);
+        hvm_event_traps(curr, d->monitor.guest_request_sync, &req);
     }
 }
 #endif // HAS_VM_EVENT_GUEST_REQUEST
@@ -126,14 +126,14 @@ int hvm_event_breakpoint(unsigned long ip,
                          enum hvm_event_breakpoint_type type)
 {
     struct vcpu *curr = current;
-    struct arch_domain *ad = &curr->domain->arch;
+    struct domain *d = curr->domain;
     vm_event_request_t req;
 
     switch ( type )
     {
 #if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
     case HVM_EVENT_SOFTWARE_BREAKPOINT:
-        if ( !ad->monitor.software_breakpoint_enabled )
+        if ( !d->monitor.software_breakpoint_enabled )
             return 0;
         req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
         req.u.software_breakpoint.gfn = arch_hvm_event_gfn_of_ip(ip);
@@ -142,7 +142,7 @@ int hvm_event_breakpoint(unsigned long ip,
 
 #if CONFIG_HAS_VM_EVENT_SINGLESTEP
     case HVM_EVENT_SINGLESTEP_BREAKPOINT:
-        if ( !ad->monitor.singlestep_enabled )
+        if ( !d->monitor.singlestep_enabled )
             return 0;
         req.reason = VM_EVENT_REASON_SINGLESTEP;
         req.u.singlestep.gfn = arch_hvm_event_gfn_of_ip(ip);
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 03063bb..3c69b5e 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -116,14 +116,13 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 #if CONFIG_HAS_VM_EVENT_SINGLESTEP
     case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
     {
-        struct arch_domain *ad = &d->arch;
-        bool_t old_status = ad->monitor.singlestep_enabled;
+        bool_t old_status = d->monitor.singlestep_enabled;
 
         if ( unlikely(old_status == requested_status) )
             return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.singlestep_enabled = !old_status;
+        d->monitor.singlestep_enabled = !old_status;
         domain_unpause(d);
         break;
     }
@@ -132,14 +131,13 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 #if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
     case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
     {
-        struct arch_domain *ad = &d->arch;
-        bool_t old_status = ad->monitor.software_breakpoint_enabled;
+        bool_t old_status = d->monitor.software_breakpoint_enabled;
 
         if ( unlikely(old_status == requested_status) )
             return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.software_breakpoint_enabled = !old_status;
+        d->monitor.software_breakpoint_enabled = !old_status;
         domain_unpause(d);
         break;
     }
@@ -148,15 +146,14 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 #if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
     case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
     {
-        struct arch_domain *ad = &d->arch;
-        bool_t old_status = ad->monitor.guest_request_enabled;
+        bool_t old_status = d->monitor.guest_request_enabled;
 
         if ( unlikely(old_status == requested_status) )
             return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
-        ad->monitor.guest_request_enabled = !old_status;
+        d->monitor.guest_request_sync = mop->u.guest_request.sync;
+        d->monitor.guest_request_enabled = !old_status;
         domain_unpause(d);
         break;
     }
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4072e27..6254060 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,13 @@ struct arch_domain
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
 
-    /* Monitor options */
+    /* Arch-specific monitor vm-event options */
     struct {
-        unsigned int write_ctrlreg_enabled       : 4;
-        unsigned int write_ctrlreg_sync          : 4;
-        unsigned int write_ctrlreg_onchangeonly  : 4;
-        unsigned int mov_to_msr_enabled          : 1;
-        unsigned int mov_to_msr_extended         : 1;
-        unsigned int singlestep_enabled          : 1;
-        unsigned int software_breakpoint_enabled : 1;
-        unsigned int guest_request_enabled       : 1;
-        unsigned int guest_request_sync          : 1;
+        uint16_t write_ctrlreg_enabled       : 4;
+        uint16_t write_ctrlreg_sync          : 4;
+        uint16_t write_ctrlreg_onchangeonly  : 4;
+        uint16_t mov_to_msr_enabled          : 1;
+        uint16_t mov_to_msr_extended         : 1;
     } monitor;
 
     /* Mem_access emulation control */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b47a3fe..5b01a7f 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -464,6 +464,22 @@ struct domain
     /* vNUMA topology accesses are protected by rwlock. */
     rwlock_t vnuma_rwlock;
     struct vnuma_info *vnuma;
+
+    /* Common monitor vm-events options */
+    struct {
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP
+        uint8_t singlestep_enabled          : 1;
+#endif // HAS_VM_EVENT_SINGLESTEP
+
+#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+        uint8_t software_breakpoint_enabled : 1;
+#endif // HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+
+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
+        uint8_t guest_request_enabled       : 1;
+        uint8_t guest_request_sync          : 1;
+#endif // HAS_VM_EVENT_GUEST_REQUEST
+    } monitor;
 };
 
 /* Protect updates/reads (resp.) of domain_list and domain_hash. */
-- 
2.5.0

Re: [PATCH v2 7/7] xen/vm-events: move arch_domain.monitor bits to common

[Jan Beulich]

>>> On 10.02.16 at 17:00, <czuzu@bitdefender.com> wrote:
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -464,6 +464,22 @@ struct domain
>      /* vNUMA topology accesses are protected by rwlock. */
>      rwlock_t vnuma_rwlock;
>      struct vnuma_info *vnuma;
> +
> +    /* Common monitor vm-events options */
> +    struct {
> +#if CONFIG_HAS_VM_EVENT_SINGLESTEP
> +        uint8_t singlestep_enabled          : 1;
> +#endif // HAS_VM_EVENT_SINGLESTEP
> +
> +#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
> +        uint8_t software_breakpoint_enabled : 1;
> +#endif // HAS_VM_EVENT_SOFTWARE_BREAKPOINT
> +
> +#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
> +        uint8_t guest_request_enabled       : 1;
> +        uint8_t guest_request_sync          : 1;
> +#endif // HAS_VM_EVENT_GUEST_REQUEST
> +    } monitor;
>  };

Please use C-style comments only (see ./CODING_STYLE).

Jan

Re: [PATCH v2 7/7] xen/vm-events: move arch_domain.monitor bits to common

[Corneliu ZUZU]

On 2/10/2016 6:29 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 17:00, <czuzu@bitdefender.com> wrote:
>> +#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
>> +        uint8_t guest_request_enabled       : 1;
>> +        uint8_t guest_request_sync          : 1;
>> +#endif // HAS_VM_EVENT_GUEST_REQUEST
>> +    } monitor;
>>   };
> Please use C-style comments only (see ./CODING_STYLE).
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
Noted.

Corneliu.