| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87eenhr01m.wl-maz@kernel.org>
Date: Fri, 04 Sep 2020 17:15:01 +0100
From: Marc Zyngier <maz@...nel.org>
To: Jianyong Wu <jianyong.wu@....com>
Cc: netdev@...r.kernel.org, yangbo.lu@....com, john.stultz@...aro.org,
tglx@...utronix.de, pbonzini@...hat.com,
sean.j.christopherson@...el.com, richardcochran@...il.com,
Mark.Rutland@....com, will@...nel.org, suzuki.poulose@....com,
steven.price@....com, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
kvm@...r.kernel.org, Steve.Capper@....com, justin.he@....com,
nd@....com
Subject: Re: [PATCH v14 07/10] arm64/kvm: Add hypercall service for kvm ptp.
On Fri, 04 Sep 2020 10:27:41 +0100,
Jianyong Wu <jianyong.wu@....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@....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.
Powered by blists - more mailing lists