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

2016-02-16 Thread Tamas K Lengyel 
On Tue, Feb 16, 2016 at 10:48 AM, Corneliu ZUZU 
wrote:

> On 2/16/2016 6:02 PM, Tamas K Lengyel wrote:
>
>
>> @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
>> xen_domctl_monitor_op *mop)
>>
>>  case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
>>  {
>>
>
> So since we will now have two separate booleans, requested_status and
> old_status and then manually verify they are opposite..
>
>
>> -bool_t status = ad->monitor.mov_to_msr_enabled;
>> +bool_t old_status = ad->monitor.mov_to_msr_enabled;
>>
>
> ...here we should set the field to requested_status, not !old_status.
> While they are technically equivalent, the code would read better to other
> way around.
>
>
>>
>> -ad->monitor.mov_to_msr_enabled = !status;
>> +ad->monitor.mov_to_msr_enabled = !old_status;
>
>  domain_unpause(d);
>>  break;
>>  }
>>
>>  case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
>>  {
>> -bool_t status = ad->monitor.singlestep_enabled;
>> +bool_t old_status = ad->monitor.singlestep_enabled;
>>
>> -rc = status_check(mop, status);
>> -if ( rc )
>> -return rc;
>> +if ( unlikely(old_status == requested_status) )
>> +return -EEXIST;
>>
>>  domain_pause(d);
>>
>
> Here as well..
>
>
>> -ad->monitor.singlestep_enabled = !status;
>> +ad->monitor.singlestep_enabled = !old_status;
>>  domain_unpause(d);
>>  break;
>>  }
>>
>>  case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
>>  {
>> -bool_t status = ad->monitor.software_breakpoint_enabled;
>> +bool_t old_status = ad->monitor.software_breakpoint_enabled;
>>
>> -rc = status_check(mop, status);
>> -if ( rc )
>> -return rc;
>> +if ( unlikely(old_status == requested_status) )
>> +return -EEXIST;
>>
>>  domain_pause(d);
>>
>
> ..and here..
>
>
>> -ad->monitor.software_breakpoint_enabled = !status;
>> +ad->monitor.software_breakpoint_enabled = !old_status;
>>  domain_unpause(d);
>>  break;
>>  }
>>
>>  case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
>>  {
>> -bool_t status = ad->monitor.guest_request_enabled;
>> +bool_t old_status = ad->monitor.guest_request_enabled;
>>
>> -rc = status_check(mop, status);
>> -if ( rc )
>> -return rc;
>> +if ( unlikely(old_status == requested_status) )
>> +return -EEXIST;
>>
>>  domain_pause(d);
>>  ad->monitor.guest_request_sync = mop->u.guest_request.sync;
>>
>
> ..and here.
>
>
>> -ad->monitor.guest_request_enabled = !status;
>> +ad->monitor.guest_request_enabled = !old_status;
>>  domain_unpause(d);
>>  break;
>>  }
>>
>
> Otherwise the patch looks good.
>
> Thanks,
> Tamas
>
>
> Oh, right, that would look better. Shall I send a v5 then with that
> change? (and if yes I guess it won't hurt if I also include the left-shift
> sanity checks I mentioned I should have added (?))
>

Please do send another revision with these changes. As I understood Jan
prefers the sanity-checks to be added as a separate patch, so do that.

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

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

2016-02-16 Thread Corneliu ZUZU 

On 2/16/2016 6:02 PM, Tamas K Lengyel wrote:



