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

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

[Corneliu ZUZU]

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

Patches summary:

1. Move xen/arch/arm/hvm.c to xen/arch/arm/hvm/hvm.c

2. Merge almost identical functions hvm_event_int3 + This patch series is an attempt to move (most) of the monitor vm-events code to
the common-side.

Patches summary:

1. Move xen/arch/arm/hvm.c to xen/arch/arm/hvm/hvm.c

2. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
hvm_event_software_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. Some file renames. Read (*) below.

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

6. Some file renames, plus some minor fixes. Read (*) below.

7. Move monitor bitfield members from struct arch_domain to struct domain.
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).hvm_event_single_step ->
hvm_event_software_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. Some file renames. Read (*) below.

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

6. Some file renames, plus some minor fixes. Read (*) below.

7. Move monitor bitfield members from struct arch_domain to struct domain.
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).

[PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/arm/Makefile     |  2 +-
 xen/arch/arm/hvm.c        | 67 -----------------------------------------------
 xen/arch/arm/hvm/Makefile |  1 +
 xen/arch/arm/hvm/hvm.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+), 68 deletions(-)
 delete mode 100644 xen/arch/arm/hvm.c
 create mode 100644 xen/arch/arm/hvm/Makefile
 create mode 100644 xen/arch/arm/hvm/hvm.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 1783912..22f1c75 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -1,6 +1,7 @@
 subdir-$(CONFIG_ARM_32) += arm32
 subdir-$(CONFIG_ARM_64) += arm64
 subdir-y += platforms
+subdir-y += hvm
 subdir-$(CONFIG_ARM_64) += efi
 
 obj-$(EARLY_PRINTK) += early_printk.o
@@ -34,7 +35,6 @@ obj-y += vgic.o vgic-v2.o
 obj-$(CONFIG_ARM_64) += vgic-v3.o
 obj-y += vtimer.o
 obj-y += vuart.o
-obj-y += hvm.o
 obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
deleted file mode 100644
index 5fd0753..0000000
--- a/xen/arch/arm/hvm.c
+++ /dev/null
@@ -1,67 +0,0 @@
-#include <xen/config.h>
-#include <xen/init.h>
-#include <xen/lib.h>
-#include <xen/errno.h>
-#include <xen/guest_access.h>
-#include <xen/sched.h>
-
-#include <xsm/xsm.h>
-
-#include <public/xen.h>
-#include <public/hvm/params.h>
-#include <public/hvm/hvm_op.h>
-
-#include <asm/hypercall.h>
-
-long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
-{
-    long rc = 0;
-
-    switch ( op )
-    {
-    case HVMOP_set_param:
-    case HVMOP_get_param:
-    {
-        struct xen_hvm_param a;
-        struct domain *d;
-
-        if ( copy_from_guest(&a, arg, 1) )
-            return -EFAULT;
-
-        if ( a.index >= HVM_NR_PARAMS )
-            return -EINVAL;
-
-        d = rcu_lock_domain_by_any_id(a.domid);
-        if ( d == NULL )
-            return -ESRCH;
-
-        rc = xsm_hvm_param(XSM_TARGET, d, op);
-        if ( rc )
-            goto param_fail;
-
-        if ( op == HVMOP_set_param )
-        {
-            d->arch.hvm_domain.params[a.index] = a.value;
-        }
-        else
-        {
-            a.value = d->arch.hvm_domain.params[a.index];
-            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
-        }
-
-    param_fail:
-        rcu_unlock_domain(d);
-        break;
-    }
-
-    default:
-    {
-        gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
-        rc = -ENOSYS;
-        break;
-    }
-    }
-
-    return rc;
-}
diff --git a/xen/arch/arm/hvm/Makefile b/xen/arch/arm/hvm/Makefile
new file mode 100644
index 0000000..6ee4054
--- /dev/null
+++ b/xen/arch/arm/hvm/Makefile
@@ -0,0 +1 @@
+obj-y += hvm.o
diff --git a/xen/arch/arm/hvm/hvm.c b/xen/arch/arm/hvm/hvm.c
new file mode 100644
index 0000000..1ae681f
--- /dev/null
+++ b/xen/arch/arm/hvm/hvm.c
@@ -0,0 +1,66 @@
+#include <xen/config.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <xen/guest_access.h>
+#include <xen/sched.h>
+
+#include <xsm/xsm.h>
+
+#include <public/xen.h>
+#include <public/hvm/params.h>
+#include <public/hvm/hvm_op.h>
+
+#include <asm/hypercall.h>
+
+long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    long rc = 0;
+
+    switch ( op )
+    {
+    case HVMOP_set_param:
+    case HVMOP_get_param:
+    {
+        struct xen_hvm_param a;
+        struct domain *d;
+
+        if ( copy_from_guest(&a, arg, 1) )
+            return -EFAULT;
+
+        if ( a.index >= HVM_NR_PARAMS )
+            return -EINVAL;
+
+        d = rcu_lock_domain_by_any_id(a.domid);
+        if ( d == NULL )
+            return -ESRCH;
+
+        rc = xsm_hvm_param(XSM_TARGET, d, op);
+        if ( rc )
+            goto param_fail;
+
+        if ( op == HVMOP_set_param )
+        {
+            d->arch.hvm_domain.params[a.index] = a.value;
+        }
+        else
+        {
+            a.value = d->arch.hvm_domain.params[a.index];
+            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+        }
+
+    param_fail:
+        rcu_unlock_domain(d);
+        break;
+    }
+
+    default:
+    {
+        gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
+        rc = -ENOSYS;
+        break;
+    }
+    }
+
+    return rc;
+}
-- 
2.5.0

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Andrew Cooper]

On 08/02/16 16:57, Corneliu ZUZU wrote:
> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>

For future reference, constructing your patches with -M (detect renames)
makes reviews of patches like this far easier.

While you are editing this file, please put a local variable block on
the bottom of the file.  See the final section of CODING_STYLE in the root.

~Andrew

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

On 2/8/2016 7:04 PM, Andrew Cooper wrote:
> On 08/02/16 16:57, Corneliu ZUZU wrote:
>> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
>> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
>>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> For future reference, constructing your patches with -M (detect renames)
> makes reviews of patches like this far easier.
>
> While you are editing this file, please put a local variable block on
> the bottom of the file.  See the final section of CODING_STYLE in the root.
>
> ~Andrew
>
I'm really sorry, I forgot, I was actually *counting* on that option, 
wanted to use it as -M40%
actually. And I really don't get why git malformed the introductory message.
Since the diffs would indeed look *much* better w/ the -M option and I 
should also add
the variable block @ the end of that file (which was originally missing) 
would it be advised to
resend this series?

Corneliu.

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Andrew Cooper]

On 08/02/16 17:12, Corneliu ZUZU wrote:
> On 2/8/2016 7:04 PM, Andrew Cooper wrote:
>> On 08/02/16 16:57, Corneliu ZUZU wrote:
>>> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
>>> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
>>>
>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>> For future reference, constructing your patches with -M (detect renames)
>> makes reviews of patches like this far easier.
>>
>> While you are editing this file, please put a local variable block on
>> the bottom of the file.  See the final section of CODING_STYLE in the
>> root.
>>
>> ~Andrew
>>
> I'm really sorry, I forgot, I was actually *counting* on that option,
> wanted to use it as -M40%
> actually. And I really don't get why git malformed the introductory
> message.
> Since the diffs would indeed look *much* better w/ the -M option and I
> should also add
> the variable block @ the end of that file (which was originally
> missing) would it be advised to
> resend this series?

I would wait for some other review first.  There are some useful
comments to be given even with the series like this.

~Andrew

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Stefano Stabellini]

On Mon, 8 Feb 2016, Corneliu ZUZU wrote:
> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.

Why are we doing this? These are not header files, their paths don't
necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
different from xen/arch/arm/hvm.c.

Please state the reason more clearly.


> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>  xen/arch/arm/Makefile     |  2 +-
>  xen/arch/arm/hvm.c        | 67 -----------------------------------------------
>  xen/arch/arm/hvm/Makefile |  1 +
>  xen/arch/arm/hvm/hvm.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 68 insertions(+), 68 deletions(-)
>  delete mode 100644 xen/arch/arm/hvm.c
>  create mode 100644 xen/arch/arm/hvm/Makefile
>  create mode 100644 xen/arch/arm/hvm/hvm.c
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 1783912..22f1c75 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -1,6 +1,7 @@
>  subdir-$(CONFIG_ARM_32) += arm32
>  subdir-$(CONFIG_ARM_64) += arm64
>  subdir-y += platforms
> +subdir-y += hvm
>  subdir-$(CONFIG_ARM_64) += efi
>  
>  obj-$(EARLY_PRINTK) += early_printk.o
> @@ -34,7 +35,6 @@ obj-y += vgic.o vgic-v2.o
>  obj-$(CONFIG_ARM_64) += vgic-v3.o
>  obj-y += vtimer.o
>  obj-y += vuart.o
> -obj-y += hvm.o
>  obj-y += device.o
>  obj-y += decode.o
>  obj-y += processor.o
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> deleted file mode 100644
> index 5fd0753..0000000
> --- a/xen/arch/arm/hvm.c
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -#include <xen/config.h>
> -#include <xen/init.h>
> -#include <xen/lib.h>
> -#include <xen/errno.h>
> -#include <xen/guest_access.h>
> -#include <xen/sched.h>
> -
> -#include <xsm/xsm.h>
> -
> -#include <public/xen.h>
> -#include <public/hvm/params.h>
> -#include <public/hvm/hvm_op.h>
> -
> -#include <asm/hypercall.h>
> -
> -long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> -
> -{
> -    long rc = 0;
> -
> -    switch ( op )
> -    {
> -    case HVMOP_set_param:
> -    case HVMOP_get_param:
> -    {
> -        struct xen_hvm_param a;
> -        struct domain *d;
> -
> -        if ( copy_from_guest(&a, arg, 1) )
> -            return -EFAULT;
> -
> -        if ( a.index >= HVM_NR_PARAMS )
> -            return -EINVAL;
> -
> -        d = rcu_lock_domain_by_any_id(a.domid);
> -        if ( d == NULL )
> -            return -ESRCH;
> -
> -        rc = xsm_hvm_param(XSM_TARGET, d, op);
> -        if ( rc )
> -            goto param_fail;
> -
> -        if ( op == HVMOP_set_param )
> -        {
> -            d->arch.hvm_domain.params[a.index] = a.value;
> -        }
> -        else
> -        {
> -            a.value = d->arch.hvm_domain.params[a.index];
> -            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> -        }
> -
> -    param_fail:
> -        rcu_unlock_domain(d);
> -        break;
> -    }
> -
> -    default:
> -    {
> -        gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
> -        rc = -ENOSYS;
> -        break;
> -    }
> -    }
> -
> -    return rc;
> -}
> diff --git a/xen/arch/arm/hvm/Makefile b/xen/arch/arm/hvm/Makefile
> new file mode 100644
> index 0000000..6ee4054
> --- /dev/null
> +++ b/xen/arch/arm/hvm/Makefile
> @@ -0,0 +1 @@
> +obj-y += hvm.o
> diff --git a/xen/arch/arm/hvm/hvm.c b/xen/arch/arm/hvm/hvm.c
> new file mode 100644
> index 0000000..1ae681f
> --- /dev/null
> +++ b/xen/arch/arm/hvm/hvm.c
> @@ -0,0 +1,66 @@
> +#include <xen/config.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/guest_access.h>
> +#include <xen/sched.h>
> +
> +#include <xsm/xsm.h>
> +
> +#include <public/xen.h>
> +#include <public/hvm/params.h>
> +#include <public/hvm/hvm_op.h>
> +
> +#include <asm/hypercall.h>
> +
> +long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    long rc = 0;
> +
> +    switch ( op )
> +    {
> +    case HVMOP_set_param:
> +    case HVMOP_get_param:
> +    {
> +        struct xen_hvm_param a;
> +        struct domain *d;
> +
> +        if ( copy_from_guest(&a, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( a.index >= HVM_NR_PARAMS )
> +            return -EINVAL;
> +
> +        d = rcu_lock_domain_by_any_id(a.domid);
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        rc = xsm_hvm_param(XSM_TARGET, d, op);
> +        if ( rc )
> +            goto param_fail;
> +
> +        if ( op == HVMOP_set_param )
> +        {
> +            d->arch.hvm_domain.params[a.index] = a.value;
> +        }
> +        else
> +        {
> +            a.value = d->arch.hvm_domain.params[a.index];
> +            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +        }
> +
> +    param_fail:
> +        rcu_unlock_domain(d);
> +        break;
> +    }
> +
> +    default:
> +    {
> +        gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
> +        rc = -ENOSYS;
> +        break;
> +    }
> +    }
> +
> +    return rc;
> +}
> -- 
> 2.5.0
> 

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

On 2/9/2016 1:03 PM, Stefano Stabellini wrote:
> On Mon, 8 Feb 2016, Corneliu ZUZU wrote:
>> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
>> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
> Why are we doing this? These are not header files, their paths don't
> necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
> different from xen/arch/arm/hvm.c.
>
> Please state the reason more clearly.
>
>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>> ---
>>   xen/arch/arm/Makefile     |  2 +-
>>   xen/arch/arm/hvm.c        | 67 -----------------------------------------------
>>   xen/arch/arm/hvm/Makefile |  1 +
>>   xen/arch/arm/hvm/hvm.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>>

Because the ARM side hvm.c currently exists solely to add an implementation for
do_hvm_op, which on x86 is @ arch/x86/hvm/hvm.c. I presume the ARM hvm.c got its name
from the X86 file, so I thought a symmetry between the two was intended from the start.
Also, the hvm directory was created to separate hvm-specific code, which is the case w/
do_hvm_op on any arch.

Corneliu.

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Jan Beulich]

>>> On 09.02.16 at 12:28, <czuzu@bitdefender.com> wrote:
> On 2/9/2016 1:03 PM, Stefano Stabellini wrote:
>> On Mon, 8 Feb 2016, Corneliu ZUZU wrote:
>>> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
>>> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
>> Why are we doing this? These are not header files, their paths don't
>> necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
>> different from xen/arch/arm/hvm.c.
>>
>> Please state the reason more clearly.
>>
>>
>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>> ---
>>>   xen/arch/arm/Makefile     |  2 +-
>>>   xen/arch/arm/hvm.c        | 67 -----------------------------------------------
>>>   xen/arch/arm/hvm/Makefile |  1 +
>>>   xen/arch/arm/hvm/hvm.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>>>
> 
> Because the ARM side hvm.c currently exists solely to add an implementation 
> for
> do_hvm_op, which on x86 is @ arch/x86/hvm/hvm.c. I presume the ARM hvm.c got 
> its name
> from the X86 file, so I thought a symmetry between the two was intended from 
> the start.
> Also, the hvm directory was created to separate hvm-specific code, which is 
> the case w/
> do_hvm_op on any arch.

While I'm not an ARM maintainer, this change still strikes me as odd
(or a change for the change's sake). A directory with just one file
in it (and - afaict - no current perspective to gain more) is just
pointless. In fact it's usually the other way around: When a file
grows (or would grow) too large, a similarly named subdirectory
gets introduced with the contents "scattered" across multiple files
in that directory.

Jan

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Stefano Stabellini]

On Tue, 9 Feb 2016, Jan Beulich wrote:
> >>> On 09.02.16 at 12:28, <czuzu@bitdefender.com> wrote:
> > On 2/9/2016 1:03 PM, Stefano Stabellini wrote:
> >> On Mon, 8 Feb 2016, Corneliu ZUZU wrote:
> >>> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
> >>> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
> >> Why are we doing this? These are not header files, their paths don't
> >> necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
> >> different from xen/arch/arm/hvm.c.
> >>
> >> Please state the reason more clearly.
> >>
> >>
> >>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> >>> ---
> >>>   xen/arch/arm/Makefile     |  2 +-
> >>>   xen/arch/arm/hvm.c        | 67 -----------------------------------------------
> >>>   xen/arch/arm/hvm/Makefile |  1 +
> >>>   xen/arch/arm/hvm/hvm.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>
> > 
> > Because the ARM side hvm.c currently exists solely to add an implementation 
> > for
> > do_hvm_op, which on x86 is @ arch/x86/hvm/hvm.c. I presume the ARM hvm.c got 
> > its name
> > from the X86 file, so I thought a symmetry between the two was intended from 
> > the start.
> > Also, the hvm directory was created to separate hvm-specific code, which is 
> > the case w/
> > do_hvm_op on any arch.
> 
> While I'm not an ARM maintainer, this change still strikes me as odd
> (or a change for the change's sake). A directory with just one file
> in it (and - afaict - no current perspective to gain more) is just
> pointless. In fact it's usually the other way around: When a file
> grows (or would grow) too large, a similarly named subdirectory
> gets introduced with the contents "scattered" across multiple files
> in that directory.
 
That's exactly what I thought.

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

On 2/9/2016 1:55 PM, Jan Beulich wrote:
>>>> On 09.02.16 at 12:28, <czuzu@bitdefender.com> wrote:
>> On 2/9/2016 1:03 PM, Stefano Stabellini wrote:
>>> On Mon, 8 Feb 2016, Corneliu ZUZU wrote:
>>>> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
>>>> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
>>> Why are we doing this? These are not header files, their paths don't
>>> necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
>>> different from xen/arch/arm/hvm.c.
>>>
>>> Please state the reason more clearly.
>>>
>>>
>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>>> ---
>>>>    xen/arch/arm/Makefile     |  2 +-
>>>>    xen/arch/arm/hvm.c        | 67 -----------------------------------------------
>>>>    xen/arch/arm/hvm/Makefile |  1 +
>>>>    xen/arch/arm/hvm/hvm.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>
>> Because the ARM side hvm.c currently exists solely to add an implementation
>> for
>> do_hvm_op, which on x86 is @ arch/x86/hvm/hvm.c. I presume the ARM hvm.c got
>> its name
>> from the X86 file, so I thought a symmetry between the two was intended from
>> the start.
>> Also, the hvm directory was created to separate hvm-specific code, which is
>> the case w/
>> do_hvm_op on any arch.
> While I'm not an ARM maintainer, this change still strikes me as odd
> (or a change for the change's sake). A directory with just one file
> in it (and - afaict - no current perspective to gain more) is just
> pointless. In fact it's usually the other way around: When a file
> grows (or would grow) too large, a similarly named subdirectory
> gets introduced with the contents "scattered" across multiple files
> in that directory.
>
> Jan

There are already directories w/ just one/a few files in them, even 
small (e.g. common/gcov/gcov.c).
IMHO no harm is done if a file is put in its proper directory even 
before it grows too large.
This way one can find the file more easily, future additions are more 
visibly encouraged to
also be separated in that directory when it makes sense, symmetry 
between arch directories remains
intact (=> easier to compare between code for different arches/find 
equivalent files between them).

But I am not a Xen maintainer, I'm only a contributor so I can only 
suggest :).
If ARM maintainers (e.g. Tamas) feel the same, I will move the file back.

Corneliu.

Re: [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Tamas K Lengyel]

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

On Tue, Feb 9, 2016 at 5:32 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:

> On 2/9/2016 1:55 PM, Jan Beulich wrote:
>
>> On 09.02.16 at 12:28, <czuzu@bitdefender.com> wrote:
>>>>>
>>>> On 2/9/2016 1:03 PM, Stefano Stabellini wrote:
>>>
>>>> On Mon, 8 Feb 2016, Corneliu ZUZU wrote:
>>>>
>>>>> X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
>>>>> also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.
>>>>>
>>>> Why are we doing this? These are not header files, their paths don't
>>>> necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
>>>> different from xen/arch/arm/hvm.c.
>>>>
>>>> Please state the reason more clearly.
>>>>
>>>>
>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>>>> ---
>>>>>    xen/arch/arm/Makefile     |  2 +-
>>>>>    xen/arch/arm/hvm.c        | 67
>>>>> -----------------------------------------------
>>>>>    xen/arch/arm/hvm/Makefile |  1 +
>>>>>    xen/arch/arm/hvm/hvm.c    | 66
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>
>>>>> Because the ARM side hvm.c currently exists solely to add an
>>> implementation
>>> for
>>> do_hvm_op, which on x86 is @ arch/x86/hvm/hvm.c. I presume the ARM hvm.c
>>> got
>>> its name
>>> from the X86 file, so I thought a symmetry between the two was intended
>>> from
>>> the start.
>>> Also, the hvm directory was created to separate hvm-specific code, which
>>> is
>>> the case w/
>>> do_hvm_op on any arch.
>>>
>> While I'm not an ARM maintainer, this change still strikes me as odd
>> (or a change for the change's sake). A directory with just one file
>> in it (and - afaict - no current perspective to gain more) is just
>> pointless. In fact it's usually the other way around: When a file
>> grows (or would grow) too large, a similarly named subdirectory
>> gets introduced with the contents "scattered" across multiple files
>> in that directory.
>>
>> Jan
>>
>
> There are already directories w/ just one/a few files in them, even small
> (e.g. common/gcov/gcov.c).
> IMHO no harm is done if a file is put in its proper directory even before
> it grows too large.
> This way one can find the file more easily, future additions are more
> visibly encouraged to
> also be separated in that directory when it makes sense, symmetry between
> arch directories remains
> intact (=> easier to compare between code for different arches/find
> equivalent files between them).
>
> But I am not a Xen maintainer, I'm only a contributor so I can only
> suggest :).
> If ARM maintainers (e.g. Tamas) feel the same, I will move the file back.
>

I don't really have a strong opinion about this either way, so if the
others prefer it staying as it is then there is no point moving it.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4165 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 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

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

On 2/9/2016 7:40 PM, Tamas K Lengyel wrote:
>
>
> On Tue, Feb 9, 2016 at 5:32 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     There are already directories w/ just one/a few files in them,
>     even small (e.g. common/gcov/gcov.c).
>     IMHO no harm is done if a file is put in its proper directory even
>     before it grows too large.
>     This way one can find the file more easily, future additions are
>     more visibly encouraged to
>     also be separated in that directory when it makes sense, symmetry
>     between arch directories remains
>     intact (=> easier to compare between code for different
>     arches/find equivalent files between them).
>
>     But I am not a Xen maintainer, I'm only a contributor so I can
>     only suggest :).
>     If ARM maintainers (e.g. Tamas) feel the same, I will move the
>     file back.
>
>
> I don't really have a strong opinion about this either way, so if the 
> others prefer it staying as it is then there is no point moving it.
>
> Tamas
>

