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

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

[Corneliu ZUZU]

This patch series is an attempt to move some of the monitor vm-events code to
the common-side. Done to make it easier to move additional parts that can be
moved to common when ARM-side implementations are to be added.

Patches summary:
1. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
   hvm_event_breakpoint.
2. Move monitor_domctl to common-side.

Note: ARM support for guest-request, control-register write monitor vm-events
will follow after review of this patch-series.

---
Changed since v2:
  Major:
    * moved back all monitor vm-event handling on X86-side
    * removed Kconfigs
    * arch_monitor_domctl_event & arch_monitor_domctl_op now directly return rc
  Minor:
    * squashed commits 4 & 6 on their previous commits
    * fixed coding-style mistakes (e.g. use C-style comments only)
    * drop xen/config.h include

[PATCH v3 1/2] 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.
Also fixes event.c file header comment in the process.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/hvm/event.c        | 108 +++++++++++++++++++---------------------
 xen/arch/x86/hvm/vmx/vmx.c      |  15 +++---
 xen/include/asm-x86/hvm/event.h |  11 ++--
 3 files changed, 67 insertions(+), 67 deletions(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index a3d4892..874a36c 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -1,22 +1,24 @@
 /*
-* 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.
-*
-* 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/>.
-*/
+ * 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.
+ *
+ * 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/vm_event.h>
 #include <xen/paging.h>
@@ -151,61 +153,51 @@ void hvm_event_guest_request(void)
     }
 }
 
-int hvm_event_int3(unsigned long rip)
+static inline unsigned long 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 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 5e99226..44e778f 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 v3 1/2] xen/x86: merge 2 hvm_event_... functions into 1

[Razvan Cojocaru]

On 02/15/2016 08:35 AM, Corneliu ZUZU wrote:
> This patch merges almost identical functions hvm_event_int3 and
> hvm_event_single_step into a single function called hvm_event_breakpoint.
> Also fixes event.c file header comment in the process.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>  xen/arch/x86/hvm/event.c        | 108 +++++++++++++++++++---------------------
>  xen/arch/x86/hvm/vmx/vmx.c      |  15 +++---
>  xen/include/asm-x86/hvm/event.h |  11 ++--
>  3 files changed, 67 insertions(+), 67 deletions(-)

Looks good to me.

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

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

[Corneliu ZUZU]

On 2/15/2016 10:30 AM, Razvan Cojocaru wrote:
> On 02/15/2016 08:35 AM, Corneliu ZUZU wrote:
>> This patch merges almost identical functions hvm_event_int3 and
>> hvm_event_single_step into a single function called hvm_event_breakpoint.
>> Also fixes event.c file header comment in the process.
>>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>> ---
>>   xen/arch/x86/hvm/event.c        | 108 +++++++++++++++++++---------------------
>>   xen/arch/x86/hvm/vmx/vmx.c      |  15 +++---
>>   xen/include/asm-x86/hvm/event.h |  11 ++--
>>   3 files changed, 67 insertions(+), 67 deletions(-)
> Looks good to me.
>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>
>
> Thanks,
> Razvan
>

I forgot to ask: when getting an Acked-By response, should I include 
that patch in the next patch-series
or ommit it? I've done that w/ the first patch in the last patch-series, 
but IDK if I was correct to do so.

Thanks,
Corneliu.

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

[Tamas K Lengyel]

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

On Mon, Feb 15, 2016 at 10:40 AM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:

