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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.10.2021 18:15, Dongli Zhang wrote:
> 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.

There's not really any code for RISC-V yet, so that'll be of concern to
those who are working on adding actual code later on. I'm actually
wondering about the status of that effort - after the initial bits were
added over 3 months ago, no further activity has been visible.

Jan