Ok, will do, thanks for the feedback.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 2394 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 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

This patch merges almost identical functions hvm_event_int3
and hvm_event_single_step into a single function called
hvm_event_software_breakpoint.

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

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index a3d4892..9dc533b 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -151,17 +151,20 @@ void hvm_event_guest_request(void)
     }
 }
 
-int hvm_event_int3(unsigned long rip)
+int hvm_event_software_breakpoint(unsigned long rip, bool_t single_step)
 {
     int rc = 0;
     struct vcpu *curr = current;
+    struct arch_domain *ad = &curr->domain->arch;
+    bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled
+                                   : ad->monitor.software_breakpoint_enabled );
 
-    if ( curr->domain->arch.monitor.software_breakpoint_enabled )
+    if ( enabled )
     {
+        uint64_t gfn;
         struct segment_register sreg;
         uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
         vm_event_request_t req = {
-            .reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
             .vcpu_id = curr->vcpu_id,
         };
 
@@ -170,37 +173,18 @@ int hvm_event_int3(unsigned long rip)
             pfec |= PFEC_user_mode;
 
         hvm_get_segment_register(curr, x86_seg_cs, &sreg);
-        req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
-                                                          sreg.base + rip,
-                                                          &pfec);
-
-        rc = hvm_event_traps(1, &req);
-    }
-
-    return rc;
-}
-
-int hvm_event_single_step(unsigned long rip)
-{
-    int rc = 0;
-    struct vcpu *curr = current;
-
-    if ( curr->domain->arch.monitor.singlestep_enabled )
-    {
-        struct segment_register sreg;
-        uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-        vm_event_request_t req = {
-            .reason = VM_EVENT_REASON_SINGLESTEP,
-            .vcpu_id = curr->vcpu_id,
-        };
-
-        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
-        if ( sreg.attr.fields.dpl == 3 )
-            pfec |= PFEC_user_mode;
-
-        hvm_get_segment_register(curr, x86_seg_cs, &sreg);
-        req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
-                                                 &pfec);
+        gfn = paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
+
+        if ( single_step )
+        {
+            req.reason = VM_EVENT_REASON_SINGLESTEP;
+            req.u.singlestep.gfn = gfn;
+        }
+        else
+        {
+            req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
+            req.u.software_breakpoint.gfn = gfn;
+        }
 
         rc = hvm_event_traps(1, &req);
     }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b9f340c..1a24788 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3192,7 +3192,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                 break;
             }
             else {
-                int handled = hvm_event_int3(regs->eip);
+                int handled = hvm_event_software_breakpoint(regs->eip, 0);
                 
                 if ( handled < 0 ) 
                 {
@@ -3517,10 +3517,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
     case EXIT_REASON_MONITOR_TRAP_FLAG:
         v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MONITOR_TRAP_FLAG;
         vmx_update_cpu_exec_control(v);
-        if ( v->arch.hvm_vcpu.single_step ) {
-          hvm_event_single_step(regs->eip);
-          if ( v->domain->debugger_attached )
-              domain_pause_for_debugger();
+        if ( v->arch.hvm_vcpu.single_step )
+        {
+            hvm_event_software_breakpoint(regs->eip, 1);
+            if ( v->domain->debugger_attached )
+                domain_pause_for_debugger();
         }
 
         break;
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index 11eb1fe..7c2252b 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value,
 #define hvm_event_crX(what, new, old) \
     hvm_event_cr(VM_EVENT_X86_##what, new, old)
 void hvm_event_msr(unsigned int msr, uint64_t value);
-/* Called for current VCPU: returns -1 if no listener */
-int hvm_event_int3(unsigned long rip);
-int hvm_event_single_step(unsigned long rip);
+int hvm_event_software_breakpoint(unsigned long rip,
+                                  bool_t single_step);
 void hvm_event_guest_request(void);
 
 #endif /* __ASM_X86_HVM_EVENT_H__ */
-- 
2.5.0

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

[Andrew Cooper]

On 08/02/16 16:57, Corneliu ZUZU wrote:
> diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
> index 11eb1fe..7c2252b 100644
> --- a/xen/include/asm-x86/hvm/event.h
> +++ b/xen/include/asm-x86/hvm/event.h
> @@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value,
>  #define hvm_event_crX(what, new, old) \
>      hvm_event_cr(VM_EVENT_X86_##what, new, old)
>  void hvm_event_msr(unsigned int msr, uint64_t value);
> -/* Called for current VCPU: returns -1 if no listener */
> -int hvm_event_int3(unsigned long rip);
> -int hvm_event_single_step(unsigned long rip);
> +int hvm_event_software_breakpoint(unsigned long rip,
> +                                  bool_t single_step);

Are we liable to ever gain any other type of software breakpoint?  Might
it be more sense to pass an enum rather than a bool here?

Otherwise, the changes look sensible.

~Andrew

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

[Tamas K Lengyel]

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

On Mon, Feb 8, 2016 at 10:15 AM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 08/02/16 16:57, Corneliu ZUZU wrote:
> > diff --git a/xen/include/asm-x86/hvm/event.h
> b/xen/include/asm-x86/hvm/event.h
> > index 11eb1fe..7c2252b 100644
> > --- a/xen/include/asm-x86/hvm/event.h
> > +++ b/xen/include/asm-x86/hvm/event.h
> > @@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long
> value,
> >  #define hvm_event_crX(what, new, old) \
> >      hvm_event_cr(VM_EVENT_X86_##what, new, old)
> >  void hvm_event_msr(unsigned int msr, uint64_t value);
> > -/* Called for current VCPU: returns -1 if no listener */
> > -int hvm_event_int3(unsigned long rip);
> > -int hvm_event_single_step(unsigned long rip);
> > +int hvm_event_software_breakpoint(unsigned long rip,
> > +                                  bool_t single_step);
>
> Are we liable to ever gain any other type of software breakpoint?  Might
> it be more sense to pass an enum rather than a bool here?
>

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

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1545 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 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/8/2016 7:15 PM, Andrew Cooper wrote:
> On 08/02/16 16:57, Corneliu ZUZU wrote:
>> diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
>> index 11eb1fe..7c2252b 100644
>> --- a/xen/include/asm-x86/hvm/event.h
>> +++ b/xen/include/asm-x86/hvm/event.h
>> @@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value,
>>   #define hvm_event_crX(what, new, old) \
>>       hvm_event_cr(VM_EVENT_X86_##what, new, old)
>>   void hvm_event_msr(unsigned int msr, uint64_t value);
>> -/* Called for current VCPU: returns -1 if no listener */
>> -int hvm_event_int3(unsigned long rip);
>> -int hvm_event_single_step(unsigned long rip);
>> +int hvm_event_software_breakpoint(unsigned long rip,
>> +                                  bool_t single_step);
> Are we liable to ever gain any other type of software breakpoint?  Might
> it be more sense to pass an enum rather than a bool here?
>
> Otherwise, the changes look sensible.
>
> ~Andrew
>

Well, IMHO, no. Besides breakpointing/trapping after each instruction 
(i.e. VM_EVENT_REASON_SINGLESTEP)
and on arbitrary instructions (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I 
don't see what other
types of breakpointing one can define (at least from the hypervisor's 
point of view), and if in the future
there will be other types, that could also be changed into an enum then.
But making that param an enum now would also be fine by me.
Since I noticed Tamas also prefers this option, I will make that change.

Corneliu.

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

[Tamas K Lengyel]

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

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

> On 2/8/2016 7:15 PM, Andrew Cooper wrote:
>
>> On 08/02/16 16:57, Corneliu ZUZU wrote:
>>
>>> diff --git a/xen/include/asm-x86/hvm/event.h
>>> b/xen/include/asm-x86/hvm/event.h
>>> index 11eb1fe..7c2252b 100644
>>> --- a/xen/include/asm-x86/hvm/event.h
>>> +++ b/xen/include/asm-x86/hvm/event.h
>>> @@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long
>>> value,
>>>   #define hvm_event_crX(what, new, old) \
>>>       hvm_event_cr(VM_EVENT_X86_##what, new, old)
>>>   void hvm_event_msr(unsigned int msr, uint64_t value);
>>> -/* Called for current VCPU: returns -1 if no listener */
>>> -int hvm_event_int3(unsigned long rip);
>>> -int hvm_event_single_step(unsigned long rip);
>>> +int hvm_event_software_breakpoint(unsigned long rip,
>>> +                                  bool_t single_step);
>>>
>> Are we liable to ever gain any other type of software breakpoint?  Might
>> it be more sense to pass an enum rather than a bool here?
>>
>> Otherwise, the changes look sensible.
>>
>> ~Andrew
>>
>>
> Well, IMHO, no. Besides breakpointing/trapping after each instruction
> (i.e. VM_EVENT_REASON_SINGLESTEP)
> and on arbitrary instructions (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I
> don't see what other
> types of breakpointing one can define (at least from the hypervisor's
> point of view), and if in the future
> there will be other types, that could also be changed into an enum then.
> But making that param an enum now would also be fine by me.
> Since I noticed Tamas also prefers this option, I will make that change.
>

It's just about code readability. Functionally it would be the same as it
is right now, two options in the enum will likely be represented by 0/1.
But when you read the code you don't have to keep that boolean state in
mind, you explicitly have the path spelled out.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2665 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 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

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

On 2/8/2016 8:17 PM, Tamas K Lengyel wrote:
>
>
> On Mon, Feb 8, 2016 at 10:49 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     On 2/8/2016 7:15 PM, Andrew Cooper wrote:
>
>         On 08/02/16 16:57, Corneliu ZUZU wrote:
>
>             diff --git a/xen/include/asm-x86/hvm/event.h
>             b/xen/include/asm-x86/hvm/event.h
>             index 11eb1fe..7c2252b 100644
>             --- a/xen/include/asm-x86/hvm/event.h
>             +++ b/xen/include/asm-x86/hvm/event.h
>             @@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index,
>             unsigned long value,
>               #define hvm_event_crX(what, new, old) \
>                   hvm_event_cr(VM_EVENT_X86_##what, new, old)
>               void hvm_event_msr(unsigned int msr, uint64_t value);
>             -/* Called for current VCPU: returns -1 if no listener */
>             -int hvm_event_int3(unsigned long rip);
>             -int hvm_event_single_step(unsigned long rip);
>             +int hvm_event_software_breakpoint(unsigned long rip,
>             +                                  bool_t single_step);
>
>         Are we liable to ever gain any other type of software
>         breakpoint?  Might
>         it be more sense to pass an enum rather than a bool here?
>
>         Otherwise, the changes look sensible.
>
>         ~Andrew
>
>
>     Well, IMHO, no. Besides breakpointing/trapping after each
>     instruction (i.e. VM_EVENT_REASON_SINGLESTEP)
>     and on arbitrary instructions
>     (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I don't see what other
>     types of breakpointing one can define (at least from the
>     hypervisor's point of view), and if in the future
>     there will be other types, that could also be changed into an enum
>     then.
>     But making that param an enum now would also be fine by me.
>     Since I noticed Tamas also prefers this option, I will make that
>     change.
>
>
> It's just about code readability. Functionally it would be the same as 
> it is right now, two options in the enum will likely be represented by 
> 0/1. But when you read the code you don't have to keep that boolean 
> state in mind, you explicitly have the path spelled out.
>
> Tamas
>

That makes sense, and probably that enum value will be handled in a switch.
Will do.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 4599 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 2/7] x86: hvm events: merge 2 functions into 1

[Jan Beulich]

>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote:
> This patch merges almost identical functions hvm_event_int3
> and hvm_event_single_step into a single function called
> hvm_event_software_breakpoint.

Except that "software breakpoint" is rather questionable a name
here, considering that on x86 this is basically an alias for "int3".
If it was "breakpoint", one might argue (see the other responses
you've got) that breakpoint event resulting from debug register
settings might then be candidates to come here too.

Jan

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

[Corneliu ZUZU]

On 2/9/2016 1:19 PM, Jan Beulich wrote:
>>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote:
>> This patch merges almost identical functions hvm_event_int3
>> and hvm_event_single_step into a single function called
>> hvm_event_software_breakpoint.
> Except that "software breakpoint" is rather questionable a name
> here, considering that on x86 this is basically an alias for "int3".
> If it was "breakpoint", one might argue (see the other responses
> you've got) that breakpoint event resulting from debug register
> settings might then be candidates to come here too.
>
> Jan

Yeah..should I then:
* keep both functions and only rename hvm_event_int3 to 
hvm_event_software_breakpoint
* separate the code that gets the GFN of the instruction pointer in a 
static inline function
?

Corneliu.

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

[Jan Beulich]

>>> On 09.02.16 at 12:52, <czuzu@bitdefender.com> wrote:
> On 2/9/2016 1:19 PM, Jan Beulich wrote:
>>>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote:
>>> This patch merges almost identical functions hvm_event_int3
>>> and hvm_event_single_step into a single function called
>>> hvm_event_software_breakpoint.
>> Except that "software breakpoint" is rather questionable a name
>> here, considering that on x86 this is basically an alias for "int3".
>> If it was "breakpoint", one might argue (see the other responses
>> you've got) that breakpoint event resulting from debug register
>> settings might then be candidates to come here too.
> 
> Yeah..should I then:
> * keep both functions and only rename hvm_event_int3 to 
> hvm_event_software_breakpoint

I actually think that the intention of folding two almost identical
functions is a good one. I'm merely suggesting to think of a
better name - perhaps just "breakpoint" or "debug event"?

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

[Corneliu ZUZU]

On 2/9/2016 2:12 PM, Jan Beulich wrote:
>>>> On 09.02.16 at 12:52, <czuzu@bitdefender.com> wrote:
>> On 2/9/2016 1:19 PM, Jan Beulich wrote:
>>>>>> On 08.02.16 at 17:57, <czuzu@bitdefender.com> wrote:
>>>> This patch merges almost identical functions hvm_event_int3
>>>> and hvm_event_single_step into a single function called
>>>> hvm_event_software_breakpoint.
>>> Except that "software breakpoint" is rather questionable a name
>>> here, considering that on x86 this is basically an alias for "int3".
>>> If it was "breakpoint", one might argue (see the other responses
>>> you've got) that breakpoint event resulting from debug register
>>> settings might then be candidates to come here too.
>> Yeah..should I then:
>> * keep both functions and only rename hvm_event_int3 to
>> hvm_event_software_breakpoint
> I actually think that the intention of folding two almost identical
> functions is a good one. I'm merely suggesting to think of a
> better name - perhaps just "breakpoint" or "debug event"?
>
>

SGTM. I'll change it to hvm_event_breakpoint then.

Corneliu.

[PATCH 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.c             | 228 -------------------------------------
 xen/arch/x86/monitor_x86.c         |  72 ++++++++++++
 xen/common/Kconfig                 |  20 ++++
 xen/common/Makefile                |   1 +
 xen/common/domctl.c                |   2 +-
 xen/common/monitor.c               | 203 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/monitor.h      |  33 ------
 xen/include/asm-arm/monitor_arch.h |  53 +++++++++
 xen/include/asm-x86/monitor.h      |  31 -----
 xen/include/asm-x86/monitor_arch.h |  74 ++++++++++++
 xen/include/xen/monitor.h          |  36 ++++++
 16 files changed, 468 insertions(+), 297 deletions(-)
 delete mode 100644 xen/arch/x86/monitor.c
 create mode 100644 xen/arch/x86/monitor_x86.c
 create mode 100644 xen/common/monitor.c
 delete mode 100644 xen/include/asm-arm/monitor.h
 create mode 100644 xen/include/asm-arm/monitor_arch.h
 delete mode 100644 xen/include/asm-x86/monitor.h
 create mode 100644 xen/include/asm-x86/monitor_arch.h
 create mode 100644 xen/include/xen/monitor.h

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 9dc533b..5ffc485 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -20,8 +20,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 35ec6c9..9063eb5 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 1a24788..f7708fe 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.c b/xen/arch/x86/monitor.c
deleted file mode 100644
index 1d43880..0000000
--- a/xen/arch/x86/monitor.c
+++ /dev/null
@@ -1,228 +0,0 @@
-/*
- * arch/x86/monitor.c
- *
- * Architecture-specific monitor_op domctl handler.
- *
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
- *
- * 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/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>
-
-/*
- * 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 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;
-
-    switch ( mop->event )
-    {
-    case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
-    {
-        unsigned int ctrlreg_bitmask =
-            monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
-        bool_t status =
-            !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
-        struct vcpu *v;
-
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
-
-        domain_pause(d);
-
-        if ( mop->u.mov_to_cr.sync )
-            ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask;
-        else
-            ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask;
-
-        if ( mop->u.mov_to_cr.onchangeonly )
-            ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
-        else
-            ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
-
-        if ( !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 */
-            for_each_vcpu ( d, v )
-                hvm_update_guest_cr(v, 0);
-
-        domain_unpause(d);
-
-        break;
-    }
-
-    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;
-    }
-
-    case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
-    {
-        bool_t status = ad->monitor.singlestep_enabled;
-
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
-
-        domain_pause(d);
-        ad->monitor.singlestep_enabled = !status;
-        domain_unpause(d);
-        break;
-    }
-
-    case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
-    {
-        bool_t status = ad->monitor.software_breakpoint_enabled;
-
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
-
-        domain_pause(d);
-        ad->monitor.software_breakpoint_enabled = !status;
-        domain_unpause(d);
-        break;
-    }
-
-    case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
-    {
-        bool_t status = ad->monitor.guest_request_enabled;
-
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
-
-        domain_pause(d);
-        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
-        ad->monitor.guest_request_enabled = !status;
-        domain_unpause(d);
-        break;
-    }
-
-    default:
-        return -EOPNOTSUPP;
-
-    };
-
-    return 0;
-}
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
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/common/monitor.c b/xen/common/monitor.c
new file mode 100644
index 0000000..7bbeba5
--- /dev/null
+++ b/xen/common/monitor.c
@@ -0,0 +1,203 @@
+/*
+ * 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>              /* for domain_pause, ... */
+#include <xen/config.h>             /* for XENLOG_WARNING */
+#include <xsm/xsm.h>
+#include <public/domctl.h>
+
+#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;
+    bool_t requested_status;
+
+    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;
+
+    if ( unlikely(mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
+                  mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE) )
+    {
+        if ( XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES == mop->op )
+        {
+            mop->event = arch_monitor_get_capabilities(d);
+            return 0;
+        }
+
+        /* The monitor op is proly handled on the arch-side. */
+        if ( likely(arch_monitor_domctl_op(d, mop, &rc)) )
+            return rc;
+
+        /* unrecognized op */
+        return -EOPNOTSUPP;
+    }
+
+    /* Check if event type is available. */
+    if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) )
+        return -EOPNOTSUPP;
+
+    requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+    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 old_status =
+            !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+
+        if ( mop->u.mov_to_cr.sync )
+            ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask;
+        else
+            ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask;
+
+        if ( mop->u.mov_to_cr.onchangeonly )
+            ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
+        else
+            ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
+
+        if ( !old_status )
+            ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
+        else
+            ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
+
+#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
+
+#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;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        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:
+    {
+        struct arch_domain *ad = &d->arch;
+        bool_t old_status = ad->monitor.software_breakpoint_enabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        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:
+    {
+        struct arch_domain *ad = &d->arch;
+        bool_t old_status = ad->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;
+        domain_unpause(d);
+        break;
+    }
+
+#endif // HAS_VM_EVENT_GUEST_REQUEST
+
+    default:
+        /* 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;
+}
+
+/*
+ * 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
deleted file mode 100644
index a3a9703..0000000
--- a/xen/include/asm-arm/monitor.h
+++ /dev/null
@@ -1,33 +0,0 @@
-/*
- * include/asm-arm/monitor.h
- *
- * Architecture-specific monitor_op domctl handler.
- *
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
- *
- * 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_ARM_MONITOR_H__
-#define __ASM_ARM_MONITOR_H__
-
-#include <xen/sched.h>
-#include <public/domctl.h>
-
-static inline
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op)
-{
-    return -ENOSYS;
-}
-
-#endif /* __ASM_X86_MONITOR_H__ */
diff --git a/xen/include/asm-arm/monitor_arch.h b/xen/include/asm-arm/monitor_arch.h
new file mode 100644
index 0000000..d0df66c
--- /dev/null
+++ b/xen/include/asm-arm/monitor_arch.h
@@ -0,0 +1,53 @@
+/*
+ * include/asm-arm/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_ARM_MONITOR_ARCH_H__
+#define __ASM_ARM_MONITOR_ARCH_H__
+
+#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
+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.h b/xen/include/asm-x86/monitor.h
deleted file mode 100644
index 7c8280b..0000000
--- a/xen/include/asm-x86/monitor.h
+++ /dev/null
@@ -1,31 +0,0 @@
-/*
- * include/asm-x86/monitor.h
- *
- * Architecture-specific monitor_op domctl handler.
- *
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
- *
- * 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_H__
-#define __ASM_X86_MONITOR_H__
-
-struct domain;
-struct xen_domctl_monitor_op;
-
-#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
-
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
-
-#endif /* __ASM_X86_MONITOR_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/xen/monitor.h b/xen/include/xen/monitor.h
new file mode 100644
index 0000000..8ccf13c
--- /dev/null
+++ b/xen/include/xen/monitor.h
@@ -0,0 +1,36 @@
+/*
+ * 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>
+
+#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 /* __MONITOR_H__ */
-- 
2.5.0

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

[Tamas K Lengyel]

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

On Mon, Feb 8, 2016 at 9:57 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
>
> 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.c             | 228
> -------------------------------------
>  xen/arch/x86/monitor_x86.c         |  72 ++++++++++++
>  xen/common/Kconfig                 |  20 ++++
>  xen/common/Makefile                |   1 +
>  xen/common/domctl.c                |   2 +-
>  xen/common/monitor.c               | 203 +++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/monitor.h      |  33 ------
>  xen/include/asm-arm/monitor_arch.h |  53 +++++++++
>  xen/include/asm-x86/monitor.h      |  31 -----
>  xen/include/asm-x86/monitor_arch.h |  74 ++++++++++++
>  xen/include/xen/monitor.h          |  36 ++++++
>  16 files changed, 468 insertions(+), 297 deletions(-)
>  delete mode 100644 xen/arch/x86/monitor.c
>  create mode 100644 xen/arch/x86/monitor_x86.c
>  create mode 100644 xen/common/monitor.c
>  delete mode 100644 xen/include/asm-arm/monitor.h
>  create mode 100644 xen/include/asm-arm/monitor_arch.h
>  delete mode 100644 xen/include/asm-x86/monitor.h
>  create mode 100644 xen/include/asm-x86/monitor_arch.h
>  create mode 100644 xen/include/xen/monitor.h
>
> 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 9dc533b..5ffc485 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -20,8 +20,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 35ec6c9..9063eb5 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 1a24788..f7708fe 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.c b/xen/arch/x86/monitor.c
> deleted file mode 100644
> index 1d43880..0000000
> --- a/xen/arch/x86/monitor.c
> +++ /dev/null
> @@ -1,228 +0,0 @@
> -/*
> - * arch/x86/monitor.c
> - *
> - * Architecture-specific monitor_op domctl handler.
> - *
> - * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> - *
> - * 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/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>
> -
> -/*
> - * 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 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;
> -
> -    switch ( mop->event )
> -    {
> -    case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
> -    {
> -        unsigned int ctrlreg_bitmask =
> -            monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
> -        bool_t status =
> -            !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
> -        struct vcpu *v;
> -
> -        rc = status_check(mop, status);
> -        if ( rc )
> -            return rc;
> -
> -        domain_pause(d);
> -
> -        if ( mop->u.mov_to_cr.sync )
> -            ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask;
> -        else
> -            ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask;
> -
> -        if ( mop->u.mov_to_cr.onchangeonly )
> -            ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
> -        else
> -            ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
> -
> -        if ( !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 */
> -            for_each_vcpu ( d, v )
> -                hvm_update_guest_cr(v, 0);
> -
> -        domain_unpause(d);
> -
> -        break;
> -    }
> -
> -    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;
> -    }
> -
> -    case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
> -    {
> -        bool_t status = ad->monitor.singlestep_enabled;
> -
> -        rc = status_check(mop, status);
> -        if ( rc )
> -            return rc;
> -
> -        domain_pause(d);
> -        ad->monitor.singlestep_enabled = !status;
> -        domain_unpause(d);
> -        break;
> -    }
> -
> -    case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
> -    {
> -        bool_t status = ad->monitor.software_breakpoint_enabled;
> -
> -        rc = status_check(mop, status);
> -        if ( rc )
> -            return rc;
> -
> -        domain_pause(d);
> -        ad->monitor.software_breakpoint_enabled = !status;
> -        domain_unpause(d);
> -        break;
> -    }
> -
> -    case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
> -    {
> -        bool_t status = ad->monitor.guest_request_enabled;
> -
> -        rc = status_check(mop, status);
> -        if ( rc )
> -            return rc;
> -
> -        domain_pause(d);
> -        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
> -        ad->monitor.guest_request_enabled = !status;
> -        domain_unpause(d);
> -        break;
> -    }
> -
> -    default:
> -        return -EOPNOTSUPP;
> -
> -    };
> -
> -    return 0;
> -}
> -
> -/*
> - * Local variables:
> - * mode: C
> - * c-file-style: "BSD"
> - * c-basic-offset: 4
> - * indent-tabs-mode: nil
> - * End:
> - */
> 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/common/monitor.c b/xen/common/monitor.c
> new file mode 100644
> index 0000000..7bbeba5
> --- /dev/null
> +++ b/xen/common/monitor.c
> @@ -0,0 +1,203 @@
> +/*
> + * 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>              /* for domain_pause, ... */
> +#include <xen/config.h>             /* for XENLOG_WARNING */
> +#include <xsm/xsm.h>
> +#include <public/domctl.h>
> +
> +#include <asm/monitor_arch.h>       /* for monitor_arch_# */
> +
> +#if CONFIG_X86
>

Wouldn't it be safe to include these headers on ARM as well?


> +#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;
> +    bool_t requested_status;
> +
> +    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;
> +
> +    if ( unlikely(mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
> +                  mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE) )
> +    {
>

Please make a switch on mop->op instead where the default case is to call
arch_monitor_domctl. It would be a bit easier to follow.

+        if ( XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES == mop->op )
> +        {
> +            mop->event = arch_monitor_get_capabilities(d);
> +            return 0;
> +        }
> +
>

"proly"->probably?


> +        /* The monitor op is proly handled on the arch-side. */
> +        if ( likely(arch_monitor_domctl_op(d, mop, &rc)) )
> +            return rc;
> +
> +        /* unrecognized op */
> +        return -EOPNOTSUPP;
> +    }
> +
> +    /* Check if event type is available. */
> +    if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 <<
> mop->event))) )
> +        return -EOPNOTSUPP;
> +
> +    requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
> +
> +    switch ( mop->event )
> +    {
>

Empty line not needed.

> +
> +#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG
>

Emtpy line not needed.

> +
> +    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 old_status =
> +            !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
> +
> +        if ( unlikely(old_status == requested_status) )
> +            return -EEXIST;
> +
> +        domain_pause(d);
> +
> +        if ( mop->u.mov_to_cr.sync )
> +            ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask;
> +        else
> +            ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask;
> +
> +        if ( mop->u.mov_to_cr.onchangeonly )
> +            ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
> +        else
> +            ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
> +
> +        if ( !old_status )
> +            ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
> +        else
> +            ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
> +
>

So I don't particularly like this #if check here. IMHO this should be done
in arch-specific function that you call from here that is just empty for
ARM. It could be a static inline function as it's rather small.


> +#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
> +
> +#if CONFIG_HAS_VM_EVENT_SINGLESTEP
>

Emtpy line not needed.

> +
> +    case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
> +    {
> +        struct arch_domain *ad = &d->arch;
> +        bool_t old_status = ad->monitor.singlestep_enabled;
> +
> +        if ( unlikely(old_status == requested_status) )
> +            return -EEXIST;
> +
> +        domain_pause(d);
> +        ad->monitor.singlestep_enabled = !old_status;
> +        domain_unpause(d);
> +        break;
> +    }
> +
> +#endif // HAS_VM_EVENT_SINGLESTEP
> +
> +#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
>

Emtpy line not needed.

> +
> +    case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
> +    {
> +        struct arch_domain *ad = &d->arch;
> +        bool_t old_status = ad->monitor.software_breakpoint_enabled;
> +
> +        if ( unlikely(old_status == requested_status) )
> +            return -EEXIST;
> +
> +        domain_pause(d);
> +        ad->monitor.software_breakpoint_enabled = !old_status;
> +        domain_unpause(d);
> +        break;
> +    }
> +
> +#endif // HAS_VM_EVENT_SOFTWARE_BREAKPOINT
> +
> +#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
>

Emtpy line not needed.

> +
> +    case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
> +    {
> +        struct arch_domain *ad = &d->arch;
> +        bool_t old_status = ad->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;
> +        domain_unpause(d);
> +        break;
> +    }
>
>
Looks good otherwise.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 30100 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 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

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

On 2/8/2016 8:15 PM, Tamas K Lengyel wrote:
> On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     +#if CONFIG_X86
>
>
> Wouldn't it be safe to include these headers on ARM as well?
>
>     +#include <public/vm_event.h>        /* for VM_EVENT_X86_CR3 */
>     +#include <asm/hvm/hvm.h>            /* for hvm_update_guest_cr,
>     ... */
>     +#endif
>

It wouldn't break anything, but they are only necessary on X86, so why 
include them?

>     +
>     +    if ( unlikely(mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
>     +                  mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE) )
>     +    {
>
>
> Please make a switch on mop->op instead where the default case is to 
> call arch_monitor_domctl. It would be a bit easier to follow.
>
>     +        if ( XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES == mop->op )
>     +        {
>     +            mop->event = arch_monitor_get_capabilities(d);
>     +            return 0;
>     +        }
>     +
>

Noted, good point.

>
> "proly"->probably?
>
>     +        /* The monitor op is proly handled on the arch-side. */
>     +        if ( likely(arch_monitor_domctl_op(d, mop, &rc)) )
>     +            return rc;
>

Yep, will "proly" change that, heh, just kidding. Noted :D.

> Empty line not needed.  (*)

Noted.

>
> So I don't particularly like this #if check here. IMHO this should be 
> done in arch-specific function that you call from here that is just 
> empty for ARM. It could be a static inline function as it's rather small.
>
>     +#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
>

I agree, but I was actually hoping to approach that in a future 
ARM-patch I'm going to send after this one.
On ARM, that part of code (where hypervisor CR trapping is enabled based 
on write_ctrlreg_enabled bits)
is done in another place (on the scheduling tail). I wanted to approach 
removing that #ifdef and finding maybe
a solution to do that similarly for X86. Would it be ok to leave this 
for that discussion? It would be simpler to illustrate
what I have in mind.

>
> Looks good otherwise.
>
> Thanks,
> Tamas

Thanks,
Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 7193 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 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Tamas K Lengyel]

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

>
>
> So I don't particularly like this #if check here. IMHO this should be done
> in arch-specific function that you call from here that is just empty for
> ARM. It could be a static inline function as it's rather small.
>
>
>> +#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
>>
>
> I agree, but I was actually hoping to approach that in a future ARM-patch
> I'm going to send after this one.
> On ARM, that part of code (where hypervisor CR trapping is enabled based
> on write_ctrlreg_enabled bits)
> is done in another place (on the scheduling tail). I wanted to approach
> removing that #ifdef and finding maybe
> a solution to do that similarly for X86. Would it be ok to leave this for
> that discussion? It would be simpler to illustrate
> what I have in mind.
>

SGTM.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 2251 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 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

(last commit before this one explains why this was necessary)

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/Makefile              |  2 +-
 xen/arch/x86/monitor.c             | 72 +++++++++++++++++++++++++++++++++++++
 xen/arch/x86/monitor_x86.c         | 72 -------------------------------------
 xen/common/monitor.c               |  2 +-
 xen/include/asm-arm/monitor.h      | 53 +++++++++++++++++++++++++++
 xen/include/asm-arm/monitor_arch.h | 53 ---------------------------
 xen/include/asm-x86/monitor.h      | 74 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/monitor_arch.h | 74 --------------------------------------
 8 files changed, 201 insertions(+), 201 deletions(-)
 create mode 100644 xen/arch/x86/monitor.c
 delete mode 100644 xen/arch/x86/monitor_x86.c
 create mode 100644 xen/include/asm-arm/monitor.h
 delete mode 100644 xen/include/asm-arm/monitor_arch.h
 create mode 100644 xen/include/asm-x86/monitor.h
 delete mode 100644 xen/include/asm-x86/monitor_arch.h

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.c b/xen/arch/x86/monitor.c
new file mode 100644
index 0000000..568def2
--- /dev/null
+++ b/xen/arch/x86/monitor.c
@@ -0,0 +1,72 @@
+/*
+ * arch/x86/monitor.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.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/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c
deleted file mode 100644
index d19fd15..0000000
--- a/xen/arch/x86/monitor_x86.c
+++ /dev/null
@@ -1,72 +0,0 @@
-/*
- * 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/monitor.c b/xen/common/monitor.c
index 7bbeba5..1165aeb 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 */
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
new file mode 100644
index 0000000..eb770da
--- /dev/null
+++ b/xen/include/asm-arm/monitor.h
@@ -0,0 +1,53 @@
+/*
+ * include/asm-arm/monitor.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_ARM_MONITOR_H__
+#define __ASM_ARM_MONITOR_H__
+
+#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
+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_H__ */
diff --git a/xen/include/asm-arm/monitor_arch.h b/xen/include/asm-arm/monitor_arch.h
deleted file mode 100644
index d0df66c..0000000
--- a/xen/include/asm-arm/monitor_arch.h
+++ /dev/null
@@ -1,53 +0,0 @@
-/*
- * include/asm-arm/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_ARM_MONITOR_ARCH_H__
-#define __ASM_ARM_MONITOR_ARCH_H__
-
-#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
-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.h b/xen/include/asm-x86/monitor.h
new file mode 100644
index 0000000..b12823c
--- /dev/null
+++ b/xen/include/asm-x86/monitor.h
@@ -0,0 +1,74 @@
+/*
+ * include/asm-x86/monitor.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_H__
+#define __ASM_X86_MONITOR_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_H__ */
diff --git a/xen/include/asm-x86/monitor_arch.h b/xen/include/asm-x86/monitor_arch.h
deleted file mode 100644
index d9daf65..0000000
--- a/xen/include/asm-x86/monitor_arch.h
+++ /dev/null
@@ -1,74 +0,0 @@
-/*
- * 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__ */
-- 
2.5.0

Re: [PATCH 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: 299 bytes --]

On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:

> (last commit before this one explains why this was necessary)
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>

I assume this patch will be gone in the next iteration after using -M so
skipping it now.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 759 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 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: 998 bytes --]

On 2/8/2016 8:18 PM, Tamas K Lengyel wrote:
>
>
> On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     (last commit before this one explains why this was necessary)
>
>     Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
>     <mailto:czuzu@bitdefender.com>>
>
>
> I assume this patch will be gone in the next iteration after using -M 
> so skipping it now.
>
> Tamas
>

No, originally I intended to use the -M option, I just forgot.
This is needed even w/ the -M option.
The reason is git seeing file <somepath1>/a.c  as being modified
when moving it to <somepath2>/a.c and at the same time adding
<somepath1>/a.c in place of the old one, *even if* the similarity
between the old <somepath1>/a.c and <somepath2>/a.c would be *100%*
and you'd use the -M option. A google-search led me to a 2009-dated page
that described this as a lack of git diff's algo that would be improved 
upon.
It seems they aren't there yet.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 2279 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 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_software_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/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_software_breakpoint to arch-side
			(see arch_hvm_event_gfn_of_ip) and renamed rip param to ip - i.e.
			now the parameter is a 'generic' instruction pointer, rather than
			the Intel 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,
						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.c             | 203 -----------------------------------
 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/hvm/Makefile              |   1 +
 xen/common/hvm/event.c               | 172 +++++++++++++++++++++++++++++
 xen/include/asm-arm/hvm/event_arch.h |  40 +++++++
 xen/include/asm-x86/hvm/event.h      |  44 --------
 xen/include/asm-x86/hvm/event_arch.h |  93 ++++++++++++++++
 xen/include/xen/hvm/event.h          |  71 ++++++++++++
 11 files changed, 432 insertions(+), 250 deletions(-)
 delete mode 100644 xen/arch/x86/hvm/event.c
 create mode 100644 xen/arch/x86/hvm/event_x86.c
 create mode 100644 xen/common/hvm/event.c
 create mode 100644 xen/include/asm-arm/hvm/event_arch.h
 delete mode 100644 xen/include/asm-x86/hvm/event.h
 create mode 100644 xen/include/asm-x86/hvm/event_arch.h
 create mode 100644 xen/include/xen/hvm/event.h

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.c b/xen/arch/x86/hvm/event.c
deleted file mode 100644
index 5ffc485..0000000
--- a/xen/arch/x86/hvm/event.c
+++ /dev/null
@@ -1,203 +0,0 @@
-/*
-* 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/>.
-*/
-
-#include <xen/vm_event.h>
-#include <xen/paging.h>
-#include <xen/monitor.h>
-#include <asm/hvm/event.h>
-#include <asm/altp2m.h>
-#include <public/vm_event.h>
-
-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 rc;
-    struct vcpu *curr = current;
-    struct domain *currd = curr->domain;
-
-    rc = vm_event_claim_slot(currd, &currd->vm_event->monitor);
-    switch ( rc )
-    {
-    case 0:
-        break;
-    case -ENOSYS:
-        /*
-         * If there was no ring to handle the event, then
-         * simply continue executing normally.
-         */
-        return 1;
-    default:
-        return rc;
-    };
-
-    if ( sync )
-    {
-        req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
-        vm_event_vcpu_pause(curr);
-    }
-
-    if ( altp2m_active(currd) )
-    {
-        req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
-        req->altp2m_idx = vcpu_altp2m(curr).p2midx;
-    }
-
-    hvm_event_fill_regs(req);
-    vm_event_put_request(currd, &currd->vm_event->monitor, req);
-
-    return 1;
-}
-
-bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
-{
-    struct arch_domain *currad = &current->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) ||
-          value != old) )
-    {
-        vm_event_request_t req = {
-            .reason = VM_EVENT_REASON_WRITE_CTRLREG,
-            .vcpu_id = current->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);
-        return 1;
-    }
-
-    return 0;
-}
-
-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);
-}
-
-void hvm_event_guest_request(void)
-{
-    struct vcpu *curr = current;
-    struct arch_domain *currad = &curr->domain->arch;
-
-    if ( currad->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);
-    }
-}
-
-int hvm_event_software_breakpoint(unsigned long rip, bool_t single_step)
-{
-    int rc = 0;
-    struct vcpu *curr = current;
-    struct arch_domain *ad = &curr->domain->arch;
-    bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled
-                                   : ad->monitor.software_breakpoint_enabled );
-
-    if ( enabled )
-    {
-        uint64_t gfn;
-        struct segment_register sreg;
-        uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-        vm_event_request_t req = {
-            .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_cs, &sreg);
-        gfn = paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
-
-        if ( single_step )
-        {
-            req.reason = VM_EVENT_REASON_SINGLESTEP;
-            req.u.singlestep.gfn = gfn;
-        }
-        else
-        {
-            req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
-            req.u.software_breakpoint.gfn = gfn;
-        }
-
-        rc = hvm_event_traps(1, &req);
-    }
-
-    return rc;
-}
-
-/*
- * 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/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 9063eb5..d90ca10 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 f7708fe..23e7bc5 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/hvm/Makefile b/xen/common/hvm/Makefile
index a464a57..66ae866 100644
--- a/xen/common/hvm/Makefile
+++ b/xen/common/hvm/Makefile
@@ -1 +1,2 @@
+obj-y += event.o
 obj-y += save.o
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
new file mode 100644
index 0000000..149ea79
--- /dev/null
+++ b/xen/common/hvm/event.c
@@ -0,0 +1,172 @@
+/*
+* 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 */
+#if CONFIG_X86
+#include <asm/altp2m.h>
+#endif
+
+int hvm_event_traps(struct vcpu *v,
+                    uint8_t sync,
+                    vm_event_request_t *req)
+{
+    int rc;
+    struct domain *d = v->domain;
+
+    rc = vm_event_claim_slot(d, &d->vm_event->monitor);
+    switch ( rc )
+    {
+    case 0:
+        break;
+    case -ENOSYS:
+        /*
+         * If there was no ring to handle the event, then
+         * simply continue executing normally.
+         */
+        return 1;
+    default:
+        return rc;
+    };
+
+    if ( sync )
+    {
+        req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+        vm_event_vcpu_pause(v);
+    }
+
+#if CONFIG_X86
+    if ( altp2m_active(d) )
+    {
+        req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
+        req->altp2m_idx = vcpu_altp2m(v).p2midx;
+    }
+#endif
+
+    arch_hvm_event_fill_regs(req);
+
+    vm_event_put_request(d, &d->vm_event->monitor, req);
+
+    return 1;
+}
+
+#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG
+
+bool_t hvm_event_cr(unsigned int index,
+                    unsigned long value,
+                    unsigned long old)
+{
+    struct vcpu *curr = current;
+    struct arch_domain *ad = &curr->domain->arch;
+    unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
+
+    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 = curr->vcpu_id,
+            .u.write_ctrlreg.index = index,
+            .u.write_ctrlreg.new_value = value,
+            .u.write_ctrlreg.old_value = old
+        };
+
+        hvm_event_traps(curr, sync, &req);
+        return 1;
+    }
+
+    return 0;
+}
+
+#endif // HAS_VM_EVENT_WRITE_CTRLREG
+
+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
+
+void hvm_event_guest_request(void)
+{
+    struct vcpu *curr = current;
+    struct arch_domain *ad = &curr->domain->arch;
+
+    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(curr, ad->monitor.guest_request_sync, &req);
+    }
+}
+
+#endif // HAS_VM_EVENT_GUEST_REQUEST
+
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP || CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+
+int hvm_event_software_breakpoint(unsigned long ip, bool_t single_step)
+{
+    int rc = 0;
+    struct vcpu *curr = current;
+    struct arch_domain *ad = &curr->domain->arch;
+    bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled
+                                   : ad->monitor.software_breakpoint_enabled );
+
+    if ( enabled )
+    {
+        uint64_t gfn = arch_hvm_event_gfn_of_ip(ip);
+        vm_event_request_t req = {
+            .vcpu_id = curr->vcpu_id,
+        };
+
+        if ( single_step )
+        {
+            req.reason = VM_EVENT_REASON_SINGLESTEP;
+            req.u.singlestep.gfn = gfn;
+        }
+        else
+        {
+            req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
+            req.u.software_breakpoint.gfn = gfn;
+        }
+
+        rc = hvm_event_traps(curr, 1, &req);
+    }
+
+    return rc;
+}
+
+#endif // HAS_VM_EVENT_SINGLESTEP || HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+
+/*
+ * 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-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.h b/xen/include/asm-x86/hvm/event.h
deleted file mode 100644
index 7c2252b..0000000
--- a/xen/include/asm-x86/hvm/event.h
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * event.h: Hardware virtual machine assist events.
- *
- * 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_H__
-#define __ASM_X86_HVM_EVENT_H__
-
-/*
- * 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,
-                    unsigned long old);
-#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_software_breakpoint(unsigned long rip,
-                                  bool_t single_step);
-void hvm_event_guest_request(void);
-
-#endif /* __ASM_X86_HVM_EVENT_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/xen/hvm/event.h b/xen/include/xen/hvm/event.h
new file mode 100644
index 0000000..85e63d6
--- /dev/null
+++ b/xen/include/xen/hvm/event.h
@@ -0,0 +1,71 @@
+/*
+ * 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,
+ * 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 __HVM_EVENT_H__
+#define __HVM_EVENT_H__
+
+#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,
+                    unsigned long old);
+
+#if CONFIG_X86
+#define hvm_event_crX(what, new, old) \
+    hvm_event_cr(VM_EVENT_X86_##what, new, old)
+#endif
+
+#endif // HAS_VM_EVENT_WRITE_CTRLREG
+
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP || CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+
+int hvm_event_software_breakpoint(unsigned long ip,
+                                  bool_t single_step);
+#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 /* __HVM_EVENT_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.5.0

[PATCH 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 to arch/x86/hvm/event.c and
	asm-{x86,arm}/hvm/event_arch.h to asm-{x86/arm}/hvm/event.h (last commit
	before this one explains why this was necessary)

Minor fixes:
* ARM fix: xen/common/hvm/event.c was not actually compiled
	(see file-changes: xen/common/Makefile, xen/common/hvm/Makefile)
* 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.c             | 51 ++++++++++++++++++++
 xen/arch/x86/hvm/event_x86.c         | 51 --------------------
 xen/arch/x86/hvm/hvm.c               |  2 +-
 xen/common/Makefile                  |  2 +-
 xen/common/hvm/Makefile              |  2 +-
 xen/common/hvm/event.c               | 44 ++++++++---------
 xen/include/asm-arm/hvm/event.h      | 40 ++++++++++++++++
 xen/include/asm-arm/hvm/event_arch.h | 40 ----------------
 xen/include/asm-x86/hvm/event.h      | 93 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/event_arch.h | 93 ------------------------------------
 xen/include/xen/hvm/event.h          |  6 +--
 12 files changed, 213 insertions(+), 213 deletions(-)
 create mode 100644 xen/arch/x86/hvm/event.c
 delete mode 100644 xen/arch/x86/hvm/event_x86.c
 create mode 100644 xen/include/asm-arm/hvm/event.h
 delete mode 100644 xen/include/asm-arm/hvm/event_arch.h
 create mode 100644 xen/include/asm-x86/hvm/event.h
 delete mode 100644 xen/include/asm-x86/hvm/event_arch.h

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.c b/xen/arch/x86/hvm/event.c
new file mode 100644
index 0000000..3349c34
--- /dev/null
+++ b/xen/arch/x86/hvm/event.c
@@ -0,0 +1,51 @@
+/*
+ * 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,
+ * 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/event_x86.c b/xen/arch/x86/hvm/event_x86.c
deleted file mode 100644
index 7b54f18..0000000
--- a/xen/arch/x86/hvm/event_x86.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- * 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 d90ca10..6a1ec3b 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/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 66ae866..12d13b2 100644
--- a/xen/common/hvm/Makefile
+++ b/xen/common/hvm/Makefile
@@ -1,2 +1,2 @@
 obj-y += event.o
-obj-y += save.o
+obj-$(CONFIG_X86) += save.o
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
index 149ea79..d4ce97a 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.h b/xen/include/asm-arm/hvm/event.h
new file mode 100644
index 0000000..4aa797b
--- /dev/null
+++ b/xen/include/asm-arm/hvm/event.h
@@ -0,0 +1,40 @@
+/*
+ * include/asm-arm/hvm/event.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_H__
+#define __ASM_ARM_HVM_EVENT_H__
+
+static inline
+void arch_hvm_event_fill_regs(vm_event_request_t *req)
+{
+    /* Not supported on ARM. */
+}
+
+#endif /* __ASM_ARM_HVM_EVENT_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-arm/hvm/event_arch.h b/xen/include/asm-arm/hvm/event_arch.h
deleted file mode 100644
index beebca2..0000000
--- a/xen/include/asm-arm/hvm/event_arch.h
+++ /dev/null
@@ -1,40 +0,0 @@
-/*
- * 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.h b/xen/include/asm-x86/hvm/event.h
new file mode 100644
index 0000000..88a81f0
--- /dev/null
+++ b/xen/include/asm-x86/hvm/event.h
@@ -0,0 +1,93 @@
+/*
+ * include/asm-x86/hvm/event.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_H__
+#define __ASM_X86_HVM_EVENT_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_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
deleted file mode 100644
index b9fb559..0000000
--- a/xen/include/asm-x86/hvm/event_arch.h
+++ /dev/null
@@ -1,93 +0,0 @@
-/*
- * 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/xen/hvm/event.h b/xen/include/xen/hvm/event.h
index 85e63d6..0548945 100644
--- a/xen/include/xen/hvm/event.h
+++ b/xen/include/xen/hvm/event.h
@@ -27,7 +27,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.
  */