> On 2/15/2016 10:30 AM, Razvan Cojocaru wrote:
>
>> On 02/15/2016 08:35 AM, Corneliu ZUZU wrote:
>>
>>> This patch merges almost identical functions hvm_event_int3 and
>>> hvm_event_single_step into a single function called hvm_event_breakpoint.
>>> Also fixes event.c file header comment in the process.
>>>
>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>> ---
>>>   xen/arch/x86/hvm/event.c        | 108
>>> +++++++++++++++++++---------------------
>>>   xen/arch/x86/hvm/vmx/vmx.c      |  15 +++---
>>>   xen/include/asm-x86/hvm/event.h |  11 ++--
>>>   3 files changed, 67 insertions(+), 67 deletions(-)
>>>
>> Looks good to me.
>>
>> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>
>>
>> Thanks,
>> Razvan
>>
>>
If the patch hasn't been merged to staging yet then include it. If it's a
longer series the cover page can indicate which patches are still in need
of review and which ones have been already acked. Furthermore, for each
patch under the Signed-off-by tag you can say what changed in each
revision. If nothing changed, you can say no change or just don't put
anything for that revision. See
http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html for an
example. Also keep in mind that if the changes in a revision are
significant enough you can't keep the Acked-by tag on the patch, it will
need to be re-acked.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2383 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 v3 1/2] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

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

On 2/15/2016 7:47 PM, Tamas K Lengyel wrote:
>
>
> On Mon, Feb 15, 2016 at 10:40 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     On 2/15/2016 10:30 AM, Razvan Cojocaru wrote:
>
>         On 02/15/2016 08:35 AM, Corneliu ZUZU wrote:
>
>             This patch merges almost identical functions
>             hvm_event_int3 and
>             hvm_event_single_step into a single function called
>             hvm_event_breakpoint.
>             Also fixes event.c file header comment in the process.
>
>             Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
>             <mailto:czuzu@bitdefender.com>>
>             ---
>               xen/arch/x86/hvm/event.c        | 108
>             +++++++++++++++++++---------------------
>               xen/arch/x86/hvm/vmx/vmx.c      |  15 +++---
>               xen/include/asm-x86/hvm/event.h |  11 ++--
>               3 files changed, 67 insertions(+), 67 deletions(-)
>
>         Looks good to me.
>
>         Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com
>         <mailto:rcojocaru@bitdefender.com>>
>
>
>         Thanks,
>         Razvan
>
>
> If the patch hasn't been merged to staging yet then include it. If 
> it's a longer series the cover page can indicate which patches are 
> still in need of review and which ones have been already acked. 
> Furthermore, for each patch under the Signed-off-by tag you can say 
> what changed in each revision. If nothing changed, you can say no 
> change or just don't put anything for that revision. See 
> http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html for 
> an example. Also keep in mind that if the changes in a revision are 
> significant enough you can't keep the Acked-by tag on the patch, it 
> will need to be re-acked.
>
> Tamas

Got it, thanks.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 4361 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 v3 1/2] xen/x86: merge 2 hvm_event_... functions into 1

[Razvan Cojocaru]

On 02/15/2016 07:54 PM, Corneliu ZUZU wrote:
> On 2/15/2016 7:47 PM, Tamas K Lengyel wrote:
>>
>>
>> On Mon, Feb 15, 2016 at 10:40 AM, Corneliu ZUZU <czuzu@bitdefender.com
>> <mailto:czuzu@bitdefender.com>> wrote:
>>
>>     On 2/15/2016 10:30 AM, Razvan Cojocaru wrote:
>>
>>         On 02/15/2016 08:35 AM, Corneliu ZUZU wrote:
>>
>>             This patch merges almost identical functions
>>             hvm_event_int3 and
>>             hvm_event_single_step into a single function called
>>             hvm_event_breakpoint.
>>             Also fixes event.c file header comment in the process.
>>
>>             Signed-off-by: Corneliu ZUZU
>>             <<mailto:czuzu@bitdefender.com>czuzu@bitdefender.com>
>>             ---
>>               xen/arch/x86/hvm/event.c        | 108
>>             +++++++++++++++++++---------------------
>>               xen/arch/x86/hvm/vmx/vmx.c      |  15 +++---
>>               xen/include/asm-x86/hvm/event.h |  11 ++--
>>               3 files changed, 67 insertions(+), 67 deletions(-)
>>
>>         Looks good to me.
>>
>>         Acked-by: Razvan Cojocaru
>>         <<mailto:rcojocaru@bitdefender.com>rcojocaru@bitdefender.com>
>>
>>
>>         Thanks,
>>         Razvan
>>
>>
>> If the patch hasn't been merged to staging yet then include it. If
>> it's a longer series the cover page can indicate which patches are
>> still in need of review and which ones have been already acked.
>> Furthermore, for each patch under the Signed-off-by tag you can say
>> what changed in each revision. If nothing changed, you can say no
>> change or just don't put anything for that revision. See
>> <http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html>http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html
>> for an example. Also keep in mind that if the changes in a revision
>> are significant enough you can't keep the Acked-by tag on the patch,
>> it will need to be re-acked.
>>
>> Tamas
> 
> Got it, thanks.

