KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Price <steven.price@arm.com>
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@arm.com, will@kernel.org,
	suzuki.poulose@arm.com
Cc: justin.he@arm.com, Wei.Chen@arm.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, nd@arm.com,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH v12 07/11] psci: Add hypercall service for kvm ptp.
Date: Fri, 22 May 2020 15:18:22 +0100
Message-ID: <87fce07b-d0f5-47b0-05ce-dd664ce53eec@arm.com> (raw)
In-Reply-To: <20200522083724.38182-8-jianyong.wu@arm.com>

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.

Second the use of "default:" means that there's no possibility to later 
extend this interface for more clocks if needed in the future.

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.

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.

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);
>   	}
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22  8:37 [RFC PATCH v12 0/11] Enable ptp_kvm for arm64 Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 01/11] arm64: Probe for the presence of KVM hypervisor services during boot Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 02/11] arm/arm64: KVM: Advertise KVM UID to guests via SMCCC Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 03/11] psci: export smccc conduit get helper Jianyong Wu
2020-05-22 13:12   ` Sudeep Holla
2020-05-25  1:37     ` Jianyong Wu
2020-05-26 10:10       ` Sudeep Holla
2020-05-27  1:18         ` Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 04/11] ptp: Reorganize ptp_kvm modules to make it arch-independent Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 05/11] time: Add mechanism to recognize clocksource in time_get_snapshot Jianyong Wu
2020-05-28 16:36   ` Thomas Gleixner
2020-05-29  1:05     ` Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 06/11] clocksource: Add clocksource id for arm arch counter Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 07/11] psci: Add hypercall service for kvm ptp Jianyong Wu
2020-05-22 14:18   ` Steven Price [this message]
2020-05-25  2:11     ` Jianyong Wu
2020-05-26 11:02       ` Steven Price
2020-05-27  6:06         ` Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 08/11] ptp: arm64: Enable ptp_kvm for arm/arm64 Jianyong Wu
2020-05-22  8:37 ` [RFC PATCH v12 09/11] ptp: extend input argument for getcrosstimestamp API Jianyong Wu
2020-05-24  1:42   ` Richard Cochran
2020-05-22  8:37 ` [RFC PATCH v12 10/11] arm64: add mechanism to let user choose which counter to return Jianyong Wu
2020-05-24  1:47   ` Richard Cochran
2020-05-24  2:11   ` Richard Cochran
2020-05-25  4:50     ` Jianyong Wu
2020-05-25  6:16       ` Richard Cochran
2020-05-25  6:29         ` Jianyong Wu
2020-05-25  9:17     ` Marc Zyngier
2020-05-25 14:18       ` Jianyong Wu
2020-05-25 15:28         ` Marc Zyngier
2020-05-22  8:37 ` [RFC PATCH v12 11/11] arm64: Add kvm capability check extension for ptp_kvm Jianyong Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87fce07b-d0f5-47b0-05ce-dd664ce53eec@arm.com \
    --to=steven.price@arm.com \
    --cc=Mark.Rutland@arm.com \
    --cc=Wei.Chen@arm.com \
    --cc=jianyong.wu@arm.com \
    --cc=john.stultz@linaro.org \
    --cc=justin.he@arm.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=nd@arm.com \
    --cc=netdev@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=richardcochran@gmail.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=suzuki.poulose@arm.com \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    --cc=yangbo.lu@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git