@@ -36,8 +36,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 7/7] arch.monitor: move bits to common (arch_domain to domain)

[Corneliu ZUZU]

This patch moves bitfield members for single-step, software-breakpoint and
guest-request monitor vm-events from the arch-side (struct arch_domain) to
the common-side (struct domain). 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 d4ce97a..2660093 100644
--- a/xen/common/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -110,16 +110,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);
     }
 }
 
@@ -131,9 +131,9 @@ int hvm_event_software_breakpoint(unsigned long ip, bool_t single_step)
 {
     int rc = 0;
     struct vcpu *curr = current;
-    struct arch_domain *ad = &curr->domain->arch;
-    bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled
-                                   : ad->monitor.software_breakpoint_enabled );
+    struct domain *d = curr->domain;
+    bool_t enabled = ( single_step ? d->monitor.singlestep_enabled
+                                   : d->monitor.software_breakpoint_enabled );
 
     if ( enabled )
     {
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 1165aeb..b745aa4 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -121,14 +121,13 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     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;
     }
@@ -139,14 +138,13 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     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;
     }
@@ -157,15 +155,14 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     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 7/7] arch.monitor: move bits to common (arch_domain to domain)

[Tamas K Lengyel]

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

On Mon, Feb 8, 2016 at 9:58 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:

