Re: [PATCH v14 07/10] arm64/kvm: Add hypercall service for kvm ptp.

[Marc Zyngier <maz@kernel.org>]

On Fri, 04 Sep 2020 10:27:41 +0100,
Jianyong Wu <jianyong.wu@arm.com> wrote:
> 
> ptp_kvm will get this service through smccc call.
> The service offers wall time and counter cycle of host for guest.
> caller must explicitly determines which cycle of virtual counter or
> physical counter to return if it needs counter cycle.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  arch/arm64/kvm/Kconfig       |  6 +++++
>  arch/arm64/kvm/arch_timer.c  |  2 +-
>  arch/arm64/kvm/hypercalls.c  | 49 ++++++++++++++++++++++++++++++++++++
>  include/kvm/arm_arch_timer.h |  1 +
>  include/linux/arm-smccc.h    | 16 ++++++++++++
>  5 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 318c8f2df245..bbdfacec4813 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -60,6 +60,12 @@ config KVM_ARM_PMU
>  config KVM_INDIRECT_VECTORS
>  	def_bool HARDEN_BRANCH_PREDICTOR || RANDOMIZE_BASE
>  
> +config ARM64_KVM_PTP_HOST
> +	bool "KVM PTP clock host service for arm64"

The "for arm64" is not that useful.

> +	default y
> +	help
> +	  virtual kvm ptp clock hypercall service for arm64
> +

I'm not keen on making this a compile option, because whatever is not
always on ends up bit-rotting. Please drop the option.

>  endif # KVM
>  
>  endif # VIRTUALIZATION
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 32ba6fbc3814..eb85f6701845 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -81,7 +81,7 @@ u64 timer_get_cval(struct arch_timer_context *ctxt)
>  	}
>  }
>  
> -static u64 timer_get_offset(struct arch_timer_context *ctxt)
> +u64 timer_get_offset(struct arch_timer_context *ctxt)
>  {
>  	struct kvm_vcpu *vcpu = ctxt->vcpu;
>  
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 901c60f119c2..2628ddc13abd 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/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 = -1;
> +#endif

Please move all the PTP-related code to its own function, rather than
keeping it in the main HVC dispatcher. Also assigning a negative value
to something that is unsigned hurts my eyes. Consider using ~0UL instead.
See the comment below though.

>  	u32 func_id = smccc_get_function(vcpu);
>  	u64 val[4] = {SMCCC_RET_NOT_SUPPORTED};
>  	u32 feature;
> @@ -21,6 +26,10 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  		val[0] = ARM_SMCCC_VERSION_1_1;
>  		break;
>  	case ARM_SMCCC_ARCH_FEATURES_FUNC_ID:
> +		/*
> +		 * Note: keep in mind that feature is u32 and smccc_get_arg1
> +		 * will return u64, so need auto cast here.
> +		 */
>  		feature = smccc_get_arg1(vcpu);
>  		switch (feature) {
>  		case ARM_SMCCC_ARCH_WORKAROUND_1:
> @@ -70,7 +79,47 @@ 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.

This comment doesn't match what I read below.

> +	 */
> +	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] = systime_snapshot.real;
> +		/*
> +		 * which of virtual counter or physical counter being
> +		 * asked for is decided by the r1 value of smccc

nit: s/smccc/SMCCC/

> +		 * call. If no invalid r1 value offered, default cycle

nit: If r1 is an invalid value...

> +		 * value(-1) will return.

nit: will be returned.

> +		 */
> +		feature = smccc_get_arg1(vcpu);
> +		switch (feature) {
> +		case ARM_PTP_VIRT_COUNTER:
> +			cycles = systime_snapshot.cycles -
> +				 vcpu_read_sys_reg(vcpu, CNTVOFF_EL2);

nit: On a single line, please.

> +			break;
> +		case ARM_PTP_PHY_COUNTER:
> +			cycles = systime_snapshot.cycles;
> +			break;

It'd be a lot clearer if you had a default: case here, handling the
invalid case.

> +		}
> +		val[1] = cycles;

Given that cycles is a 64bit value, how does it work for a 32bit
guest? Or have you removed support for 32bit guests altogether?

> +		break;
> +#endif
>  	default:
>  		return kvm_psci_call(vcpu);
>  	}
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 51c19381108c..5a2b6da9be7a 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -105,5 +105,6 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
>  /* Needed for tracing */
>  u32 timer_get_ctl(struct arch_timer_context *ctxt);
>  u64 timer_get_cval(struct arch_timer_context *ctxt);
> +u64 timer_get_offset(struct arch_timer_context *ctxt);
>  
>  #endif
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index f7b5dd7dbf9f..0724840eb5f7 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -103,6 +103,7 @@
>  
>  /* KVM "vendor specific" services */
>  #define ARM_SMCCC_KVM_FUNC_FEATURES		0
> +#define ARM_SMCCC_KVM_FUNC_KVM_PTP		1
>  #define ARM_SMCCC_KVM_FUNC_FEATURES_2		127
>  #define ARM_SMCCC_KVM_NUM_FUNCS			128
>  
> @@ -112,6 +113,21 @@
>  			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
>  			   ARM_SMCCC_KVM_FUNC_FEATURES)
>  
> +/*
> + * ptp_kvm is a feature used for time sync between vm and host.
> + * ptp_kvm module in guest kernel will get service from host using
> + * this hypercall ID.
> + */
> +#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)
> +
> +/* ptp_kvm counter type ID */
> +#define ARM_PTP_VIRT_COUNTER			0
> +#define ARM_PTP_PHY_COUNTER			1
> +
>  /* Paravirtualised time calls (defined by ARM DEN0057A) */
>  #define ARM_SMCCC_HV_PV_TIME_FEATURES				\
>  	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
> -- 
> 2.17.1
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.