Re: [RFC PATCH v11 5/9] psci: Add hypercall service for ptp_kvm.

[Jianyong Wu <Jianyong.Wu@arm.com>]

On 2020/4/24 6:39 PM, Mark Rutland wrote:

it's a resend of the last email, please ignore if you have received this.

Hi Mark,

thank you for remainder, I hope this email is in the correct format.

> On Fri, Apr 24, 2020 at 03:50:22AM +0100, Jianyong Wu wrote:
>> On 2020/4/21 5:57 PM, Mark Rutland wrote:
>>> On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote:
>>>> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
>>>> index 550dfa3e53cd..a5309c28d4dc 100644
>>>> --- a/virt/kvm/arm/hypercalls.c
>>>> +++ b/virt/kvm/arm/hypercalls.c
>>>> @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>>>>    if (gpa != GPA_INVALID)
>>>>    val = gpa;
>>>>    break;
>>>> +/*
>>>> + * This serves virtual kvm_ptp.
>>>> + * Four values will be passed back.
>>>> + * reg0 stores high 32-bit host ktime;
>>>> + * reg1 stores low 32-bit host ktime;
>>>> + * reg2 stores high 32-bit difference of host cycles and cntvoff;
>>>> + * reg3 stores low 32-bit difference of host cycles and cntvoff.
>>>> + */
>>>> +case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID:
>>> Shouldn't the host opt-in to providing this to the guest, as with other
>>> features?
>> er, do you mean that "ARM_SMCCC_HV_PV_TIME_XXX" as "opt-in"? if so, I
>> think this
>>
>> kvm_ptp doesn't need a buddy. the driver in guest will call this service
>> in a definite way.
> I mean that when creating the VM, userspace should be able to choose
> whether the PTP service is provided to the guest. The host shouldn't
> always provide it as there may be cases where doing so is undesireable.
>
I think I have implemented in patch 9/9 that userspace can get the info 
that if the host offers the kvm_ptp service. But for now, the host 
kernel will always offer the kvm_ptp capability in the current 
implementation. I think x86 follow the same behavior (see [1]). so I 
have not considered when and how to disable this kvm_ptp service in 
host. Do you think we should offer this opt-in?

[1] kvm_pv_clock_pairing() in 
https://github.com/torvalds/linux/blob/master/arch/x86/kvm/x86.c

>>>> +/*
>>>> + * system time and counter value must captured in the same
>>>> + * time to keep consistency and precision.
>>>> + */
>>>> +ktime_get_snapshot(&systime_snapshot);
>>>> +if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER)
>>>> +break;
>>>> +arg[0] = upper_32_bits(systime_snapshot.real);
>>>> +arg[1] = lower_32_bits(systime_snapshot.real);
>>> Why exactly does the guest need the host's real time? Neither the cover
>>> letter nor this commit message have explained that, and for those of us
>>> unfamliar with PTP it would be very helpful to know that to understand
>>> what's going on.
>> oh, sorry, I should have added more background knowledge here.
>>
>> just give some hints here:
>>
>> the kvm_ptp targets to sync guest time with host. some services in user
>> space
>>
>> like chrony can do time sync by inputing time(in kvm_ptp also clock
>> counter sometimes) from
>>
>> remote clocksource(often network clocksource). This kvm_ptp driver can
>> offer a interface for
>>
>> those user space service in guest to get the host time to do time sync
>> in guest.
> I think it would be very helpful for the commit message and/or cover
> letter to have a high-level desctiption of what PTP is meant to do, and
> an outline of the algorithmm (clearly splitting the host and guest
> bits).

ok, I will add high-level principle of kvm_ptp in commit message.

> It's also not clear to me what notion of host time is being exposed to
> the guest (and consequently how this would interact with time changes on
> the host, time namespaces, etc). Having some description of that would
> be very helpful.

sorry to have not made it clear.

Time will not change in host and only time in guest will change to sync 
with host. host time is the target that time in guest want to adjust to. 
guest need to get the host time then compute the different of the time 
in guest and host, so the guest can adjust the time base on the difference.

I will add the base principle of time sync service in guest using 
kvm_ptp in commit message.


Thanks

Jianyong

> Thanks,
> Mark.


-->