This particular patch did make it into staging. It's
557c7873f35aa39bd84977b28948457b1b342f92.

[PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.

* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
* remove status_check

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 MAINTAINERS                   |   1 +
 xen/arch/x86/monitor.c        | 158 ++++++++++++------------------------------
 xen/common/Makefile           |   1 +
 xen/common/domctl.c           |   2 +-
 xen/common/monitor.c          |  69 ++++++++++++++++++
 xen/include/asm-arm/monitor.h |  34 +++++++--
 xen/include/asm-x86/monitor.h |  53 ++++++++++++--
 xen/include/xen/monitor.h     |  30 ++++++++
 8 files changed, 226 insertions(+), 122 deletions(-)
 create mode 100644 xen/common/monitor.c
 create mode 100644 xen/include/xen/monitor.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f07384c..5cbb1dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,6 +355,7 @@ M:	Tamas K Lengyel <tamas@tklengyel.com>
 S:	Supported
 F:	xen/common/vm_event.c
 F:	xen/common/mem_access.c
+F:	xen/common/monitor.c
 F:	xen/arch/x86/hvm/event.c
 F:	xen/arch/x86/monitor.c
 F:	xen/arch/*/vm_event.c
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1d43880..069c3c7 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -1,9 +1,10 @@
 /*
  * arch/x86/monitor.c
  *
- * 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,87 +19,14 @@
  * 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 <xsm/xsm.h>
+#include <public/vm_event.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;
-}
-
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+int arch_monitor_domctl_event(struct domain *d,
+                              struct xen_domctl_monitor_op *mop)
 {
-    int rc;
     struct arch_domain *ad = &d->arch;
-    uint32_t capabilities = get_capabilities(d);
-
-    if ( current->domain == d ) /* no domain_pause() */
-        return -EPERM;
-
-    rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
-    if ( rc )
-        return rc;
-
-    switch ( mop->op )
-    {
-    case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
-        mop->event = capabilities;
-        return 0;
-
-    case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
-        domain_pause(d);
-        ad->mem_access_emulate_each_rep = !!mop->event;
-        domain_unpause(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)) )
-        return -EOPNOTSUPP;
+    bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
 
     switch ( mop->event )
     {
@@ -106,13 +34,11 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
     {
         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,15 +52,18 @@ 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 ( 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);
+        }
 
         domain_unpause(d);
 
@@ -143,77 +72,80 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
     {
-        bool_t status = ad->monitor.mov_to_msr_enabled;
+        bool_t old_status = ad->monitor.mov_to_msr_enabled;
 
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
 
-        if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
-             mop->u.mov_to_msr.extended_capture &&
+        if ( requested_status && 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;
+        if ( requested_status && 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;
+        ad->monitor.mov_to_msr_enabled = !old_status;
         domain_unpause(d);
         break;
     }
 
     case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
     {
-        bool_t status = ad->monitor.singlestep_enabled;
+        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;
     }
 
     case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
     {
-        bool_t status = ad->monitor.software_breakpoint_enabled;
+        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;
     }
 
     case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
     {
-        bool_t status = ad->monitor.guest_request_enabled;
+        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;
     }
 
     default:
-        return -EOPNOTSUPP;
+        /*
+         * 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/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 22fa5d5..b34c0a1 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/common/monitor.c b/xen/common/monitor.c
new file mode 100644
index 0000000..b708cab
--- /dev/null
+++ b/xen/common/monitor.c
@@ -0,0 +1,69 @@
+/*
+ * xen/common/monitor.c
+ *
+ * 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
+ * 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 <xen/monitor.h>
+#include <xen/sched.h>
+#include <xsm/xsm.h>
+#include <public/domctl.h>
+#include <asm/monitor.h>
+
+int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+    int rc;
+    bool_t requested_status = 0;
+
+    if ( unlikely(current->domain == d) ) /* no domain_pause() */
+        return -EPERM;
+
+    rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
+    if ( unlikely(rc) )
+        return rc;
+
+    switch ( mop->op )
+    {
+    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;
+        /* Arch-side handles enable/disable ops. */
+        return arch_monitor_domctl_event(d, mop);
+
+    case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
+        mop->event = arch_monitor_get_capabilities(d);
+        return 0;
+
+    default:
+        /* The monitor op is probably handled on the arch-side. */
+        return arch_monitor_domctl_op(d, mop);
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index a3a9703..ef7547e 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -1,9 +1,10 @@
 /*
  * include/asm-arm/monitor.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
@@ -24,10 +25,35 @@
 #include <xen/sched.h>
 #include <public/domctl.h>
 
+static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+    /* No monitor vm-events implemented on ARM. */
+    return 0;
+}
+
+static inline
+int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+    /* No arch-specific monitor ops on ARM. */
+    return -EOPNOTSUPP;
+}
+
 static inline
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op)
+int arch_monitor_domctl_event(struct domain *d,
+                              struct xen_domctl_monitor_op *mop)
 {
-    return -ENOSYS;
+    /* No arch-specific monitor vm-events on ARM.
+     *
+     * 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;
 }
 
-#endif /* __ASM_X86_MONITOR_H__ */
+#endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 7c8280b..c789f71 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -1,9 +1,10 @@
 /*
  * include/asm-x86/monitor.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
@@ -21,11 +22,55 @@
 #ifndef __ASM_X86_MONITOR_H__
 #define __ASM_X86_MONITOR_H__
 
-struct domain;
-struct xen_domctl_monitor_op;
+#include <xen/sched.h>
+#include <public/domctl.h>
+#include <asm/cpufeature.h>
+#include <asm/hvm/hvm.h>
 
 #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
 
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
+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
+int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+    switch ( mop->op )
+    {
+    case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
+        domain_pause(d);
+        d->arch.mem_access_emulate_each_rep = !!mop->event;
+        domain_unpause(d);
+        break;
+
+    default:
+        return -EOPNOTSUPP;
+    }
+
+    return 0;
+}
+
+int arch_monitor_domctl_event(struct domain *d,
+                              struct xen_domctl_monitor_op *mop);
 
 #endif /* __ASM_X86_MONITOR_H__ */
diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h
new file mode 100644
index 0000000..ae49a39
--- /dev/null
+++ b/xen/include/xen/monitor.h
@@ -0,0 +1,30 @@
+/*
+ * include/xen/monitor.h
+ *
+ * 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
+ * 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 __MONITOR_H__
+#define __MONITOR_H__
+
+#include <xen/sched.h>
+#include <public/domctl.h>
+
+int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
+
+#endif /* __MONITOR_H__ */
-- 
2.5.0

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 8:37 AM, Corneliu ZUZU wrote:
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> new file mode 100644
> index 0000000..b708cab
> --- /dev/null
> +++ b/xen/common/monitor.c
> +    int rc;
> +    bool_t requested_status = 0;
> +
> +    if ( unlikely(current->domain == d) ) /* no domain_pause() */
> +        return -EPERM;
> +
> +    rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
> +    if ( unlikely(rc) )
> +        return rc;
> +
> +    switch ( mop->op )
> +    {
> +    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;
> +        /* Arch-side handles enable/disable ops. */
> +        return arch_monitor_domctl_event(d, mop);
>

Only noticed now, requested_status now became unused in this function.
Will remove in v4.

Corneliu.

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Jan Beulich]

>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote:
>      default:
> -        return -EOPNOTSUPP;
> +        /*
> +         * 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;
> +    }

I disagree with the issuing of a message here. At the very least this
should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
the way to go? What's worse though is that I can't see the checking
which would make true the "should not be reached" statement above
(not that you must not rely on the caller of the hypercall to be well
behaved).

> --- /dev/null
> +++ b/xen/include/xen/monitor.h
> @@ -0,0 +1,30 @@
> +/*
> + * include/xen/monitor.h
> + *
> + * 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
> + * 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 __MONITOR_H__
> +#define __MONITOR_H__

__XEN_MONITOR_H__ please.

> +#include <xen/sched.h>
> +#include <public/domctl.h>
> +
> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);

Neither of the two includes seem necessary for this prototype, all
you need are forward declarations of the two involved structures.

Jan

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 1:41 PM, Jan Beulich wrote:
>>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote:
>>       default:
>> -        return -EOPNOTSUPP;
>> +        /*
>> +         * 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;
>> +    }
> I disagree with the issuing of a message here. At the very least this
> should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
> the way to go?

IDK, but I agree that it doesn't look so elegant like that.
Won't ASSERT_UNREACHABLE crash the hypervisor?

> What's worse though is that I can't see the checking
> which would make true the "should not be reached" statement above
> (not that you must not rely on the caller of the hypercall to be well
> behaved).

The reasoning is as follows.
 From this part in monitor_domctl:

     switch ( mop->op )
     {
     case XEN_DOMCTL_MONITOR_OP_ENABLE:
     case XEN_DOMCTL_MONITOR_OP_DISABLE:
         /* Check if event type is available. */
         if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << 
mop->event))) )
             return -EOPNOTSUPP;
         /* Arch-side handles enable/disable ops. */
         return arch_monitor_domctl_event(d, mop);

