[PATCH v2 0/2] Support consistent reads of mapped vcpu_runstate_info

[PATCH v2 0/2] Support consistent reads of mapped vcpu_runstate_info

[Juergen Gross]

A guest mapping vcpu_runstate_info into its memory can't read this
information from another cpu but the one the data is referring to.
Reason is there is no reliable way for the guest to detect a concurrent
data update by the hypervisor.

This patch series adds an update flag to the mapped data which can be
used by the guest to detect an update is occurring. As this flag is
modifying the current interface it has to be activated by using a
vm_assist hypercall, which in turn has to be made available for ARM.

Runtime tested on x86 with a modified Linux kernel using the new
feature.
Compile tested only for ARM.

Changes in V2:
- patch 1: readded the #ifdef's as requested by Jan Beulich
- patch 2: smp_wmb() instead of wmb() as requested by Andrew Cooper
- patch 2: use sizeof() as requested by Jan Beulich
- patch 2: eliminate new variable update_flag as requested by Jan Beulich

Juergen Gross (2):
  xen/arm: add support for vm_assist hypercall
  xen: add update indicator to vcpu_runstate_info

 xen/arch/arm/domain.c        | 21 +++++++++++++++++++++
 xen/arch/arm/traps.c         |  1 +
 xen/arch/x86/domain.c        | 30 ++++++++++++++++++++++++++++++
 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 +++++++
 7 files changed, 68 insertions(+)

-- 
2.6.6


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

[PATCH v2 1/2] xen/arm: add support for vm_assist hypercall

[Juergen Gross]

Up to now the vm_assist hypercall hasn't been supported on ARM, as
there are only x86 specific features to switch. Add support of
vm_assist on ARM for future use.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
V2: readded the #ifdef's as requested by Jan Beulich
---
 xen/arch/arm/traps.c         | 1 +
 xen/include/asm-arm/config.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index aa3e3c2..44926ca 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1282,6 +1282,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(multicall, 2),
     HYPERCALL(platform_op, 1),
     HYPERCALL_ARM(vcpu_op, 3),
+    HYPERCALL(vm_assist, 2),
 };
 
 #ifndef NDEBUG
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 2d11b62..563f49b 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -199,6 +199,8 @@ extern unsigned long frametable_virt_end;
 #define watchdog_disable() ((void)0)
 #define watchdog_enable()  ((void)0)
 
