RE: [RFC PATCH v12 07/11] psci: Add hypercall service for kvm ptp.

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

Hi Steven,

> -----Original Message-----
> From: Steven Price <steven.price@arm.com>
> Sent: Friday, May 22, 2020 10:18 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; netdev@vger.kernel.org;
> yangbo.lu@nxp.com; john.stultz@linaro.org; tglx@linutronix.de;
> pbonzini@redhat.com; sean.j.christopherson@intel.com; maz@kernel.org;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> <Justin.He@arm.com>; Wei Chen <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v12 07/11] psci: Add hypercall service for kvm ptp.
> 
> On 22/05/2020 09:37, 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 | 14 ++++++++++++
> >   virt/kvm/Kconfig          |  4 ++++
> >   virt/kvm/arm/hypercalls.c | 47
> +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 65 insertions(+)
> >
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index bdc0124a064a..badadc390809 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -94,6 +94,8 @@
> >
> >   /* KVM "vendor specific" services */
> >   #define ARM_SMCCC_KVM_FUNC_FEATURES		0
> > +#define ARM_SMCCC_KVM_FUNC_KVM_PTP		1
> > +#define ARM_SMCCC_KVM_FUNC_KVM_PTP_PHY		2
> >   #define ARM_SMCCC_KVM_FUNC_FEATURES_2		127
> >   #define ARM_SMCCC_KVM_NUM_FUNCS			128
> >
> > @@ -103,6 +105,18 @@
> >   			   ARM_SMCCC_OWNER_VENDOR_HYP,
> 		\
> >   			   ARM_SMCCC_KVM_FUNC_FEATURES)
> >
> > +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID
> 		\
> > +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
> 		\
> > +			   ARM_SMCCC_SMC_32,
> 	\
> > +			   ARM_SMCCC_OWNER_VENDOR_HYP,
> 		\
> > +			   ARM_SMCCC_KVM_FUNC_KVM_PTP)
> > +
> > +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_PHY_FUNC_ID
> 		\
> > +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
> 		\
> > +			   ARM_SMCCC_SMC_32,
> 	\
> > +			   ARM_SMCCC_OWNER_VENDOR_HYP,
> 		\
> > +			   ARM_SMCCC_KVM_FUNC_KVM_PTP_PHY)
> > +
> >   #ifndef __ASSEMBLY__
> >
> >   #include <linux/linkage.h>
> > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig index
> > aad9284c043a..bf820811e815 100644
> > --- a/virt/kvm/Kconfig
> > +++ b/virt/kvm/Kconfig
> > @@ -60,3 +60,7 @@ config HAVE_KVM_VCPU_RUN_PID_CHANGE
> >
> >   config HAVE_KVM_NO_POLL
> >          bool
> > +
> > +config ARM64_KVM_PTP_HOST
> > +       def_bool y
> > +       depends on ARM64 && KVM
> > diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
> > index db6dce3d0e23..c964122f8dae 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,6 +12,10 @@
> >
> >   int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >   {
> > +#ifdef CONFIG_ARM64_KVM_PTP_HOST
> > +	struct system_time_snapshot systime_snapshot;
> > +	u64 cycles;
> > +#endif
> >   	u32 func_id = smccc_get_function(vcpu);
> >   	u32 val[4] = {SMCCC_RET_NOT_SUPPORTED};
> >   	u32 feature;
> > @@ -70,7 +75,49 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >   		break;
> >   	case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> >   		val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> > +
> > +#ifdef CONFIG_ARM64_KVM_PTP_HOST
> > +		val[0] |= BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP); #endif
> >   		break;
> > +
> > +#ifdef CONFIG_ARM64_KVM_PTP_HOST
> > +	/*
> > +	 * 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_VENDOR_HYP_KVM_PTP_FUNC_ID:
> > +		/*
> > +		 * 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;
> > +		val[0] = upper_32_bits(systime_snapshot.real);
> > +		val[1] = lower_32_bits(systime_snapshot.real);
> > +		/*
> > +		 * 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_VENDOR_HYP_KVM_PTP_PHY_FUNC_ID:
> > +			cycles = systime_snapshot.cycles;
> > +			break;
> > +		default:
> 
> There's something a bit odd here.
> 
> ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID and
> ARM_SMCCC_VENDOR_HYP_KVM_PTP_PHY_FUNC_ID look like they should
> be names of separate (top-level) functions, but actually the _PHY_ one is a
> parameter for the first. If the intention is to have a parameter then it would
> be better to pick a better name for the _PHY_ define and not define it using
> ARM_SMCCC_CALL_VAL.
> 
Yeah, _PHY_ is not the same meaning with _PTP_FUNC_ID,  so I think it should be a different name.
What about ARM_SMCCC_VENDOR_HYP_KVM_PTP_PHY_COUNTER?

> Second the use of "default:" means that there's no possibility to later extend
> this interface for more clocks if needed in the future.
> 
I think we can add more clocks by adding more cases, this "default" means we can use no first arg to determine the default clock.

> Alternatively you could indeed implement as two top-level functions and
> change this to a...
> 
> 	switch (func_id)
> 
> ... along with multiple case labels as the functions would obviously be mostly
> the same.
> 
> Also a minor style issue - you might want to consider splitting this into it's
> own function.
> 
I think "switch (feature)" maybe better as this _PHY_ is not like a function id. Just like:
"
case ARM_SMCCC_ARCH_FEATURES_FUNC_ID:
                feature = smccc_get_arg1(vcpu);
                switch (feature) {
                case ARM_SMCCC_ARCH_WORKAROUND_1:
...
"
> Finally I do think it would be useful to add some documentation of the new
> SMC calls. It would be easier to review the interface based on that
> documentation rather than trying to reverse-engineer the interface from the
> code.
> 
Yeah, more doc needed here.

Thanks
Jianyong 

> Steve
> 
> > +			cycles = systime_snapshot.cycles -
> > +				 vcpu_vtimer(vcpu)->cntvoff;
> > +		}
> > +		val[2] = upper_32_bits(cycles);
> > +		val[3] = lower_32_bits(cycles);
> > +		break;
> > +#endif
> > +
> >   	default:
> >   		return kvm_psci_call(vcpu);
> >   	}
> >