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

[Dongli Zhang <dongli.zhang@xxxxxxxxxx>]

Hi Jan,

On 10/12/21 8:49 AM, Jan Beulich wrote:
> 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.
> 

I do not see pvclock used by arm/arm64 in linux kernel for xen.

In addition, the implementation at xen hypervisor side is empty.

348 /* VCPU PV clock. */
349 void update_vcpu_system_time(struct vcpu *v)
350 {
351     /* XXX update shared_info->wc_* */
352 }

I will add a wrapper for it.

The bad thing is I see riscv is supported by xen and we may need to add the
function for riscv as well.

Thank you very much!

Dongli Zhang