[PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info

[PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info

[Dongli Zhang]

The guest may access the pv vcpu_time_info immediately after
VCPUOP_register_vcpu_info. This is to borrow the idea of
VCPUOP_register_vcpu_time_memory_area, where the
force_update_vcpu_system_time() is called immediately when the new memory
area is registered.

Otherwise, we may observe clock drift at the VM side if the VM accesses
the clocksource immediately after VCPUOP_register_vcpu_info().

Reference:
https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00571.html
Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
Changed since v1:
  - Implement force_update_vcpu_system_time() for ARM
    (suggested by Jan Beulich)
    While I have tested ARM compilation with aarch64-linux-gnu-gcc, I did
    not test on ARM platform.

 xen/arch/arm/time.c        | 5 +++++
 xen/common/domain.c        | 2 ++
 xen/include/asm-arm/time.h | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 7dbd363537..dec53b5f7d 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -351,6 +351,11 @@ void update_vcpu_system_time(struct vcpu *v)
     /* XXX update shared_info->wc_* */
 }
 
+void force_update_vcpu_system_time(struct vcpu *v)
+{
+    update_vcpu_system_time(v);
+}
+
 void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
 {
     d->time_offset.seconds = time_offset_seconds;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8b53c49d1e..d71fcab88c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1704,6 +1704,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
         rc = map_vcpu_info(v, info.mfn, info.offset);
         domain_unlock(d);
 
+        force_update_vcpu_system_time(v);
+
         break;
     }
 
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index 6b8fd839dd..4b401c1110 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -105,6 +105,8 @@ extern uint64_t ns_to_ticks(s_time_t ns);
 
 void preinit_xen_time(void);
 
+void force_update_vcpu_system_time(struct vcpu *v);
+
 #endif /* __ARM_TIME_H__ */
 /*
  * Local variables:
-- 
2.17.1

Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info

[Stefano Stabellini]

On Mon, 25 Oct 2021, Dongli Zhang wrote:
> The guest may access the pv vcpu_time_info immediately after
> VCPUOP_register_vcpu_info. This is to borrow the idea of
> VCPUOP_register_vcpu_time_memory_area, where the
> force_update_vcpu_system_time() is called immediately when the new memory
> area is registered.
> 
> Otherwise, we may observe clock drift at the VM side if the VM accesses
> the clocksource immediately after VCPUOP_register_vcpu_info().
> 
> Reference:
> https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00571.html
> Cc: Joe Jin <joe.jin@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> Changed since v1:
>   - Implement force_update_vcpu_system_time() for ARM
>     (suggested by Jan Beulich)
>     While I have tested ARM compilation with aarch64-linux-gnu-gcc, I did
>     not test on ARM platform.

This is fine. Of course it is not going to do anything on ARM given that
update_vcpu_system_time in unimplemented but it is certainly harmless.

For the ARM part:
Acked-by: Stefano Stabellini <sstabellini@kernel.org>


>  xen/arch/arm/time.c        | 5 +++++
>  xen/common/domain.c        | 2 ++
>  xen/include/asm-arm/time.h | 2 ++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 7dbd363537..dec53b5f7d 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -351,6 +351,11 @@ void update_vcpu_system_time(struct vcpu *v)
>      /* XXX update shared_info->wc_* */
>  }
>  
> +void force_update_vcpu_system_time(struct vcpu *v)
> +{
> +    update_vcpu_system_time(v);
> +}
> +
>  void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
>  {
>      d->time_offset.seconds = time_offset_seconds;
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 8b53c49d1e..d71fcab88c 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1704,6 +1704,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = map_vcpu_info(v, info.mfn, info.offset);
>          domain_unlock(d);
>  
> +        force_update_vcpu_system_time(v);
> +
>          break;
>      }
>  
> diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
> index 6b8fd839dd..4b401c1110 100644
> --- a/xen/include/asm-arm/time.h
> +++ b/xen/include/asm-arm/time.h
> @@ -105,6 +105,8 @@ extern uint64_t ns_to_ticks(s_time_t ns);
>  
>  void preinit_xen_time(void);
>  
> +void force_update_vcpu_system_time(struct vcpu *v);
> +
>  #endif /* __ARM_TIME_H__ */
>  /*
>   * Local variables:
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info

[Jan Beulich]

On 25.10.2021 19:35, Dongli Zhang wrote:
> The guest may access the pv vcpu_time_info immediately after
> VCPUOP_register_vcpu_info. This is to borrow the idea of
> VCPUOP_register_vcpu_time_memory_area, where the
> force_update_vcpu_system_time() is called immediately when the new memory
> area is registered.
> 
> Otherwise, we may observe clock drift at the VM side if the VM accesses
> the clocksource immediately after VCPUOP_register_vcpu_info().
> 
> Reference:
> https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00571.html
> Cc: Joe Jin <joe.jin@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Ian - any thoughts towards 4.16 here either way?

Jan

Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info"):
> On 25.10.2021 19:35, Dongli Zhang wrote:
> > The guest may access the pv vcpu_time_info immediately after
> > VCPUOP_register_vcpu_info. This is to borrow the idea of
> > VCPUOP_register_vcpu_time_memory_area, where the
> > force_update_vcpu_system_time() is called immediately when the new memory
> > area is registered.
> > 
> > Otherwise, we may observe clock drift at the VM side if the VM accesses
> > the clocksource immediately after VCPUOP_register_vcpu_info().
> > 
> > Reference:
> > https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00571.html
> > Cc: Joe Jin <joe.jin@oracle.com>
> > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Ian - any thoughts towards 4.16 here either way?

This looks like a bugfix to me, and the diff is certainly small.  I am
positively inclined.  I would like to know what the risks are.
Stefano says this does nothing on ARM so the risk would be to x86.
Can you advise ?

Thanks,
Ian.

Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info

[Jan Beulich]

On 02.11.2021 17:00, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info"):
>> On 25.10.2021 19:35, Dongli Zhang wrote:
>>> The guest may access the pv vcpu_time_info immediately after
>>> VCPUOP_register_vcpu_info. This is to borrow the idea of
>>> VCPUOP_register_vcpu_time_memory_area, where the
>>> force_update_vcpu_system_time() is called immediately when the new memory
>>> area is registered.
>>>
>>> Otherwise, we may observe clock drift at the VM side if the VM accesses
>>> the clocksource immediately after VCPUOP_register_vcpu_info().
>>>
>>> Reference:
>>> https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00571.html
>>> Cc: Joe Jin <joe.jin@oracle.com>
>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Ian - any thoughts towards 4.16 here either way?
> 
> This looks like a bugfix to me, and the diff is certainly small.  I am
> positively inclined.  I would like to know what the risks are.
> Stefano says this does nothing on ARM so the risk would be to x86.
> Can you advise ?

I don't see any noteworthy risks - a call to a function gets added
in a 2nd place; the function itself has been working fine for years,
and it is fine to be used in this new context.

Jan

Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2 1/1] xen: update system time immediately when VCPUOP_register_vcpu_info"):
> I don't see any noteworthy risks - a call to a function gets added
> in a 2nd place; the function itself has been working fine for years,
> and it is fine to be used in this new context.

Thanks.

I think this part:

| it is fine to be used in this new context.

was where I wanted a 2nd opinion for confirmation.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>