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

From: Jianyong Wu
Date: Sun May 24 2020 - 22:12:16 EST

Hi Steven,

> -----Original Message-----
> From: Steven Price <steven.price@xxxxxxx>
> Sent: Friday, May 22, 2020 10:18 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> yangbo.lu@xxxxxxx; john.stultz@xxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> pbonzini@xxxxxxxxxx; sean.j.christopherson@xxxxxxxxx; maz@xxxxxxxxxx;
> richardcochran@xxxxxxxxx; Mark Rutland <Mark.Rutland@xxxxxxx>;
> will@xxxxxxxxxx; Suzuki Poulose <Suzuki.Poulose@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> kvmarm@xxxxxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Steve Capper
> <Steve.Capper@xxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; Justin He
> <Justin.He@xxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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);
> > }
> >