[PATCH v4 0/3] x86/xen: pvclock vdso support

[PATCH v4 0/3] x86/xen: pvclock vdso support

[Joao Martins]

Hey,

This is take 4 for vdso for Xen. PVCLOCK_TSC_STABLE_BIT can be set starting Xen
 4.8 which is required for vdso time related calls. In order to have it on, you
need to have the hypervisor clocksource be TSC e.g. with the following boot
params "clocksource=tsc tsc=stable:socket".

Series is structured as following:

Patch 1 streamlines pvti page get/set in pvclock for both of its users
Patch 2 registers the pvti page on Xen and sets it in pvclock accordingly
Patch 3 adds a file to KVM/Xen maintainers for tracking pvclock ABI changes.

Changelog is included in individual patches.
(only patch 2 changed in this version)

Thanks,
Joao

Joao Martins (3):
  x86/pvclock: add setter for pvclock_pvti_cpu0_va
  x86/xen/time: setup vcpu 0 time info page
  MAINTAINERS: xen, kvm: track pvclock-abi.h changes

 MAINTAINERS                    |   2 +
 arch/x86/include/asm/pvclock.h |  19 ++++----
 arch/x86/kernel/kvmclock.c     |   7 +--
 arch/x86/kernel/pvclock.c      |  14 ++++++
 arch/x86/xen/suspend.c         |   4 ++
 arch/x86/xen/time.c            | 100 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/xen-ops.h         |   2 +
 include/xen/interface/vcpu.h   |  42 +++++++++++++++++
 8 files changed, 175 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH v4 1/3] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Joao Martins]

Right now there is only a pvclock_pvti_cpu0_va() which is defined
on kvmclock since:

commit dac16fba6fc5
("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap")

The only user of this interface so far is kvm. This commit adds a
setter function for the pvti page and moves pvclock_pvti_cpu0_va
to pvclock, which is a more generic place to have it; and would
allow other PV clocksources to use it, such as Xen.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Acked-by: Andy Lutomirski <luto@kernel.org>
---
Changes since v1:
 * Rebased: the only conflict was that I had move the export
 pvclock_pvti_cpu0_va() symbol as it is used by kvm PTP driver.
 * Do not initialize pvti_cpu0_va to NULL (checkpatch error)
 ( Comments from Andy Lutomirski )
 * Removed asm/pvclock.h 'pvclock_set_pvti_cpu0_va' definition
 for non !PARAVIRT_CLOCK to better track screwed Kconfig stuff.
 * Add his Acked-by (provided the previous adjustment was made)

Changes since RFC:
 (Comments from Andy Lutomirski)
 * Add __init to pvclock_set_pvti_cpu0_va
 * Add WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)) to
 pvclock_set_pvti_cpu0_va
---
 arch/x86/include/asm/pvclock.h | 19 ++++++++++---------
 arch/x86/kernel/kvmclock.c     |  7 +------
 arch/x86/kernel/pvclock.c      | 14 ++++++++++++++
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 448cfe1b48cf..6f228f90cdd7 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -4,15 +4,6 @@
 #include <linux/clocksource.h>
 #include <asm/pvclock-abi.h>
 
-#ifdef CONFIG_KVM_GUEST
-extern struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void);
-#else
-static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
-{
-	return NULL;
-}
-#endif
-
 /* some helper functions for xen and kvm pv clock sources */
 u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src);
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src);
@@ -101,4 +92,14 @@ struct pvclock_vsyscall_time_info {
 
 #define PVTI_SIZE sizeof(struct pvclock_vsyscall_time_info)
 
+#ifdef CONFIG_PARAVIRT_CLOCK
+void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti);
+struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void);
+#else
+static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
+{
+	return NULL;
+}
+#endif
+
 #endif /* _ASM_X86_PVCLOCK_H */
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index d88967659098..538738047ff5 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -47,12 +47,6 @@ early_param("no-kvmclock", parse_no_kvmclock);
 static struct pvclock_vsyscall_time_info *hv_clock;
 static struct pvclock_wall_clock wall_clock;
 
-struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
-{
-	return hv_clock;
-}
-EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va);
-
 /*
  * The wallclock is the time of day when we booted. Since then, some time may
  * have elapsed since the hypervisor wrote the data. So we try to account for
@@ -334,6 +328,7 @@ int __init kvm_setup_vsyscall_timeinfo(void)
 		return 1;
 	}
 
+	pvclock_set_pvti_cpu0_va(hv_clock);
 	put_cpu();
 
 	kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 5c3f6d6a5078..cb7d6d9c9c2d 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -25,8 +25,10 @@
 
 #include <asm/fixmap.h>
 #include <asm/pvclock.h>
+#include <asm/vgtod.h>
 
 static u8 valid_flags __read_mostly = 0;
+static struct pvclock_vsyscall_time_info *pvti_cpu0_va __read_mostly;
 
 void pvclock_set_flags(u8 flags)
 {
@@ -144,3 +146,15 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall_clock,
 
 	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
 }
+
+void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti)
+{
+	WARN_ON(vclock_was_used(VCLOCK_PVCLOCK));
+	pvti_cpu0_va = pvti;
+}
+
+struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
+{
+	return pvti_cpu0_va;
+}
+EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va);
-- 
2.11.0

[PATCH v4 1/3] x86/pvclock: add setter for pvclock_pvti_cpu0_va

[Joao Martins]

Right now there is only a pvclock_pvti_cpu0_va() which is defined
on kvmclock since:

commit dac16fba6fc5
("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap")

The only user of this interface so far is kvm. This commit adds a
setter function for the pvti page and moves pvclock_pvti_cpu0_va
to pvclock, which is a more generic place to have it; and would
allow other PV clocksources to use it, such as Xen.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Acked-by: Andy Lutomirski <luto@kernel.org>
---
Changes since v1:
 * Rebased: the only conflict was that I had move the export
 pvclock_pvti_cpu0_va() symbol as it is used by kvm PTP driver.
 * Do not initialize pvti_cpu0_va to NULL (checkpatch error)
 ( Comments from Andy Lutomirski )
 * Removed asm/pvclock.h 'pvclock_set_pvti_cpu0_va' definition
 for non !PARAVIRT_CLOCK to better track screwed Kconfig stuff.
 * Add his Acked-by (provided the previous adjustment was made)

Changes since RFC:
 (Comments from Andy Lutomirski)
 * Add __init to pvclock_set_pvti_cpu0_va
 * Add WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)) to
 pvclock_set_pvti_cpu0_va
---
 arch/x86/include/asm/pvclock.h | 19 ++++++++++---------
 arch/x86/kernel/kvmclock.c     |  7 +------
 arch/x86/kernel/pvclock.c      | 14 ++++++++++++++
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 448cfe1b48cf..6f228f90cdd7 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -4,15 +4,6 @@
 #include <linux/clocksource.h>
 #include <asm/pvclock-abi.h>
 
-#ifdef CONFIG_KVM_GUEST
-extern struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void);
-#else
-static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
-{
-	return NULL;
-}
-#endif
-
 /* some helper functions for xen and kvm pv clock sources */
 u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src);
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src);
@@ -101,4 +92,14 @@ struct pvclock_vsyscall_time_info {
 
 #define PVTI_SIZE sizeof(struct pvclock_vsyscall_time_info)
 
+#ifdef CONFIG_PARAVIRT_CLOCK
+void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti);
+struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void);
+#else
+static inline struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
+{
+	return NULL;
+}
+#endif
+
 #endif /* _ASM_X86_PVCLOCK_H */
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index d88967659098..538738047ff5 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -47,12 +47,6 @@ early_param("no-kvmclock", parse_no_kvmclock);
 static struct pvclock_vsyscall_time_info *hv_clock;
 static struct pvclock_wall_clock wall_clock;
 
-struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
-{
-	return hv_clock;
-}
-EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va);
-
 /*
  * The wallclock is the time of day when we booted. Since then, some time may
  * have elapsed since the hypervisor wrote the data. So we try to account for
@@ -334,6 +328,7 @@ int __init kvm_setup_vsyscall_timeinfo(void)
 		return 1;
 	}
 
+	pvclock_set_pvti_cpu0_va(hv_clock);
 	put_cpu();
 
 	kvm_clock.archdata.vclock_mode = VCLOCK_PVCLOCK;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 5c3f6d6a5078..cb7d6d9c9c2d 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -25,8 +25,10 @@
 
 #include <asm/fixmap.h>
 #include <asm/pvclock.h>
+#include <asm/vgtod.h>
 
 static u8 valid_flags __read_mostly = 0;
+static struct pvclock_vsyscall_time_info *pvti_cpu0_va __read_mostly;
 
 void pvclock_set_flags(u8 flags)
 {
@@ -144,3 +146,15 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall_clock,
 
 	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
 }
+
+void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti)
+{
+	WARN_ON(vclock_was_used(VCLOCK_PVCLOCK));
+	pvti_cpu0_va = pvti;
+}
+
+struct pvclock_vsyscall_time_info *pvclock_pvti_cpu0_va(void)
+{
+	return pvti_cpu0_va;
+}
+EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

In order to support pvclock vdso on xen we need to setup the time
info page for vcpu 0 and register the page with Xen using the
VCPUOP_register_vcpu_time_memory_area hypercall. This hypercall
will also forcefully update the pvti which will set some of the
necessary flags for vdso. Afterwards we check if it supports the
PVCLOCK_TSC_STABLE_BIT flag which is mandatory for having
vdso/vsyscall support. And if so, it will set the cpu 0 pvti that
will be later on used when mapping the vdso image.

The xen headers are also updated to include the new hypercall for
registering the secondary vcpu_time_info struct.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
Changes since v3:
 (Comments from Juergen)
 * Remove _t added suffix from *GUEST_HANDLE* when sync vcpu.h
 with the latest

Changes since v2:
 (Comments from Juergen)
 * Omit the blank after the cast on all 3 occurrences.
 * Change last VCLOCK_PVCLOCK message to be more descriptive
 * Sync the complete vcpu.h header instead of just adding the
 needed one. (IOW adding VCPUOP_get_physid)

Changes since v1:
 * Check flags ahead to see if the  primary clock can use
 PVCLOCK_TSC_STABLE_BIT even if secondary registration fails.
 (Comments from Boris)
 * Remove addr, addr variables;
 * Change first pr_debug to pr_warn;
 * Change last pr_debug to pr_notice;
 * Add routine to solely register secondary time info.
 * Move xen_clock to outside xen_setup_vsyscall_time_info to allow
 restore path to simply re-register secondary time info. Let us
 handle the restore path more gracefully without re-allocating a
 page.
 * Removed cpu argument from xen_setup_vsyscall_time_info()
 * Adjustment failed registration error messages/loglevel to be the same
 * Also teardown secondary time info on suspend

Changes since RFC:
 (Comments from Boris and David)
 * Remove Kconfig option
 * Use get_zeroed_page/free/page
 * Remove the hypercall availability check
 * Unregister pvti with arg.addr.v = NULL if stable bit isn't supported.
 (New)
 * Set secondary copy on restore such that it works on migration.
 * Drop global xen_clock variable and stash it locally on
 xen_setup_vsyscall_time_info.
 * WARN_ON(ret) if we fail to unregister the pvti.
---
 arch/x86/xen/suspend.c       |   4 ++
 arch/x86/xen/time.c          | 100 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/xen-ops.h       |   2 +
 include/xen/interface/vcpu.h |  42 ++++++++++++++++++
 4 files changed, 148 insertions(+)

diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
index d6b1680693a9..800ed36ecfba 100644
--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -16,6 +16,8 @@
 
 void xen_arch_pre_suspend(void)
 {
+	xen_save_time_memory_area();
+
 	if (xen_pv_domain())
 		xen_pv_pre_suspend();
 }
@@ -26,6 +28,8 @@ void xen_arch_post_suspend(int cancelled)
 		xen_pv_post_suspend(cancelled);
 	else
 		xen_hvm_post_suspend(cancelled);
+
+	xen_restore_time_memory_area();
 }
 
 static void xen_vcpu_notify_restore(void *data)
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 1ecb05db3632..3bf72b933825 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -370,6 +370,105 @@ static const struct pv_time_ops xen_time_ops __initconst = {
 	.steal_clock = xen_steal_clock,
 };
 
+static struct pvclock_vsyscall_time_info *xen_clock __read_mostly;
+
+void xen_save_time_memory_area(void)
+{
+	struct vcpu_register_time_memory_area t;
+	int ret;
+
+	if (!xen_clock)
+		return;
+
+	t.addr.v = NULL;
+
+	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+	if (ret != 0)
+		pr_notice("Cannot save secondary vcpu_time_info (err %d)",
+			  ret);
+	else
+		clear_page(xen_clock);
+}
+
+void xen_restore_time_memory_area(void)
+{
+	struct vcpu_register_time_memory_area t;
+	int ret;
+
+	if (!xen_clock)
+		return;
+
+	t.addr.v = &xen_clock->pvti;
+
+	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+
+	/*
+	 * We don't disable VCLOCK_PVCLOCK entirely if it fails to register the
+	 * secondary time info with Xen or if we migrated to a host without the
+	 * necessary flags. On both of these cases what happens is either
+	 * process seeing a zeroed out pvti or seeing no PVCLOCK_TSC_STABLE_BIT
+	 * bit set. Userspace checks the latter and if 0, it discards the data
+	 * in pvti and fallbacks to a system call for a reliable timestamp.
+	 */
+	if (ret != 0)
+		pr_notice("Cannot restore secondary vcpu_time_info (err %d)",
+			  ret);
+}
+
+static void xen_setup_vsyscall_time_info(void)
+{
+	struct vcpu_register_time_memory_area t;
+	struct pvclock_vsyscall_time_info *ti;
+	struct pvclock_vcpu_time_info *pvti;
+	int ret;
+
+	pvti = &__this_cpu_read(xen_vcpu)->time;
+
+	/*
+	 * We check ahead on the primary time info if this
+	 * bit is supported hence speeding up Xen clocksource.
+	 */
+	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
+		return;
+
+	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
+
+	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
+	if (!ti)
+		return;
+
+	t.addr.v = &ti->pvti;
+
+	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+	if (ret) {
+		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
+		free_page((unsigned long)ti);
+		return;
+	}
+
+	/*
+	 * If the check above succedded this one should too since it's the
+	 * same data on both primary and secondary time infos just different
+	 * memory regions. But we still check it in case hypervisor is buggy.
+	 */
+	pvti = &ti->pvti;
+	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
+		t.addr.v = NULL;
+		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
+					 0, &t);
+		if (!ret)
+			free_page((unsigned long)ti);
+
+		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
+		return;
+	}
+
+	xen_clock = ti;
+	pvclock_set_pvti_cpu0_va(xen_clock);
+
+	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
+}
+
 static void __init xen_time_init(void)
 {
 	int cpu = smp_processor_id();
@@ -396,6 +495,7 @@ static void __init xen_time_init(void)
 	setup_force_cpu_cap(X86_FEATURE_TSC);
 
 	xen_setup_runstate_info(cpu);
+	xen_setup_vsyscall_time_info();
 	xen_setup_timer(cpu);
 	xen_setup_cpu_clockevents();
 
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index c8a6d224f7ed..f96dbedb33d4 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -69,6 +69,8 @@ void xen_setup_runstate_info(int cpu);
 void xen_teardown_timer(int cpu);
 u64 xen_clocksource_read(void);
 void xen_setup_cpu_clockevents(void);
+void xen_save_time_memory_area(void);
+void xen_restore_time_memory_area(void);
 void __init xen_init_time_ops(void);
 void __init xen_hvm_init_time_ops(void);
 
diff --git a/include/xen/interface/vcpu.h b/include/xen/interface/vcpu.h
index 98188c87f5c1..504c71601511 100644
--- a/include/xen/interface/vcpu.h
+++ b/include/xen/interface/vcpu.h
@@ -178,4 +178,46 @@ DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_vcpu_info);
 
 /* Send an NMI to the specified VCPU. @extra_arg == NULL. */
 #define VCPUOP_send_nmi             11
+
+/*
+ * Get the physical ID information for a pinned vcpu's underlying physical
+ * processor.  The physical ID informmation is architecture-specific.
+ * On x86: id[31:0]=apic_id, id[63:32]=acpi_id.
+ * This command returns -EINVAL if it is not a valid operation for this VCPU.
+ */
+#define VCPUOP_get_physid           12 /* arg == vcpu_get_physid_t */
+struct vcpu_get_physid {
+	uint64_t phys_id;
+};
+DEFINE_GUEST_HANDLE_STRUCT(vcpu_get_physid);
+#define xen_vcpu_physid_to_x86_apicid(physid) ((uint32_t)(physid))
+#define xen_vcpu_physid_to_x86_acpiid(physid) ((uint32_t)((physid) >> 32))
+
+/*
+ * Register a memory location to get a secondary copy of the vcpu time
+ * parameters.  The master copy still exists as part of the vcpu shared
+ * memory area, and this secondary copy is updated whenever the master copy
+ * is updated (and using the same versioning scheme for synchronisation).
+ *
+ * The intent is that this copy may be mapped (RO) into userspace so
+ * that usermode can compute system time using the time info and the
+ * tsc.  Usermode will see an array of vcpu_time_info structures, one
+ * for each vcpu, and choose the right one by an existing mechanism
+ * which allows it to get the current vcpu number (such as via a
+ * segment limit).  It can then apply the normal algorithm to compute
+ * system time from the tsc.
+ *
+ * @extra_arg == pointer to vcpu_register_time_info_memory_area structure.
+ */
+#define VCPUOP_register_vcpu_time_memory_area   13
+DEFINE_GUEST_HANDLE_STRUCT(vcpu_time_info);
+struct vcpu_register_time_memory_area {
+	union {
+		GUEST_HANDLE(vcpu_time_info) h;
+		struct pvclock_vcpu_time_info *v;
+		uint64_t p;
+	} addr;
+};
+DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_time_memory_area);
+
 #endif /* __XEN_PUBLIC_VCPU_H__ */
-- 
2.11.0

[PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

In order to support pvclock vdso on xen we need to setup the time
info page for vcpu 0 and register the page with Xen using the
VCPUOP_register_vcpu_time_memory_area hypercall. This hypercall
will also forcefully update the pvti which will set some of the
necessary flags for vdso. Afterwards we check if it supports the
PVCLOCK_TSC_STABLE_BIT flag which is mandatory for having
vdso/vsyscall support. And if so, it will set the cpu 0 pvti that
will be later on used when mapping the vdso image.

The xen headers are also updated to include the new hypercall for
registering the secondary vcpu_time_info struct.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
Changes since v3:
 (Comments from Juergen)
 * Remove _t added suffix from *GUEST_HANDLE* when sync vcpu.h
 with the latest

Changes since v2:
 (Comments from Juergen)
 * Omit the blank after the cast on all 3 occurrences.
 * Change last VCLOCK_PVCLOCK message to be more descriptive
 * Sync the complete vcpu.h header instead of just adding the
 needed one. (IOW adding VCPUOP_get_physid)

Changes since v1:
 * Check flags ahead to see if the  primary clock can use
 PVCLOCK_TSC_STABLE_BIT even if secondary registration fails.
 (Comments from Boris)
 * Remove addr, addr variables;
 * Change first pr_debug to pr_warn;
 * Change last pr_debug to pr_notice;
 * Add routine to solely register secondary time info.
 * Move xen_clock to outside xen_setup_vsyscall_time_info to allow
 restore path to simply re-register secondary time info. Let us
 handle the restore path more gracefully without re-allocating a
 page.
 * Removed cpu argument from xen_setup_vsyscall_time_info()
 * Adjustment failed registration error messages/loglevel to be the same
 * Also teardown secondary time info on suspend

Changes since RFC:
 (Comments from Boris and David)
 * Remove Kconfig option
 * Use get_zeroed_page/free/page
 * Remove the hypercall availability check
 * Unregister pvti with arg.addr.v = NULL if stable bit isn't supported.
 (New)
 * Set secondary copy on restore such that it works on migration.
 * Drop global xen_clock variable and stash it locally on
 xen_setup_vsyscall_time_info.
 * WARN_ON(ret) if we fail to unregister the pvti.
---
 arch/x86/xen/suspend.c       |   4 ++
 arch/x86/xen/time.c          | 100 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/xen-ops.h       |   2 +
 include/xen/interface/vcpu.h |  42 ++++++++++++++++++
 4 files changed, 148 insertions(+)

diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
index d6b1680693a9..800ed36ecfba 100644
--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -16,6 +16,8 @@
 
 void xen_arch_pre_suspend(void)
 {
+	xen_save_time_memory_area();
+
 	if (xen_pv_domain())
 		xen_pv_pre_suspend();
 }
@@ -26,6 +28,8 @@ void xen_arch_post_suspend(int cancelled)
 		xen_pv_post_suspend(cancelled);
 	else
 		xen_hvm_post_suspend(cancelled);
+
+	xen_restore_time_memory_area();
 }
 
 static void xen_vcpu_notify_restore(void *data)
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 1ecb05db3632..3bf72b933825 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -370,6 +370,105 @@ static const struct pv_time_ops xen_time_ops __initconst = {
 	.steal_clock = xen_steal_clock,
 };
 
+static struct pvclock_vsyscall_time_info *xen_clock __read_mostly;
+
+void xen_save_time_memory_area(void)
+{
+	struct vcpu_register_time_memory_area t;
+	int ret;
+
+	if (!xen_clock)
+		return;
+
+	t.addr.v = NULL;
+
+	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+	if (ret != 0)
+		pr_notice("Cannot save secondary vcpu_time_info (err %d)",
+			  ret);
+	else
+		clear_page(xen_clock);
+}
+
+void xen_restore_time_memory_area(void)
+{
+	struct vcpu_register_time_memory_area t;
+	int ret;
+
+	if (!xen_clock)
+		return;
+
+	t.addr.v = &xen_clock->pvti;
+
+	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+
+	/*
+	 * We don't disable VCLOCK_PVCLOCK entirely if it fails to register the
+	 * secondary time info with Xen or if we migrated to a host without the
+	 * necessary flags. On both of these cases what happens is either
+	 * process seeing a zeroed out pvti or seeing no PVCLOCK_TSC_STABLE_BIT
+	 * bit set. Userspace checks the latter and if 0, it discards the data
+	 * in pvti and fallbacks to a system call for a reliable timestamp.
+	 */
+	if (ret != 0)
+		pr_notice("Cannot restore secondary vcpu_time_info (err %d)",
+			  ret);
+}
+
+static void xen_setup_vsyscall_time_info(void)
+{
+	struct vcpu_register_time_memory_area t;
+	struct pvclock_vsyscall_time_info *ti;
+	struct pvclock_vcpu_time_info *pvti;
+	int ret;
+
+	pvti = &__this_cpu_read(xen_vcpu)->time;
+
+	/*
+	 * We check ahead on the primary time info if this
+	 * bit is supported hence speeding up Xen clocksource.
+	 */
+	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
+		return;
+
+	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
+
+	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
+	if (!ti)
+		return;
+
+	t.addr.v = &ti->pvti;
+
+	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+	if (ret) {
+		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
+		free_page((unsigned long)ti);
+		return;
+	}
+
+	/*
+	 * If the check above succedded this one should too since it's the
+	 * same data on both primary and secondary time infos just different
+	 * memory regions. But we still check it in case hypervisor is buggy.
+	 */
+	pvti = &ti->pvti;
+	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
+		t.addr.v = NULL;
+		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
+					 0, &t);
+		if (!ret)
+			free_page((unsigned long)ti);
+
+		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
+		return;
+	}
+
+	xen_clock = ti;
+	pvclock_set_pvti_cpu0_va(xen_clock);
+
+	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
+}
+
 static void __init xen_time_init(void)
 {
 	int cpu = smp_processor_id();
@@ -396,6 +495,7 @@ static void __init xen_time_init(void)
 	setup_force_cpu_cap(X86_FEATURE_TSC);
 
 	xen_setup_runstate_info(cpu);
+	xen_setup_vsyscall_time_info();
 	xen_setup_timer(cpu);
 	xen_setup_cpu_clockevents();
 
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index c8a6d224f7ed..f96dbedb33d4 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -69,6 +69,8 @@ void xen_setup_runstate_info(int cpu);
 void xen_teardown_timer(int cpu);
 u64 xen_clocksource_read(void);
 void xen_setup_cpu_clockevents(void);
+void xen_save_time_memory_area(void);
+void xen_restore_time_memory_area(void);
 void __init xen_init_time_ops(void);
 void __init xen_hvm_init_time_ops(void);
 
diff --git a/include/xen/interface/vcpu.h b/include/xen/interface/vcpu.h
index 98188c87f5c1..504c71601511 100644
--- a/include/xen/interface/vcpu.h
+++ b/include/xen/interface/vcpu.h
@@ -178,4 +178,46 @@ DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_vcpu_info);
 
 /* Send an NMI to the specified VCPU. @extra_arg == NULL. */
 #define VCPUOP_send_nmi             11
+
+/*
+ * Get the physical ID information for a pinned vcpu's underlying physical
+ * processor.  The physical ID informmation is architecture-specific.
+ * On x86: id[31:0]=apic_id, id[63:32]=acpi_id.
+ * This command returns -EINVAL if it is not a valid operation for this VCPU.
+ */
+#define VCPUOP_get_physid           12 /* arg == vcpu_get_physid_t */
+struct vcpu_get_physid {
+	uint64_t phys_id;
+};
+DEFINE_GUEST_HANDLE_STRUCT(vcpu_get_physid);
+#define xen_vcpu_physid_to_x86_apicid(physid) ((uint32_t)(physid))
+#define xen_vcpu_physid_to_x86_acpiid(physid) ((uint32_t)((physid) >> 32))
+
+/*
+ * Register a memory location to get a secondary copy of the vcpu time
+ * parameters.  The master copy still exists as part of the vcpu shared
+ * memory area, and this secondary copy is updated whenever the master copy
+ * is updated (and using the same versioning scheme for synchronisation).
+ *
+ * The intent is that this copy may be mapped (RO) into userspace so
+ * that usermode can compute system time using the time info and the
+ * tsc.  Usermode will see an array of vcpu_time_info structures, one
+ * for each vcpu, and choose the right one by an existing mechanism
+ * which allows it to get the current vcpu number (such as via a
+ * segment limit).  It can then apply the normal algorithm to compute
+ * system time from the tsc.
+ *
+ * @extra_arg == pointer to vcpu_register_time_info_memory_area structure.
+ */
+#define VCPUOP_register_vcpu_time_memory_area   13
+DEFINE_GUEST_HANDLE_STRUCT(vcpu_time_info);
+struct vcpu_register_time_memory_area {
+	union {
+		GUEST_HANDLE(vcpu_time_info) h;
+		struct pvclock_vcpu_time_info *v;
+		uint64_t p;
+	} addr;
+};
+DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_time_memory_area);
+
 #endif /* __XEN_PUBLIC_VCPU_H__ */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 3/3] MAINTAINERS: xen, kvm: track pvclock-abi.h changes

[Joao Martins]

This file defines an ABI shared between guest and hypervisor(s)
(KVM, Xen) and as such there should be an correspondent entry in
MAINTAINERS file. Notice that there's already a text notice at the
top of the header file, hence this commit simply enforces it more
explicitly and have both peers noticed when such changes happen.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Acked-by: Juergen Gross <jgross@suse.com>
---
In the end, I choose the originally posted because this is so far the
only ABI shared between Xen/KVM. Therefore whenever we have more things
shared it would deserve its own place in MAINTAINERS file. If the
thinking is wrong, I can switch to the alternative with a
"PARAVIRT ABIS" section.

