From: Jianyong Wu <Jianyong.Wu@arm.com>
To: Steven Price <Steven.Price@arm.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"yangbo.lu@nxp.com" <yangbo.lu@nxp.com>,
"john.stultz@linaro.org" <john.stultz@linaro.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"sean.j.christopherson@intel.com"
<sean.j.christopherson@intel.com>,
"maz@kernel.org" <maz@kernel.org>,
"richardcochran@gmail.com" <richardcochran@gmail.com>,
Mark Rutland <Mark.Rutland@arm.com>,
"will@kernel.org" <will@kernel.org>,
Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
"kvm@vger.kernel.org" <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.
Date: Mon, 25 May 2020 02:11:48 +0000 [thread overview]
Message-ID: <HE1PR0802MB2555A66F063927D5B855E1C6F4B30@HE1PR0802MB2555.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <87fce07b-d0f5-47b0-05ce-dd664ce53eec@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);
> > }
> >
next prev parent reply other threads:[~2020-05-25 2:12 UTC|newest]
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
2020-05-25 2:11 ` Jianyong Wu [this message]
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=HE1PR0802MB2555A66F063927D5B855E1C6F4B30@HE1PR0802MB2555.eurprd08.prod.outlook.com \
--to=jianyong.wu@arm.com \
--cc=Justin.He@arm.com \
--cc=Kaly.Xin@arm.com \
--cc=Mark.Rutland@arm.com \
--cc=Steve.Capper@arm.com \
--cc=Steven.Price@arm.com \
--cc=Suzuki.Poulose@arm.com \
--cc=Wei.Chen@arm.com \
--cc=john.stultz@linaro.org \
--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=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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).