we can see that arch_monitor_domctl_event gets to be called only when 
arch_monitor_get_capabilities reports
an 'available' mop->event.
But if then in arch_monitor_domctl_event the default case is reached, it 
would mean
that arch_monitor_get_capabilities reported a mop->event as being 
available/supported
when is *not actually handled* anywhere.

>
>> --- /dev/null
>> +++ b/xen/include/xen/monitor.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * include/xen/monitor.h
>> + *
>> + * 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
>> + * 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 __MONITOR_H__
>> +#define __MONITOR_H__
> __XEN_MONITOR_H__ please.
>
>> +#include <xen/sched.h>
>> +#include <public/domctl.h>
>> +
>> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
> Neither of the two includes seem necessary for this prototype, all
> you need are forward declarations of the two involved structures.
>
> Jan

Noted.

Corneliu.

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Jan Beulich]

>>> On 15.02.16 at 13:14, <czuzu@bitdefender.com> wrote:
> On 2/15/2016 1:41 PM, Jan Beulich wrote:
>>>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote:
>>>       default:
>>> -        return -EOPNOTSUPP;
>>> +        /*
>>> +         * 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;
>>> +    }
>> I disagree with the issuing of a message here. At the very least this
>> should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
>> the way to go?
> 
> IDK, but I agree that it doesn't look so elegant like that.
> Won't ASSERT_UNREACHABLE crash the hypervisor?

In a debug build yes. In a release build no.

>> What's worse though is that I can't see the checking
>> which would make true the "should not be reached" statement above
>> (not that you must not rely on the caller of the hypercall to be well
>> behaved).
> 
> The reasoning is as follows.
>  From this part in monitor_domctl:
> 
>      switch ( mop->op )
>      {
>      case XEN_DOMCTL_MONITOR_OP_ENABLE:
>      case XEN_DOMCTL_MONITOR_OP_DISABLE:
>          /* Check if event type is available. */
>          if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) )
>              return -EOPNOTSUPP;
>          /* Arch-side handles enable/disable ops. */
>          return arch_monitor_domctl_event(d, mop);
> 
> we can see that arch_monitor_domctl_event gets to be called only when 
> arch_monitor_get_capabilities reports
> an 'available' mop->event.
> But if then in arch_monitor_domctl_event the default case is reached, it 
> would mean
> that arch_monitor_get_capabilities reported a mop->event as being 
> available/supported
> when is *not actually handled* anywhere.