Changes since v1:
 * Add Juergen Gross Acked-by.
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6671f375f7fc..a4834c3c377a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7603,6 +7603,7 @@ S:	Supported
 F:	arch/x86/kvm/
 F:	arch/x86/include/uapi/asm/kvm*
 F:	arch/x86/include/asm/kvm*
+F:	arch/x86/include/asm/pvclock-abi.h
 F:	arch/x86/kernel/kvm.c
 F:	arch/x86/kernel/kvmclock.c
 
@@ -14718,6 +14719,7 @@ F:	arch/x86/xen/
 F:	drivers/*/xen-*front.c
 F:	drivers/xen/
 F:	arch/x86/include/asm/xen/
+F:	arch/x86/include/asm/pvclock-abi.h
 F:	include/xen/
 F:	include/uapi/xen/
 F:	Documentation/ABI/stable/sysfs-hypervisor-xen
-- 
2.11.0

[PATCH v4 3/3] MAINTAINERS: xen, kvm: track pvclock-abi.h changes

[Joao Martins]

This file defines an ABI shared between guest and hypervisor(s)
(KVM, Xen) and as such there should be an correspondent entry in
MAINTAINERS file. Notice that there's already a text notice at the
top of the header file, hence this commit simply enforces it more
explicitly and have both peers noticed when such changes happen.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Acked-by: Juergen Gross <jgross@suse.com>
---
In the end, I choose the originally posted because this is so far the
only ABI shared between Xen/KVM. Therefore whenever we have more things
shared it would deserve its own place in MAINTAINERS file. If the
thinking is wrong, I can switch to the alternative with a
"PARAVIRT ABIS" section.

Changes since v1:
 * Add Juergen Gross Acked-by.
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6671f375f7fc..a4834c3c377a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7603,6 +7603,7 @@ S:	Supported
 F:	arch/x86/kvm/
 F:	arch/x86/include/uapi/asm/kvm*
 F:	arch/x86/include/asm/kvm*
+F:	arch/x86/include/asm/pvclock-abi.h
 F:	arch/x86/kernel/kvm.c
 F:	arch/x86/kernel/kvmclock.c
 
@@ -14718,6 +14719,7 @@ F:	arch/x86/xen/
 F:	drivers/*/xen-*front.c
 F:	drivers/xen/
 F:	arch/x86/include/asm/xen/
+F:	arch/x86/include/asm/pvclock-abi.h
 F:	include/xen/
 F:	include/uapi/xen/
 F:	Documentation/ABI/stable/sysfs-hypervisor-xen
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Juergen Gross]

On 27/09/17 15:46, Joao Martins wrote:
> In order to support pvclock vdso on xen we need to setup the time
> info page for vcpu 0 and register the page with Xen using the
> VCPUOP_register_vcpu_time_memory_area hypercall. This hypercall
> will also forcefully update the pvti which will set some of the
> necessary flags for vdso. Afterwards we check if it supports the
> PVCLOCK_TSC_STABLE_BIT flag which is mandatory for having
> vdso/vsyscall support. And if so, it will set the cpu 0 pvti that
> will be later on used when mapping the vdso image.
> 
> The xen headers are also updated to include the new hypercall for
> registering the secondary vcpu_time_info struct.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Juergen Gross]

On 27/09/17 15:46, Joao Martins wrote:
> In order to support pvclock vdso on xen we need to setup the time
> info page for vcpu 0 and register the page with Xen using the
> VCPUOP_register_vcpu_time_memory_area hypercall. This hypercall
> will also forcefully update the pvti which will set some of the
> necessary flags for vdso. Afterwards we check if it supports the
> PVCLOCK_TSC_STABLE_BIT flag which is mandatory for having
> vdso/vsyscall support. And if so, it will set the cpu 0 pvti that
> will be later on used when mapping the vdso image.
> 
> The xen headers are also updated to include the new hypercall for
> registering the secondary vcpu_time_info struct.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Boris Ostrovsky]

> +static void xen_setup_vsyscall_time_info(void)
> +{
> +	struct vcpu_register_time_memory_area t;
> +	struct pvclock_vsyscall_time_info *ti;
> +	struct pvclock_vcpu_time_info *pvti;
> +	int ret;
> +
> +	pvti = &__this_cpu_read(xen_vcpu)->time;
> +
> +	/*
> +	 * We check ahead on the primary time info if this
> +	 * bit is supported hence speeding up Xen clocksource.
> +	 */
> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
> +		return;
> +
> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);

Is it OK to have this flag set if anything below fails?

(I can see in the changelog that apparently at some point I've asked
about this at v1 but I can't remember/find what exactly it was)

-boris

> +
> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
> +	if (!ti)
> +		return;
> +
> +	t.addr.v = &ti->pvti;
> +
> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
> +	if (ret) {
> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
> +		free_page((unsigned long)ti);
> +		return;
> +	}
> +
> +	/*
> +	 * If the check above succedded this one should too since it's the
> +	 * same data on both primary and secondary time infos just different
> +	 * memory regions. But we still check it in case hypervisor is buggy.
> +	 */
> +	pvti = &ti->pvti;
> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
> +		t.addr.v = NULL;
> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
> +					 0, &t);
> +		if (!ret)
> +			free_page((unsigned long)ti);
> +
> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
> +		return;
> +	}
> +
> +	xen_clock = ti;
> +	pvclock_set_pvti_cpu0_va(xen_clock);
> +
> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
> +}
> +

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Boris Ostrovsky]

> +static void xen_setup_vsyscall_time_info(void)
> +{
> +	struct vcpu_register_time_memory_area t;
> +	struct pvclock_vsyscall_time_info *ti;
> +	struct pvclock_vcpu_time_info *pvti;
> +	int ret;
> +
> +	pvti = &__this_cpu_read(xen_vcpu)->time;
> +
> +	/*
> +	 * We check ahead on the primary time info if this
> +	 * bit is supported hence speeding up Xen clocksource.
> +	 */
> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
> +		return;
> +
> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);

Is it OK to have this flag set if anything below fails?

(I can see in the changelog that apparently at some point I've asked
about this at v1 but I can't remember/find what exactly it was)

-boris

> +
> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
> +	if (!ti)
> +		return;
> +
> +	t.addr.v = &ti->pvti;
> +
> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
> +	if (ret) {
> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
> +		free_page((unsigned long)ti);
> +		return;
> +	}
> +
> +	/*
> +	 * If the check above succedded this one should too since it's the
> +	 * same data on both primary and secondary time infos just different
> +	 * memory regions. But we still check it in case hypervisor is buggy.
> +	 */
> +	pvti = &ti->pvti;
> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
> +		t.addr.v = NULL;
> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
> +					 0, &t);
> +		if (!ret)
> +			free_page((unsigned long)ti);
> +
> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
> +		return;
> +	}
> +
> +	xen_clock = ti;
> +	pvclock_set_pvti_cpu0_va(xen_clock);
> +
> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
> +}
> +


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>> +static void xen_setup_vsyscall_time_info(void)
>> +{
>> +	struct vcpu_register_time_memory_area t;
>> +	struct pvclock_vsyscall_time_info *ti;
>> +	struct pvclock_vcpu_time_info *pvti;
>> +	int ret;
>> +
>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>> +
>> +	/*
>> +	 * We check ahead on the primary time info if this
>> +	 * bit is supported hence speeding up Xen clocksource.
>> +	 */
>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>> +		return;
>> +
>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
> 
> Is it OK to have this flag set if anything below fails?
> 
Yes - if anything below fails it will only affect userspace mapped page. What I
do above is just allowing xen clocksource to use/check that bit (consequently
speeding up sched_clock) given the necessary support is there in the master
copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
has the same data from the master copy, just separate memory regions. The checks
below are just for the unlikely cases of failing to register the secondary copy
or if its content were to differ from master copy in future releases - and
therefore we handle those more gracefully.

> (I can see in the changelog that apparently at some point I've asked
> about this at v1 but I can't remember/find what exactly it was)
> 
>> +
>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>> +	if (!ti)
>> +		return;
>> +
>> +	t.addr.v = &ti->pvti;
>> +
>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>> +	if (ret) {
>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>> +		free_page((unsigned long)ti);
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * If the check above succedded this one should too since it's the
>> +	 * same data on both primary and secondary time infos just different
>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>> +	 */
>> +	pvti = &ti->pvti;
>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>> +		t.addr.v = NULL;
>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>> +					 0, &t);
>> +		if (!ret)
>> +			free_page((unsigned long)ti);
>> +
>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>> +		return;
>> +	}
>> +
>> +	xen_clock = ti;
>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>> +
>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>> +}
>> +
> 

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>> +static void xen_setup_vsyscall_time_info(void)
>> +{
>> +	struct vcpu_register_time_memory_area t;
>> +	struct pvclock_vsyscall_time_info *ti;
>> +	struct pvclock_vcpu_time_info *pvti;
>> +	int ret;
>> +
>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>> +
>> +	/*
>> +	 * We check ahead on the primary time info if this
>> +	 * bit is supported hence speeding up Xen clocksource.
>> +	 */
>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>> +		return;
>> +
>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
> 
> Is it OK to have this flag set if anything below fails?
> 
Yes - if anything below fails it will only affect userspace mapped page. What I
do above is just allowing xen clocksource to use/check that bit (consequently
speeding up sched_clock) given the necessary support is there in the master
copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
has the same data from the master copy, just separate memory regions. The checks
below are just for the unlikely cases of failing to register the secondary copy
or if its content were to differ from master copy in future releases - and
therefore we handle those more gracefully.

> (I can see in the changelog that apparently at some point I've asked
> about this at v1 but I can't remember/find what exactly it was)
> 
>> +
>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>> +	if (!ti)
>> +		return;
>> +
>> +	t.addr.v = &ti->pvti;
>> +
>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>> +	if (ret) {
>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>> +		free_page((unsigned long)ti);
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * If the check above succedded this one should too since it's the
>> +	 * same data on both primary and secondary time infos just different
>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>> +	 */
>> +	pvti = &ti->pvti;
>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>> +		t.addr.v = NULL;
>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>> +					 0, &t);
>> +		if (!ret)
>> +			free_page((unsigned long)ti);
>> +
>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>> +		return;
>> +	}
>> +
>> +	xen_clock = ti;
>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>> +
>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>> +}
>> +
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 3/3] MAINTAINERS: xen, kvm: track pvclock-abi.h changes