> This patch moves bitfield members for single-step, software-breakpoint and
> guest-request monitor vm-events from the arch-side (struct arch_domain) to
> the common-side (struct domain). 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>
>

Technically this looks fine, but I do wonder if and what plans you have to
actually implement these events for ARM. I haven't spent too much time
looking into it, but I'm not aware of equivalent features on ARM to Intel
MTF (singlestepping) or to software-breakpoint trapping. The only
instruction I know that functionally comes close to software-breakpoint
trapping (INT3) is the SMC instruction which can be trapped into the VMM,
but I would not call that a "breakpoint" in the traditional sense.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1463 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 7/7] arch.monitor: move bits to common (arch_domain to domain)

[Corneliu ZUZU]

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

On 2/8/2016 8:29 PM, Tamas K Lengyel wrote:
>
>
> On Mon, Feb 8, 2016 at 9:58 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     This patch moves bitfield members for single-step,
>     software-breakpoint and
>     guest-request monitor vm-events from the arch-side (struct
>     arch_domain) to
>     the common-side (struct domain). 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
>     <mailto:czuzu@bitdefender.com>>
>
>
> Technically this looks fine, but I do wonder if and what plans you 
> have to actually implement these events for ARM.

Currently I've only planned implementations for control-register write 
events & guest-requests.
The other two also seem feasible though, I might give adding those a 
shot sometime after sending the other patches.