Ah, I see now that I've mis-read the default: code further below,
which actually calls arch_monitor_domctl_op(), not ..._event().
However, there's an "undefined behavior" issue with the code
above then when mop->event >= 31 - I think you want to left
shift 1U instead of plain 1, and you need to range check
mop->event first.

Jan

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

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

On 2/15/2016 2:44 PM, Jan Beulich wrote:
>
>>       switch ( mop->op )
>>       {
>>       case XEN_DOMCTL_MONITOR_OP_ENABLE:
>>       case XEN_DOMCTL_MONITOR_OP_DISABLE:
>>           /* Check if event type is available. */
>>           if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) )
>>               return -EOPNOTSUPP;
>>           /* Arch-side handles enable/disable ops. */
>>           return arch_monitor_domctl_event(d, mop);
> Ah, I see now that I've mis-read the default: code further below,
> which actually calls arch_monitor_domctl_op(), not ..._event().
> However, there's an "undefined behavior" issue with the code
> above then when mop->event >= 31 - I think you want to left
> shift 1U instead of plain 1, and you need to range check
> mop->event first.
>
> Jan
>
Never looked @ that part before, used it the way it was.
I suppose that's because "according to the C specification, the result 
of a bit shift
operation on a signed argument gives implementation-defined results, so 
in/theory/|1U << i|is
more portable than|1 << i|" (taken from a stackoverflow post).