+#define VM_ASSIST_VALID          (0)
+
 #endif /* __ARM_CONFIG_H__ */
 /*
  * Local variables:
-- 
2.6.6


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

[PATCH v2 2/2] xen: add update indicator to vcpu_runstate_info

[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 <jgross@suse.com>
---
V2: smp_wmb() instead of wmb() as requested by Andrew Cooper
    use sizeof() as requested by Jan Beulich
    eliminate new variable update_flag as requested by Jan Beulich
---
 xen/arch/arm/domain.c        | 21 +++++++++++++++++++++
 xen/arch/x86/domain.c        | 30 ++++++++++++++++++++++++++++++
 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, 66 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 1365b4a..ad50b19 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -239,10 +239,31 @@ 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)
 {
+    void __user *guest_handle = NULL;
+    unsigned off = 0;
+
     if ( guest_handle_is_null(runstate_guest(v)) )
         return;
 
+    if ( VM_ASSIST(v->domain, runstate_update_flag) )
+    {
+        off = offsetof(struct vcpu_runstate_info, state_entry_time) +
+              sizeof(v->runstate.state_entry_time) - 1;
+        guest_handle = v->runstate_guest.p;
+        guest_handle += off;
+        v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
+        __raw_copy_to_guest(guest_handle, (void *)&v->runstate + off, 1);
+        smp_wmb();
+    }
+
     __copy_to_guest(runstate_guest(v), &v->runstate, 1);
+
+    if ( guest_handle )
+    {
+        v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
+        smp_wmb();
+        __raw_copy_to_guest(guest_handle, (void *)&v->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 989bc74..8890661 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1926,12 +1926,35 @@ bool_t update_runstate_area(struct vcpu *v)
 {
     bool_t rc;
     smap_check_policy_t smap_policy;
+    void __user *guest_handle = NULL;
+    unsigned off = 0;
 
     if ( guest_handle_is_null(runstate_guest(v)) )
         return 1;
 
     smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
 
+    if ( VM_ASSIST(v->domain, runstate_update_flag) )
+    {
+        off = offsetof(struct vcpu_runstate_info, state_entry_time) +
+              sizeof(v->runstate.state_entry_time) - 1;
+        if ( has_32bit_shinfo(v->domain) )
+        {
+            guest_handle = v->runstate_guest.compat.p;
+            guest_handle +=
+                offsetof(struct compat_vcpu_runstate_info, state_entry_time) +
+                sizeof(v->runstate.state_entry_time) - 1;
+        }
+        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 *)&v->runstate + off, 1);
+        smp_wmb();
+    }
+
     if ( has_32bit_shinfo(v->domain) )
     {
         struct compat_vcpu_runstate_info info;
@@ -1944,6 +1967,13 @@ bool_t update_runstate_area(struct vcpu *v)
         rc = __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
              sizeof(v->runstate);
 
+    if ( guest_handle )
+    {
+        v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
+        smp_wmb();
+        __raw_copy_to_guest(guest_handle, (void *)&v->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)    | \
                                   (1UL << VMASST_TYPE_architectural_iopl)  | \
+                                  (1UL << VMASST_TYPE_runstate_update_flag)| \
                                   (1UL << VMASST_TYPE_m2p_strict))
 #define VM_ASSIST_VALID          NATIVE_VM_ASSIST_VALID
 #define COMPAT_VM_ASSIST_VALID   (NATIVE_VM_ASSIST_VALID & \
diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
index 692b87a..2aa230d 100644
--- 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)
+    /*
      * Time spent in each RUNSTATE_* (ns). The sum of these times is
      * guaranteed not to drift from system time.
      */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 37bbb22..b9e5e0f 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -510,6 +510,13 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
 #define VMASST_TYPE_architectural_iopl   4
 
 /*
+ * All guests: activate update indicator in vcpu_runstate_info
+ * Enable setting the XEN_RUNSTATE_UPDATE flag in guest memory mapped
+ * vcpu_runstate_info during updates of the runstate information.
+ */
+#define VMASST_TYPE_runstate_update_flag 5
+
+/*
  * x86/64 guests: strictly hide M2P from user mode.
  * This allows the guest to control respective hypervisor behavior:
  * - when not set, L4 tables get created with the respective slot blank,
-- 
2.6.6


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

Re: [PATCH v2 2/2] xen: add update indicator to vcpu_runstate_info

[Jan Beulich]

>>> On 15.06.16 at 13:34, <JGross@suse.com> wrote:
> 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.

Neither here nor in the cover letter you mention why this is useful
to have (and to be honest I did forget what you need this for since
the time the original discussion happened), yet that's quite relevant
since for many years we lived happily without it.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1926,12 +1926,35 @@ bool_t update_runstate_area(struct vcpu *v)
>  {
>      bool_t rc;
>      smap_check_policy_t smap_policy;
> +    void __user *guest_handle = NULL;
> +    unsigned off = 0;
>  
>      if ( guest_handle_is_null(runstate_guest(v)) )
>          return 1;
>  
>      smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>  
> +    if ( VM_ASSIST(v->domain, runstate_update_flag) )
> +    {
> +        off = offsetof(struct vcpu_runstate_info, state_entry_time) +
> +              sizeof(v->runstate.state_entry_time) - 1;
> +        if ( has_32bit_shinfo(v->domain) )
> +        {
> +            guest_handle = v->runstate_guest.compat.p;
> +            guest_handle +=
> +                offsetof(struct compat_vcpu_runstate_info, state_entry_time) +
> +                sizeof(v->runstate.state_entry_time) - 1;

The sizes of the native and compat fields happen to be the same,
but it would be nice if the right field/type could be used here.

> --- 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 continue to be not particularly happy with the C99ism here, but
I see we have a few other instances of such already in the public
headers.

Jan


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

Re: [PATCH v2 2/2] xen: add update indicator to vcpu_runstate_info

[Juergen Gross]

On 16/06/16 13:15, Jan Beulich wrote:
>>>> On 15.06.16 at 13:34, <JGross@suse.com> wrote:
>> 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.
> 
> Neither here nor in the cover letter you mention why this is useful
> to have (and to be honest I did forget what you need this for since
> the time the original discussion happened), yet that's quite relevant
> since for many years we lived happily without it.

I'll add the following in the cover letter:

There has been a report about incorrect vruntime accounting in Linux
guests under Xen. A Linux kernel with CONFIG_PARAVIRT_TIME_ACCOUNTING
set is capable to do correct vruntime accounting, but this would
require the kernel to be able to read the runstate data of other cpus.

> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1926,12 +1926,35 @@ bool_t update_runstate_area(struct vcpu *v)
>>  {
>>      bool_t rc;
>>      smap_check_policy_t smap_policy;
>> +    void __user *guest_handle = NULL;
>> +    unsigned off = 0;
>>  
>>      if ( guest_handle_is_null(runstate_guest(v)) )
>>          return 1;
>>  
>>      smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>>  
>> +    if ( VM_ASSIST(v->domain, runstate_update_flag) )
>> +    {
>> +        off = offsetof(struct vcpu_runstate_info, state_entry_time) +
>> +              sizeof(v->runstate.state_entry_time) - 1;
>> +        if ( has_32bit_shinfo(v->domain) )
>> +        {
>> +            guest_handle = v->runstate_guest.compat.p;
>> +            guest_handle +=
>> +                offsetof(struct compat_vcpu_runstate_info, state_entry_time) +
>> +                sizeof(v->runstate.state_entry_time) - 1;
> 
> The sizes of the native and compat fields happen to be the same,
> but it would be nice if the right field/type could be used here.

Hmm, this will require some ugly type casting, but it is probably
cleaner.

>> --- 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 continue to be not particularly happy with the C99ism here, but
> I see we have a few other instances of such already in the public
> headers.

Indeed. And UINT64_C() is used nowhere in the hypervisor and defined in
xen/include/crypto/vmac.h


Juergen

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

Re: [PATCH v2 2/2] xen: add update indicator to vcpu_runstate_info

[Jan Beulich]

>>> On 16.06.16 at 14:11, <JGross@suse.com> wrote:
> On 16/06/16 13:15, Jan Beulich wrote:
>>>>> On 15.06.16 at 13:34, <JGross@suse.com> wrote:
>>> +    if ( VM_ASSIST(v->domain, runstate_update_flag) )
>>> +    {
>>> +        off = offsetof(struct vcpu_runstate_info, state_entry_time) +
>>> +              sizeof(v->runstate.state_entry_time) - 1;
>>> +        if ( has_32bit_shinfo(v->domain) )
>>> +        {
>>> +            guest_handle = v->runstate_guest.compat.p;
>>> +            guest_handle +=
>>> +                offsetof(struct compat_vcpu_runstate_info, state_entry_time) +
>>> +                sizeof(v->runstate.state_entry_time) - 1;
>> 
>> The sizes of the native and compat fields happen to be the same,
>> but it would be nice if the right field/type could be used here.
> 
> Hmm, this will require some ugly type casting, but it is probably
> cleaner.

Type casting? I would expect you to be able to use
v->runstate_guest.compat.p->state_entry_time. In fact I think
you also could get rid of the offsetof() if you used
&v->runstate_guest.compat.p->state_entry_time for initializing
guest_handle.

Jan


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

Re: [PATCH v2 2/2] xen: add update indicator to vcpu_runstate_info

[Juergen Gross]

On 16/06/16 14:36, Jan Beulich wrote:
>>>> On 16.06.16 at 14:11, <JGross@suse.com> wrote:
>> On 16/06/16 13:15, Jan Beulich wrote:
>>>>>> On 15.06.16 at 13:34, <JGross@suse.com> wrote:
>>>> +    if ( VM_ASSIST(v->domain, runstate_update_flag) )
>>>> +    {
>>>> +        off = offsetof(struct vcpu_runstate_info, state_entry_time) +
>>>> +              sizeof(v->runstate.state_entry_time) - 1;
>>>> +        if ( has_32bit_shinfo(v->domain) )
>>>> +        {
>>>> +            guest_handle = v->runstate_guest.compat.p;
>>>> +            guest_handle +=
>>>> +                offsetof(struct compat_vcpu_runstate_info, state_entry_time) +
>>>> +                sizeof(v->runstate.state_entry_time) - 1;
>>>
>>> The sizes of the native and compat fields happen to be the same,
>>> but it would be nice if the right field/type could be used here.
>>
>> Hmm, this will require some ugly type casting, but it is probably
>> cleaner.
> 
> Type casting? I would expect you to be able to use
> v->runstate_guest.compat.p->state_entry_time. In fact I think
> you also could get rid of the offsetof() if you used
> &v->runstate_guest.compat.p->state_entry_time for initializing
> guest_handle.

Aah, of course. This can then be simplified even more:

if ( VM_ASSIST(v->domain, runstate_update_flag) )
{
    guest_handle = has_32bit_shinfo(v->domain)
        ? &v->runstate_guest.compat.p->state_entry_time + 1
        : &v->runstate_guest.native.p->state_entry_time + 1;
    guest_handle--;
    v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
    __raw_copy_to_guest(guest_handle,
                 (void *)(&v->runstate.state_entry_time + 1) - 1, 1);
    smp_wmb();
}

This will reduce code size by 25 bytes. :-)


Juergen

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