Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-21 Thread Stefano Stabellini 
On Fri, 20 May 2016, Jan Beulich wrote:
> >>> On 20.05.16 at 17:54,  wrote:
> > On 20/05/16 17:36, Jan Beulich wrote:
> > On 20.05.16 at 17:04,  wrote:
> >>> On 20/05/16 16:49, Jan Beulich wrote:
> >>> On 20.05.16 at 15:22,  wrote:
> >  if ( guest_handle_is_null(runstate_guest(v)) )
> >  return 1;
> >  
> > +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
> > +
> >  smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
> >  
> > +if ( update_flag )
> > +{
> > +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 
> > 7;
> 
>  How come this is outside the following if()? Also sizeof(...) - 1 please
>  instead of the literal 7.
> >>>
> >>> I'm using off for the source address in __raw_copy_to_guest(), too.
> >> 
> >> But the offset should, afaict, be different for 32-bit (x86) and
> >> 64-bit (or ARM).
> > 
> > Why? The offset is applied to v->runstate which clearly is the same
> > for 32 and 64 bit domains, as it is the hypervisor private structure.
> > Different offsets have to be applied at the destination side only, and
> > this is done properly (at least I think so).
> 
> But as you say you use the offset for two purposes: The use on
> the guest handle is which is problematic; the use on the hypervisor
> internal structure is of course fine.

The code is a bit ugly, but it looks like it would work on ARM

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

Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Jan Beulich 
>>> On 20.05.16 at 17:54,  wrote:
> On 20/05/16 17:36, Jan Beulich wrote:
> On 20.05.16 at 17:04,  wrote:
>>> On 20/05/16 16:49, Jan Beulich wrote:
>>> On 20.05.16 at 15:22,  wrote:
>  if ( guest_handle_is_null(runstate_guest(v)) )
>  return 1;
>  
> +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
> +
>  smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>  
> +if ( update_flag )
> +{
> +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;

 How come this is outside the following if()? Also sizeof(...) - 1 please
 instead of the literal 7.
>>>
>>> I'm using off for the source address in __raw_copy_to_guest(), too.
>> 
>> But the offset should, afaict, be different for 32-bit (x86) and
>> 64-bit (or ARM).
> 
> Why? The offset is applied to v->runstate which clearly is the same
> for 32 and 64 bit domains, as it is the hypervisor private structure.
> Different offsets have to be applied at the destination side only, and
> this is done properly (at least I think so).

But as you say you use the offset for two purposes: The use on
the guest handle is which is problematic; the use on the hypervisor
internal structure is of course fine.

Jan


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

Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Juergen Gross 
On 20/05/16 17:36, Jan Beulich wrote:
 On 20.05.16 at 17:04,  wrote:
>> On 20/05/16 16:49, Jan Beulich wrote:
>> On 20.05.16 at 15:22,  wrote:
  if ( guest_handle_is_null(runstate_guest(v)) )
  return 1;
  
 +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
 +
  smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
  
 +if ( update_flag )
 +{
 +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
>>>
>>> How come this is outside the following if()? Also sizeof(...) - 1 please
>>> instead of the literal 7.
>>
>> I'm using off for the source address in __raw_copy_to_guest(), too.
> 
> But the offset should, afaict, be different for 32-bit (x86) and
> 64-bit (or ARM).

Why? The offset is applied to v->runstate which clearly is the same
for 32 and 64 bit domains, as it is the hypervisor private structure.
Different offsets have to be applied at the destination side only, and
this is done properly (at least I think so).


Juergen


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

Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Jan Beulich 
>>> On 20.05.16 at 17:04,  wrote:
> On 20/05/16 16:49, Jan Beulich wrote:
> On 20.05.16 at 15:22,  wrote:
>>>  if ( guest_handle_is_null(runstate_guest(v)) )
>>>  return 1;
>>>  
>>> +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
>>> +
>>>  smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>>>  
>>> +if ( update_flag )
>>> +{
>>> +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
>> 
>> How come this is outside the following if()? Also sizeof(...) - 1 please
>> instead of the literal 7.
> 
> I'm using off for the source address in __raw_copy_to_guest(), too.

But the offset should, afaict, be different for 32-bit (x86) and
64-bit (or ARM).

Jan


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

Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Juergen Gross 
On 20/05/16 16:49, Jan Beulich wrote:
 On 20.05.16 at 15:22,  wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1925,13 +1925,37 @@ static void paravirt_ctxt_switch_to(struct vcpu *v)
>>  bool_t update_runstate_area(struct vcpu *v)
>>  {
>>  bool_t rc;
>> +bool_t update_flag;
> 
> I think this variable is superfluous (and causes more register pressure
> in the compiler), since ...
> 
>>  smap_check_policy_t smap_policy;
>> +void __user *guest_handle = NULL;
> 
> ... you can key off of this being non-NULL or ...
> 
>> +unsigned off = 0;
> 
> ... this being non-zero in the second if().

Okay. Will change.

> 
>>  if ( guest_handle_is_null(runstate_guest(v)) )
>>  return 1;
>>  
>> +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
>> +
>>  smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>>  
>> +if ( update_flag )
>> +{
>> +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
> 
> How come this is outside the following if()? Also sizeof(...) - 1 please
> instead of the literal 7.

I'm using off for the source address in __raw_copy_to_guest(), too.
Regarding sizeof(): okay.

> 
>> --- a/xen/include/public/vcpu.h
>> +++ b/xen/include/public/vcpu.h
>> @@ -84,6 +84,12 @@ struct vcpu_runstate_info {
>>  /* When was current state entered (system time, ns)? */
>>  uint64_t state_entry_time;
>>  /*
>> + * Update indicator set in state_entry_time:
>> + * When activated via VMASST_TYPE_runstate_update_flag, set during
>> + * updates in guest memory mapped copy of vcpu_runstate_info.
>> + */
>> +#define XEN_RUNSTATE_UPDATE  (1ULL << 63)
> 
> I think this should be UINT64_C(1), as ULL is not a C89 compatible
> suffix, but by requiring uint64_t I think we can imply that along with
> that C99 type the platform also surfaces respective macros.

Okay.


Juergen

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

Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Jan Beulich 
>>> On 20.05.16 at 15:22,  wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1925,13 +1925,37 @@ static void paravirt_ctxt_switch_to(struct vcpu *v)
>  bool_t update_runstate_area(struct vcpu *v)
>  {
>  bool_t rc;
> +bool_t update_flag;

I think this variable is superfluous (and causes more register pressure
in the compiler), since ...

>  smap_check_policy_t smap_policy;
> +void __user *guest_handle = NULL;

... you can key off of this being non-NULL or ...

> +unsigned off = 0;

... this being non-zero in the second if().

>  if ( guest_handle_is_null(runstate_guest(v)) )
>  return 1;
>  
> +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
> +
>  smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>  
> +if ( update_flag )
> +{
> +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;

How come this is outside the following if()? Also sizeof(...) - 1 please
instead of the literal 7.

> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -84,6 +84,12 @@ struct vcpu_runstate_info {
>  /* When was current state entered (system time, ns)? */
>  uint64_t state_entry_time;
>  /*
> + * Update indicator set in state_entry_time:
> + * When activated via VMASST_TYPE_runstate_update_flag, set during
> + * updates in guest memory mapped copy of vcpu_runstate_info.
> + */
> +#define XEN_RUNSTATE_UPDATE  (1ULL << 63)

I think this should be UINT64_C(1), as ULL is not a C89 compatible
suffix, but by requiring uint64_t I think we can imply that along with
that C99 type the platform also surfaces respective macros.

Jan


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

Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Andrew Cooper 
On 20/05/16 14:22, Juergen Gross wrote:
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 1365b4a..91f256b 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -239,10 +239,32 @@ static void ctxt_switch_to(struct vcpu *n)
>  /* Update per-VCPU guest runstate shared memory area (if registered). */
>  static void update_runstate_area(struct vcpu *v)
>  {
> +bool_t update_flag;
> +void __user *guest_handle = NULL;
> +unsigned off = 0;
> +
>  if ( guest_handle_is_null(runstate_guest(v)) )
>  return;
>  
> +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
> +if ( update_flag )
> +{
> +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
> +guest_handle = v->runstate_guest.p;
> +guest_handle += off;
> +v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> +__raw_copy_to_guest(guest_handle, (void *)>runstate + off, 1);
> +wmb();

smp_wmb(), throughout.

A whole lot of code gets this wrong in Xen, and plan to clean it all up
in 4.8

~Andrew

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

Re: [Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Juergen Gross 
On 20/05/16 15:34, Andrew Cooper wrote:
> On 20/05/16 14:22, Juergen Gross wrote:
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 1365b4a..91f256b 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -239,10 +239,32 @@ static void ctxt_switch_to(struct vcpu *n)
>>  /* Update per-VCPU guest runstate shared memory area (if registered). */
>>  static void update_runstate_area(struct vcpu *v)
>>  {
>> +bool_t update_flag;
>> +void __user *guest_handle = NULL;
>> +unsigned off = 0;
>> +
>>  if ( guest_handle_is_null(runstate_guest(v)) )
>>  return;
>>  
>> +update_flag = VM_ASSIST(v->domain, runstate_update_flag);
>> +if ( update_flag )
>> +{
>> +off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
>> +guest_handle = v->runstate_guest.p;
>> +guest_handle += off;
>> +v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
>> +__raw_copy_to_guest(guest_handle, (void *)>runstate + off, 1);
>> +wmb();
> 
> smp_wmb(), throughout.

Okay.


Juergen

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

[Xen-devel] [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

2016-05-20 Thread Juergen Gross 
In order to support reading another vcpu's mapped vcpu_runstate_info
an indicator for an occurring update of the runstate information is
needed.

Add the possibility to activate setting this indicator in the highest
bit of state_entry_time via a vm_assist hypercall. When activated the
update indicator will be set before the runstate information is
modified in guest memory and it will be reset after modification is
done. As state_entry_time is guaranteed to be different after each
update the guest can detect any update (either in progress or while
reading the runstate data) by comparing state_entry_time before and
after reading runstate data: in case the values differ or the update
indicator was set the data might be inconsistent and should be reread.

Signed-off-by: Juergen Gross 
---
 xen/arch/arm/domain.c| 22 ++
 xen/arch/x86/domain.c| 31 +++
 xen/include/asm-arm/config.h |  2 +-
 xen/include/asm-x86/config.h |  1 +
 xen/include/public/vcpu.h|  6 ++
 xen/include/public/xen.h |  7 +++
 6 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 1365b4a..91f256b 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -239,10 +239,32 @@ static void ctxt_switch_to(struct vcpu *n)
 /* Update per-VCPU guest runstate shared memory area (if registered). */
 static void update_runstate_area(struct vcpu *v)
 {
+bool_t update_flag;
+void __user *guest_handle = NULL;
+unsigned off = 0;
+
 if ( guest_handle_is_null(runstate_guest(v)) )
 return;
 
+update_flag = VM_ASSIST(v->domain, runstate_update_flag);
+if ( update_flag )
+{
+off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
+guest_handle = v->runstate_guest.p;
+guest_handle += off;
+v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
+__raw_copy_to_guest(guest_handle, (void *)>runstate + off, 1);
+wmb();
+}
+
 __copy_to_guest(runstate_guest(v), >runstate, 1);
+
+if ( update_flag )
+{
+v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
+wmb();
+__raw_copy_to_guest(guest_handle, (void *)>runstate + off, 1);
+}
 }
 
 static void schedule_tail(struct vcpu *prev)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5af2cc5..dfaee5d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1925,13 +1925,37 @@ static void paravirt_ctxt_switch_to(struct vcpu *v)
 bool_t update_runstate_area(struct vcpu *v)
 {
 bool_t rc;
+bool_t update_flag;
 smap_check_policy_t smap_policy;
+void __user *guest_handle = NULL;
+unsigned off = 0;
 
 if ( guest_handle_is_null(runstate_guest(v)) )
 return 1;
 
+update_flag = VM_ASSIST(v->domain, runstate_update_flag);
+
 smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
 
+if ( update_flag )
+{
+off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
+if ( has_32bit_shinfo(v->domain) )
+{
+guest_handle = v->runstate_guest.compat.p;
+guest_handle += offsetof(struct compat_vcpu_runstate_info,
+ state_entry_time) + 7;
+}
+else
+{
+guest_handle = v->runstate_guest.native.p;
+guest_handle += off;
+}
+v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
+__raw_copy_to_guest(guest_handle, (void *)>runstate + off, 1);
+wmb();
+}
+
 if ( has_32bit_shinfo(v->domain) )
 {
 struct compat_vcpu_runstate_info info;
@@ -1944,6 +1968,13 @@ bool_t update_runstate_area(struct vcpu *v)
 rc = __copy_to_guest(runstate_guest(v), >runstate, 1) !=
  sizeof(v->runstate);
 
+if ( update_flag )
+{
+v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
+wmb();
+__raw_copy_to_guest(guest_handle, (void *)>runstate + off, 1);
+}
+
 smap_policy_change(v, smap_policy);
 
 return rc;
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 563f49b..ce3edc2 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -199,7 +199,7 @@ extern unsigned long frametable_virt_end;
 #define watchdog_disable() ((void)0)
 #define watchdog_enable()  ((void)0)
 
-#define VM_ASSIST_VALID  (0)
+#define VM_ASSIST_VALID  (1UL << VMASST_TYPE_runstate_update_flag)
 
 #endif /* __ARM_CONFIG_H__ */
 /*
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index c10129d..6fd84e7 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -332,6 +332,7 @@ extern unsigned long xen_phys_start;
   (1UL << VMASST_TYPE_writable_pagetables) | \
   (1UL << VMASST_TYPE_pae_extended_cr3)| \