@@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
xen_domctl_monitor_op *mop)

 case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
 {


So since we will now have two separate booleans, requested_status and 
old_status and then manually verify they are opposite..


-bool_t status = ad->monitor.mov_to_msr_enabled;
+bool_t old_status = ad->monitor.mov_to_msr_enabled;


...here we should set the field to requested_status, not !old_status. 
While they are technically equivalent, the code would read better to 
other way around.



-ad->monitor.mov_to_msr_enabled = !status;
+ad->monitor.mov_to_msr_enabled = !old_status; 


 domain_unpause(d);
 break;
 }

 case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
 {
-bool_t status = ad->monitor.singlestep_enabled;
+bool_t old_status = ad->monitor.singlestep_enabled;

-rc = status_check(mop, status);
-if ( rc )
-return rc;
+if ( unlikely(old_status == requested_status) )
+return -EEXIST;

 domain_pause(d);


Here as well..

-ad->monitor.singlestep_enabled = !status;
+ad->monitor.singlestep_enabled = !old_status;
 domain_unpause(d);
 break;
 }

 case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
 {
-bool_t status = ad->monitor.software_breakpoint_enabled;
+bool_t old_status = ad->monitor.software_breakpoint_enabled;

-rc = status_check(mop, status);
-if ( rc )
-return rc;
+if ( unlikely(old_status == requested_status) )
+return -EEXIST;

 domain_pause(d);


..and here..

-ad->monitor.software_breakpoint_enabled = !status;
+ad->monitor.software_breakpoint_enabled = !old_status;
 domain_unpause(d);
 break;
 }

 case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
 {
-bool_t status = ad->monitor.guest_request_enabled;
+bool_t old_status = ad->monitor.guest_request_enabled;

-rc = status_check(mop, status);
-if ( rc )
-return rc;
+if ( unlikely(old_status == requested_status) )
+return -EEXIST;

 domain_pause(d);
 ad->monitor.guest_request_sync = mop->u.guest_request.sync;


..and here.

-ad->monitor.guest_request_enabled = !status;
+ad->monitor.guest_request_enabled = !old_status;
 domain_unpause(d);
 break;
 }


Otherwise the patch looks good.

Thanks,
Tamas



Oh, right, that would look better. Shall I send a v5 then with that 
change? (and if yes I guess it won't hurt if I also include the 
left-shift sanity checks I mentioned I should have added (?))


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

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

2016-02-16 Thread Tamas K Lengyel 
>
>
> @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
> xen_domctl_monitor_op *mop)
>
>  case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
>  {
>

So since we will now have two separate booleans, requested_status and
old_status and then manually verify they are opposite..


> -bool_t status = ad->monitor.mov_to_msr_enabled;
> +bool_t old_status = ad->monitor.mov_to_msr_enabled;
>

...here we should set the field to requested_status, not !old_status. While
they are technically equivalent, the code would read better to other way
around.


>
> -ad->monitor.mov_to_msr_enabled = !status;
> +ad->monitor.mov_to_msr_enabled = !old_status;

 domain_unpause(d);
>  break;
>  }
>
>  case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
>  {
> -bool_t status = ad->monitor.singlestep_enabled;
> +bool_t old_status = ad->monitor.singlestep_enabled;
>
> -rc = status_check(mop, status);
> -if ( rc )
> -return rc;
> +if ( unlikely(old_status == requested_status) )
> +return -EEXIST;
>
>  domain_pause(d);
>

Here as well..


> -ad->monitor.singlestep_enabled = !status;
> +ad->monitor.singlestep_enabled = !old_status;
>  domain_unpause(d);
>  break;
>  }
>
>  case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
>  {
> -bool_t status = ad->monitor.software_breakpoint_enabled;
> +bool_t old_status = ad->monitor.software_breakpoint_enabled;
>
> -rc = status_check(mop, status);
> -if ( rc )
> -return rc;
> +if ( unlikely(old_status == requested_status) )
> +return -EEXIST;
>
>  domain_pause(d);
>

..and here..


> -ad->monitor.software_breakpoint_enabled = !status;
> +ad->monitor.software_breakpoint_enabled = !old_status;
>  domain_unpause(d);
>  break;
>  }
>
>  case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
>  {
> -bool_t status = ad->monitor.guest_request_enabled;
> +bool_t old_status = ad->monitor.guest_request_enabled;
>
> -rc = status_check(mop, status);
> -if ( rc )
> -return rc;
> +if ( unlikely(old_status == requested_status) )
> +return -EEXIST;
>
>  domain_pause(d);
>  ad->monitor.guest_request_sync = mop->u.guest_request.sync;
>

..and here.


> -ad->monitor.guest_request_enabled = !status;
> +ad->monitor.guest_request_enabled = !old_status;
>  domain_unpause(d);
>  break;
>  }
>

