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

[Jianyong Wu]

Hi Steven,

> -----Original Message-----
> From: Steven Price <steven.price@xxxxxxx>
> Sent: Tuesday, May 26, 2020 7:02 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 25/05/2020 03:11, Jianyong Wu wrote:
> > Hi Steven,
> 
> Hi Jianyong,
> 
> [...]>>> 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?
> 
> Personally I'd go with something much shorter, e.g.
> ARM_PTP_PHY_COUNTER.
> This is just an argument to an SMCCC call so there's no need for most of the
> prefix, indeed if (for whatever reason) there was a non-SMCCC mechanism
> added to do the same thing it would be reasonable to reuse the same values.
> 
Ok ,  this shorter name is better.

> >> 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.
> 
> The problem with the 'default' is it means it's not possible to probe whether
> the kernel supports any more clocks. If we used a different value (that the
> kernel doesn't support) then we end up in the default case and have no idea
> whether the clock value is the one we requested or not.
> 
Yeah,  it's more meaningful. Should return the exact value back to user.

> It's generally better when defining an ABI to explicitly return an error for
> unknown parameters, that way a future user of the ABI can discover
> whether the call did what was expected or not.
> 

ok. I will fix it.

> >> 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:
> > ...
> > "
> 
> I'm happy either way - it's purely that the definition/naming of
> ARM_SMCCC_VENDOR_HYP_KVM_PTP_PHY_FUNC_ID made it look like that
> was the intention. My preference would be to stick with the 'feature'
> approach as above because there's no need to "use up" the top-level SMCCC
> calls (but equally there's a large space so we'd have to work very hard to run
> out... ;) )
>
We can change the name of "_PHY_COUNTER", but it will remain in the same level with "_FUNC_ID" as
It will still occupy a place reserved for VENDOR SMCCC call.
Just like ARM_SMCCC_ARCH_WORKAROUND_1,

#define ARM_SMCCC_ARCH_WORKAROUND_1                                     \
        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                         \
                           ARM_SMCCC_SMC_32,                            \
                           0, 0x8000)

 It will be a ARCH SMCCC call id from the view of its definition.

> >> 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, I think it's a good idea to get the ABI nailed down before worrying
> too much about the code, and it's easier to discuss based on documentation
> rather than code.
>
Yeah, a document here is in favor of code review.
 
Thanks
Jianyong 

> Thanks,
> 
> Steve