After changing 1 to 1U though, I don't understand why we should also 
range-check mop->event.
I'm imagining when (mop->event > 31):
* (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)
* in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) 
would be = 0
* in which case we would return -EOPNOTSUPP
, no?

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 5984 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 v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Jan Beulich]

>>> On 15.02.16 at 14:29, <czuzu@bitdefender.com> wrote:
> On 2/15/2016 2:44 PM, Jan Beulich wrote:
>>
>>>       switch ( mop->op )
>>>       {
>>>       case XEN_DOMCTL_MONITOR_OP_ENABLE:
>>>       case XEN_DOMCTL_MONITOR_OP_DISABLE:
>>>           /* Check if event type is available. */
>>>           if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) )
>>>               return -EOPNOTSUPP;
>>>           /* Arch-side handles enable/disable ops. */
>>>           return arch_monitor_domctl_event(d, mop);
>> Ah, I see now that I've mis-read the default: code further below,
>> which actually calls arch_monitor_domctl_op(), not ..._event().
>> However, there's an "undefined behavior" issue with the code
>> above then when mop->event >= 31 - I think you want to left
>> shift 1U instead of plain 1, and you need to range check
>> mop->event first.
>>
> Never looked @ that part before, used it the way it was.
> I suppose that's because "according to the C specification, the result 
> of a bit shift
> operation on a signed argument gives implementation-defined results, so 
> in/theory/|1U << i|is
> more portable than|1 << i|" (taken from a stackoverflow post).

Yes.

> After changing 1 to 1U though, I don't understand why we should also 
> range-check mop->event.
> I'm imagining when (mop->event > 31):
> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)

No, it's plain undefined.

> * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) 
> would be = 0
> * in which case we would return -EOPNOTSUPP
> , no?

And even that would be true only today, and would break once
bit 31 gets a meaning. Whenever possible we should avoid
introducing such latent issues.

Jan

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Tamas K Lengyel]

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