Otherwise the patch looks good.

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

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

2016-02-16 Thread Razvan Cojocaru 
On 02/16/2016 09:08 AM, Corneliu ZUZU wrote:
> This patch moves monitor_domctl to common-side.
> Purpose: move what's common to common, prepare for implementation
> of such vm-events on ARM.
> 
> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
> * add arch-side monitor op handling function => arch_monitor_domctl_op.
>   e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
> * add arch-side monitor event handling function => arch_monitor_domctl_event.
>   e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event 
> enable/disable
> * remove status_check
> 
> Signed-off-by: Corneliu ZUZU 
> 
> ---
> Changed since v3:
>   * monitor_domctl @ common/monitor.c:
> - remove unused requested_status
> - sanity check mop->event range to avoid left-shift undefined behavior
>   * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning
>   * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef
> ---
>  MAINTAINERS   |   1 +
>  xen/arch/x86/monitor.c| 153 
> +++---
>  xen/common/Makefile   |   1 +
>  xen/common/domctl.c   |   2 +-
>  xen/common/monitor.c  |  69 +++
>  xen/include/asm-arm/monitor.h |  30 +++--
>  xen/include/asm-x86/monitor.h |  53 +--
>  xen/include/xen/monitor.h |  30 +
>  8 files changed, 217 insertions(+), 122 deletions(-)
>  create mode 100644 xen/common/monitor.c
>  create mode 100644 xen/include/xen/monitor.h

Fair enough.
Acked-by: Razvan Cojocaru 


Thanks,
Razvan

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

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

2016-02-16 Thread Corneliu ZUZU 

On 2/16/2016 2:34 PM, Jan Beulich wrote:

On 16.02.16 at 12:20,  wrote:

On 2/16/2016 12:45 PM, Jan Beulich wrote:

On 16.02.16 at 09:13,  wrote:

On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:

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

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

enable/disable

* remove status_check

Signed-off-by: Corneliu ZUZU 

---
Changed since v3:
 * monitor_domctl @ common/monitor.c:
   - remove unused requested_status
   - sanity check mop->event range to avoid left-shift undefined behavior

Due to left-shift undefined behavior situations, shouldn't I also:

* in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'

There's no undefinedness there, since the right side operands of
<< are all constant. Using 1U here would be okay, but is not
strictly needed.

I reasoned based on this ISO C99 quote:
[for an E1 << E2 operation, ]
"If E1 has a signed type and nonnegative value, and E1 × 2^E2 is
representable in the result type, then that is the resulting value;
otherwise, the behavior is undefined."

I inferred that this means that code such as '(1 << 31)' would render
undefined behavior, since (1 x 2^31) is not representable on 'int'.
The standard doesn't seem to mention different behavior if the operands
are constants.
This would render undefined behavior if bit 31 of capabilities would be
used @ some point, i.e. if one day someone would e.g. unknowingly:
  #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
Have I misinterpreted the 'representable in the result type' part?

No, that's all correct. It's just that right now no
XEN_DOMCTL_MONITOR_EVENT_* has value 31, and hence
there's only a very minor latent issue here (someone blindly
copying the existing 1 << ... without adding the necessary U at
that point; one might hope the compiler would then point this out
though).

Jan