[Konrad Rzeszutek Wilk]

On Wed, Sep 27, 2017 at 02:46:23PM +0100, Joao Martins wrote:
> This file defines an ABI shared between guest and hypervisor(s)
> (KVM, Xen) and as such there should be an correspondent entry in
> MAINTAINERS file. Notice that there's already a text notice at the
> top of the header file, hence this commit simply enforces it more
> explicitly and have both peers noticed when such changes happen.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Acked-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> In the end, I choose the originally posted because this is so far the
> only ABI shared between Xen/KVM. Therefore whenever we have more things
> shared it would deserve its own place in MAINTAINERS file. If the
> thinking is wrong, I can switch to the alternative with a
> "PARAVIRT ABIS" section.
> 
> Changes since v1:
>  * Add Juergen Gross Acked-by.
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6671f375f7fc..a4834c3c377a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7603,6 +7603,7 @@ S:	Supported
>  F:	arch/x86/kvm/
>  F:	arch/x86/include/uapi/asm/kvm*
>  F:	arch/x86/include/asm/kvm*
> +F:	arch/x86/include/asm/pvclock-abi.h
>  F:	arch/x86/kernel/kvm.c
>  F:	arch/x86/kernel/kvmclock.c
>  
> @@ -14718,6 +14719,7 @@ F:	arch/x86/xen/
>  F:	drivers/*/xen-*front.c
>  F:	drivers/xen/
>  F:	arch/x86/include/asm/xen/
> +F:	arch/x86/include/asm/pvclock-abi.h
>  F:	include/xen/
>  F:	include/uapi/xen/
>  F:	Documentation/ABI/stable/sysfs-hypervisor-xen
> -- 
> 2.11.0
> 

Re: [PATCH v4 3/3] MAINTAINERS: xen, kvm: track pvclock-abi.h changes

[Konrad Rzeszutek Wilk]

On Wed, Sep 27, 2017 at 02:46:23PM +0100, Joao Martins wrote:
> This file defines an ABI shared between guest and hypervisor(s)
> (KVM, Xen) and as such there should be an correspondent entry in
> MAINTAINERS file. Notice that there's already a text notice at the
> top of the header file, hence this commit simply enforces it more
> explicitly and have both peers noticed when such changes happen.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Acked-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> In the end, I choose the originally posted because this is so far the
> only ABI shared between Xen/KVM. Therefore whenever we have more things
> shared it would deserve its own place in MAINTAINERS file. If the
> thinking is wrong, I can switch to the alternative with a
> "PARAVIRT ABIS" section.
> 
> Changes since v1:
>  * Add Juergen Gross Acked-by.
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6671f375f7fc..a4834c3c377a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7603,6 +7603,7 @@ S:	Supported
>  F:	arch/x86/kvm/
>  F:	arch/x86/include/uapi/asm/kvm*
>  F:	arch/x86/include/asm/kvm*
> +F:	arch/x86/include/asm/pvclock-abi.h
>  F:	arch/x86/kernel/kvm.c
>  F:	arch/x86/kernel/kvmclock.c
>  
> @@ -14718,6 +14719,7 @@ F:	arch/x86/xen/
>  F:	drivers/*/xen-*front.c
>  F:	drivers/xen/
>  F:	arch/x86/include/asm/xen/
> +F:	arch/x86/include/asm/pvclock-abi.h
>  F:	include/xen/
>  F:	include/uapi/xen/
>  F:	Documentation/ABI/stable/sysfs-hypervisor-xen
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Boris Ostrovsky]

On 09/27/2017 11:26 AM, Joao Martins wrote:
> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>> +static void xen_setup_vsyscall_time_info(void)
>>> +{
>>> +	struct vcpu_register_time_memory_area t;
>>> +	struct pvclock_vsyscall_time_info *ti;
>>> +	struct pvclock_vcpu_time_info *pvti;
>>> +	int ret;
>>> +
>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>> +
>>> +	/*
>>> +	 * We check ahead on the primary time info if this
>>> +	 * bit is supported hence speeding up Xen clocksource.
>>> +	 */
>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>> +		return;
>>> +
>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>> Is it OK to have this flag set if anything below fails?
>>
> Yes - if anything below fails it will only affect userspace mapped page.

Then should it be set somewhere else, like in xen_time_init()?

-boris

>  What I
> do above is just allowing xen clocksource to use/check that bit (consequently
> speeding up sched_clock) given the necessary support is there in the master
> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
> has the same data from the master copy, just separate memory regions. The checks
> below are just for the unlikely cases of failing to register the secondary copy
> or if its content were to differ from master copy in future releases - and
> therefore we handle those more gracefully.
>
>> (I can see in the changelog that apparently at some point I've asked
>> about this at v1 but I can't remember/find what exactly it was)
>>
>>> +
>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>> +	if (!ti)
>>> +		return;
>>> +
>>> +	t.addr.v = &ti->pvti;
>>> +
>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>> +	if (ret) {
>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>> +		free_page((unsigned long)ti);
>>> +		return;
>>> +	}
>>> +
>>> +	/*
>>> +	 * If the check above succedded this one should too since it's the
>>> +	 * same data on both primary and secondary time infos just different
>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>> +	 */
>>> +	pvti = &ti->pvti;
>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>> +		t.addr.v = NULL;
>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>> +					 0, &t);
>>> +		if (!ret)
>>> +			free_page((unsigned long)ti);
>>> +
>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>> +		return;
>>> +	}
>>> +
>>> +	xen_clock = ti;
>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>> +
>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>> +}
>>> +

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Boris Ostrovsky]

On 09/27/2017 11:26 AM, Joao Martins wrote:
> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>> +static void xen_setup_vsyscall_time_info(void)
>>> +{
>>> +	struct vcpu_register_time_memory_area t;
>>> +	struct pvclock_vsyscall_time_info *ti;
>>> +	struct pvclock_vcpu_time_info *pvti;
>>> +	int ret;
>>> +
>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>> +
>>> +	/*
>>> +	 * We check ahead on the primary time info if this
>>> +	 * bit is supported hence speeding up Xen clocksource.
>>> +	 */
>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>> +		return;
>>> +
>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>> Is it OK to have this flag set if anything below fails?
>>
> Yes - if anything below fails it will only affect userspace mapped page.

Then should it be set somewhere else, like in xen_time_init()?

-boris

>  What I
> do above is just allowing xen clocksource to use/check that bit (consequently
> speeding up sched_clock) given the necessary support is there in the master
> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
> has the same data from the master copy, just separate memory regions. The checks
> below are just for the unlikely cases of failing to register the secondary copy
> or if its content were to differ from master copy in future releases - and
> therefore we handle those more gracefully.
>
>> (I can see in the changelog that apparently at some point I've asked
>> about this at v1 but I can't remember/find what exactly it was)
>>
>>> +
>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>> +	if (!ti)
>>> +		return;
>>> +
>>> +	t.addr.v = &ti->pvti;
>>> +
>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>> +	if (ret) {
>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>> +		free_page((unsigned long)ti);
>>> +		return;
>>> +	}
>>> +
>>> +	/*
>>> +	 * If the check above succedded this one should too since it's the
>>> +	 * same data on both primary and secondary time infos just different
>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>> +	 */
>>> +	pvti = &ti->pvti;
>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>> +		t.addr.v = NULL;
>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>> +					 0, &t);
>>> +		if (!ret)
>>> +			free_page((unsigned long)ti);
>>> +
>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>> +		return;
>>> +	}
>>> +
>>> +	xen_clock = ti;
>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>> +
>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>> +}
>>> +


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
> On 09/27/2017 11:26 AM, Joao Martins wrote:
>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>> +static void xen_setup_vsyscall_time_info(void)
>>>> +{
>>>> +	struct vcpu_register_time_memory_area t;
>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>> +	int ret;
>>>> +
>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>> +
>>>> +	/*
>>>> +	 * We check ahead on the primary time info if this
>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>> +	 */
>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>> +		return;
>>>> +
>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>> Is it OK to have this flag set if anything below fails?
>>>
>> Yes - if anything below fails it will only affect userspace mapped page.
> 
> Then should it be set somewhere else, like in xen_time_init()?
>
Hm, I could move it if you think it's better - but given the importance of the
bit we are checking and its direct correlation to whether or not we can setup
VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
thing I failed to mention before is that checking ahead like above, let us also
avoid allocating a page plus an hypercall to register the pvti just to check the
one bit of info we need for using VCLOCK_PVCLOCK.

It is very unlikely with current Xen code that 1) the secondary copy register
below fails, or 2) master and secondary don't have the same bits set. So in case
you're reconsidering the "shortcut" check above I can move it like we had in v1
and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().