On Mon, Feb 15, 2016 at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 15.02.16 at 14:29, <czuzu@bitdefender.com> wrote:
> > On 2/15/2016 2:44 PM, Jan Beulich wrote:
> >>
> >>>       switch ( mop->op )
> >>>       {
> >>>       case XEN_DOMCTL_MONITOR_OP_ENABLE:
> >>>       case XEN_DOMCTL_MONITOR_OP_DISABLE:
> >>>           /* Check if event type is available. */
> >>>           if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 <<
> mop->event))) )
> >>>               return -EOPNOTSUPP;
> >>>           /* Arch-side handles enable/disable ops. */
> >>>           return arch_monitor_domctl_event(d, mop);
> >> Ah, I see now that I've mis-read the default: code further below,
> >> which actually calls arch_monitor_domctl_op(), not ..._event().
> >> However, there's an "undefined behavior" issue with the code
> >> above then when mop->event >= 31 - I think you want to left
> >> shift 1U instead of plain 1, and you need to range check
> >> mop->event first.
> >>
> > Never looked @ that part before, used it the way it was.
> > I suppose that's because "according to the C specification, the result
> > of a bit shift
> > operation on a signed argument gives implementation-defined results, so
> > in/theory/|1U << i|is
> > more portable than|1 << i|" (taken from a stackoverflow post).
>
> Yes.
>
> > After changing 1 to 1U though, I don't understand why we should also
> > range-check mop->event.
> > I'm imagining when (mop->event > 31):
> > * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)
>
> No, it's plain undefined.
>
> > * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event)
> > would be = 0
> > * in which case we would return -EOPNOTSUPP
> > , no?
>
> And even that would be true only today, and would break once
> bit 31 gets a meaning. Whenever possible we should avoid
> introducing such latent issues.
>

Ah yes, good catch, +1 for doing this extra check.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 2969 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 v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 4:08 PM, Jan Beulich wrote:
>
>> After changing 1 to 1U though, I don't understand why we should also
>> range-check mop->event.
>> I'm imagining when (mop->event > 31):
>> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)
> No, it's plain undefined.

Weirdo C, didn't know that!
I've just read http://www.danielvik.com/2010/05/c-language-quirks.html .
That's crazy, I can't believe such 'quirks' exist and that I never knew 
of them.
So then, would this do:

/* sanity check - avoid '<<' operator undefined behavior */
if ( unlikely(mop->event > 31) )
     return -EINVAL;
if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
     return -EOPNOTSUPP;


?

Thanks,
Corneliu.

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Jan Beulich]

>>> On 15.02.16 at 17:28, <czuzu@bitdefender.com> wrote:
> On 2/15/2016 4:08 PM, Jan Beulich wrote:
>>
>>> After changing 1 to 1U though, I don't understand why we should also
>>> range-check mop->event.
>>> I'm imagining when (mop->event > 31):
>>> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)
>> No, it's plain undefined.
> 
> Weirdo C, didn't know that!
> I've just read http://www.danielvik.com/2010/05/c-language-quirks.html .
> That's crazy, I can't believe such 'quirks' exist and that I never knew 
> of them.
> So then, would this do:
> 
> /* sanity check - avoid '<<' operator undefined behavior */
> if ( unlikely(mop->event > 31) )
>      return -EINVAL;
> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
>      return -EOPNOTSUPP;

I'd say -EOPNOTSUPP in both cases, but if the maintainers like
-EINVAL better I wouldn't insist on my preference.

Jan

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Tamas K Lengyel]

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

On Mon, Feb 15, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 15.02.16 at 17:28, <czuzu@bitdefender.com> wrote:
> > On 2/15/2016 4:08 PM, Jan Beulich wrote:
> >>
> >>> After changing 1 to 1U though, I don't understand why we should also
> >>> range-check mop->event.
> >>> I'm imagining when (mop->event > 31):
> >>> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)
> >> No, it's plain undefined.
> >
> > Weirdo C, didn't know that!
> > I've just read http://www.danielvik.com/2010/05/c-language-quirks.html .
> > That's crazy, I can't believe such 'quirks' exist and that I never knew
> > of them.
> > So then, would this do:
> >
> > /* sanity check - avoid '<<' operator undefined behavior */
> > if ( unlikely(mop->event > 31) )
> >      return -EINVAL;
> > if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
> >      return -EOPNOTSUPP;
>
> I'd say -EOPNOTSUPP in both cases, but if the maintainers like
> -EINVAL better I wouldn't insist on my preference.
>