Ah, I see. Did a fast test earlier w/ GCC 5.1, unfortunately I think the 
compiler doesn't
issue any warning in this situation, would have preferred a heads-up too 
(couldn't even force it to do so).
(it would be nice if Xen shipped w/ a gravitational-waves detector 
though)


Corneliu.

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

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

2016-02-16 Thread Jan Beulich 
>>> On 16.02.16 at 12:20,  wrote:
> On 2/16/2016 12:45 PM, Jan Beulich wrote:
> On 16.02.16 at 09:13,  wrote:
>>> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:
 This patch moves monitor_domctl to common-side.
 Purpose: move what's common to common, prepare for implementation
 of such vm-events on ARM.

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

 Signed-off-by: Corneliu ZUZU 

 ---
 Changed since v3:
 * monitor_domctl @ common/monitor.c:
   - remove unused requested_status
   - sanity check mop->event range to avoid left-shift undefined 
 behavior
>>> Due to left-shift undefined behavior situations, shouldn't I also:
>>>
>>> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'
>> There's no undefinedness there, since the right side operands of
>> << are all constant. Using 1U here would be okay, but is not
>> strictly needed.
> 
> I reasoned based on this ISO C99 quote:
> [for an E1 << E2 operation, ]
> "If E1 has a signed type and nonnegative value, and E1 × 2^E2 is 
> representable in the result type, then that is the resulting value; 
> otherwise, the behavior is undefined."
> 
> I inferred that this means that code such as '(1 << 31)' would render 
> undefined behavior, since (1 x 2^31) is not representable on 'int'.
> The standard doesn't seem to mention different behavior if the operands 
> are constants.
> This would render undefined behavior if bit 31 of capabilities would be 
> used @ some point, i.e. if one day someone would e.g. unknowingly:
>  #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
> Have I misinterpreted the 'representable in the result type' part?

No, that's all correct. It's just that right now no
XEN_DOMCTL_MONITOR_EVENT_* has value 31, and hence
there's only a very minor latent issue here (someone blindly
copying the existing 1 << ... without adding the necessary U at
that point; one might hope the compiler would then point this out
though).

Jan

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

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

2016-02-16 Thread Corneliu ZUZU 

On 2/16/2016 12:45 PM, Jan Beulich wrote:

On 16.02.16 at 09:13,  wrote:

On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:

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

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

enable/disable

* remove status_check

Signed-off-by: Corneliu ZUZU 

---
Changed since v3:
* monitor_domctl @ common/monitor.c:
  - remove unused requested_status
  - sanity check mop->event range to avoid left-shift undefined behavior

Due to left-shift undefined behavior situations, shouldn't I also:

* in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'

There's no undefinedness there, since the right side operands of
<< are all constant. Using 1U here would be okay, but is not
strictly needed.


I reasoned based on this ISO C99 quote:
[for an E1 << E2 operation, ]
"If E1 has a signed type and nonnegative value, and E1 × 2^E2 is 
representable in the result type, then that is the resulting value; 
otherwise, the behavior is undefined."


I inferred that this means that code such as '(1 << 31)' would render 
undefined behavior, since (1 x 2^31) is not representable on 'int'.
The standard doesn't seem to mention different behavior if the operands 
are constants.
This would render undefined behavior if bit 31 of capabilities would be 
used @ some point, i.e. if one day someone would e.g. unknowingly:

#define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
Have I misinterpreted the 'representable in the result type' part?




* in X86 arch_monitor_domctl_event,
XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case
add a sanity check of mop->u.mov_to_cr.index before:
  unsigned int ctrlreg_bitmask =
monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
, which basically translates to:
  unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);

? (especially since mop->u.mov_to_cr.index is set by the caller).

Yes, there a range check would be needed, but preferably as a
separate patch (as this has nothing to do with the code motion
you perform here).

Jan




Great, I'll do these changes in a separate patch then.
Let me know if you have any other comments on this.

Thanks,
Corneliu.

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

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

2016-02-16 Thread Stefano Stabellini 
On Tue, 16 Feb 2016, Corneliu ZUZU wrote:
> This patch moves monitor_domctl to common-side.
> Purpose: move what's common to common, prepare for implementation
> of such vm-events on ARM.
> 
> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
> * add arch-side monitor op handling function => arch_monitor_domctl_op.
>   e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
> * add arch-side monitor event handling function => arch_monitor_domctl_event.
>   e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event 
> enable/disable
> * remove status_check
> 
> Signed-off-by: Corneliu ZUZU 

For the ARM part

Acked-by: Stefano Stabellini 