>>  What I
>> do above is just allowing xen clocksource to use/check that bit (consequently
>> speeding up sched_clock) given the necessary support is there in the master
>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>> has the same data from the master copy, just separate memory regions. The checks
>> below are just for the unlikely cases of failing to register the secondary copy
>> or if its content were to differ from master copy in future releases - and
>> therefore we handle those more gracefully.
>>
>>> (I can see in the changelog that apparently at some point I've asked
>>> about this at v1 but I can't remember/find what exactly it was)
>>>
>>>> +
>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>> +	if (!ti)
>>>> +		return;
>>>> +
>>>> +	t.addr.v = &ti->pvti;
>>>> +
>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>> +	if (ret) {
>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>> +		free_page((unsigned long)ti);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * If the check above succedded this one should too since it's the
>>>> +	 * same data on both primary and secondary time infos just different
>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>> +	 */
>>>> +	pvti = &ti->pvti;
>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>> +		t.addr.v = NULL;
>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>> +					 0, &t);
>>>> +		if (!ret)
>>>> +			free_page((unsigned long)ti);
>>>> +
>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	xen_clock = ti;
>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>> +
>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>> +}
>>>> +
> 

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
> On 09/27/2017 11:26 AM, Joao Martins wrote:
>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>> +static void xen_setup_vsyscall_time_info(void)
>>>> +{
>>>> +	struct vcpu_register_time_memory_area t;
>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>> +	int ret;
>>>> +
>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>> +
>>>> +	/*
>>>> +	 * We check ahead on the primary time info if this
>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>> +	 */
>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>> +		return;
>>>> +
>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>> Is it OK to have this flag set if anything below fails?
>>>
>> Yes - if anything below fails it will only affect userspace mapped page.
> 
> Then should it be set somewhere else, like in xen_time_init()?
>
Hm, I could move it if you think it's better - but given the importance of the
bit we are checking and its direct correlation to whether or not we can setup
VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
thing I failed to mention before is that checking ahead like above, let us also
avoid allocating a page plus an hypercall to register the pvti just to check the
one bit of info we need for using VCLOCK_PVCLOCK.

It is very unlikely with current Xen code that 1) the secondary copy register
below fails, or 2) master and secondary don't have the same bits set. So in case
you're reconsidering the "shortcut" check above I can move it like we had in v1
and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().

>>  What I
>> do above is just allowing xen clocksource to use/check that bit (consequently
>> speeding up sched_clock) given the necessary support is there in the master
>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>> has the same data from the master copy, just separate memory regions. The checks
>> below are just for the unlikely cases of failing to register the secondary copy
>> or if its content were to differ from master copy in future releases - and
>> therefore we handle those more gracefully.
>>
>>> (I can see in the changelog that apparently at some point I've asked
>>> about this at v1 but I can't remember/find what exactly it was)
>>>
>>>> +
>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>> +	if (!ti)
>>>> +		return;
>>>> +
>>>> +	t.addr.v = &ti->pvti;
>>>> +
>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>> +	if (ret) {
>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>> +		free_page((unsigned long)ti);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * If the check above succedded this one should too since it's the
>>>> +	 * same data on both primary and secondary time infos just different
>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>> +	 */
>>>> +	pvti = &ti->pvti;
>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>> +		t.addr.v = NULL;
>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>> +					 0, &t);
>>>> +		if (!ret)
>>>> +			free_page((unsigned long)ti);
>>>> +
>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	xen_clock = ti;
>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>> +
>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>> +}
>>>> +
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Boris Ostrovsky]

On 09/27/2017 04:57 PM, Joao Martins wrote:
> On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
>> On 09/27/2017 11:26 AM, Joao Martins wrote:
>>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>>> +static void xen_setup_vsyscall_time_info(void)
>>>>> +{
>>>>> +	struct vcpu_register_time_memory_area t;
>>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>>> +	int ret;
>>>>> +
>>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>>> +
>>>>> +	/*
>>>>> +	 * We check ahead on the primary time info if this
>>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>>> +	 */
>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>>> +		return;
>>>>> +
>>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>>> Is it OK to have this flag set if anything below fails?
>>>>
>>> Yes - if anything below fails it will only affect userspace mapped page.
>> Then should it be set somewhere else, like in xen_time_init()?
>>
> Hm, I could move it if you think it's better - but given the importance of the
> bit we are checking and its direct correlation to whether or not we can setup
> VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
> thing I failed to mention before is that checking ahead like above, let us also
> avoid allocating a page plus an hypercall to register the pvti just to check the
> one bit of info we need for using VCLOCK_PVCLOCK.
>
> It is very unlikely with current Xen code that 1) the secondary copy register
> below fails, or 2) master and secondary don't have the same bits set. So in case
> you're reconsidering the "shortcut" check above I can move it like we had in v1
> and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().

I think it would be more logical to move it to the end like in v1.

But can you explain again why this flag should not be set in
xen_time_init()? It seems to me that it would be useful not just for
vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.

-boris


>
>>>  What I
>>> do above is just allowing xen clocksource to use/check that bit (consequently
>>> speeding up sched_clock) given the necessary support is there in the master
>>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>>> has the same data from the master copy, just separate memory regions. The checks
>>> below are just for the unlikely cases of failing to register the secondary copy
>>> or if its content were to differ from master copy in future releases - and
>>> therefore we handle those more gracefully.
>>>
>>>> (I can see in the changelog that apparently at some point I've asked
>>>> about this at v1 but I can't remember/find what exactly it was)
>>>>
>>>>> +
>>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>>> +	if (!ti)
>>>>> +		return;
>>>>> +
>>>>> +	t.addr.v = &ti->pvti;
>>>>> +
>>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>>> +	if (ret) {
>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>>> +		free_page((unsigned long)ti);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	/*
>>>>> +	 * If the check above succedded this one should too since it's the
>>>>> +	 * same data on both primary and secondary time infos just different
>>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>>> +	 */
>>>>> +	pvti = &ti->pvti;
>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>>> +		t.addr.v = NULL;
>>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>>> +					 0, &t);
>>>>> +		if (!ret)
>>>>> +			free_page((unsigned long)ti);
>>>>> +
>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	xen_clock = ti;
>>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>>> +
>>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>>> +}
>>>>> +

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Boris Ostrovsky]

On 09/27/2017 04:57 PM, Joao Martins wrote:
> On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
>> On 09/27/2017 11:26 AM, Joao Martins wrote:
>>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>>> +static void xen_setup_vsyscall_time_info(void)
>>>>> +{
>>>>> +	struct vcpu_register_time_memory_area t;
>>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>>> +	int ret;
>>>>> +
>>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>>> +
>>>>> +	/*
>>>>> +	 * We check ahead on the primary time info if this
>>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>>> +	 */
>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>>> +		return;
>>>>> +
>>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>>> Is it OK to have this flag set if anything below fails?
>>>>
>>> Yes - if anything below fails it will only affect userspace mapped page.
>> Then should it be set somewhere else, like in xen_time_init()?
>>
> Hm, I could move it if you think it's better - but given the importance of the
> bit we are checking and its direct correlation to whether or not we can setup
> VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
> thing I failed to mention before is that checking ahead like above, let us also
> avoid allocating a page plus an hypercall to register the pvti just to check the
> one bit of info we need for using VCLOCK_PVCLOCK.
>
> It is very unlikely with current Xen code that 1) the secondary copy register
> below fails, or 2) master and secondary don't have the same bits set. So in case
> you're reconsidering the "shortcut" check above I can move it like we had in v1
> and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().

I think it would be more logical to move it to the end like in v1.

But can you explain again why this flag should not be set in
xen_time_init()? It seems to me that it would be useful not just for
vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.

-boris