The best approach of course would be if we had __MAX values defined for
such enums to check against but that doesn't seem to be part of Xen's
coding practice. So in this case I would say leave it as -EINVAL as it's
more descriptive of the problem and may even signal to the caller some
inherent bug on their side, not just that the requested option is not
supported.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 2261 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 v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

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

On 2/15/2016 6:51 PM, Tamas K Lengyel wrote:
>
>
> On Mon, Feb 15, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com 
> <mailto:JBeulich@suse.com>> wrote:
>
>     >>> On 15.02.16 at 17:28, <czuzu@bitdefender.com <mailto:czuzu@bitdefender.com>> wrote:
>     > On 2/15/2016 4:08 PM, Jan Beulich wrote:
>     >>
>     >>> After changing 1 to 1U though, I don't understand why we
>     should also
>     >>> range-check mop->event.
>     >>> I'm imagining when (mop->event > 31):
>     >>> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)
>     >> No, it's plain undefined.
>     >
>     > Weirdo C, didn't know that!
>     > I've just read
>     http://www.danielvik.com/2010/05/c-language-quirks.html .
>     > That's crazy, I can't believe such 'quirks' exist and that I
>     never knew
>     > of them.
>     > So then, would this do:
>     >
>     > /* sanity check - avoid '<<' operator undefined behavior */
>     > if ( unlikely(mop->event > 31) )
>     >      return -EINVAL;
>     > if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U <<
>     mop->event))) )
>     >      return -EOPNOTSUPP;
>
>     I'd say -EOPNOTSUPP in both cases, but if the maintainers like
>     -EINVAL better I wouldn't insist on my preference.
>
>
> The best approach of course would be if we had __MAX values defined 
> for such enums to check against but that doesn't seem to be part of 
> Xen's coding practice. So in this case I would say leave it as -EINVAL 
> as it's more descriptive of the problem and may even signal to the 
> caller some inherent bug on their side, not just that the requested 
> option is not supported.
>
> Thanks,
> Tamas
>

Good points, noted.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 3782 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 v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 1:41 PM, Jan Beulich wrote:
>> +++ b/xen/include/xen/monitor.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * include/xen/monitor.h
>> + *
>> + * 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
>> + * 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 __MONITOR_H__
>> +#define __MONITOR_H__
> __XEN_MONITOR_H__ please.

Hadn't noticed this comment, also noted.

Thanks,
Corneliu.

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Stefano Stabellini]

On Mon, 15 Feb 2016, Jan Beulich wrote:
> >>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote:
> >      default:
> > -        return -EOPNOTSUPP;
> > +        /*
> > +         * 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;
> > +    }
> 
> I disagree with the issuing of a message here. At the very least this
> should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
> the way to go? What's worse though is that I can't see the checking
> which would make true the "should not be reached" statement above
> (not that you must not rely on the caller of the hypercall to be well
> behaved).

ASSERT_UNREACHABLE() is appropriate here

Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 2:25 PM, Stefano Stabellini wrote:
> On Mon, 15 Feb 2016, Jan Beulich wrote:
>>>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> wrote:
>>>       default:
>>> -        return -EOPNOTSUPP;
>>> +        /*
>>> +         * 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;
>>> +    }
>> I disagree with the issuing of a message here. At the very least this
>> should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
>> the way to go? What's worse though is that I can't see the checking
>> which would make true the "should not be reached" statement above
>> (not that you must not rely on the caller of the hypercall to be well
>> behaved).
> ASSERT_UNREACHABLE() is appropriate here
>

Noted.

Corneliu.