> Changed since v3:
>   * monitor_domctl @ common/monitor.c:
> - remove unused requested_status
> - sanity check mop->event range to avoid left-shift undefined behavior
>   * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning
>   * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef
> ---
>  MAINTAINERS   |   1 +
>  xen/arch/x86/monitor.c| 153 
> +++---
>  xen/common/Makefile   |   1 +
>  xen/common/domctl.c   |   2 +-
>  xen/common/monitor.c  |  69 +++
>  xen/include/asm-arm/monitor.h |  30 +++--
>  xen/include/asm-x86/monitor.h |  53 +--
>  xen/include/xen/monitor.h |  30 +
>  8 files changed, 217 insertions(+), 122 deletions(-)
>  create mode 100644 xen/common/monitor.c
>  create mode 100644 xen/include/xen/monitor.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f07384c..5cbb1dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -355,6 +355,7 @@ M:Tamas K Lengyel 
>  S:   Supported
>  F:   xen/common/vm_event.c
>  F:   xen/common/mem_access.c
> +F:   xen/common/monitor.c
>  F:   xen/arch/x86/hvm/event.c
>  F:   xen/arch/x86/monitor.c
>  F:   xen/arch/*/vm_event.c
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 1d43880..660b92c 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -1,9 +1,10 @@
>  /*
>   * arch/x86/monitor.c
>   *
> - * Architecture-specific monitor_op domctl handler.
> + * Arch-specific monitor_op domctl handler.
>   *
>   * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public
> @@ -18,87 +19,14 @@
>   * License along with this program; If not, see 
> .
>   */
>  
> -#include 
> -#include 
> -#include 
> -#include 
>  #include 
> -#include 
> -#include 
> +#include 
>  
> -/*
> - * Sanity check whether option is already enabled/disabled
> - */
> -static inline
> -int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
> -{
> -bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
> -
> -if ( status == requested_status )
> -return -EEXIST;
> -
> -return 0;
> -}
> -
> -static inline uint32_t get_capabilities(struct domain *d)
> -{
> -uint32_t capabilities = 0;
> -
> -/*
> - * At the moment only Intel HVM domains are supported. However, event
> - * delivery could be extended to AMD and PV domains.
> - */
> -if ( !is_hvm_domain(d) || !cpu_has_vmx )
> -return capabilities;
> -
> -capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> -   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
> -   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
> -   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> -
> -/* Since we know this is on VMX, we can just call the hvm func */
> -if ( hvm_is_singlestep_supported() )
> -capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> -
> -return capabilities;
> -}
> -
> -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
> +int arch_monitor_domctl_event(struct domain *d,
> +  struct xen_domctl_monitor_op *mop)
>  {
> -int rc;
>  struct arch_domain *ad = >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 &&
> -

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

2016-02-16 Thread Jan Beulich 
>>> On 16.02.16 at 09:13,  wrote:
> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:
>> This patch moves monitor_domctl to common-side.
>> Purpose: move what's common to common, prepare for implementation
>> of such vm-events on ARM.
>>
>> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
>> * add arch-side monitor op handling function => arch_monitor_domctl_op.
>>e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
>> * add arch-side monitor event handling function => arch_monitor_domctl_event.
>>e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event 
> enable/disable
>> * remove status_check
>>
>> Signed-off-by: Corneliu ZUZU 
>>
>> ---
>> Changed since v3:
>>* monitor_domctl @ common/monitor.c:
>>  - remove unused requested_status
>>  - sanity check mop->event range to avoid left-shift undefined behavior
> 
> Due to left-shift undefined behavior situations, shouldn't I also:
> 
> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'

There's no undefinedness there, since the right side operands of
<< are all constant. Using 1U here would be okay, but is not
strictly needed.

> * in X86 arch_monitor_domctl_event, 
> XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case
> add a sanity check of mop->u.mov_to_cr.index before:
>  unsigned int ctrlreg_bitmask = 
> monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
> , which basically translates to:
>  unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);
> 
> ? (especially since mop->u.mov_to_cr.index is set by the caller).