> I haven't spent too much time looking into it, but I'm not aware of 
> equivalent features on ARM to Intel MTF (singlestepping) or to 
> software-breakpoint trapping. The only instruction I know that 
> functionally comes close to software-breakpoint trapping (INT3) is the 
> SMC instruction which can be trapped into the VMM, but I would not 
> call that a "breakpoint" in the traditional sense.
>
> Tamas
>

There's the debugging architecture, hypervisor control of that is 
possible on both 32-bit & 64-bit ARM.
It isn't as easy as for X86 though, where MTF is a hypervisor-internal 
feature and INT3 can be
trapped specifically, whereas on ARM granularity of trap-setting is less 
of a concern apparently.
For this reason, the only issue I see here is the performance penalty 
these traps would cause for
arbitrary software breakpoints (for obvious reasons that doesn't matter 
in the case of single-stepping).

For INT3, the ARM equivalent is be the BKPT/BRK (set HDCR.TDE on 
AArch32/MDCR_EL2.TDE AArch64) instruction.
Trapping on this instruction implies trapping on
- AArch32: some other debug exceptions (looking @ B1.8.9, ARMv7 DDI 0406C.b)
- AArch64: *all software debug exceptions* + *all debug register 
accesses* (this might cause some headaches)
For MTF-like functionality, the debug architecture also provides ways 
for single-stepping.
That would similarly generate software breakpoint exceptions which can 
be routed to the hypervisor.

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 4170 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 0/7] Vm-events: move monitor vm-events code to common code.

[Corneliu ZUZU]

On 2/8/2016 6:57 PM, Corneliu ZUZU wrote:
> This patch series is an attempt to move (most) of the monitor vm-events code to
> the common-side.
>
> Patches summary:
>
> 1. Move xen/arch/arm/hvm.c to xen/arch/arm/hvm/hvm.c
>
> 2. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
> hvm_event_software_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. Some file renames. Read (*) below.
>
> 5. Move hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
> hvm_event_software_breakpoint functions to common-side.
> (note: arch_hvm_event_fill_regs on ARM-side will be implemented in a separate
> patch)
>
> 6. Some file renames, plus some minor fixes. Read (*) below.
>
> 7. Move monitor bitfield members from struct arch_domain to struct domain.
> 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).
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Somehow git send-email managed to malform the introductory message. 
Corrections made, read above instead :)