On 2020/4/21 5:57 PM, Mark Rutland wrote:
Hi Mark,
> On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote:
>> ptp_kvm modules will get this service through smccc call.
>> The service offers real time and counter cycle of host for guest.
>> Also let caller determine which cycle of virtual counter or physical counter
>> to return.
>>
>> Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
>> ---
>> include/linux/arm-smccc.h | 21 +++++++++++++++++++
>> virt/kvm/arm/hypercalls.c | 44 ++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>> index 59494df0f55b..747b7595d0c6 100644
>> --- a/include/linux/arm-smccc.h
>> +++ b/include/linux/arm-smccc.h
>> @@ -77,6 +77,27 @@
>> ARM_SMCCC_SMC_32, \
>> 0, 0x7fff)
>>
>> +/* PTP KVM call requests clock time from guest OS to host */
>> +#define ARM_SMCCC_HYP_KVM_PTP_FUNC_ID \
>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
>> + ARM_SMCCC_SMC_32, \
>> + ARM_SMCCC_OWNER_STANDARD_HYP, \
>> + 0)
>> +
>> +/* request for virtual counter from ptp_kvm guest */
>> +#define ARM_SMCCC_HYP_KVM_PTP_VIRT \
>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
>> + ARM_SMCCC_SMC_32, \
>> + ARM_SMCCC_OWNER_STANDARD_HYP, \
>> + 1)
>> +
>> +/* request for physical counter from ptp_kvm guest */
>> +#define ARM_SMCCC_HYP_KVM_PTP_PHY \
>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
>> + ARM_SMCCC_SMC_32, \
>> + ARM_SMCCC_OWNER_STANDARD_HYP, \
>> + 2)
> ARM_SMCCC_OWNER_STANDARD_HYP is for standard calls as defined in SMCCC
> and companion documents, so we should refer to the specific
> documentation here. Where are these calls defined?
yeah, should add reference docs of "SMC CALLING CONVENTION" here.
> If these calls are Linux-specific then ARM_SMCCC_OWNER_STANDARD_HYP
> isn't appropriate to use, as they are vendor-specific hypervisor service
> call.
yeah, vendor-specific service is more suitable for ptp_kvm.
>
> It looks like we don't currently have a ARM_SMCCC_OWNER_HYP for that
> (which IIUC would be 6), but we can add one as necessary. I think that
> Will might have added that as part of his SMCCC probing bits.
ok, I will add a new "ARM_SMCCC_OWNER_VENDOR_HYP" whose IIUC is 6
and create "ARM_SMCCC_HYP_KVM_PTP_ID" base on it.
>
>> +
>> #ifndef __ASSEMBLY__
>>
>> #include <linux/linkage.h>
>> 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
>> @@ -3,6 +3,7 @@
>>
>> #include <linux/arm-smccc.h>
>> #include <linux/kvm_host.h>
>> +#include <linux/clocksource_ids.h>
>>
>> #include <asm/kvm_emulate.h>
>>
>> @@ -11,8 +12,11 @@
>>
>> int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>> {
>> - u32 func_id = smccc_get_function(vcpu);
>> + struct system_time_snapshot systime_snapshot;
>> + long arg[4];
>> + u64 cycles;
>> long val = SMCCC_RET_NOT_SUPPORTED;
>> + u32 func_id = smccc_get_function(vcpu);
>> u32 feature;
>> gpa_t gpa;
>>
>> @@ -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.
>> + /*
>> + * 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.
>> + /*
>> + * which of virtual counter or physical counter being
>> + * asked for is decided by the first argument.
>> + */
>> + feature = smccc_get_arg1(vcpu);
>> + switch (feature) {
>> + case ARM_SMCCC_HYP_KVM_PTP_PHY:
>> + cycles = systime_snapshot.cycles;
>> + break;
>> + case ARM_SMCCC_HYP_KVM_PTP_VIRT:
>> + default:
>> + cycles = systime_snapshot.cycles -
>> + vcpu_vtimer(vcpu)->cntvoff;
>> + }
>> + arg[2] = upper_32_bits(cycles);
>> + arg[3] = lower_32_bits(cycles);
>> +
>> + smccc_set_retval(vcpu, arg[0], arg[1], arg[2], arg[3]);
> I think the 'arg' buffer is confusing here, and it'd be clearer to have:
>
> u64 snaphot;
> u64 cycles;
>
> ... and here do:
>
> smccc_set_retval(vcpu,
> upper_32_bits(snaphot),
> lower_32_bits(snapshot),
> upper_32_bits(cycles),
> lower_32_bits(cycles));
it's better to use a meaningful variant name. I will fix it.
thanks
Jianyong
>
> Thanks,
> Mark.
