Currently, we offen use ntp (sync time with remote network clock)
to sync time in VM. But the precision of ntp is subject to network delay
so it's difficult to sync time in a high precision.
kvm virtual ptp clock (ptp_kvm) offers another way to sync time in VM,
as the remote clock locates in the host instead of remote network clock.
It targets to sync time between guest and host in virtualization
environment and in this way, we can keep the time of all the VMs running
in the same host in sync. In general, the delay of communication between
host and guest is quiet small, so ptp_kvm can offer time sync precision
up to in order of nanosecond. Please keep in mind that ptp_kvm just
limits itself to be a channel which transmit the remote clock from
host to guest and leaves the time sync jobs to an application, eg. chrony,
in usersapce in VM.
How ptp_kvm works:
After ptp_kvm initialized, there will be a new device node under
/dev called ptp%d. A guest userspace service, like chrony, can use this
device to get host walltime, sometimes also counter cycle, which depends
on the service it calls. Then this guest userspace service can use those
data to do the time sync for guest.
here is a rough sketch to show how kvm ptp clock works.
|----------------------------| |--------------------------|
| guest userspace | | host |
|ioctl -> /dev/ptp%d | | |
| ^ | | | |
|----------------------------| | |
| | | guest kernel | | |
| | V (get host walltime/counter cycle) |
| ptp_kvm -> hypercall - - - - - - - - - - ->hypercall service |
| <- - - - - - - - - - - - |
|----------------------------| |--------------------------|
1. time sync service in guest userspace call ptp device through /dev/ptp%d.
2. ptp_kvm module in guest recive this request then invoke hypercall to route
into host kernel to request host walltime/counter cycle.
3. ptp_kvm hypercall service in host response to the request and send data back.
4. ptp (not ptp_kvm) in guest copy the data to userspace.
This ptp_kvm implementation focuses itself to step 2 and 3 and step 2 works
in guest comparing step 3 works in host kernel.
change log:
from v13 to v14
(1) rebase code on 5.9-rc3.
(2) add a document to introduce implementation of PTP_KVM on
arm64.
(3) fix comments issue in hypercall.c.
(4) export arm_smccc_1_1_get_conduit using EXPORT_SYMBOL_GPL.
(5) fix make issue on x86 reported by kernel test robot.
from v12 to v13:
(1) rebase code on 5.8-rc1.
(2) this patch set base on 2 patches of 1/8 and 2/8 from Will Decon.
(3) remove the change to ptp device code of extend getcrosststamp.
(4) remove the mechanism of letting user choose the counter type in
ptp_kvm for arm64.
(5) add virtual counter option in ptp_kvm service to let user choose
the specific counter explicitly.
from v11 to v12:
(1) rebase code on 5.7-rc6 and rebase 2 patches from Will Decon
including 1/11 and 2/11. as these patches introduce discover mechanism of
vendor smccc service.
(2) rebase ptp_kvm hypercall service from standard smccc to vendor
smccc and add ptp_kvm to vendor smccc service discover mechanism.
(3) add detail of why we need ptp_kvm and how ptp_kvm works in cover
letter.
from v10 to v11:
(1) rebase code on 5.7-rc2.
(2) remove support for arm32, as kvm support for arm32 will be
removed [1]
(3) add error report in ptp_kvm initialization.
from v9 to v10:
(1) change code base to v5.5.
(2) enable ptp_kvm both for arm32 and arm64.
(3) let user choose which of virtual counter or physical counter
should return when using crosstimestamp mode of ptp_kvm for arm/arm64.
(4) extend input argument for getcrosstimestamp API.
from v8 to v9:
(1) move ptp_kvm.h to driver/ptp/
(2) replace license declaration of ptp_kvm.h the same with other
header files in the same directory.
from v7 to v8:
(1) separate adding clocksource id for arm_arch_counter as a
single patch.
(2) update commit message for patch 4/8.
(3) refine patch 7/8 and patch 8/8 to make them more independent.
from v5 to v6:
(1) apply Mark's patch[4] to get SMCCC conduit.
(2) add mechanism to recognize current clocksource by add
clocksouce_id value into struct clocksource instead of method in patch-v5.
(3) rename kvm_arch_ptp_get_clock_fn into
kvm_arch_ptp_get_crosststamp.
from v4 to v5:
(1) remove hvc delay compensasion as it should leave to userspace.
(2) check current clocksource in hvc call service.
(3) expose current clocksource by adding it to
system_time_snapshot.
(4) add helper to check if clocksource is arm_arch_counter.
(5) rename kvm_ptp.c to ptp_kvm_common.c
from v3 to v4:
(1) fix clocksource of ptp_kvm to arch_sys_counter.
(2) move kvm_arch_ptp_get_clock_fn into arm_arch_timer.c
(3) subtract cntvoff before return cycles from host.
(4) use ktime_get_snapshot instead of getnstimeofday and
get_current_counterval to return time and counter value.
(5) split ktime and counter into two 32-bit block respectively
to avoid Y2038-safe issue.
(6) set time compensation to device time as half of the delay of
hvc call.
(7) add ARM_ARCH_TIMER as dependency of ptp_kvm for
arm64.
from v2 to v3:
(1) fix some issues in commit log.
(2) add some receivers in send list.
from v1 to v2:
(1) move arch-specific code from arch/ to driver/ptp/
(2) offer mechanism to inform userspace if ptp_kvm service is
available.
(3) separate ptp_kvm code for arm64 into hypervisor part and
guest part.
(4) add API to expose monotonic clock and counter value.
(5) refine code: remove no necessary part and reconsitution.
[1] https://patchwork.kernel.org/cover/11373351/
Jianyong Wu (9):
arm64: Probe for the presence of KVM hypervisor services during boot
smccc: Export smccc conduit get helper.
ptp: Reorganize ptp_kvm module to make it arch-independent.
time: Add mechanism to recognize clocksource in time_get_snapshot
clocksource: Add clocksource id for arm arch counter
arm64/kvm: Add hypercall service for kvm ptp.
ptp: arm64: Enable ptp_kvm for arm64
doc: add ptp_kvm introduction for arm64 support
arm64: Add kvm capability check extension for ptp_kvm
Will Deacon (1):
arm/arm64: KVM: Advertise KVM UID to guests via SMCCC
Documentation/virt/kvm/arm/ptp_kvm.rst | 72 +++++++++++++++++
arch/arm64/include/asm/hypervisor.h | 11 +++
arch/arm64/kernel/setup.c | 36 +++++++++
arch/arm64/kvm/Kconfig | 6 ++
arch/arm64/kvm/arch_timer.c | 2 +-
arch/arm64/kvm/arm.c | 4 +
arch/arm64/kvm/hypercalls.c | 78 +++++++++++++++---
drivers/clocksource/arm_arch_timer.c | 26 ++++++
drivers/firmware/smccc/smccc.c | 1 +
drivers/ptp/Kconfig | 2 +-
drivers/ptp/Makefile | 5 ++
drivers/ptp/ptp_kvm.h | 11 +++
drivers/ptp/ptp_kvm_arm64.c | 53 ++++++++++++
drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 80 +++++-------------
drivers/ptp/ptp_kvm_x86.c | 89 +++++++++++++++++++++
include/kvm/arm_arch_timer.h | 1 +
include/linux/arm-smccc.h | 42 ++++++++++
include/linux/clocksource.h | 6 ++
include/linux/clocksource_ids.h | 12 +++
include/linux/timekeeping.h | 12 +--
include/uapi/linux/kvm.h | 1 +
kernel/time/clocksource.c | 2 +
kernel/time/timekeeping.c | 1 +
23 files changed, 477 insertions(+), 76 deletions(-)
create mode 100644 Documentation/virt/kvm/arm/ptp_kvm.rst
create mode 100644 drivers/ptp/ptp_kvm.h
create mode 100644 drivers/ptp/ptp_kvm_arm64.c
rename drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} (63%)
create mode 100644 drivers/ptp/ptp_kvm_x86.c
create mode 100644 include/linux/clocksource_ids.h
--
2.17.1
Add clocksource id for arm arch counter to let it be identified easily and
elegantly in ptp_kvm implementation for arm.
Signed-off-by: Jianyong Wu <[email protected]>
---
drivers/clocksource/arm_arch_timer.c | 2 ++
include/linux/clocksource_ids.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 6c3e84180146..d55acffb0b90 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -16,6 +16,7 @@
#include <linux/cpu_pm.h>
#include <linux/clockchips.h>
#include <linux/clocksource.h>
+#include <linux/clocksource_ids.h>
#include <linux/interrupt.h>
#include <linux/of_irq.h>
#include <linux/of_address.h>
@@ -191,6 +192,7 @@ static u64 arch_counter_read_cc(const struct cyclecounter *cc)
static struct clocksource clocksource_counter = {
.name = "arch_sys_counter",
+ .id = CSID_ARM_ARCH_COUNTER,
.rating = 400,
.read = arch_counter_read,
.mask = CLOCKSOURCE_MASK(56),
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
index 4d8e19e05328..16775d7d8f8d 100644
--- a/include/linux/clocksource_ids.h
+++ b/include/linux/clocksource_ids.h
@@ -5,6 +5,7 @@
/* Enum to give clocksources a unique identifier */
enum clocksource_ids {
CSID_GENERIC = 0,
+ CSID_ARM_ARCH_COUNTER,
CSID_MAX,
};
--
2.17.1
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 <[email protected]>
---
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"
+ default y
+ help
+ virtual kvm ptp clock hypercall service for arm64
+
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
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.
+ */
+ 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
+ * call. If no invalid r1 value offered, default cycle
+ * value(-1) will return.
+ */
+ feature = smccc_get_arg1(vcpu);
+ switch (feature) {
+ case ARM_PTP_VIRT_COUNTER:
+ cycles = systime_snapshot.cycles -
+ vcpu_read_sys_reg(vcpu, CNTVOFF_EL2);
+ break;
+ case ARM_PTP_PHY_COUNTER:
+ cycles = systime_snapshot.cycles;
+ break;
+ }
+ val[1] = cycles;
+ 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
Let userspace check if there is kvm ptp service in host.
Before VMs migrate to another host, VMM may check if this
cap is available to determine the next behavior.
Signed-off-by: Jianyong Wu <[email protected]>
Suggested-by: Marc Zyngier <[email protected]>
---
arch/arm64/kvm/arm.c | 4 ++++
include/uapi/linux/kvm.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 691d21e4c717..8e99ad2f0b83 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -178,6 +178,10 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
case KVM_CAP_ARM_NISV_TO_USER:
case KVM_CAP_ARM_INJECT_EXT_DABT:
+
+#ifdef CONFIG_ARM64_KVM_PTP_HOST
+ case KVM_CAP_ARM_PTP_KVM:
+#endif
r = 1;
break;
case KVM_CAP_ARM_SET_DEVICE_ADDR:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index f6d86033c4fa..dd58ebe0daf5 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_LAST_CPU 184
#define KVM_CAP_SMALLER_MAXPHYADDR 185
#define KVM_CAP_S390_DIAG318 186
+#define KVM_CAP_ARM_PTP_KVM 187
#ifdef KVM_CAP_IRQ_ROUTING
--
2.17.1
From: Will Deacon <[email protected]>
Although the SMCCC specification provides some limited functionality for
describing the presence of hypervisor and firmware services, this is
generally applicable only to functions designated as "Arm Architecture
Service Functions" and no portable discovery mechanism is provided for
standard hypervisor services, despite having a designated range of
function identifiers reserved by the specification.
In an attempt to avoid the need for additional firmware changes every
time a new function is added, introduce a UID to identify the service
provider as being compatible with KVM. Once this has been established,
additional services can be discovered via a feature bitmap.
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
Signed-off-by: Jianyong Wu <[email protected]>
---
arch/arm64/include/asm/hypervisor.h | 11 +++++++++
arch/arm64/kernel/setup.c | 36 +++++++++++++++++++++++++++++
include/linux/arm-smccc.h | 26 +++++++++++++++++++++
3 files changed, 73 insertions(+)
diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
index f9cc1d021791..91e4bd890819 100644
--- a/arch/arm64/include/asm/hypervisor.h
+++ b/arch/arm64/include/asm/hypervisor.h
@@ -2,6 +2,17 @@
#ifndef _ASM_ARM64_HYPERVISOR_H
#define _ASM_ARM64_HYPERVISOR_H
+#include <linux/arm-smccc.h>
#include <asm/xen/hypervisor.h>
+static inline bool kvm_arm_hyp_service_available(u32 func_id)
+{
+ extern DECLARE_BITMAP(__kvm_arm_hyp_services, ARM_SMCCC_KVM_NUM_FUNCS);
+
+ if (func_id >= ARM_SMCCC_KVM_NUM_FUNCS)
+ return -EINVAL;
+
+ return test_bit(func_id, __kvm_arm_hyp_services);
+}
+
#endif
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 77c4c9bad1b8..cb4a18fe5ad4 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -7,6 +7,7 @@
*/
#include <linux/acpi.h>
+#include <linux/arm-smccc.h>
#include <linux/export.h>
#include <linux/kernel.h>
#include <linux/stddef.h>
@@ -276,6 +277,40 @@ arch_initcall(reserve_memblock_reserved_regions);
u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };
+DECLARE_BITMAP(__kvm_arm_hyp_services, ARM_SMCCC_KVM_NUM_FUNCS) = { };
+
+static void __init kvm_init_hyp_services(void)
+{
+ int i;
+ struct arm_smccc_res res;
+
+ if (arm_smccc_get_version() == ARM_SMCCC_VERSION_1_0)
+ return;
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
+ if (res.a0 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 ||
+ res.a1 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 ||
+ res.a2 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 ||
+ res.a3 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3)
+ return;
+
+ memset(&res, 0, sizeof(res));
+ arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID, &res);
+ for (i = 0; i < 32; ++i) {
+ if (res.a0 & (i))
+ set_bit(i + (32 * 0), __kvm_arm_hyp_services);
+ if (res.a1 & (i))
+ set_bit(i + (32 * 1), __kvm_arm_hyp_services);
+ if (res.a2 & (i))
+ set_bit(i + (32 * 2), __kvm_arm_hyp_services);
+ if (res.a3 & (i))
+ set_bit(i + (32 * 3), __kvm_arm_hyp_services);
+ }
+
+ pr_info("KVM hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
+ res.a3, res.a2, res.a1, res.a0);
+}
+
u64 cpu_logical_map(int cpu)
{
return __cpu_logical_map[cpu];
@@ -354,6 +389,7 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
else
psci_acpi_init();
+ kvm_init_hyp_services();
init_bootcpu_ops();
smp_init_cpus();
smp_build_mpidr_hash();
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 15c706fb0a37..f7b5dd7dbf9f 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -49,11 +49,14 @@
#define ARM_SMCCC_OWNER_OEM 3
#define ARM_SMCCC_OWNER_STANDARD 4
#define ARM_SMCCC_OWNER_STANDARD_HYP 5
+#define ARM_SMCCC_OWNER_VENDOR_HYP 6
#define ARM_SMCCC_OWNER_TRUSTED_APP 48
#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
#define ARM_SMCCC_OWNER_TRUSTED_OS 50
#define ARM_SMCCC_OWNER_TRUSTED_OS_END 63
+#define ARM_SMCCC_FUNC_QUERY_CALL_UID 0xff01
+
#define ARM_SMCCC_QUIRK_NONE 0
#define ARM_SMCCC_QUIRK_QCOM_A6 1 /* Save/restore register a6 */
@@ -86,6 +89,29 @@
ARM_SMCCC_SMC_32, \
0, 0x7fff)
+#define ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ ARM_SMCCC_OWNER_VENDOR_HYP, \
+ ARM_SMCCC_FUNC_QUERY_CALL_UID)
+
+/* KVM UID value: 28b46fb6-2ec5-11e9-a9ca-4b564d003a74 */
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 0xb66fb428U
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 0xe911c52eU
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 0x564bcaa9U
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3 0x743a004dU
+
+/* KVM "vendor specific" services */
+#define ARM_SMCCC_KVM_FUNC_FEATURES 0
+#define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
+#define ARM_SMCCC_KVM_NUM_FUNCS 128
+
+#define ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ ARM_SMCCC_OWNER_VENDOR_HYP, \
+ ARM_SMCCC_KVM_FUNC_FEATURES)
+
/* 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
Currently, there is no mechanism to keep time sync between guest and host
in arm64 virtualization environment. Time in guest will drift compared
with host after boot up as they may both use third party time sources
to correct their time respectively. The time deviation will be in order
of milliseconds. But in some scenarios,like in cloud envirenment, we ask
for higher time precision.
kvm ptp clock, which choose the host clock source as a reference
clock to sync time between guest and host, has been adopted by x86
which makes the time sync order from milliseconds to nanoseconds.
This patch enables kvm ptp clock for arm64 and improve clock sync precison
significantly.
Test result comparisons between with kvm ptp clock and without it in arm64
are as follows. This test derived from the result of command 'chronyc
sources'. we should take more care of the last sample column which shows
the offset between the local clock and the source at the last measurement.
no kvm ptp in guest:
MS Name/IP address Stratum Poll Reach LastRx Last sample
========================================================================
^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
in host:
MS Name/IP address Stratum Poll Reach LastRx Last sample
========================================================================
^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
The dns1.synet.edu.cn is the network reference clock for guest and
120.25.115.20 is the network reference clock for host. we can't get the
clock error between guest and host directly, but a roughly estimated value
will be in order of hundreds of us to ms.
with kvm ptp in guest:
chrony has been disabled in host to remove the disturb by network clock.
MS Name/IP address Stratum Poll Reach LastRx Last sample
========================================================================
* PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
* PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
* PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
* PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
* PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
* PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
* PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
* PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
* PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
* PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
The PHC0 is the ptp clock which choose the host clock as its source
clock. So we can see that the clock difference between host and guest
is in order of ns.
Signed-off-by: Jianyong Wu <[email protected]>
---
drivers/clocksource/arm_arch_timer.c | 24 +++++++++++++
drivers/ptp/Kconfig | 2 +-
drivers/ptp/ptp_kvm_arm64.c | 53 ++++++++++++++++++++++++++++
3 files changed, 78 insertions(+), 1 deletion(-)
create mode 100644 drivers/ptp/ptp_kvm_arm64.c
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index d55acffb0b90..aaf286e90092 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1650,3 +1650,27 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
}
TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
#endif
+
+#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
+#include <linux/arm-smccc.h>
+int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct timespec64 *ts,
+ struct clocksource **cs)
+{
+ struct arm_smccc_res hvc_res;
+ ktime_t ktime;
+
+ /* Currently, linux guest will always use the virtual counter */
+ arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+ ARM_PTP_VIRT_COUNTER, &hvc_res);
+ if ((long long)(hvc_res.a0) < 0)
+ return -EOPNOTSUPP;
+
+ ktime = (long long)hvc_res.a0;
+ *ts = ktime_to_timespec64(ktime);
+ *cycle = (long long)hvc_res.a1;
+ *cs = &clocksource_counter;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
+#endif
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 942f72d8151d..127e96f14f89 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -106,7 +106,7 @@ config PTP_1588_CLOCK_PCH
config PTP_1588_CLOCK_KVM
tristate "KVM virtual PTP clock"
depends on PTP_1588_CLOCK
- depends on KVM_GUEST && X86
+ depends on KVM_GUEST && X86 || ARM64 && ARM_ARCH_TIMER && ARM_PSCI_FW
default y
help
This driver adds support for using kvm infrastructure as a PTP
diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
new file mode 100644
index 000000000000..961abed93dfd
--- /dev/null
+++ b/drivers/ptp/ptp_kvm_arm64.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Virtual PTP 1588 clock for use with KVM guests
+ * Copyright (C) 2019 ARM Ltd.
+ * All Rights Reserved
+ */
+
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <asm/hypervisor.h>
+#include <linux/module.h>
+#include <linux/psci.h>
+#include <linux/arm-smccc.h>
+#include <linux/timecounter.h>
+#include <linux/sched/clock.h>
+#include <asm/arch_timer.h>
+
+int kvm_arch_ptp_init(void)
+{
+ struct arm_smccc_res hvc_res;
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID,
+ &hvc_res);
+ if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
+int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
+ struct arm_smccc_res *hvc_res)
+{
+ ktime_t ktime;
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+ hvc_res);
+ if ((long long)(hvc_res->a0) < 0)
+ return -EOPNOTSUPP;
+
+ ktime = (long long)hvc_res->a0;
+ *ts = ktime_to_timespec64(ktime);
+
+ return 0;
+}
+
+int kvm_arch_ptp_get_clock(struct timespec64 *ts)
+{
+ struct arm_smccc_res hvc_res;
+
+ kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
+
+ return 0;
+}
--
2.17.1
On Fri, 04 Sep 2020 10:27:41 +0100,
Jianyong Wu <[email protected]> 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 <[email protected]>
> ---
> 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.
On Fri, 04 Sep 2020 10:27:41 +0100,
Jianyong Wu <[email protected]> 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 <[email protected]>
> ---
> 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(-)
>
[...]
On top of what I said yesterday:
> 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;
>
Why has this become global? I can't see a reason why we would want to
expose this purely KVM internal helper.
M.
--
Without deviation from the norm, progress is not possible.
On Fri, 04 Sep 2020 10:27:42 +0100,
Jianyong Wu <[email protected]> wrote:
>
> Currently, there is no mechanism to keep time sync between guest and host
> in arm64 virtualization environment. Time in guest will drift compared
> with host after boot up as they may both use third party time sources
> to correct their time respectively. The time deviation will be in order
> of milliseconds. But in some scenarios,like in cloud envirenment, we ask
> for higher time precision.
>
> kvm ptp clock, which choose the host clock source as a reference
> clock to sync time between guest and host, has been adopted by x86
> which makes the time sync order from milliseconds to nanoseconds.
>
> This patch enables kvm ptp clock for arm64 and improve clock sync precison
> significantly.
>
> Test result comparisons between with kvm ptp clock and without it in arm64
> are as follows. This test derived from the result of command 'chronyc
> sources'. we should take more care of the last sample column which shows
> the offset between the local clock and the source at the last measurement.
>
> no kvm ptp in guest:
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
>
> in host:
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
>
> The dns1.synet.edu.cn is the network reference clock for guest and
> 120.25.115.20 is the network reference clock for host. we can't get the
> clock error between guest and host directly, but a roughly estimated value
> will be in order of hundreds of us to ms.
>
> with kvm ptp in guest:
> chrony has been disabled in host to remove the disturb by network clock.
>
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
>
> The PHC0 is the ptp clock which choose the host clock as its source
> clock. So we can see that the clock difference between host and guest
> is in order of ns.
>
> Signed-off-by: Jianyong Wu <[email protected]>
> ---
> drivers/clocksource/arm_arch_timer.c | 24 +++++++++++++
> drivers/ptp/Kconfig | 2 +-
> drivers/ptp/ptp_kvm_arm64.c | 53 ++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+), 1 deletion(-)
> create mode 100644 drivers/ptp/ptp_kvm_arm64.c
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index d55acffb0b90..aaf286e90092 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1650,3 +1650,27 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
> }
> TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
> #endif
> +
> +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> +#include <linux/arm-smccc.h>
> +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct timespec64 *ts,
> + struct clocksource **cs)
> +{
> + struct arm_smccc_res hvc_res;
> + ktime_t ktime;
> +
> + /* Currently, linux guest will always use the virtual counter */
> + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> + ARM_PTP_VIRT_COUNTER, &hvc_res);
> + if ((long long)(hvc_res.a0) < 0)
> + return -EOPNOTSUPP;
> +
> + ktime = (long long)hvc_res.a0;
> + *ts = ktime_to_timespec64(ktime);
> + *cycle = (long long)hvc_res.a1;
> + *cs = &clocksource_counter;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
> +#endif
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 942f72d8151d..127e96f14f89 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -106,7 +106,7 @@ config PTP_1588_CLOCK_PCH
> config PTP_1588_CLOCK_KVM
> tristate "KVM virtual PTP clock"
> depends on PTP_1588_CLOCK
> - depends on KVM_GUEST && X86
> + depends on KVM_GUEST && X86 || ARM64 && ARM_ARCH_TIMER && ARM_PSCI_FW
> default y
> help
> This driver adds support for using kvm infrastructure as a PTP
> diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
> new file mode 100644
> index 000000000000..961abed93dfd
> --- /dev/null
> +++ b/drivers/ptp/ptp_kvm_arm64.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Virtual PTP 1588 clock for use with KVM guests
> + * Copyright (C) 2019 ARM Ltd.
> + * All Rights Reserved
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <asm/hypervisor.h>
> +#include <linux/module.h>
> +#include <linux/psci.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/timecounter.h>
> +#include <linux/sched/clock.h>
> +#include <asm/arch_timer.h>
> +
> +int kvm_arch_ptp_init(void)
> +{
> + struct arm_smccc_res hvc_res;
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID,
> + &hvc_res);
> + if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
> + return -EOPNOTSUPP;
> +
> + return 0;
What happens if the ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID function
isn't implemented (on an old kernel or a non-KVM hypervisor)? The
expected behaviour is that a0 will contain SMCCC_RET_NOT_SUPPORTED,
which is -1. The result is that this function always returns
"supported". Not an acceptable behaviour.
> +}
> +
> +int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
> + struct arm_smccc_res *hvc_res)
Why isn't this static?
> +{
> + ktime_t ktime;
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> + hvc_res);
> + if ((long long)(hvc_res->a0) < 0)
> + return -EOPNOTSUPP;
Really? What if the cycle counter is a full 64 bit value, as it is
*mandated* on ARMv8.6? It means that the counter is now invalid for
half the lifetime of the system. Not acceptable either.
> +
> + ktime = (long long)hvc_res->a0;
> + *ts = ktime_to_timespec64(ktime);
> +
> + return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock(struct timespec64 *ts)
> +{
> + struct arm_smccc_res hvc_res;
> +
> + kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
> +
> + return 0;
> +}
> --
> 2.17.1
>
>
It is now obvious that the API between kernel and hypervisor is pretty
busted, and it goes beyond the 32bit support. I wish you paid more
attention to this kind of detail.
M.
--
Without deviation from the norm, progress is not possible.
On Fri, 04 Sep 2020 10:27:42 +0100,
Jianyong Wu <[email protected]> wrote:
>
> Currently, there is no mechanism to keep time sync between guest and host
> in arm64 virtualization environment. Time in guest will drift compared
> with host after boot up as they may both use third party time sources
> to correct their time respectively. The time deviation will be in order
> of milliseconds. But in some scenarios,like in cloud envirenment, we ask
> for higher time precision.
>
> kvm ptp clock, which choose the host clock source as a reference
> clock to sync time between guest and host, has been adopted by x86
> which makes the time sync order from milliseconds to nanoseconds.
>
> This patch enables kvm ptp clock for arm64 and improve clock sync precison
> significantly.
>
> Test result comparisons between with kvm ptp clock and without it in arm64
> are as follows. This test derived from the result of command 'chronyc
> sources'. we should take more care of the last sample column which shows
> the offset between the local clock and the source at the last measurement.
>
> no kvm ptp in guest:
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
>
> in host:
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
>
> The dns1.synet.edu.cn is the network reference clock for guest and
> 120.25.115.20 is the network reference clock for host. we can't get the
> clock error between guest and host directly, but a roughly estimated value
> will be in order of hundreds of us to ms.
>
> with kvm ptp in guest:
> chrony has been disabled in host to remove the disturb by network clock.
>
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
>
> The PHC0 is the ptp clock which choose the host clock as its source
> clock. So we can see that the clock difference between host and guest
> is in order of ns.
>
> Signed-off-by: Jianyong Wu <[email protected]>
> ---
> drivers/clocksource/arm_arch_timer.c | 24 +++++++++++++
> drivers/ptp/Kconfig | 2 +-
> drivers/ptp/ptp_kvm_arm64.c | 53 ++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+), 1 deletion(-)
> create mode 100644 drivers/ptp/ptp_kvm_arm64.c
And I missed that one earlier:
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index d55acffb0b90..aaf286e90092 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1650,3 +1650,27 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
> }
> TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
> #endif
> +
> +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> +#include <linux/arm-smccc.h>
No conditional includes, please.
> +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct timespec64 *ts,
> + struct clocksource **cs)
> +{
> + struct arm_smccc_res hvc_res;
> + ktime_t ktime;
> +
> + /* Currently, linux guest will always use the virtual counter */
> + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> + ARM_PTP_VIRT_COUNTER, &hvc_res);
You don't need to assume anything. This driver already provides you
with the information you need to tell the hypervisor which counter to
use:
if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)
ptp_counter = ARM_PTP_VIRT_COUNTER;
else
ptp_counter = ARM_PTP_PHYS_COUNTER;
arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
ptp_counter, &hvc_res);
At least, this is vaguely future proof.
The hypervisor will still have to discriminate between a call between
a call from vEL1 or vEL2 to decide whether to subtract the offset from
the counter value, but that's out of scope for now.
M.
--
Without deviation from the norm, progress is not possible.
On Sat, 05 Sep 2020 12:01:42 +0100,
Marc Zyngier <[email protected]> wrote:
>
> On Fri, 04 Sep 2020 10:27:42 +0100,
> Jianyong Wu <[email protected]> wrote:
[...]
> > +{
> > + ktime_t ktime;
> > +
> > + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> > + hvc_res);
> > + if ((long long)(hvc_res->a0) < 0)
> > + return -EOPNOTSUPP;
>
> Really? What if the cycle counter is a full 64 bit value, as it is
> *mandated* on ARMv8.6? It means that the counter is now invalid for
> half the lifetime of the system. Not acceptable either.
Having re-read this, this field doesn't contain the cycle counter, but
the time in ns. So checking for a negative value should actually be
fine for quite a while. My other comments still stand though.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Saturday, September 5, 2020 12:15 AM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Mark Rutland <[email protected]>;
> [email protected]; Suzuki Poulose <[email protected]>; Steven Price
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; Steve Capper <[email protected]>; Justin He
> <[email protected]>; nd <[email protected]>
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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.
Yeah,
>
> > + 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.
>
Ok, I will remove this option next time.
> > 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.
Ok, much better.
>
> > 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.
>
Sorry, should have changed according this time. But should keep this next time as
we really need use 32-bits value to support HVC32.
> > + */
> > + 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/
Thanks
>
> > + * call. If no invalid r1 value offered, default cycle
>
> nit: If r1 is an invalid value...
>
> > + * value(-1) will return.
>
> nit: will be returned.
>
Yeah.
> > + */
> > + 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.
Ok.
>
> > + 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.
Ok, much better.
>
> > + }
> > + 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?
>
Yeah, I will arm32 support back.
Thanks
Jianyong
> > + 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.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Saturday, September 5, 2020 7:04 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Mark Rutland <[email protected]>;
> [email protected]; Suzuki Poulose <[email protected]>; Steven Price
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; Steve Capper <[email protected]>; Justin He
> <[email protected]>; nd <[email protected]>
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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(-)
> >
>
> [...]
>
> On top of what I said yesterday:
>
> > 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;
> >
>
> Why has this become global? I can't see a reason why we would want to
> expose this purely KVM internal helper.
>
Sorry to have forgotten remove this change.
Thanks
Jianyong
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Saturday, September 5, 2020 7:02 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Mark Rutland <[email protected]>;
> [email protected]; Suzuki Poulose <[email protected]>; Steven Price
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; Steve Capper <[email protected]>; Justin He
> <[email protected]>; nd <[email protected]>
> Subject: Re: [PATCH v14 08/10] ptp: arm64: Enable ptp_kvm for arm64
>
> On Fri, 04 Sep 2020 10:27:42 +0100,
> Jianyong Wu <[email protected]> wrote:
> >
> > Currently, there is no mechanism to keep time sync between guest and
> > host in arm64 virtualization environment. Time in guest will drift
> > compared with host after boot up as they may both use third party time
> > sources to correct their time respectively. The time deviation will be
> > in order of milliseconds. But in some scenarios,like in cloud
> > envirenment, we ask for higher time precision.
> >
> > kvm ptp clock, which choose the host clock source as a reference clock
> > to sync time between guest and host, has been adopted by x86 which
> > makes the time sync order from milliseconds to nanoseconds.
> >
> > This patch enables kvm ptp clock for arm64 and improve clock sync
> > precison significantly.
> >
> > Test result comparisons between with kvm ptp clock and without it in
> > arm64 are as follows. This test derived from the result of command
> > 'chronyc sources'. we should take more care of the last sample column
> > which shows the offset between the local clock and the source at the last
> measurement.
> >
> > no kvm ptp in guest:
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
> >
> > in host:
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
> >
> > The dns1.synet.edu.cn is the network reference clock for guest and
> > 120.25.115.20 is the network reference clock for host. we can't get
> > the clock error between guest and host directly, but a roughly
> > estimated value will be in order of hundreds of us to ms.
> >
> > with kvm ptp in guest:
> > chrony has been disabled in host to remove the disturb by network clock.
> >
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
> >
> > The PHC0 is the ptp clock which choose the host clock as its source
> > clock. So we can see that the clock difference between host and guest
> > is in order of ns.
> >
> > Signed-off-by: Jianyong Wu <[email protected]>
> > ---
> > drivers/clocksource/arm_arch_timer.c | 24 +++++++++++++
> > drivers/ptp/Kconfig | 2 +-
> > drivers/ptp/ptp_kvm_arm64.c | 53
> ++++++++++++++++++++++++++++
> > 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644
> > drivers/ptp/ptp_kvm_arm64.c
> >
> > diff --git a/drivers/clocksource/arm_arch_timer.c
> > b/drivers/clocksource/arm_arch_timer.c
> > index d55acffb0b90..aaf286e90092 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -1650,3 +1650,27 @@ static int __init arch_timer_acpi_init(struct
> > acpi_table_header *table) } TIMER_ACPI_DECLARE(arch_timer,
> > ACPI_SIG_GTDT, arch_timer_acpi_init); #endif
> > +
> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> > +#include <linux/arm-smccc.h>
> > +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct
> timespec64 *ts,
> > + struct clocksource **cs)
> > +{
> > + struct arm_smccc_res hvc_res;
> > + ktime_t ktime;
> > +
> > + /* Currently, linux guest will always use the virtual counter */
> > +
> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > + ARM_PTP_VIRT_COUNTER, &hvc_res);
> > + if ((long long)(hvc_res.a0) < 0)
> > + return -EOPNOTSUPP;
> > +
> > + ktime = (long long)hvc_res.a0;
> > + *ts = ktime_to_timespec64(ktime);
> > + *cycle = (long long)hvc_res.a1;
> > + *cs = &clocksource_counter;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
> > +#endif
> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
> > 942f72d8151d..127e96f14f89 100644
> > --- a/drivers/ptp/Kconfig
> > +++ b/drivers/ptp/Kconfig
> > @@ -106,7 +106,7 @@ config PTP_1588_CLOCK_PCH config
> > PTP_1588_CLOCK_KVM
> > tristate "KVM virtual PTP clock"
> > depends on PTP_1588_CLOCK
> > - depends on KVM_GUEST && X86
> > + depends on KVM_GUEST && X86 || ARM64 && ARM_ARCH_TIMER
> &&
> > +ARM_PSCI_FW
> > default y
> > help
> > This driver adds support for using kvm infrastructure as a PTP
> > diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
> > new file mode 100644 index 000000000000..961abed93dfd
> > --- /dev/null
> > +++ b/drivers/ptp/ptp_kvm_arm64.c
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Virtual PTP 1588 clock for use with KVM guests
> > + * Copyright (C) 2019 ARM Ltd.
> > + * All Rights Reserved
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <asm/hypervisor.h>
> > +#include <linux/module.h>
> > +#include <linux/psci.h>
> > +#include <linux/arm-smccc.h>
> > +#include <linux/timecounter.h>
> > +#include <linux/sched/clock.h>
> > +#include <asm/arch_timer.h>
> > +
> > +int kvm_arch_ptp_init(void)
> > +{
> > + struct arm_smccc_res hvc_res;
> > +
> > +
> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATUR
> ES_FUNC_ID,
> > + &hvc_res);
> > + if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
> > + return -EOPNOTSUPP;
> > +
> > + return 0;
>
> What happens if the
> ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID function isn't
> implemented (on an old kernel or a non-KVM hypervisor)? The expected
> behaviour is that a0 will contain SMCCC_RET_NOT_SUPPORTED, which is -1.
> The result is that this function always returns "supported". Not an acceptable
> behaviour.
>
Oh! it's really a stupid mistake, should be "&" not "|".
> > +}
> > +
> > +int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
> > + struct arm_smccc_res *hvc_res)
>
> Why isn't this static?
>
yeah, should be static.
> > +{
> > + ktime_t ktime;
> > +
> > +
> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > + hvc_res);
> > + if ((long long)(hvc_res->a0) < 0)
> > + return -EOPNOTSUPP;
>
> Really? What if the cycle counter is a full 64 bit value, as it is
> *mandated* on ARMv8.6? It means that the counter is now invalid for half
> the lifetime of the system. Not acceptable either.
>
> > +
> > + ktime = (long long)hvc_res->a0;
> > + *ts = ktime_to_timespec64(ktime);
> > +
> > + return 0;
> > +}
> > +
> > +int kvm_arch_ptp_get_clock(struct timespec64 *ts) {
> > + struct arm_smccc_res hvc_res;
> > +
> > + kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
> > +
> > + return 0;
> > +}
> > --
> > 2.17.1
> >
> >
>
> It is now obvious that the API between kernel and hypervisor is pretty
> busted, and it goes beyond the 32bit support. I wish you paid more attention
> to this kind of detail.
Yeah, I will.
Thanks
Jianyong
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Saturday, September 5, 2020 7:33 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Mark Rutland <[email protected]>;
> [email protected]; Suzuki Poulose <[email protected]>; Steven Price
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; Steve Capper <[email protected]>; Justin He
> <[email protected]>; nd <[email protected]>
> Subject: Re: [PATCH v14 08/10] ptp: arm64: Enable ptp_kvm for arm64
>
> On Fri, 04 Sep 2020 10:27:42 +0100,
> Jianyong Wu <[email protected]> wrote:
> >
> > Currently, there is no mechanism to keep time sync between guest and
> > host in arm64 virtualization environment. Time in guest will drift
> > compared with host after boot up as they may both use third party time
> > sources to correct their time respectively. The time deviation will be
> > in order of milliseconds. But in some scenarios,like in cloud
> > envirenment, we ask for higher time precision.
> >
> > kvm ptp clock, which choose the host clock source as a reference clock
> > to sync time between guest and host, has been adopted by x86 which
> > makes the time sync order from milliseconds to nanoseconds.
> >
> > This patch enables kvm ptp clock for arm64 and improve clock sync
> > precison significantly.
> >
> > Test result comparisons between with kvm ptp clock and without it in
> > arm64 are as follows. This test derived from the result of command
> > 'chronyc sources'. we should take more care of the last sample column
> > which shows the offset between the local clock and the source at the last
> measurement.
> >
> > no kvm ptp in guest:
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
> >
> > in host:
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
> >
> > The dns1.synet.edu.cn is the network reference clock for guest and
> > 120.25.115.20 is the network reference clock for host. we can't get
> > the clock error between guest and host directly, but a roughly
> > estimated value will be in order of hundreds of us to ms.
> >
> > with kvm ptp in guest:
> > chrony has been disabled in host to remove the disturb by network clock.
> >
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
> >
> > The PHC0 is the ptp clock which choose the host clock as its source
> > clock. So we can see that the clock difference between host and guest
> > is in order of ns.
> >
> > Signed-off-by: Jianyong Wu <[email protected]>
> > ---
> > drivers/clocksource/arm_arch_timer.c | 24 +++++++++++++
> > drivers/ptp/Kconfig | 2 +-
> > drivers/ptp/ptp_kvm_arm64.c | 53
> ++++++++++++++++++++++++++++
> > 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644
> > drivers/ptp/ptp_kvm_arm64.c
>
> And I missed that one earlier:
>
> > diff --git a/drivers/clocksource/arm_arch_timer.c
> > b/drivers/clocksource/arm_arch_timer.c
> > index d55acffb0b90..aaf286e90092 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -1650,3 +1650,27 @@ static int __init arch_timer_acpi_init(struct
> > acpi_table_header *table) } TIMER_ACPI_DECLARE(arch_timer,
> > ACPI_SIG_GTDT, arch_timer_acpi_init); #endif
> > +
> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> > +#include <linux/arm-smccc.h>
>
> No conditional includes, please.
>
Ok.
> > +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct
> timespec64 *ts,
> > + struct clocksource **cs)
> > +{
> > + struct arm_smccc_res hvc_res;
> > + ktime_t ktime;
> > +
> > + /* Currently, linux guest will always use the virtual counter */
> > +
> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > + ARM_PTP_VIRT_COUNTER, &hvc_res);
>
> You don't need to assume anything. This driver already provides you with the
> information you need to tell the hypervisor which counter to
> use:
>
> if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)
> ptp_counter = ARM_PTP_VIRT_COUNTER;
> else
> ptp_counter = ARM_PTP_PHYS_COUNTER;
> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> ptp_counter, &hvc_res);
>
> At least, this is vaguely future proof.
>
> The hypervisor will still have to discriminate between a call between a call
> from vEL1 or vEL2 to decide whether to subtract the offset from the counter
> value, but that's out of scope for now.
Very kind of you!
Thanks
Jianyong
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On 2020-09-07 09:40, Jianyong Wu wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Zyngier <[email protected]>
>> Sent: Saturday, September 5, 2020 7:02 PM
>> To: Jianyong Wu <[email protected]>
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected];
>> [email protected]; Mark Rutland <[email protected]>;
>> [email protected]; Suzuki Poulose <[email protected]>; Steven Price
>> <[email protected]>; [email protected]; linux-arm-
>> [email protected]; [email protected];
>> [email protected]; Steve Capper <[email protected]>; Justin He
>> <[email protected]>; nd <[email protected]>
>> Subject: Re: [PATCH v14 08/10] ptp: arm64: Enable ptp_kvm for arm64
>>
>> On Fri, 04 Sep 2020 10:27:42 +0100,
>> Jianyong Wu <[email protected]> wrote:
>> >
>> > Currently, there is no mechanism to keep time sync between guest and
>> > host in arm64 virtualization environment. Time in guest will drift
>> > compared with host after boot up as they may both use third party time
>> > sources to correct their time respectively. The time deviation will be
>> > in order of milliseconds. But in some scenarios,like in cloud
>> > envirenment, we ask for higher time precision.
>> >
>> > kvm ptp clock, which choose the host clock source as a reference clock
>> > to sync time between guest and host, has been adopted by x86 which
>> > makes the time sync order from milliseconds to nanoseconds.
>> >
>> > This patch enables kvm ptp clock for arm64 and improve clock sync
>> > precison significantly.
>> >
>> > Test result comparisons between with kvm ptp clock and without it in
>> > arm64 are as follows. This test derived from the result of command
>> > 'chronyc sources'. we should take more care of the last sample column
>> > which shows the offset between the local clock and the source at the last
>> measurement.
>> >
>> > no kvm ptp in guest:
>> > MS Name/IP address Stratum Poll Reach LastRx Last sample
>> >
>> ==========================================================
>> ==============
>> > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
>> > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
>> >
>> > in host:
>> > MS Name/IP address Stratum Poll Reach LastRx Last sample
>> >
>> ==========================================================
>> ==============
>> > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
>> > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
>> > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
>> > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
>> > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
>> > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
>> > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
>> > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
>> > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
>> > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
>> >
>> > The dns1.synet.edu.cn is the network reference clock for guest and
>> > 120.25.115.20 is the network reference clock for host. we can't get
>> > the clock error between guest and host directly, but a roughly
>> > estimated value will be in order of hundreds of us to ms.
>> >
>> > with kvm ptp in guest:
>> > chrony has been disabled in host to remove the disturb by network clock.
>> >
>> > MS Name/IP address Stratum Poll Reach LastRx Last sample
>> >
>> ==========================================================
>> ==============
>> > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
>> > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
>> > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
>> > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
>> > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
>> > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
>> > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
>> > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
>> > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
>> > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
>> >
>> > The PHC0 is the ptp clock which choose the host clock as its source
>> > clock. So we can see that the clock difference between host and guest
>> > is in order of ns.
>> >
>> > Signed-off-by: Jianyong Wu <[email protected]>
>> > ---
>> > drivers/clocksource/arm_arch_timer.c | 24 +++++++++++++
>> > drivers/ptp/Kconfig | 2 +-
>> > drivers/ptp/ptp_kvm_arm64.c | 53
>> ++++++++++++++++++++++++++++
>> > 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644
>> > drivers/ptp/ptp_kvm_arm64.c
>> >
>> > diff --git a/drivers/clocksource/arm_arch_timer.c
>> > b/drivers/clocksource/arm_arch_timer.c
>> > index d55acffb0b90..aaf286e90092 100644
>> > --- a/drivers/clocksource/arm_arch_timer.c
>> > +++ b/drivers/clocksource/arm_arch_timer.c
>> > @@ -1650,3 +1650,27 @@ static int __init arch_timer_acpi_init(struct
>> > acpi_table_header *table) } TIMER_ACPI_DECLARE(arch_timer,
>> > ACPI_SIG_GTDT, arch_timer_acpi_init); #endif
>> > +
>> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
>> > +#include <linux/arm-smccc.h>
>> > +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct
>> timespec64 *ts,
>> > + struct clocksource **cs)
>> > +{
>> > + struct arm_smccc_res hvc_res;
>> > + ktime_t ktime;
>> > +
>> > + /* Currently, linux guest will always use the virtual counter */
>> > +
>> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
>> NC_ID,
>> > + ARM_PTP_VIRT_COUNTER, &hvc_res);
>> > + if ((long long)(hvc_res.a0) < 0)
>> > + return -EOPNOTSUPP;
>> > +
>> > + ktime = (long long)hvc_res.a0;
>> > + *ts = ktime_to_timespec64(ktime);
>> > + *cycle = (long long)hvc_res.a1;
>> > + *cs = &clocksource_counter;
>> > +
>> > + return 0;
>> > +}
>> > +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
>> > +#endif
>> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
>> > 942f72d8151d..127e96f14f89 100644
>> > --- a/drivers/ptp/Kconfig
>> > +++ b/drivers/ptp/Kconfig
>> > @@ -106,7 +106,7 @@ config PTP_1588_CLOCK_PCH config
>> > PTP_1588_CLOCK_KVM
>> > tristate "KVM virtual PTP clock"
>> > depends on PTP_1588_CLOCK
>> > - depends on KVM_GUEST && X86
>> > + depends on KVM_GUEST && X86 || ARM64 && ARM_ARCH_TIMER
>> &&
>> > +ARM_PSCI_FW
>> > default y
>> > help
>> > This driver adds support for using kvm infrastructure as a PTP
>> > diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
>> > new file mode 100644 index 000000000000..961abed93dfd
>> > --- /dev/null
>> > +++ b/drivers/ptp/ptp_kvm_arm64.c
>> > @@ -0,0 +1,53 @@
>> > +// SPDX-License-Identifier: GPL-2.0-only
>> > +/*
>> > + * Virtual PTP 1588 clock for use with KVM guests
>> > + * Copyright (C) 2019 ARM Ltd.
>> > + * All Rights Reserved
>> > + */
>> > +
>> > +#include <linux/kernel.h>
>> > +#include <linux/err.h>
>> > +#include <asm/hypervisor.h>
>> > +#include <linux/module.h>
>> > +#include <linux/psci.h>
>> > +#include <linux/arm-smccc.h>
>> > +#include <linux/timecounter.h>
>> > +#include <linux/sched/clock.h>
>> > +#include <asm/arch_timer.h>
>> > +
>> > +int kvm_arch_ptp_init(void)
>> > +{
>> > + struct arm_smccc_res hvc_res;
>> > +
>> > +
>> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATUR
>> ES_FUNC_ID,
>> > + &hvc_res);
>> > + if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
>> > + return -EOPNOTSUPP;
>> > +
>> > + return 0;
>>
>> What happens if the
>> ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID function isn't
>> implemented (on an old kernel or a non-KVM hypervisor)? The expected
>> behaviour is that a0 will contain SMCCC_RET_NOT_SUPPORTED, which is
>> -1.
>> The result is that this function always returns "supported". Not an
>> acceptable
>> behaviour.
>>
> Oh! it's really a stupid mistake, should be "&" not "|".
But even then. (-1 & whatever) is always true.
M.
--
Jazz is not dead. It just smells funny...
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Monday, September 7, 2020 4:55 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Mark Rutland <[email protected]>;
> [email protected]; Suzuki Poulose <[email protected]>; Steven Price
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; Steve Capper <[email protected]>; Justin He
> <[email protected]>; nd <[email protected]>
> Subject: Re: [PATCH v14 08/10] ptp: arm64: Enable ptp_kvm for arm64
>
> On 2020-09-07 09:40, Jianyong Wu wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <[email protected]>
> >> Sent: Saturday, September 5, 2020 7:02 PM
> >> To: Jianyong Wu <[email protected]>
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; Mark
> >> Rutland <[email protected]>; [email protected]; Suzuki Poulose
> >> <[email protected]>; Steven Price <[email protected]>;
> >> [email protected]; linux-arm- [email protected];
> >> [email protected]; [email protected]; Steve Capper
> >> <[email protected]>; Justin He <[email protected]>; nd
> >> <[email protected]>
> >> Subject: Re: [PATCH v14 08/10] ptp: arm64: Enable ptp_kvm for arm64
> >>
> >> On Fri, 04 Sep 2020 10:27:42 +0100,
> >> Jianyong Wu <[email protected]> wrote:
> >> >
> >> > Currently, there is no mechanism to keep time sync between guest
> >> > and host in arm64 virtualization environment. Time in guest will
> >> > drift compared with host after boot up as they may both use third
> >> > party time sources to correct their time respectively. The time
> >> > deviation will be in order of milliseconds. But in some
> >> > scenarios,like in cloud envirenment, we ask for higher time precision.
> >> >
> >> > kvm ptp clock, which choose the host clock source as a reference
> >> > clock to sync time between guest and host, has been adopted by x86
> >> > which makes the time sync order from milliseconds to nanoseconds.
> >> >
> >> > This patch enables kvm ptp clock for arm64 and improve clock sync
> >> > precison significantly.
> >> >
> >> > Test result comparisons between with kvm ptp clock and without it
> >> > in
> >> > arm64 are as follows. This test derived from the result of command
> >> > 'chronyc sources'. we should take more care of the last sample
> >> > column which shows the offset between the local clock and the
> >> > source at the last
> >> measurement.
> >> >
> >> > no kvm ptp in guest:
> >> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >> >
> >>
> ==========================================================
> >> ==============
> >> > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> >> > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
> >> >
> >> > in host:
> >> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >> >
> >>
> ==========================================================
> >> ==============
> >> > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> >> > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> >> > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> >> > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> >> > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> >> > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> >> > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> >> > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> >> > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> >> > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
> >> >
> >> > The dns1.synet.edu.cn is the network reference clock for guest and
> >> > 120.25.115.20 is the network reference clock for host. we can't get
> >> > the clock error between guest and host directly, but a roughly
> >> > estimated value will be in order of hundreds of us to ms.
> >> >
> >> > with kvm ptp in guest:
> >> > chrony has been disabled in host to remove the disturb by network
> clock.
> >> >
> >> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >> >
> >>
> ==========================================================
> >> ==============
> >> > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> >> > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> >> > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> >> > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> >> > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> >> > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> >> > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> >> > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> >> > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> >> > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
> >> >
> >> > The PHC0 is the ptp clock which choose the host clock as its source
> >> > clock. So we can see that the clock difference between host and
> >> > guest is in order of ns.
> >> >
> >> > Signed-off-by: Jianyong Wu <[email protected]>
> >> > ---
> >> > drivers/clocksource/arm_arch_timer.c | 24 +++++++++++++
> >> > drivers/ptp/Kconfig | 2 +-
> >> > drivers/ptp/ptp_kvm_arm64.c | 53
> >> ++++++++++++++++++++++++++++
> >> > 3 files changed, 78 insertions(+), 1 deletion(-) create mode
> >> > 100644 drivers/ptp/ptp_kvm_arm64.c
> >> >
> >> > diff --git a/drivers/clocksource/arm_arch_timer.c
> >> > b/drivers/clocksource/arm_arch_timer.c
> >> > index d55acffb0b90..aaf286e90092 100644
> >> > --- a/drivers/clocksource/arm_arch_timer.c
> >> > +++ b/drivers/clocksource/arm_arch_timer.c
> >> > @@ -1650,3 +1650,27 @@ static int __init
> >> > arch_timer_acpi_init(struct acpi_table_header *table) }
> >> > TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT,
> >> > arch_timer_acpi_init); #endif
> >> > +
> >> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> >> > +#include <linux/arm-smccc.h>
> >> > +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct
> >> timespec64 *ts,
> >> > + struct clocksource **cs)
> >> > +{
> >> > + struct arm_smccc_res hvc_res;
> >> > + ktime_t ktime;
> >> > +
> >> > + /* Currently, linux guest will always use the virtual counter */
> >> > +
> >> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> >> NC_ID,
> >> > + ARM_PTP_VIRT_COUNTER, &hvc_res);
> >> > + if ((long long)(hvc_res.a0) < 0)
> >> > + return -EOPNOTSUPP;
> >> > +
> >> > + ktime = (long long)hvc_res.a0;
> >> > + *ts = ktime_to_timespec64(ktime);
> >> > + *cycle = (long long)hvc_res.a1;
> >> > + *cs = &clocksource_counter;
> >> > +
> >> > + return 0;
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
> >> > +#endif
> >> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
> >> > 942f72d8151d..127e96f14f89 100644
> >> > --- a/drivers/ptp/Kconfig
> >> > +++ b/drivers/ptp/Kconfig
> >> > @@ -106,7 +106,7 @@ config PTP_1588_CLOCK_PCH config
> >> > PTP_1588_CLOCK_KVM
> >> > tristate "KVM virtual PTP clock"
> >> > depends on PTP_1588_CLOCK
> >> > - depends on KVM_GUEST && X86
> >> > + depends on KVM_GUEST && X86 || ARM64 && ARM_ARCH_TIMER
> >> &&
> >> > +ARM_PSCI_FW
> >> > default y
> >> > help
> >> > This driver adds support for using kvm infrastructure as a PTP
> >> > diff --git a/drivers/ptp/ptp_kvm_arm64.c
> >> > b/drivers/ptp/ptp_kvm_arm64.c new file mode 100644 index
> >> > 000000000000..961abed93dfd
> >> > --- /dev/null
> >> > +++ b/drivers/ptp/ptp_kvm_arm64.c
> >> > @@ -0,0 +1,53 @@
> >> > +// SPDX-License-Identifier: GPL-2.0-only
> >> > +/*
> >> > + * Virtual PTP 1588 clock for use with KVM guests
> >> > + * Copyright (C) 2019 ARM Ltd.
> >> > + * All Rights Reserved
> >> > + */
> >> > +
> >> > +#include <linux/kernel.h>
> >> > +#include <linux/err.h>
> >> > +#include <asm/hypervisor.h>
> >> > +#include <linux/module.h>
> >> > +#include <linux/psci.h>
> >> > +#include <linux/arm-smccc.h>
> >> > +#include <linux/timecounter.h>
> >> > +#include <linux/sched/clock.h>
> >> > +#include <asm/arch_timer.h>
> >> > +
> >> > +int kvm_arch_ptp_init(void)
> >> > +{
> >> > + struct arm_smccc_res hvc_res;
> >> > +
> >> > +
> >> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATUR
> >> ES_FUNC_ID,
> >> > + &hvc_res);
> >> > + if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
> >> > + return -EOPNOTSUPP;
> >> > +
> >> > + return 0;
> >>
> >> What happens if the
> >> ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID function isn't
> implemented
> >> (on an old kernel or a non-KVM hypervisor)? The expected behaviour is
> >> that a0 will contain SMCCC_RET_NOT_SUPPORTED, which is -1.
> >> The result is that this function always returns "supported". Not an
> >> acceptable behaviour.
> >>
> > Oh! it's really a stupid mistake, should be "&" not "|".
>
> But even then. (-1 & whatever) is always true.
Yeah, what about checking if a0 is non-negative first? Like:
if (hvc_res.a0 < 0 || !(hvc_res.a0 & BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
return -EOPNOTSUPP;
Thanks
Jianyong
>
> M.
> --
> Jazz is not dead. It just smells funny...
On 2020-09-07 10:28, Jianyong Wu wrote:
>> -----Original Message-----
>> From: Marc Zyngier <[email protected]>
>> Sent: Monday, September 7, 2020 4:55 PM
>> To: Jianyong Wu <[email protected]>
[...]
>> >> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATUR
>> >> ES_FUNC_ID,
>> >> > + &hvc_res);
>> >> > + if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
>> >> > + return -EOPNOTSUPP;
>> >> > +
>> >> > + return 0;
>> >>
>> >> What happens if the
>> >> ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID function isn't
>> implemented
>> >> (on an old kernel or a non-KVM hypervisor)? The expected behaviour is
>> >> that a0 will contain SMCCC_RET_NOT_SUPPORTED, which is -1.
>> >> The result is that this function always returns "supported". Not an
>> >> acceptable behaviour.
>> >>
>> > Oh! it's really a stupid mistake, should be "&" not "|".
>>
>> But even then. (-1 & whatever) is always true.
>
> Yeah, what about checking if a0 is non-negative first? Like:
> if (hvc_res.a0 < 0 || !(hvc_res.a0 & BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
> return -EOPNOTSUPP;
I don't get it. You already carry a patch from Will that gives
you a way to check for a service (kvm_arm_hyp_service_available()).
Why do you need to reinvent the wheel?
M.
--
Jazz is not dead. It just smells funny...
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Monday, September 7, 2020 5:47 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Mark Rutland <[email protected]>;
> [email protected]; Suzuki Poulose <[email protected]>; Steven Price
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; Steve Capper <[email protected]>; Justin He
> <[email protected]>; nd <[email protected]>
> Subject: Re: [PATCH v14 08/10] ptp: arm64: Enable ptp_kvm for arm64
>
> On 2020-09-07 10:28, Jianyong Wu wrote:
> >> -----Original Message-----
> >> From: Marc Zyngier <[email protected]>
> >> Sent: Monday, September 7, 2020 4:55 PM
> >> To: Jianyong Wu <[email protected]>
>
> [...]
>
> >> >> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATUR
> >> >> ES_FUNC_ID,
> >> >> > + &hvc_res);
> >> >> > + if (!(hvc_res.a0 | BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
> >> >> > + return -EOPNOTSUPP;
> >> >> > +
> >> >> > + return 0;
> >> >>
> >> >> What happens if the
> >> >> ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID function isn't
> >> implemented
> >> >> (on an old kernel or a non-KVM hypervisor)? The expected behaviour
> >> >> is that a0 will contain SMCCC_RET_NOT_SUPPORTED, which is -1.
> >> >> The result is that this function always returns "supported". Not
> >> >> an acceptable behaviour.
> >> >>
> >> > Oh! it's really a stupid mistake, should be "&" not "|".
> >>
> >> But even then. (-1 & whatever) is always true.
> >
> > Yeah, what about checking if a0 is non-negative first? Like:
> > if (hvc_res.a0 < 0 || !(hvc_res.a0 &
> BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP)))
> > return -EOPNOTSUPP;
>
> I don't get it. You already carry a patch from Will that gives you a way to check
> for a service (kvm_arm_hyp_service_available()).
>
> Why do you need to reinvent the wheel?
Sorry, I should have changed this code according to Will's patch. Thanks for reminder!
Thanks
jianyong
>
> M.
> --
> Jazz is not dead. It just smells funny...