lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 25 May 2020 02:11:48 +0000 From: Jianyong Wu <Jianyong.Wu@....com> To: Steven Price <Steven.Price@....com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "yangbo.lu@....com" <yangbo.lu@....com>, "john.stultz@...aro.org" <john.stultz@...aro.org>, "tglx@...utronix.de" <tglx@...utronix.de>, "pbonzini@...hat.com" <pbonzini@...hat.com>, "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>, "maz@...nel.org" <maz@...nel.org>, "richardcochran@...il.com" <richardcochran@...il.com>, Mark Rutland <Mark.Rutland@....com>, "will@...nel.org" <will@...nel.org>, Suzuki Poulose <Suzuki.Poulose@....com> CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, Steve Capper <Steve.Capper@....com>, Kaly Xin <Kaly.Xin@....com>, Justin He <Justin.He@....com>, Wei Chen <Wei.Chen@....com>, nd <nd@....com> Subject: RE: [RFC PATCH v12 07/11] psci: Add hypercall service for kvm ptp. Hi Steven, > -----Original Message----- > From: Steven Price <steven.price@....com> > Sent: Friday, May 22, 2020 10:18 PM > To: Jianyong Wu <Jianyong.Wu@....com>; netdev@...r.kernel.org; > yangbo.lu@....com; john.stultz@...aro.org; tglx@...utronix.de; > pbonzini@...hat.com; sean.j.christopherson@...el.com; maz@...nel.org; > richardcochran@...il.com; Mark Rutland <Mark.Rutland@....com>; > will@...nel.org; Suzuki Poulose <Suzuki.Poulose@....com> > Cc: linux-kernel@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; > kvmarm@...ts.cs.columbia.edu; kvm@...r.kernel.org; Steve Capper > <Steve.Capper@....com>; Kaly Xin <Kaly.Xin@....com>; Justin He > <Justin.He@....com>; Wei Chen <Wei.Chen@....com>; nd <nd@....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@....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); > > } > >
Powered by blists - more mailing lists