Hi all,
this series introduces PV wallclock time support on arm and arm64.
Changes in v4:
- simplify xen_read_wallclock
- add a patch to support XENPF_settime64 on x86
Changes in v3:
- use ktime_get_ns instead of calling into the arch_timer functions
directly
- read the wallclock from the late_initcall
- s/%llu.%0u/%llu.%09u
- in xen_pvclock_gtod_notify use the passed struct timekeeper pointer
rather than calling __current_kernel_time64
- use the passed struct timekeeper pointer to get the system time too
- drop introduce __current_kernel_time64
Changes in v2:
- introduce __current_kernel_time64
- rename dom0_op to platform_op
- introduce XENPF_settime64
- extend pvclock_wall_clock with sec_hi
- properly convert arch_timer ticker to nsec
- use timespec64 interfaces
- use sec_hi to get a 64-bit seconds value
- use XENPF_settime64
- rename dom0_op to platform_op
Stefano Stabellini (7):
xen: rename dom0_op to platform_op
xen/arm: introduce HYPERVISOR_platform_op on arm and arm64
xen: introduce XENPF_settime64
arm: extend pvclock_wall_clock with sec_hi
xen/arm: introduce xen_read_wallclock
xen/arm: set the system time in Xen via the XENPF_settime64 hypercall
xen/x86: support XENPF_settime64
arch/arm/include/asm/xen/hypercall.h | 2 +
arch/arm/include/asm/xen/interface.h | 1 +
arch/arm/xen/enlighten.c | 83 +++++++++++++++++++++++++++++++++-
arch/arm/xen/hypercall.S | 1 +
arch/arm64/xen/hypercall.S | 1 +
arch/x86/include/asm/xen/hypercall.h | 6 +--
arch/x86/xen/apic.c | 2 +-
arch/x86/xen/enlighten.c | 8 ++--
arch/x86/xen/time.c | 35 +++++++++++---
drivers/xen/acpi.c | 2 +-
drivers/xen/efi.c | 30 ++++++------
drivers/xen/pcpu.c | 8 ++--
drivers/xen/xen-acpi-cpuhotplug.c | 2 +-
drivers/xen/xen-acpi-pad.c | 4 +-
drivers/xen/xen-acpi-processor.c | 8 ++--
drivers/xen/xenfs/xensyms.c | 4 +-
include/xen/interface/platform.h | 18 ++++++--
include/xen/interface/xen.h | 2 +-
18 files changed, 167 insertions(+), 50 deletions(-)
Cheers,
Stefano
The dom0_op hypercall has been renamed to platform_op since Xen 3.2,
which is ancient, and modern upstream Linux kernels cannot run as dom0
and it anymore anyway.
Signed-off-by: Stefano Stabellini <[email protected]>
Reviewed-by: Boris Ostrovsky <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
arch/x86/include/asm/xen/hypercall.h | 6 +++---
arch/x86/xen/apic.c | 2 +-
arch/x86/xen/enlighten.c | 8 ++++----
arch/x86/xen/time.c | 2 +-
drivers/xen/acpi.c | 2 +-
drivers/xen/efi.c | 30 +++++++++++++++---------------
drivers/xen/pcpu.c | 8 ++++----
drivers/xen/xen-acpi-cpuhotplug.c | 2 +-
drivers/xen/xen-acpi-pad.c | 4 ++--
drivers/xen/xen-acpi-processor.c | 8 ++++----
drivers/xen/xenfs/xensyms.c | 4 ++--
include/xen/interface/xen.h | 2 +-
12 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index 4c20dd3..3bcdcc8 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -310,10 +310,10 @@ HYPERVISOR_mca(struct xen_mc *mc_op)
}
static inline int
-HYPERVISOR_dom0_op(struct xen_platform_op *platform_op)
+HYPERVISOR_platform_op(struct xen_platform_op *op)
{
- platform_op->interface_version = XENPF_INTERFACE_VERSION;
- return _hypercall1(int, dom0_op, platform_op);
+ op->interface_version = XENPF_INTERFACE_VERSION;
+ return _hypercall1(int, platform_op, op);
}
static inline int
diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index acda713..abf4901 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -64,7 +64,7 @@ static u32 xen_apic_read(u32 reg)
if (reg != APIC_ID)
return 0;
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (ret)
return 0;
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 5774800..f963c40 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -415,7 +415,7 @@ static bool __init xen_check_mwait(void)
set_xen_guest_handle(op.u.set_pminfo.pdc, buf);
- if ((HYPERVISOR_dom0_op(&op) == 0) &&
+ if ((HYPERVISOR_platform_op(&op) == 0) &&
(buf[2] & (ACPI_PDC_C_C1_FFH | ACPI_PDC_C_C2C3_FFH))) {
cpuid_leaf5_ecx_val = cx;
cpuid_leaf5_edx_val = dx;
@@ -1374,7 +1374,7 @@ static void __init xen_boot_params_init_edd(void)
info->params.length = sizeof(info->params);
set_xen_guest_handle(op.u.firmware_info.u.disk_info.edd_params,
&info->params);
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (ret)
break;
@@ -1392,7 +1392,7 @@ static void __init xen_boot_params_init_edd(void)
op.u.firmware_info.type = XEN_FW_DISK_MBR_SIGNATURE;
for (nr = 0; nr < EDD_MBR_SIG_MAX; nr++) {
op.u.firmware_info.index = nr;
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (ret)
break;
mbr_signature[nr] = op.u.firmware_info.u.disk_mbr_signature.mbr_signature;
@@ -1698,7 +1698,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
xen_start_info->console.domU.mfn = 0;
xen_start_info->console.domU.evtchn = 0;
- if (HYPERVISOR_dom0_op(&op) == 0)
+ if (HYPERVISOR_platform_op(&op) == 0)
boot_params.kbd_status = op.u.firmware_info.u.kbd_shift_flags;
/* Make sure ACS will be enabled */
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 041d4cd..663c2ea 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -139,7 +139,7 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
op.u.settime.nsecs = now.tv_nsec;
op.u.settime.system_time = xen_clocksource_read();
- (void)HYPERVISOR_dom0_op(&op);
+ (void)HYPERVISOR_platform_op(&op);
/*
* Move the next drift compensation time 11 minutes
diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c
index 90307c0..6893c79 100644
--- a/drivers/xen/acpi.c
+++ b/drivers/xen/acpi.c
@@ -58,7 +58,7 @@ static int xen_acpi_notify_hypervisor_state(u8 sleep_state,
bits, val_a, val_b))
return -1;
- HYPERVISOR_dom0_op(&op);
+ HYPERVISOR_platform_op(&op);
return 1;
}
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index f745db2..be7e56a 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -42,7 +42,7 @@ static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
{
struct xen_platform_op op = INIT_EFI_OP(get_time);
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
if (tm) {
@@ -67,7 +67,7 @@ static efi_status_t xen_efi_set_time(efi_time_t *tm)
BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.set_time));
memcpy(&efi_data(op).u.set_time, tm, sizeof(*tm));
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
return efi_data(op).status;
@@ -79,7 +79,7 @@ static efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled,
{
struct xen_platform_op op = INIT_EFI_OP(get_wakeup_time);
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
if (tm) {
@@ -108,7 +108,7 @@ static efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
else
efi_data(op).misc |= XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY;
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
return efi_data(op).status;
@@ -129,7 +129,7 @@ static efi_status_t xen_efi_get_variable(efi_char16_t *name,
efi_data(op).u.get_variable.size = *data_size;
set_xen_guest_handle(efi_data(op).u.get_variable.data, data);
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
*data_size = efi_data(op).u.get_variable.size;
@@ -152,7 +152,7 @@ static efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
memcpy(&efi_data(op).u.get_next_variable_name.vendor_guid, vendor,
sizeof(*vendor));
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
*name_size = efi_data(op).u.get_next_variable_name.size;
@@ -178,7 +178,7 @@ static efi_status_t xen_efi_set_variable(efi_char16_t *name,
efi_data(op).u.set_variable.size = data_size;
set_xen_guest_handle(efi_data(op).u.set_variable.data, data);
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
return efi_data(op).status;
@@ -196,7 +196,7 @@ static efi_status_t xen_efi_query_variable_info(u32 attr,
efi_data(op).u.query_variable_info.attr = attr;
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
*storage_space = efi_data(op).u.query_variable_info.max_store_size;
@@ -210,7 +210,7 @@ static efi_status_t xen_efi_get_next_high_mono_count(u32 *count)
{
struct xen_platform_op op = INIT_EFI_OP(get_next_high_monotonic_count);
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
*count = efi_data(op).misc;
@@ -232,7 +232,7 @@ static efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
efi_data(op).u.update_capsule.capsule_count = count;
efi_data(op).u.update_capsule.sg_list = sg_list;
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
return efi_data(op).status;
@@ -252,7 +252,7 @@ static efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
capsules);
efi_data(op).u.query_capsule_capabilities.capsule_count = count;
- if (HYPERVISOR_dom0_op(&op) < 0)
+ if (HYPERVISOR_platform_op(&op) < 0)
return EFI_UNSUPPORTED;
*max_size = efi_data(op).u.query_capsule_capabilities.max_capsule_size;
@@ -331,7 +331,7 @@ efi_system_table_t __init *xen_efi_probe(void)
};
union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
- if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op) < 0)
+ if (!xen_initial_domain() || HYPERVISOR_platform_op(&op) < 0)
return NULL;
/* Here we know that Xen runs on EFI platform. */
@@ -347,7 +347,7 @@ efi_system_table_t __init *xen_efi_probe(void)
info->vendor.bufsz = sizeof(vendor);
set_xen_guest_handle(info->vendor.name, vendor);
- if (HYPERVISOR_dom0_op(&op) == 0) {
+ if (HYPERVISOR_platform_op(&op) == 0) {
efi_systab_xen.fw_vendor = __pa_symbol(vendor);
efi_systab_xen.fw_revision = info->vendor.revision;
} else
@@ -357,14 +357,14 @@ efi_system_table_t __init *xen_efi_probe(void)
op.u.firmware_info.type = XEN_FW_EFI_INFO;
op.u.firmware_info.index = XEN_FW_EFI_VERSION;
- if (HYPERVISOR_dom0_op(&op) == 0)
+ if (HYPERVISOR_platform_op(&op) == 0)
efi_systab_xen.hdr.revision = info->version;
op.cmd = XENPF_firmware_info;
op.u.firmware_info.type = XEN_FW_EFI_INFO;
op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
- if (HYPERVISOR_dom0_op(&op) == 0)
+ if (HYPERVISOR_platform_op(&op) == 0)
efi.runtime_version = info->version;
return &efi_systab_xen;
diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
index 49e88f2..cdc6daa 100644
--- a/drivers/xen/pcpu.c
+++ b/drivers/xen/pcpu.c
@@ -78,7 +78,7 @@ static int xen_pcpu_down(uint32_t cpu_id)
.u.cpu_ol.cpuid = cpu_id,
};
- return HYPERVISOR_dom0_op(&op);
+ return HYPERVISOR_platform_op(&op);
}
static int xen_pcpu_up(uint32_t cpu_id)
@@ -89,7 +89,7 @@ static int xen_pcpu_up(uint32_t cpu_id)
.u.cpu_ol.cpuid = cpu_id,
};
- return HYPERVISOR_dom0_op(&op);
+ return HYPERVISOR_platform_op(&op);
}
static ssize_t show_online(struct device *dev,
@@ -277,7 +277,7 @@ static int sync_pcpu(uint32_t cpu, uint32_t *max_cpu)
.u.pcpu_info.xen_cpuid = cpu,
};
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (ret)
return ret;
@@ -364,7 +364,7 @@ int xen_pcpu_id(uint32_t acpi_id)
op.cmd = XENPF_get_cpuinfo;
while (cpu_id <= max_id) {
op.u.pcpu_info.xen_cpuid = cpu_id;
- if (HYPERVISOR_dom0_op(&op)) {
+ if (HYPERVISOR_platform_op(&op)) {
cpu_id++;
continue;
}
diff --git a/drivers/xen/xen-acpi-cpuhotplug.c b/drivers/xen/xen-acpi-cpuhotplug.c
index f4a3694..fdc9e67 100644
--- a/drivers/xen/xen-acpi-cpuhotplug.c
+++ b/drivers/xen/xen-acpi-cpuhotplug.c
@@ -206,7 +206,7 @@ static int xen_hotadd_cpu(struct acpi_processor *pr)
op.u.cpu_add.acpi_id = pr->acpi_id;
op.u.cpu_add.pxm = pxm;
- cpu_id = HYPERVISOR_dom0_op(&op);
+ cpu_id = HYPERVISOR_platform_op(&op);
if (cpu_id < 0)
pr_err(PREFIX "Failed to hotadd CPU for acpi_id %d\n",
pr->acpi_id);
diff --git a/drivers/xen/xen-acpi-pad.c b/drivers/xen/xen-acpi-pad.c
index f83b754..23d1808 100644
--- a/drivers/xen/xen-acpi-pad.c
+++ b/drivers/xen/xen-acpi-pad.c
@@ -36,7 +36,7 @@ static int xen_acpi_pad_idle_cpus(unsigned int idle_nums)
op.u.core_parking.type = XEN_CORE_PARKING_SET;
op.u.core_parking.idle_nums = idle_nums;
- return HYPERVISOR_dom0_op(&op);
+ return HYPERVISOR_platform_op(&op);
}
static int xen_acpi_pad_idle_cpus_num(void)
@@ -46,7 +46,7 @@ static int xen_acpi_pad_idle_cpus_num(void)
op.cmd = XENPF_core_parking;
op.u.core_parking.type = XEN_CORE_PARKING_GET;
- return HYPERVISOR_dom0_op(&op)
+ return HYPERVISOR_platform_op(&op)
?: op.u.core_parking.idle_nums;
}
diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 70fa438..076970a 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -116,7 +116,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
set_xen_guest_handle(op.u.set_pminfo.power.states, dst_cx_states);
if (!no_hypercall)
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (!ret) {
pr_debug("ACPI CPU%u - C-states uploaded.\n", _pr->acpi_id);
@@ -244,7 +244,7 @@ static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
}
if (!no_hypercall)
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (!ret) {
struct acpi_processor_performance *perf;
@@ -302,7 +302,7 @@ static unsigned int __init get_max_acpi_id(void)
info = &op.u.pcpu_info;
info->xen_cpuid = 0;
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (ret)
return NR_CPUS;
@@ -310,7 +310,7 @@ static unsigned int __init get_max_acpi_id(void)
last_cpu = op.u.pcpu_info.max_present;
for (i = 0; i <= last_cpu; i++) {
info->xen_cpuid = i;
- ret = HYPERVISOR_dom0_op(&op);
+ ret = HYPERVISOR_platform_op(&op);
if (ret)
continue;
max_acpi_id = max(info->acpi_id, max_acpi_id);
diff --git a/drivers/xen/xenfs/xensyms.c b/drivers/xen/xenfs/xensyms.c
index f8b1285..a03f261 100644
--- a/drivers/xen/xenfs/xensyms.c
+++ b/drivers/xen/xenfs/xensyms.c
@@ -31,7 +31,7 @@ static int xensyms_next_sym(struct xensyms *xs)
symnum = symdata->symnum;
- ret = HYPERVISOR_dom0_op(&xs->op);
+ ret = HYPERVISOR_platform_op(&xs->op);
if (ret < 0)
return ret;
@@ -50,7 +50,7 @@ static int xensyms_next_sym(struct xensyms *xs)
set_xen_guest_handle(symdata->name, xs->name);
symdata->symnum--; /* Rewind */
- ret = HYPERVISOR_dom0_op(&xs->op);
+ ret = HYPERVISOR_platform_op(&xs->op);
if (ret < 0)
return ret;
}
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 167071c..d133112 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -48,7 +48,7 @@
#define __HYPERVISOR_set_callbacks 4
#define __HYPERVISOR_fpu_taskswitch 5
#define __HYPERVISOR_sched_op_compat 6
-#define __HYPERVISOR_dom0_op 7
+#define __HYPERVISOR_platform_op 7
#define __HYPERVISOR_set_debugreg 8
#define __HYPERVISOR_get_debugreg 9
#define __HYPERVISOR_update_descriptor 10
--
1.7.10.4
Signed-off-by: Stefano Stabellini <[email protected]>
---
Changes in v2:
- rename dom0_op to platform_op
---
arch/arm/include/asm/xen/hypercall.h | 2 ++
arch/arm/xen/enlighten.c | 1 +
arch/arm/xen/hypercall.S | 1 +
arch/arm64/xen/hypercall.S | 1 +
4 files changed, 5 insertions(+)
diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 712b50e..c3e00d0 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -35,6 +35,7 @@
#include <xen/interface/xen.h>
#include <xen/interface/sched.h>
+#include <xen/interface/platform.h>
long privcmd_call(unsigned call, unsigned long a1,
unsigned long a2, unsigned long a3,
@@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
int HYPERVISOR_physdev_op(int cmd, void *arg);
int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
int HYPERVISOR_tmem_op(void *arg);
+int HYPERVISOR_platform_op(void *arg);
int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
static inline int
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 15621b1..2f57ba3 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -328,5 +328,6 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op);
EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op);
EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op);
EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op);
+EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op);
EXPORT_SYMBOL_GPL(HYPERVISOR_multicall);
EXPORT_SYMBOL_GPL(privcmd_call);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index 10fd99c..d4539f4 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -89,6 +89,7 @@ HYPERCALL2(memory_op);
HYPERCALL2(physdev_op);
HYPERCALL3(vcpu_op);
HYPERCALL1(tmem_op);
+HYPERCALL1(platform_op);
HYPERCALL2(multicall);
ENTRY(privcmd_call)
diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S
index 8bbe940..f7d5724 100644
--- a/arch/arm64/xen/hypercall.S
+++ b/arch/arm64/xen/hypercall.S
@@ -80,6 +80,7 @@ HYPERCALL2(memory_op);
HYPERCALL2(physdev_op);
HYPERCALL3(vcpu_op);
HYPERCALL1(tmem_op);
+HYPERCALL1(platform_op);
HYPERCALL2(multicall);
ENTRY(privcmd_call)
--
1.7.10.4
Rename the current XENPF_settime hypercall and related struct to
XENPF_settime32.
Signed-off-by: Stefano Stabellini <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Reviewed-by: Boris Ostrovsky <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
arch/x86/xen/time.c | 8 ++++----
include/xen/interface/platform.h | 18 ++++++++++++++----
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 663c2ea..3bbd377 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -134,10 +134,10 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
if (!was_set && timespec_compare(&now, &next_sync) < 0)
return NOTIFY_OK;
- op.cmd = XENPF_settime;
- op.u.settime.secs = now.tv_sec;
- op.u.settime.nsecs = now.tv_nsec;
- op.u.settime.system_time = xen_clocksource_read();
+ op.cmd = XENPF_settime32;
+ op.u.settime32.secs = now.tv_sec;
+ op.u.settime32.nsecs = now.tv_nsec;
+ op.u.settime32.system_time = xen_clocksource_read();
(void)HYPERVISOR_platform_op(&op);
diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
index 8e03587..732efb0 100644
--- a/include/xen/interface/platform.h
+++ b/include/xen/interface/platform.h
@@ -35,14 +35,23 @@
* Set clock such that it would read <secs,nsecs> after 00:00:00 UTC,
* 1 January, 1970 if the current system time was <system_time>.
*/
-#define XENPF_settime 17
-struct xenpf_settime {
+#define XENPF_settime32 17
+struct xenpf_settime32 {
/* IN variables. */
uint32_t secs;
uint32_t nsecs;
uint64_t system_time;
};
-DEFINE_GUEST_HANDLE_STRUCT(xenpf_settime_t);
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_settime32_t);
+#define XENPF_settime64 62
+struct xenpf_settime64 {
+ /* IN variables. */
+ uint64_t secs;
+ uint32_t nsecs;
+ uint32_t mbz;
+ uint64_t system_time;
+};
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_settime64_t);
/*
* Request memory range (@mfn, @mfn+@nr_mfns-1) to have type @type.
@@ -495,7 +504,8 @@ struct xen_platform_op {
uint32_t cmd;
uint32_t interface_version; /* XENPF_INTERFACE_VERSION */
union {
- struct xenpf_settime settime;
+ struct xenpf_settime32 settime32;
+ struct xenpf_settime64 settime64;
struct xenpf_add_memtype add_memtype;
struct xenpf_del_memtype del_memtype;
struct xenpf_read_memtype read_memtype;
--
1.7.10.4
The hypervisor actually exposes an additional field to struct
pvclock_wall_clock, with the high 32 bit seconds.
Signed-off-by: Stefano Stabellini <[email protected]>
---
arch/arm/include/asm/xen/interface.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 5006600..5b150d7 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -76,6 +76,7 @@ struct pvclock_wall_clock {
u32 version;
u32 sec;
u32 nsec;
+ u32 sec_hi;
} __attribute__((__packed__));
#endif
--
1.7.10.4
Read the wallclock from the shared info page at boot time.
Signed-off-by: Stefano Stabellini <[email protected]>
---
Changes in v4:
- simplify xen_read_wallclock
Changes in v3:
- use ktime_get_ns instead of calling into the arch_timer functions
directly
- read the wallclock from the late_initcall
Changes in v2:
- properly convert arch_timer ticker to nsec
- use timespec64 interfaces
- use sec_hi to get a 64-bit seconds value
---
arch/arm/xen/enlighten.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 2f57ba3..6ae4ef5 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -28,6 +28,7 @@
#include <linux/cpufreq.h>
#include <linux/cpu.h>
#include <linux/console.h>
+#include <linux/timekeeping.h>
#include <linux/mm.h>
@@ -95,6 +96,27 @@ static unsigned long long xen_stolen_accounting(int cpu)
return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
}
+static void xen_read_wallclock(struct timespec64 *ts)
+{
+ u32 version;
+ struct timespec64 now, ts_monotonic;
+ struct shared_info *s = HYPERVISOR_shared_info;
+ struct pvclock_wall_clock *wall_clock = &(s->wc);
+
+ /* get wallclock at system boot */
+ do {
+ version = wall_clock->version;
+ rmb(); /* fetch version before time */
+ now.tv_sec = ((uint64_t)wall_clock->sec_hi << 32) | wall_clock->sec;
+ now.tv_nsec = wall_clock->nsec;
+ rmb(); /* fetch time before checking version */
+ } while ((wall_clock->version & 1) || (version != wall_clock->version));
+
+ /* time since system boot */
+ ktime_get_ts64(&ts_monotonic);
+ *ts = timespec64_add(now, ts_monotonic);
+}
+
static void xen_percpu_init(void)
{
struct vcpu_register_vcpu_info info;
@@ -303,6 +325,11 @@ static int __init xen_pm_init(void)
pm_power_off = xen_power_off;
arm_pm_restart = xen_restart;
+ if (!xen_initial_domain()) {
+ struct timespec64 ts;
+ xen_read_wallclock(&ts);
+ do_settimeofday64(&ts);
+ }
return 0;
}
--
1.7.10.4
If Linux is running as dom0, call XENPF_settime64 to update the system
time in Xen on pvclock_gtod notifications.
Signed-off-by: Stefano Stabellini <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
---
Changes in v3:
- s/%llu.%0u/%llu.%09u
- in xen_pvclock_gtod_notify use the passed struct timekeeper pointer
rather than calling __current_kernel_time64
- use the passed struct timekeeper pointer to get the system time too
Changes in v2:
- properly convert arch_timer ticker to nsec
- rename dom0_op to platform_op
- use timespec64 interfaces
- use XENPF_settime64
---
arch/arm/xen/enlighten.c | 55 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 6ae4ef5..613f7e0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -28,7 +28,10 @@
#include <linux/cpufreq.h>
#include <linux/cpu.h>
#include <linux/console.h>
+#include <linux/pvclock_gtod.h>
+#include <linux/time64.h>
#include <linux/timekeeping.h>
+#include <linux/timekeeper_internal.h>
#include <linux/mm.h>
@@ -117,6 +120,54 @@ static void xen_read_wallclock(struct timespec64 *ts)
*ts = timespec64_add(now, ts_monotonic);
}
+static int xen_pvclock_gtod_notify(struct notifier_block *nb,
+ unsigned long was_set, void *priv)
+{
+ /* Protected by the calling core code serialization */
+ static struct timespec64 next_sync;
+
+ struct xen_platform_op op;
+ struct timespec64 now, system_time;
+ struct timekeeper *tk = priv;
+
+ now.tv_sec = tk->xtime_sec;
+ now.tv_nsec = (long)(tk->tkr_mono.xtime_nsec >> tk->tkr_mono.shift);
+ system_time = timespec64_add(now, tk->wall_to_monotonic);
+
+ /*
+ * We only take the expensive HV call when the clock was set
+ * or when the 11 minutes RTC synchronization time elapsed.
+ */
+ if (!was_set && timespec64_compare(&now, &next_sync) < 0)
+ return NOTIFY_OK;
+
+ op.interface_version = XENPF_INTERFACE_VERSION;
+ op.cmd = XENPF_settime64;
+ op.u.settime64.mbz = 0;
+ op.u.settime64.secs = now.tv_sec;
+ op.u.settime64.nsecs = now.tv_nsec;
+ op.u.settime64.system_time = timespec64_to_ns(&system_time);
+ printk("GTOD: Setting to %llu.%09u at %llu\n",
+ op.u.settime64.secs,
+ op.u.settime64.nsecs,
+ op.u.settime64.system_time);
+ (void)HYPERVISOR_platform_op(&op);
+
+ /*
+ * Move the next drift compensation time 11 minutes
+ * ahead. That's emulating the sync_cmos_clock() update for
+ * the hardware RTC.
+ */
+ next_sync = now;
+ next_sync.tv_sec += 11 * 60;
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block xen_pvclock_gtod_notifier = {
+ .notifier_call = xen_pvclock_gtod_notify,
+};
+
static void xen_percpu_init(void)
{
struct vcpu_register_vcpu_info info;
@@ -313,7 +364,9 @@ static int __init xen_guest_init(void)
pv_time_ops.steal_clock = xen_stolen_accounting;
static_key_slow_inc(¶virt_steal_enabled);
-
+ if (xen_initial_domain())
+ pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
+
return 0;
}
early_initcall(xen_guest_init);
--
1.7.10.4
Try XENPF_settime64 first, if it is not available fall back to
XENPF_settime32.
No need to call __current_kernel_time() when all the info needed are
already passed via the struct timekeeper * argument.
Return NOTIFY_BAD in case of errors.
Signed-off-by: Stefano Stabellini <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
arch/x86/xen/time.c | 35 ++++++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 3bbd377..4b8af45 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -16,6 +16,7 @@
#include <linux/gfp.h>
#include <linux/slab.h>
#include <linux/pvclock_gtod.h>
+#include <linux/timekeeper_internal.h>
#include <asm/pvclock.h>
#include <asm/xen/hypervisor.h>
@@ -123,9 +124,13 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
static struct timespec next_sync;
struct xen_platform_op op;
- struct timespec now;
+ struct timespec64 now;
+ struct timekeeper *tk = priv;
+ static bool settime64_supported = true;
+ int ret;
- now = __current_kernel_time();
+ now.tv_sec = tk->xtime_sec;
+ now.tv_nsec = (long)(tk->tkr_mono.xtime_nsec >> tk->tkr_mono.shift);
/*
* We only take the expensive HV call when the clock was set
@@ -134,12 +139,28 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
if (!was_set && timespec_compare(&now, &next_sync) < 0)
return NOTIFY_OK;
- op.cmd = XENPF_settime32;
- op.u.settime32.secs = now.tv_sec;
- op.u.settime32.nsecs = now.tv_nsec;
- op.u.settime32.system_time = xen_clocksource_read();
+again:
+ if (settime64_supported) {
+ op.cmd = XENPF_settime64;
+ op.u.settime64.mbz = 0;
+ op.u.settime64.secs = now.tv_sec;
+ op.u.settime64.nsecs = now.tv_nsec;
+ op.u.settime64.system_time = xen_clocksource_read();
+ } else {
+ op.cmd = XENPF_settime32;
+ op.u.settime32.secs = now.tv_sec;
+ op.u.settime32.nsecs = now.tv_nsec;
+ op.u.settime32.system_time = xen_clocksource_read();
+ }
+
+ ret = HYPERVISOR_platform_op(&op);
- (void)HYPERVISOR_platform_op(&op);
+ if (ret == -ENOSYS && settime64_supported) {
+ settime64_supported = false;
+ goto again;
+ }
+ if (ret < 0)
+ return NOTIFY_BAD;
/*
* Move the next drift compensation time 11 minutes
--
1.7.10.4
On Thursday 12 November 2015 17:30:46 Stefano Stabellini wrote:
> Read the wallclock from the shared info page at boot time.
>
> Signed-off-by: Stefano Stabellini <[email protected]>
>
Acked-by: Arnd Bergmann <[email protected]>
On Thursday 12 November 2015 17:30:48 Stefano Stabellini wrote:
> Try XENPF_settime64 first, if it is not available fall back to
> XENPF_settime32.
>
> No need to call __current_kernel_time() when all the info needed are
> already passed via the struct timekeeper * argument.
>
> Return NOTIFY_BAD in case of errors.
>
> Signed-off-by: Stefano Stabellini <[email protected]>
> CC: [email protected]
> CC: [email protected]
> CC: [email protected]
>
Acked-by: Arnd Bergmann <[email protected]>
On 12/11/15 17:30, Stefano Stabellini wrote:
> Try XENPF_settime64 first, if it is not available fall back to
> XENPF_settime32.
>
> No need to call __current_kernel_time() when all the info needed are
> already passed via the struct timekeeper * argument.
>
> Return NOTIFY_BAD in case of errors.
[...]
> @@ -123,9 +124,13 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
> static struct timespec next_sync;
>
> struct xen_platform_op op;
> - struct timespec now;
> + struct timespec64 now;
> + struct timekeeper *tk = priv;
> + static bool settime64_supported = true;
> + int ret;
>
> - now = __current_kernel_time();
> + now.tv_sec = tk->xtime_sec;
> + now.tv_nsec = (long)(tk->tkr_mono.xtime_nsec >> tk->tkr_mono.shift);
I think you should introduce __current_kernel_time64() or make
tk_xtime() available.
John, what do you think?
David
On Fri, 13 Nov 2015, David Vrabel wrote:
> On 12/11/15 17:30, Stefano Stabellini wrote:
> > Try XENPF_settime64 first, if it is not available fall back to
> > XENPF_settime32.
> >
> > No need to call __current_kernel_time() when all the info needed are
> > already passed via the struct timekeeper * argument.
> >
> > Return NOTIFY_BAD in case of errors.
> [...]
> > @@ -123,9 +124,13 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
> > static struct timespec next_sync;
> >
> > struct xen_platform_op op;
> > - struct timespec now;
> > + struct timespec64 now;
> > + struct timekeeper *tk = priv;
> > + static bool settime64_supported = true;
> > + int ret;
> >
> > - now = __current_kernel_time();
> > + now.tv_sec = tk->xtime_sec;
> > + now.tv_nsec = (long)(tk->tkr_mono.xtime_nsec >> tk->tkr_mono.shift);
>
> I think you should introduce __current_kernel_time64() or make
> tk_xtime() available.
>
> John, what do you think?
We already had this discussion:
http://marc.info/?l=linux-kernel&m=144717103112014&w=2
http://marc.info/?l=linux-kernel&m=144724877203864&w=2
Hi Stefano,
On 12/11/15 17:30, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <[email protected]>
>
> ---
>
> Changes in v2:
> - rename dom0_op to platform_op
> ---
> arch/arm/include/asm/xen/hypercall.h | 2 ++
> arch/arm/xen/enlighten.c | 1 +
> arch/arm/xen/hypercall.S | 1 +
> arch/arm64/xen/hypercall.S | 1 +
> 4 files changed, 5 insertions(+)
>
> diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
> index 712b50e..c3e00d0 100644
> --- a/arch/arm/include/asm/xen/hypercall.h
> +++ b/arch/arm/include/asm/xen/hypercall.h
> @@ -35,6 +35,7 @@
>
> #include <xen/interface/xen.h>
> #include <xen/interface/sched.h>
> +#include <xen/interface/platform.h>
>
> long privcmd_call(unsigned call, unsigned long a1,
> unsigned long a2, unsigned long a3,
> @@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
> int HYPERVISOR_physdev_op(int cmd, void *arg);
> int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
> int HYPERVISOR_tmem_op(void *arg);
> +int HYPERVISOR_platform_op(void *arg);
int HYPERVISOR_platform_op(struct xen_platform_op *platform_op) to allow
compiler type checking and match the x86 version.
Also, the implementation of the helper differ from x86. On x86, the
helper takes care of setting the interface_version while here you
request the caller to do it.
It's better if we have similar requirement across the architecture as
this helpers may be called from common code.
> int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
>
> static inline int
Regards,
--
Julien Grall
Hi Stefano,
On 12/11/15 17:30, Stefano Stabellini wrote:
> The hypervisor actually exposes an additional field to struct
> pvclock_wall_clock, with the high 32 bit seconds.
>
> Signed-off-by: Stefano Stabellini <[email protected]>
Reviewed-by: Julien Grall <[email protected]>
Regards,
--
Julien Grall
On 12/11/15 17:30, Stefano Stabellini wrote:
> +static int xen_pvclock_gtod_notify(struct notifier_block *nb,
> + unsigned long was_set, void *priv)
> +{
> + /* Protected by the calling core code serialization */
> + static struct timespec64 next_sync;
> +
> + struct xen_platform_op op;
> + struct timespec64 now, system_time;
> + struct timekeeper *tk = priv;
> +
> + now.tv_sec = tk->xtime_sec;
> + now.tv_nsec = (long)(tk->tkr_mono.xtime_nsec >> tk->tkr_mono.shift);
> + system_time = timespec64_add(now, tk->wall_to_monotonic);
> +
> + /*
> + * We only take the expensive HV call when the clock was set
> + * or when the 11 minutes RTC synchronization time elapsed.
> + */
> + if (!was_set && timespec64_compare(&now, &next_sync) < 0)
> + return NOTIFY_OK;
> +
> + op.interface_version = XENPF_INTERFACE_VERSION;
> + op.cmd = XENPF_settime64;
> + op.u.settime64.mbz = 0;
> + op.u.settime64.secs = now.tv_sec;
> + op.u.settime64.nsecs = now.tv_nsec;
> + op.u.settime64.system_time = timespec64_to_ns(&system_time);
> + printk("GTOD: Setting to %llu.%09u at %llu\n",
> + op.u.settime64.secs,
> + op.u.settime64.nsecs,
> + op.u.settime64.system_time);
Is this printk really useful?
> + (void)HYPERVISOR_platform_op(&op);
> +
> + /*
> + * Move the next drift compensation time 11 minutes
> + * ahead. That's emulating the sync_cmos_clock() update for
> + * the hardware RTC.
> + */
> + next_sync = now;
> + next_sync.tv_sec += 11 * 60;
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block xen_pvclock_gtod_notifier = {
> + .notifier_call = xen_pvclock_gtod_notify,
> +};
> +
> static void xen_percpu_init(void)
> {
> struct vcpu_register_vcpu_info info;
> @@ -313,7 +364,9 @@ static int __init xen_guest_init(void)
>
> pv_time_ops.steal_clock = xen_stolen_accounting;
> static_key_slow_inc(¶virt_steal_enabled);
> -
> + if (xen_initial_domain())
> + pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
> +
I think, you've introduced a trailing whitespace.
> return 0;
> }
> early_initcall(xen_guest_init);
>
Regards,
--
Julien Grall
On Fri, 13 Nov 2015, Julien Grall wrote:
> Hi Stefano,
>
> On 12/11/15 17:30, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <[email protected]>
> >
> > ---
> >
> > Changes in v2:
> > - rename dom0_op to platform_op
> > ---
> > arch/arm/include/asm/xen/hypercall.h | 2 ++
> > arch/arm/xen/enlighten.c | 1 +
> > arch/arm/xen/hypercall.S | 1 +
> > arch/arm64/xen/hypercall.S | 1 +
> > 4 files changed, 5 insertions(+)
> >
> > diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
> > index 712b50e..c3e00d0 100644
> > --- a/arch/arm/include/asm/xen/hypercall.h
> > +++ b/arch/arm/include/asm/xen/hypercall.h
> > @@ -35,6 +35,7 @@
> >
> > #include <xen/interface/xen.h>
> > #include <xen/interface/sched.h>
> > +#include <xen/interface/platform.h>
> >
> > long privcmd_call(unsigned call, unsigned long a1,
> > unsigned long a2, unsigned long a3,
> > @@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
> > int HYPERVISOR_physdev_op(int cmd, void *arg);
> > int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
> > int HYPERVISOR_tmem_op(void *arg);
> > +int HYPERVISOR_platform_op(void *arg);
>
> int HYPERVISOR_platform_op(struct xen_platform_op *platform_op) to allow
> compiler type checking and match the x86 version.
Yeah, I am just following the same pattern as the others
> Also, the implementation of the helper differ from x86. On x86, the
> helper takes care of setting the interface_version while here you
> request the caller to do it.
>
> It's better if we have similar requirement across the architecture as
> this helpers may be called from common code.
I agree with your point (I thought about it myself) but the current
assembly scheme for hypercalls doesn't work well with that. I would have
to introduce, and maintain going forward, two special hypercall
implementations in assembly, one for arm and another for arm64, just to
set interface_version. I don't think it is worth it; I prefer to have to
maintain the explicit interface_version setting at the call sites (that
today is just one).
(CC David, Boris and Konrad)
On 13/11/15 18:10, Stefano Stabellini wrote:
> On Fri, 13 Nov 2015, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 12/11/15 17:30, Stefano Stabellini wrote:
>>> Signed-off-by: Stefano Stabellini <[email protected]>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - rename dom0_op to platform_op
>>> ---
>>> arch/arm/include/asm/xen/hypercall.h | 2 ++
>>> arch/arm/xen/enlighten.c | 1 +
>>> arch/arm/xen/hypercall.S | 1 +
>>> arch/arm64/xen/hypercall.S | 1 +
>>> 4 files changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
>>> index 712b50e..c3e00d0 100644
>>> --- a/arch/arm/include/asm/xen/hypercall.h
>>> +++ b/arch/arm/include/asm/xen/hypercall.h
>>> @@ -35,6 +35,7 @@
>>>
>>> #include <xen/interface/xen.h>
>>> #include <xen/interface/sched.h>
>>> +#include <xen/interface/platform.h>
>>>
>>> long privcmd_call(unsigned call, unsigned long a1,
>>> unsigned long a2, unsigned long a3,
>>> @@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
>>> int HYPERVISOR_physdev_op(int cmd, void *arg);
>>> int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
>>> int HYPERVISOR_tmem_op(void *arg);
>>> +int HYPERVISOR_platform_op(void *arg);
>>
>> int HYPERVISOR_platform_op(struct xen_platform_op *platform_op) to allow
>> compiler type checking and match the x86 version.
>
> Yeah, I am just following the same pattern as the others
It's not necessary to repeat a mistake just because the other does.
If it's possible to use struct xen_platform_op, then please do it.
Having extra safety from the compiler could be useful to spot stupid
mistake.
>
>> Also, the implementation of the helper differ from x86. On x86, the
>> helper takes care of setting the interface_version while here you
>> request the caller to do it.
>>
>> It's better if we have similar requirement across the architecture as
>> this helpers may be called from common code.
>
> I agree with your point (I thought about it myself) but the current
> assembly scheme for hypercalls doesn't work well with that. I would have
> to introduce, and maintain going forward, two special hypercall
> implementations in assembly, one for arm and another for arm64, just to
> set interface_version. I don't think it is worth it; I prefer to have to
> maintain the explicit interface_version setting at the call sites (that
> today is just one).
Sooner of later we will support EFI runtime and co. There are ~10
references of this platform op hypercall in the common code ([1]), and I
wouldn't be surprised if it continues to grow.
So every single time someone is using the hypercall in the code we would
have to remember that the x86 and ARM implementation is different. Are
you ready to review every future patches to check that this will work as
expected? IHMO, this will be a pain compare to maintaining a different
set of hypercall.
I really think we need to have the same behavior for all the hypercalls
helpers accross all the architectures. This is really helpful when
implementation arch-agnostic driver.
Regards,
[1] http://lxr.free-electrons.com/ident?i=HYPERVISOR_dom0_op
--
Julien Grall
On Fri, 2015-11-13 at 18:10 +0000, Stefano Stabellini wrote:
>
> I agree with your point (I thought about it myself) but the current
> assembly scheme for hypercalls doesn't work well with that. I would have
> to introduce, and maintain going forward, two special hypercall
> implementations in assembly, one for arm and another for arm64, just to
> set interface_version. I don't think it is worth it; I prefer to have to
> maintain the explicit interface_version setting at the call sites (that
> today is just one).
You could give the bare assembly stub a different name (append _core or
_raw or something) and make HYPERVISOR_platform_op a C wrapper for it which
DTRT.
Ian.
On Mon, 16 Nov 2015, Ian Campbell wrote:
> On Fri, 2015-11-13 at 18:10 +0000, Stefano Stabellini wrote:
> >
> > I agree with your point (I thought about it myself) but the current
> > assembly scheme for hypercalls doesn't work well with that. I would have
> > to introduce, and maintain going forward, two special hypercall
> > implementations in assembly, one for arm and another for arm64, just to
> > set interface_version. I don't think it is worth it; I prefer to have to
> > maintain the explicit interface_version setting at the call sites (that
> > today is just one).
>
> You could give the bare assembly stub a different name (append _core or
> _raw or something) and make HYPERVISOR_platform_op a C wrapper for it which
> DTRT.
I had an idea. I just need to
#define HYPERVISOR_platform_op_raw HYPERVISOR_platform_op
then the small wrapper can work:
diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index c3e00d0..d769972 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -50,7 +50,12 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
int HYPERVISOR_physdev_op(int cmd, void *arg);
int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
int HYPERVISOR_tmem_op(void *arg);
-int HYPERVISOR_platform_op(void *arg);
+int HYPERVISOR_platform_op_raw(void *arg);
+static inline int HYPERVISOR_platform_op(struct xen_platform_op *op)
+{
+ op->interface_version = XENPF_INTERFACE_VERSION;
+ return HYPERVISOR_platform_op_raw(op);
+}
int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
static inline int
diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 5b150d7..75d5968 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -27,6 +27,8 @@
(hnd).p = val; \
} while (0)
+#define __HYPERVISOR_platform_op_raw __HYPERVISOR_platform_op
+
#ifndef __ASSEMBLY__
/* Explicitly size integers that represent pfns in the interface with
* Xen so that we can have one ABI that works for 32 and 64 bit guests.
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index f964238..0f7be84 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -141,7 +141,6 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
if (!was_set && timespec64_compare(&now, &next_sync) < 0)
return NOTIFY_OK;
- op.interface_version = XENPF_INTERFACE_VERSION;
op.cmd = XENPF_settime64;
op.u.settime64.mbz = 0;
op.u.settime64.secs = now.tv_sec;
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index d4539f4..9a36f4f 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -89,7 +89,7 @@ HYPERCALL2(memory_op);
HYPERCALL2(physdev_op);
HYPERCALL3(vcpu_op);
HYPERCALL1(tmem_op);
-HYPERCALL1(platform_op);
+HYPERCALL1(platform_op_raw);
HYPERCALL2(multicall);
ENTRY(privcmd_call)
diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S
index f7d5724..70df80e 100644
--- a/arch/arm64/xen/hypercall.S
+++ b/arch/arm64/xen/hypercall.S
@@ -80,7 +80,7 @@ HYPERCALL2(memory_op);
HYPERCALL2(physdev_op);
HYPERCALL3(vcpu_op);
HYPERCALL1(tmem_op);
-HYPERCALL1(platform_op);
+HYPERCALL1(platform_op_raw);
HYPERCALL2(multicall);
ENTRY(privcmd_call)
On Fri, 2015-11-20 at 11:58 +0000, Stefano Stabellini wrote:
> On Mon, 16 Nov 2015, Ian Campbell wrote:
> > On Fri, 2015-11-13 at 18:10 +0000, Stefano Stabellini wrote:
> > >
> > > I agree with your point (I thought about it myself) but the current
> > > assembly scheme for hypercalls doesn't work well with that. I would
> > > have
> > > to introduce, and maintain going forward, two special hypercall
> > > implementations in assembly, one for arm and another for arm64, just
> > > to
> > > set interface_version. I don't think it is worth it; I prefer to have
> > > to
> > > maintain the explicit interface_version setting at the call sites
> > > (that
> > > today is just one).
> >
> > You could give the bare assembly stub a different name (append _core or
> > _raw or something) and make HYPERVISOR_platform_op a C wrapper for it
> > which
> > DTRT.
>
> I had an idea. I just need to
>
> #define HYPERVISOR_platform_op_raw HYPERVISOR_platform_op
The need for this #define is a bit unfortunate, but the alternatives (e.g.
a suffix argument to the HYPERCALL*() macros or a RAWHYPERCALL variant)
would seem to suck more, so I say go for it.
Ian.