>
>>>  What I
>>> do above is just allowing xen clocksource to use/check that bit (consequently
>>> speeding up sched_clock) given the necessary support is there in the master
>>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>>> has the same data from the master copy, just separate memory regions. The checks
>>> below are just for the unlikely cases of failing to register the secondary copy
>>> or if its content were to differ from master copy in future releases - and
>>> therefore we handle those more gracefully.
>>>
>>>> (I can see in the changelog that apparently at some point I've asked
>>>> about this at v1 but I can't remember/find what exactly it was)
>>>>
>>>>> +
>>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>>> +	if (!ti)
>>>>> +		return;
>>>>> +
>>>>> +	t.addr.v = &ti->pvti;
>>>>> +
>>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>>> +	if (ret) {
>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>>> +		free_page((unsigned long)ti);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	/*
>>>>> +	 * If the check above succedded this one should too since it's the
>>>>> +	 * same data on both primary and secondary time infos just different
>>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>>> +	 */
>>>>> +	pvti = &ti->pvti;
>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>>> +		t.addr.v = NULL;
>>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>>> +					 0, &t);
>>>>> +		if (!ret)
>>>>> +			free_page((unsigned long)ti);
>>>>> +
>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	xen_clock = ti;
>>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>>> +
>>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>>> +}
>>>>> +


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/27/2017 11:44 PM, Boris Ostrovsky wrote:
> On 09/27/2017 04:57 PM, Joao Martins wrote:
>> On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
>>> On 09/27/2017 11:26 AM, Joao Martins wrote:
>>>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>>>> +static void xen_setup_vsyscall_time_info(void)
>>>>>> +{
>>>>>> +	struct vcpu_register_time_memory_area t;
>>>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * We check ahead on the primary time info if this
>>>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>>>> +	 */
>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>>>> +		return;
>>>>>> +
>>>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>>>> Is it OK to have this flag set if anything below fails?
>>>>>
>>>> Yes - if anything below fails it will only affect userspace mapped page.
>>> Then should it be set somewhere else, like in xen_time_init()?
>>>
>> Hm, I could move it if you think it's better - but given the importance of the
>> bit we are checking and its direct correlation to whether or not we can setup
>> VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
>> thing I failed to mention before is that checking ahead like above, let us also
>> avoid allocating a page plus an hypercall to register the pvti just to check the
>> one bit of info we need for using VCLOCK_PVCLOCK.
>>
>> It is very unlikely with current Xen code that 1) the secondary copy register
>> below fails, or 2) master and secondary don't have the same bits set. So in case
>> you're reconsidering the "shortcut" check above I can move it like we had in v1
>> and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().
> 
> I think it would be more logical to move it to the end like in v1.
> 
> But can you explain again why this flag should not be set in
> xen_time_init()?

I didn't say we shouldn't have this flag there - I was just pointing out a
matter of taste on whether to put on xen_time_init() or in
xen_setup_vsyscall_time_info() (which is called from xen_time_init btw) so
there's no functional change.

> It seems to me that it would be useful not just for
> vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.

Right - That's what I mentioned by "allowing xen clocksource to use/check that
bit (consequently speeding up sched_clock)". The above chunk is really focused
on enabling the flag on pvclock_clocksource_read().

>>
>>>>  What I
>>>> do above is just allowing xen clocksource to use/check that bit (consequently
>>>> speeding up sched_clock) given the necessary support is there in the master
>>>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>>>> has the same data from the master copy, just separate memory regions. The checks
>>>> below are just for the unlikely cases of failing to register the secondary copy
>>>> or if its content were to differ from master copy in future releases - and
>>>> therefore we handle those more gracefully.
>>>>
>>>>> (I can see in the changelog that apparently at some point I've asked
>>>>> about this at v1 but I can't remember/find what exactly it was)
>>>>>
>>>>>> +
>>>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>>>> +	if (!ti)
>>>>>> +		return;
>>>>>> +
>>>>>> +	t.addr.v = &ti->pvti;
>>>>>> +
>>>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>>>> +	if (ret) {
>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>>>> +		free_page((unsigned long)ti);
>>>>>> +		return;
>>>>>> +	}
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If the check above succedded this one should too since it's the
>>>>>> +	 * same data on both primary and secondary time infos just different
>>>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>>>> +	 */
>>>>>> +	pvti = &ti->pvti;
>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>>>> +		t.addr.v = NULL;
>>>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>>>> +					 0, &t);
>>>>>> +		if (!ret)
>>>>>> +			free_page((unsigned long)ti);
>>>>>> +
>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>>>> +		return;
>>>>>> +	}
>>>>>> +
>>>>>> +	xen_clock = ti;
>>>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>>>> +
>>>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>>>> +}
>>>>>> +
> 

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/27/2017 11:44 PM, Boris Ostrovsky wrote:
> On 09/27/2017 04:57 PM, Joao Martins wrote:
>> On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
>>> On 09/27/2017 11:26 AM, Joao Martins wrote:
>>>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>>>> +static void xen_setup_vsyscall_time_info(void)
>>>>>> +{
>>>>>> +	struct vcpu_register_time_memory_area t;
>>>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * We check ahead on the primary time info if this
>>>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>>>> +	 */
>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>>>> +		return;
>>>>>> +
>>>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>>>> Is it OK to have this flag set if anything below fails?
>>>>>
>>>> Yes - if anything below fails it will only affect userspace mapped page.
>>> Then should it be set somewhere else, like in xen_time_init()?
>>>
>> Hm, I could move it if you think it's better - but given the importance of the
>> bit we are checking and its direct correlation to whether or not we can setup
>> VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
>> thing I failed to mention before is that checking ahead like above, let us also
>> avoid allocating a page plus an hypercall to register the pvti just to check the
>> one bit of info we need for using VCLOCK_PVCLOCK.
>>
>> It is very unlikely with current Xen code that 1) the secondary copy register
>> below fails, or 2) master and secondary don't have the same bits set. So in case
>> you're reconsidering the "shortcut" check above I can move it like we had in v1
>> and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().
> 
> I think it would be more logical to move it to the end like in v1.
> 
> But can you explain again why this flag should not be set in
> xen_time_init()?

I didn't say we shouldn't have this flag there - I was just pointing out a
matter of taste on whether to put on xen_time_init() or in
xen_setup_vsyscall_time_info() (which is called from xen_time_init btw) so
there's no functional change.

> It seems to me that it would be useful not just for
> vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.

Right - That's what I mentioned by "allowing xen clocksource to use/check that
bit (consequently speeding up sched_clock)". The above chunk is really focused
on enabling the flag on pvclock_clocksource_read().

>>
>>>>  What I
>>>> do above is just allowing xen clocksource to use/check that bit (consequently
>>>> speeding up sched_clock) given the necessary support is there in the master
>>>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>>>> has the same data from the master copy, just separate memory regions. The checks
>>>> below are just for the unlikely cases of failing to register the secondary copy
>>>> or if its content were to differ from master copy in future releases - and
>>>> therefore we handle those more gracefully.
>>>>
>>>>> (I can see in the changelog that apparently at some point I've asked
>>>>> about this at v1 but I can't remember/find what exactly it was)
>>>>>
>>>>>> +
>>>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>>>> +	if (!ti)
>>>>>> +		return;
>>>>>> +
>>>>>> +	t.addr.v = &ti->pvti;
>>>>>> +
>>>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>>>> +	if (ret) {
>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>>>> +		free_page((unsigned long)ti);
>>>>>> +		return;
>>>>>> +	}
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If the check above succedded this one should too since it's the
>>>>>> +	 * same data on both primary and secondary time infos just different
>>>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>>>> +	 */
>>>>>> +	pvti = &ti->pvti;
>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>>>> +		t.addr.v = NULL;
>>>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>>>> +					 0, &t);
>>>>>> +		if (!ret)
>>>>>> +			free_page((unsigned long)ti);
>>>>>> +
>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>>>> +		return;
>>>>>> +	}
>>>>>> +
>>>>>> +	xen_clock = ti;
>>>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>>>> +
>>>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>>>> +}
>>>>>> +
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/28/2017 12:46 AM, Joao Martins wrote:
> On 09/27/2017 11:44 PM, Boris Ostrovsky wrote:
>> On 09/27/2017 04:57 PM, Joao Martins wrote:
>>> On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
>>>> On 09/27/2017 11:26 AM, Joao Martins wrote:
>>>>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>>>>> +static void xen_setup_vsyscall_time_info(void)
>>>>>>> +{
>>>>>>> +	struct vcpu_register_time_memory_area t;
>>>>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * We check ahead on the primary time info if this
>>>>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>>>>> +	 */
>>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>>>>> +		return;
>>>>>>> +
>>>>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>>>>> Is it OK to have this flag set if anything below fails?
>>>>>>
>>>>> Yes - if anything below fails it will only affect userspace mapped page.
>>>> Then should it be set somewhere else, like in xen_time_init()?
>>>>
>>> Hm, I could move it if you think it's better - but given the importance of the
>>> bit we are checking and its direct correlation to whether or not we can setup
>>> VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
>>> thing I failed to mention before is that checking ahead like above, let us also
>>> avoid allocating a page plus an hypercall to register the pvti just to check the
>>> one bit of info we need for using VCLOCK_PVCLOCK.
>>>
>>> It is very unlikely with current Xen code that 1) the secondary copy register
>>> below fails, or 2) master and secondary don't have the same bits set. So in case
>>> you're reconsidering the "shortcut" check above I can move it like we had in v1
>>> and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().
>>
>> I think it would be more logical to move it to the end like in v1.
>>
>> But can you explain again why this flag should not be set in
>> xen_time_init()?
> 
> I didn't say we shouldn't have this flag there - I was just pointing out a
> matter of taste on whether to put on xen_time_init() or in
> xen_setup_vsyscall_time_info() (which is called from xen_time_init btw) so
> there's no functional change.
> 
To be clear, in this paragraph when I say on xen_setup_vsyscall_time_info() I
mean like it is described in this patch i.e. in the beginning of the routine.