Yes, there a range check would be needed, but preferably as a
separate patch (as this has nothing to do with the code motion
you perform here).

Jan


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

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

2016-02-16 Thread Corneliu ZUZU 

On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:

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

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

Signed-off-by: Corneliu ZUZU 

---
Changed since v3:
   * monitor_domctl @ common/monitor.c:
 - remove unused requested_status
 - sanity check mop->event range to avoid left-shift undefined behavior


Due to left-shift undefined behavior situations, shouldn't I also:

* in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'

* in X86 arch_monitor_domctl_event, 
XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case

add a sanity check of mop->u.mov_to_cr.index before:
unsigned int ctrlreg_bitmask = 
monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);

, which basically translates to:
unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);

? (especially since mop->u.mov_to_cr.index is set by the caller).

Would have been good if I'd thought of that before sending this patch 
series :).


Thanks,
Corneliu.

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

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

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

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

Signed-off-by: Corneliu ZUZU 

---
Changed since v3:
  * monitor_domctl @ common/monitor.c:
- remove unused requested_status
- sanity check mop->event range to avoid left-shift undefined behavior
  * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning
  * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef
---
 MAINTAINERS   |   1 +
 xen/arch/x86/monitor.c| 153 +++---
 xen/common/Makefile   |   1 +
 xen/common/domctl.c   |   2 +-
 xen/common/monitor.c  |  69 +++
 xen/include/asm-arm/monitor.h |  30 +++--
 xen/include/asm-x86/monitor.h |  53 +--
 xen/include/xen/monitor.h |  30 +
 8 files changed, 217 insertions(+), 122 deletions(-)
 create mode 100644 xen/common/monitor.c
 create mode 100644 xen/include/xen/monitor.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f07384c..5cbb1dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,6 +355,7 @@ M:  Tamas K Lengyel 
 S: Supported
 F: xen/common/vm_event.c
 F: xen/common/mem_access.c
+F: xen/common/monitor.c
 F: xen/arch/x86/hvm/event.c
 F: xen/arch/x86/monitor.c
 F: xen/arch/*/vm_event.c
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1d43880..660b92c 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -1,9 +1,10 @@
 /*
  * arch/x86/monitor.c
  *
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
  *
  * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public
@@ -18,87 +19,14 @@
  * License along with this program; If not, see .
  */
 
-#include 
-#include 
-#include 
-#include 
 #include 
-#include 
-#include 
+#include 
 
-/*
- * Sanity check whether option is already enabled/disabled
- */
-static inline
-int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
-{
-bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
-
-if ( status == requested_status )
-return -EEXIST;
-
-return 0;
-}
-
-static inline uint32_t get_capabilities(struct domain *d)
-{
-uint32_t capabilities = 0;
-
-/*
- * At the moment only Intel HVM domains are supported. However, event
- * delivery could be extended to AMD and PV domains.
- */
-if ( !is_hvm_domain(d) || !cpu_has_vmx )
-return capabilities;
-
-capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-/* Since we know this is on VMX, we can just call the hvm func */
-if ( hvm_is_singlestep_supported() )
-capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
-return capabilities;
-}
-
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+int arch_monitor_domctl_event(struct domain *d,
+  struct xen_domctl_monitor_op *mop)
 {
-int rc;
 struct arch_domain *ad = >arch;
-uint32_t capabilities = get_capabilities(d);
-
-if ( current->domain == d ) /* no domain_pause() */
-return -EPERM;
-
-rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
-if ( rc )
-return rc;
-
-switch ( mop->op )
-{
-case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
-mop->event = capabilities;
-return 0;
-
-case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
-domain_pause(d);
-ad->mem_access_emulate_each_rep = !!mop->event;
-domain_unpause(d);
-return 0;
-}
-
-/*
- * Sanity check
- */
-if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
- mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
-return -EOPNOTSUPP;
-
-/* Check if event type is available. */
-if ( !(capabilities & (1 << mop->event)) )
-return -EOPNOTSUPP;
+bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
 
 switch ( mop->event )
 {
@@ -106,13 +34,11 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)