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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.10.2021 17:43, Dongli Zhang wrote:
> Hi Jan,
> 
> On 10/12/21 1:40 AM, Jan Beulich wrote:
>> On 12.10.2021 09:24, 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().
>>>
>>> Cc: Joe Jin <joe.jin@xxxxxxxxxx>
>>> Signed-off-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
>>
>> While I agree with the change in principle, ...
>>
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -1695,6 +1695,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);
>>
>> ... I'm afraid you're breaking the Arm build here. Arm will first need
>> to gain this function.
>>
> 
> Since I am not familiar with the Xen ARM, would you please let me your
> suggestion if I just leave ARM as TODO to the ARM developers to verify
> and implement force_update_vcpu_system_time()?

I'd much prefer to avoid this. I don't think the function can be that
difficult to introduce. And I'm sure the Arm maintainers will apply
extra care during review if you point out that you weren't able to
actually test this.

Jan

> I have tested that the below can build with arm64/aarch64.
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 40d67ec342..644c65ecd3 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1695,6 +1695,13 @@ 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);
> 
> +#ifdef CONFIG_X86
> +        /*
> +         * TODO: ARM does not have force_update_vcpu_system_time().
> +         */
> +        force_update_vcpu_system_time(v);
> +#endif
> +
>          break;
>      }
> 
> 
> 
> Thank you very much!
> 
> Dongli Zhang
>