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

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

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@arm.com>
>> ---
>>   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.