>> It seems to me that it would be useful not just for
>> vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.
> 
> Right - That's what I mentioned by "allowing xen clocksource to use/check that
> bit (consequently speeding up sched_clock)". The above chunk is really focused
> on enabling the flag on pvclock_clocksource_read().
> 
>>>
>>>>>  What I
>>>>> do above is just allowing xen clocksource to use/check that bit (consequently
>>>>> speeding up sched_clock) given the necessary support is there in the master
>>>>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>>>>> has the same data from the master copy, just separate memory regions. The checks
>>>>> below are just for the unlikely cases of failing to register the secondary copy
>>>>> or if its content were to differ from master copy in future releases - and
>>>>> therefore we handle those more gracefully.
>>>>>
>>>>>> (I can see in the changelog that apparently at some point I've asked
>>>>>> about this at v1 but I can't remember/find what exactly it was)
>>>>>>
>>>>>>> +
>>>>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>>>>> +	if (!ti)
>>>>>>> +		return;
>>>>>>> +
>>>>>>> +	t.addr.v = &ti->pvti;
>>>>>>> +
>>>>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>>>>> +	if (ret) {
>>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>>>>> +		free_page((unsigned long)ti);
>>>>>>> +		return;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * If the check above succedded this one should too since it's the
>>>>>>> +	 * same data on both primary and secondary time infos just different
>>>>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>>>>> +	 */
>>>>>>> +	pvti = &ti->pvti;
>>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>>>>> +		t.addr.v = NULL;
>>>>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>>>>> +					 0, &t);
>>>>>>> +		if (!ret)
>>>>>>> +			free_page((unsigned long)ti);
>>>>>>> +
>>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>>>>> +		return;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	xen_clock = ti;
>>>>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>>>>> +
>>>>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>>>>> +}
>>>>>>> +
>>

Re: [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins]

On 09/28/2017 12:46 AM, Joao Martins wrote:
> On 09/27/2017 11:44 PM, Boris Ostrovsky wrote:
>> On 09/27/2017 04:57 PM, Joao Martins wrote:
>>> On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
>>>> On 09/27/2017 11:26 AM, Joao Martins wrote:
>>>>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>>>>> +static void xen_setup_vsyscall_time_info(void)
>>>>>>> +{
>>>>>>> +	struct vcpu_register_time_memory_area t;
>>>>>>> +	struct pvclock_vsyscall_time_info *ti;
>>>>>>> +	struct pvclock_vcpu_time_info *pvti;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	pvti = &__this_cpu_read(xen_vcpu)->time;
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * We check ahead on the primary time info if this
>>>>>>> +	 * bit is supported hence speeding up Xen clocksource.
>>>>>>> +	 */
>>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>>>>> +		return;
>>>>>>> +
>>>>>>> +	pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>>>>> Is it OK to have this flag set if anything below fails?
>>>>>>
>>>>> Yes - if anything below fails it will only affect userspace mapped page.
>>>> Then should it be set somewhere else, like in xen_time_init()?
>>>>
>>> Hm, I could move it if you think it's better - but given the importance of the
>>> bit we are checking and its direct correlation to whether or not we can setup
>>> VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
>>> thing I failed to mention before is that checking ahead like above, let us also
>>> avoid allocating a page plus an hypercall to register the pvti just to check the
>>> one bit of info we need for using VCLOCK_PVCLOCK.
>>>
>>> It is very unlikely with current Xen code that 1) the secondary copy register
>>> below fails, or 2) master and secondary don't have the same bits set. So in case
>>> you're reconsidering the "shortcut" check above I can move it like we had in v1
>>> and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().
>>
>> I think it would be more logical to move it to the end like in v1.
>>
>> But can you explain again why this flag should not be set in
>> xen_time_init()?
> 
> I didn't say we shouldn't have this flag there - I was just pointing out a
> matter of taste on whether to put on xen_time_init() or in
> xen_setup_vsyscall_time_info() (which is called from xen_time_init btw) so
> there's no functional change.
> 
To be clear, in this paragraph when I say on xen_setup_vsyscall_time_info() I
mean like it is described in this patch i.e. in the beginning of the routine.

>> It seems to me that it would be useful not just for
>> vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.
> 
> Right - That's what I mentioned by "allowing xen clocksource to use/check that
> bit (consequently speeding up sched_clock)". The above chunk is really focused
> on enabling the flag on pvclock_clocksource_read().
> 
>>>
>>>>>  What I
>>>>> do above is just allowing xen clocksource to use/check that bit (consequently
>>>>> speeding up sched_clock) given the necessary support is there in the master
>>>>> copy. The secondary copy (i.e. what's being set up below, mapped/used in vdso)
>>>>> has the same data from the master copy, just separate memory regions. The checks
>>>>> below are just for the unlikely cases of failing to register the secondary copy
>>>>> or if its content were to differ from master copy in future releases - and
>>>>> therefore we handle those more gracefully.
>>>>>
>>>>>> (I can see in the changelog that apparently at some point I've asked
>>>>>> about this at v1 but I can't remember/find what exactly it was)
>>>>>>
>>>>>>> +
>>>>>>> +	ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>>>>> +	if (!ti)
>>>>>>> +		return;
>>>>>>> +
>>>>>>> +	t.addr.v = &ti->pvti;
>>>>>>> +
>>>>>>> +	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>>>>> +	if (ret) {
>>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>>>>> +		free_page((unsigned long)ti);
>>>>>>> +		return;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * If the check above succedded this one should too since it's the
>>>>>>> +	 * same data on both primary and secondary time infos just different
>>>>>>> +	 * memory regions. But we still check it in case hypervisor is buggy.
>>>>>>> +	 */
>>>>>>> +	pvti = &ti->pvti;
>>>>>>> +	if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>>>>> +		t.addr.v = NULL;
>>>>>>> +		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>>>>> +					 0, &t);
>>>>>>> +		if (!ret)
>>>>>>> +			free_page((unsigned long)ti);
>>>>>>> +
>>>>>>> +		pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>>>>> +		return;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	xen_clock = ti;
>>>>>>> +	pvclock_set_pvti_cpu0_va(xen_clock);
>>>>>>> +
>>>>>>> +	xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>>>>> +}
>>>>>>> +
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 3/3] MAINTAINERS: xen, kvm: track pvclock-abi.h changes

[Paolo Bonzini]

On 27/09/2017 18:06, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 27, 2017 at 02:46:23PM +0100, Joao Martins wrote:
>> This file defines an ABI shared between guest and hypervisor(s)
>> (KVM, Xen) and as such there should be an correspondent entry in
>> MAINTAINERS file. Notice that there's already a text notice at the
>> top of the header file, hence this commit simply enforces it more
>> explicitly and have both peers noticed when such changes happen.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Acked-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

>> ---
>> In the end, I choose the originally posted because this is so far the
>> only ABI shared between Xen/KVM. Therefore whenever we have more things
>> shared it would deserve its own place in MAINTAINERS file. If the
>> thinking is wrong, I can switch to the alternative with a
>> "PARAVIRT ABIS" section.
>>
>> Changes since v1:
>>  * Add Juergen Gross Acked-by.
>> ---
>>  MAINTAINERS | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6671f375f7fc..a4834c3c377a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7603,6 +7603,7 @@ S:	Supported
>>  F:	arch/x86/kvm/
>>  F:	arch/x86/include/uapi/asm/kvm*
>>  F:	arch/x86/include/asm/kvm*
>> +F:	arch/x86/include/asm/pvclock-abi.h
>>  F:	arch/x86/kernel/kvm.c
>>  F:	arch/x86/kernel/kvmclock.c
>>  
>> @@ -14718,6 +14719,7 @@ F:	arch/x86/xen/
>>  F:	drivers/*/xen-*front.c
>>  F:	drivers/xen/
>>  F:	arch/x86/include/asm/xen/
>> +F:	arch/x86/include/asm/pvclock-abi.h
>>  F:	include/xen/
>>  F:	include/uapi/xen/
>>  F:	Documentation/ABI/stable/sysfs-hypervisor-xen
>> -- 
>> 2.11.0
>>

Re: [PATCH v4 3/3] MAINTAINERS: xen, kvm: track pvclock-abi.h changes

[Paolo Bonzini]

On 27/09/2017 18:06, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 27, 2017 at 02:46:23PM +0100, Joao Martins wrote:
>> This file defines an ABI shared between guest and hypervisor(s)
>> (KVM, Xen) and as such there should be an correspondent entry in
>> MAINTAINERS file. Notice that there's already a text notice at the
>> top of the header file, hence this commit simply enforces it more
>> explicitly and have both peers noticed when such changes happen.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Acked-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

>> ---
>> In the end, I choose the originally posted because this is so far the
>> only ABI shared between Xen/KVM. Therefore whenever we have more things
>> shared it would deserve its own place in MAINTAINERS file. If the
>> thinking is wrong, I can switch to the alternative with a
>> "PARAVIRT ABIS" section.
>>
>> Changes since v1:
>>  * Add Juergen Gross Acked-by.
>> ---
>>  MAINTAINERS | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6671f375f7fc..a4834c3c377a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7603,6 +7603,7 @@ S:	Supported
>>  F:	arch/x86/kvm/
>>  F:	arch/x86/include/uapi/asm/kvm*
>>  F:	arch/x86/include/asm/kvm*
>> +F:	arch/x86/include/asm/pvclock-abi.h
>>  F:	arch/x86/kernel/kvm.c
>>  F:	arch/x86/kernel/kvmclock.c
>>  
>> @@ -14718,6 +14719,7 @@ F:	arch/x86/xen/
>>  F:	drivers/*/xen-*front.c
>>  F:	drivers/xen/
>>  F:	arch/x86/include/asm/xen/
>> +F:	arch/x86/include/asm/pvclock-abi.h
>>  F:	include/xen/
>>  F:	include/uapi/xen/
>>  F:	Documentation/ABI/stable/sysfs-hypervisor-xen
>> -- 
>> 2.11.0
>>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel