ARM/KVM: save and restore generic timer registers

For migration to work we need to save (and later restore) the state of each core's virtual generic timer.
Since this is per VCPU, we can use the [gs]et_one_reg ioctl and export the three needed registers (control, counter, compare value). Though they live in cp15 space, we don't use the existing list, since they need special accessor functions and also the arch timer is optional.

Changes from v1:
- move code out of coproc.c and into guest.c and arch_timer.c
- present the registers with their native CP15 addresses, but without using space in the VCPU's cp15 array
- do the user space copying in the accessor functions

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/kvm_host.h |  5 ++++
 arch/arm/include/uapi/asm/kvm.h | 16 ++++++++++
 arch/arm/kvm/arch_timer.c       | 65 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/guest.c            | 26 ++++++++++++++++-
 4 files changed, 111 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_host.h +5 -0

index 57cb786..1096e33 100644
@@ -224,4 +224,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+int kvm_arm_num_timer_regs(void);
+int kvm_arm_copy_timer_indices(struct kvm_vcpu *, u64 __user *);
+int kvm_arm_timer_get_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+int kvm_arm_timer_set_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+
 #endif /* __ARM_KVM_HOST_H__ */

arch/arm/include/uapi/asm/kvm.h +16 -0

index c1ee007..e3b0115 100644
@@ -118,6 +118,22 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
 #define KVM_REG_ARM_32_CRN_SHIFT	11
 
+#define KVM_REG_ARM_32_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U32      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_64_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U64      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_TIMER_CTL		(KVM_REG_ARM_32_CP15                 | \
+					( 3ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					(14ULL << KVM_REG_ARM_32_CRN_SHIFT)  | \
+					( 0ULL << KVM_REG_ARM_OPC1_SHIFT)    | \
+					( 1ULL << KVM_REG_ARM_32_OPC2_SHIFT))
+#define KVM_REG_ARM_TIMER_CNT		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 1ULL << KVM_REG_ARM_OPC1_SHIFT))
+#define KVM_REG_ARM_TIMER_CVAL		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 3ULL << KVM_REG_ARM_OPC1_SHIFT))
+
 /* Normal registers are mapped as coprocessor 16. */
 #define KVM_REG_ARM_CORE		(0x0010 << KVM_REG_ARM_COPROC_SHIFT)
 #define KVM_REG_ARM_CORE_REG(name)	(offsetof(struct kvm_regs, name) / 4)

arch/arm/kvm/arch_timer.c +65 -0

index c55b608..8d709eb 100644
@@ -18,6 +18,7 @@
 
 #include <linux/cpu.h>
 #include <linux/of_irq.h>
+#include <linux/uaccess.h>
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>

@@ -171,6 +172,70 @@ static void kvm_timer_init_interrupt(void *info)
 	enable_percpu_irq(timer_irq.irq, 0);
 }
 
+int kvm_arm_num_timer_regs(void)
+{
+	return 3;
+}
+
+int kvm_arm_copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	if (put_user(KVM_REG_ARM_TIMER_CTL, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CNT, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices))
+		return -EFAULT;
+
+	return 0;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int ret;
+
+	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
+	if (ret != 0)
+		return ret;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = val;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;
+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		timer->cntv_cval = val;
+		break;
+	}
+
+	return 0;
+}
+
+int kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		val = timer->cntv_ctl;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		val = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		val = timer->cntv_cval;
+		break;
+	}
+	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
+}
 
 static int kvm_timer_cpu_notify(struct notifier_block *self,
 				unsigned long action, void *cpu)

arch/arm/kvm/guest.c +22 -1

index 152d036..a50ffb6 100644
@@ -121,7 +121,8 @@ static unsigned long num_core_regs(void)
  */
 unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
 {
-	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu);
+	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu)
+		+ kvm_arm_num_timer_regs();
 }
 
 /**

@@ -133,6 +134,7 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
 	unsigned int i;
 	const u64 core_reg = KVM_REG_ARM | KVM_REG_SIZE_U32 | KVM_REG_ARM_CORE;
+	int ret;
 
 	for (i = 0; i < sizeof(struct kvm_regs)/sizeof(u32); i++) {
 		if (put_user(core_reg | i, uindices))

@@ -140,9 +142,25 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
+	ret = kvm_arm_copy_timer_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += kvm_arm_num_timer_regs();
+
 	return kvm_arm_copy_coproc_indices(vcpu, uindices);
 }
 
+static bool is_timer_reg(u64 index)
+{
+	switch (index) {
+	case KVM_REG_ARM_TIMER_CTL:
+	case KVM_REG_ARM_TIMER_CNT:
+	case KVM_REG_ARM_TIMER_CVAL:
+		return true;
+	}
+	return false;
+}
+
 int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	/* We currently use nothing arch-specific in upper 32 bits */

@@ -153,6 +171,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return get_core_reg(vcpu, reg);
 
+	if (is_timer_reg(reg->id))
+		return kvm_arm_timer_get_reg(vcpu, reg);
+
 	return kvm_arm_coproc_get_reg(vcpu, reg);
 }
 

For migration to work we need to save (and later restore) the state of each core's virtual generic timer.
Since this is per VCPU, we can use the [gs]et_one_reg ioctl and export the three needed registers (control, counter, compare value). Though they live in cp15 space, we don't use the existing list, since they need special accessor functions and also the arch timer is optional.

Changes from v1:
- move code out of coproc.c and into guest.c and arch_timer.c
- present the registers with their native CP15 addresses, but without using space in the VCPU's cp15 array
- do the user space copying in the accessor functions

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/kvm_host.h |  5 ++++
 arch/arm/include/uapi/asm/kvm.h | 16 ++++++++++
 arch/arm/kvm/arch_timer.c       | 65 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/guest.c            | 26 ++++++++++++++++-
 4 files changed, 111 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_host.h +5 -0

index 57cb786..1096e33 100644
@@ -224,4 +224,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+int kvm_arm_num_timer_regs(void);
+int kvm_arm_copy_timer_indices(struct kvm_vcpu *, u64 __user *);
+int kvm_arm_timer_get_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+int kvm_arm_timer_set_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+
 #endif /* __ARM_KVM_HOST_H__ */

arch/arm/include/uapi/asm/kvm.h +16 -0

index c1ee007..e3b0115 100644
@@ -118,6 +118,22 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
 #define KVM_REG_ARM_32_CRN_SHIFT	11
 
+#define KVM_REG_ARM_32_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U32      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_64_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U64      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_TIMER_CTL		(KVM_REG_ARM_32_CP15                 | \
+					( 3ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					(14ULL << KVM_REG_ARM_32_CRN_SHIFT)  | \
+					( 0ULL << KVM_REG_ARM_OPC1_SHIFT)    | \
+					( 1ULL << KVM_REG_ARM_32_OPC2_SHIFT))
+#define KVM_REG_ARM_TIMER_CNT		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 1ULL << KVM_REG_ARM_OPC1_SHIFT))
+#define KVM_REG_ARM_TIMER_CVAL		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 3ULL << KVM_REG_ARM_OPC1_SHIFT))
+
 /* Normal registers are mapped as coprocessor 16. */
 #define KVM_REG_ARM_CORE		(0x0010 << KVM_REG_ARM_COPROC_SHIFT)
 #define KVM_REG_ARM_CORE_REG(name)	(offsetof(struct kvm_regs, name) / 4)

arch/arm/kvm/arch_timer.c +6 -0

index c55b608..8d709eb 100644
@@ -18,6 +18,7 @@
 
 #include <linux/cpu.h>
 #include <linux/of_irq.h>
+#include <linux/uaccess.h>
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>

@@ -171,6 +172,70 @@ static void kvm_timer_init_interrupt(void *info)
 	enable_percpu_irq(timer_irq.irq, 0);
 }
 
+int kvm_arm_num_timer_regs(void)
+{
+	return 3;
+}
+

arch/arm/kvm/arch_timer.c +10 -0

+	return 0;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int ret;
+

arch/arm/kvm/arch_timer.c +28 -0

+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		timer->cntv_cval = val;
+		break;
+	}
+
+	return 0;
+}
+
+int kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		val = timer->cntv_ctl;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		val = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		val = timer->cntv_cval;
+		break;
+	}
+	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
+}
 
 static int kvm_timer_cpu_notify(struct notifier_block *self,
 				unsigned long action, void *cpu)

arch/arm/kvm/guest.c +22 -1

index 152d036..a50ffb6 100644
@@ -121,7 +121,8 @@ static unsigned long num_core_regs(void)
  */
 unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
 {
-	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu);
+	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu)
+		+ kvm_arm_num_timer_regs();
 }
 
 /**

@@ -133,6 +134,7 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
 	unsigned int i;
 	const u64 core_reg = KVM_REG_ARM | KVM_REG_SIZE_U32 | KVM_REG_ARM_CORE;
+	int ret;
 
 	for (i = 0; i < sizeof(struct kvm_regs)/sizeof(u32); i++) {
 		if (put_user(core_reg | i, uindices))

@@ -140,9 +142,25 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
+	ret = kvm_arm_copy_timer_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += kvm_arm_num_timer_regs();
+
 	return kvm_arm_copy_coproc_indices(vcpu, uindices);
 }
 
+static bool is_timer_reg(u64 index)
+{
+	switch (index) {
+	case KVM_REG_ARM_TIMER_CTL:
+	case KVM_REG_ARM_TIMER_CNT:
+	case KVM_REG_ARM_TIMER_CVAL:
+		return true;
+	}
+	return false;
+}
+
 int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	/* We currently use nothing arch-specific in upper 32 bits */

@@ -153,6 +171,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return get_core_reg(vcpu, reg);
 
+	if (is_timer_reg(reg->id))
+		return kvm_arm_timer_get_reg(vcpu, reg);
+
 	return kvm_arm_coproc_get_reg(vcpu, reg);
 }
 

Quoted 84 lines · Andre Przywara

For migration to work we need to save (and later restore) the state of each core's virtual generic timer.
Since this is per VCPU, we can use the [gs]et_one_reg ioctl and export the three needed registers (control, counter, compare value). Though they live in cp15 space, we don't use the existing list, since they need special accessor functions and also the arch timer is optional.

Changes from v1:
- move code out of coproc.c and into guest.c and arch_timer.c
- present the registers with their native CP15 addresses, but without using space in the VCPU's cp15 array
- do the user space copying in the accessor functions

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/kvm_host.h |  5 ++++
 arch/arm/include/uapi/asm/kvm.h | 16 ++++++++++
 arch/arm/kvm/arch_timer.c       | 65 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/guest.c            | 26 ++++++++++++++++-
 4 files changed, 111 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_host.h +5 -0

index 57cb786..1096e33 100644
@@ -224,4 +224,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+int kvm_arm_num_timer_regs(void);
+int kvm_arm_copy_timer_indices(struct kvm_vcpu *, u64 __user *);
+int kvm_arm_timer_get_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+int kvm_arm_timer_set_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+
 #endif /* __ARM_KVM_HOST_H__ */

arch/arm/include/uapi/asm/kvm.h +16 -0

index c1ee007..e3b0115 100644
@@ -118,6 +118,22 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
 #define KVM_REG_ARM_32_CRN_SHIFT	11
 
+#define KVM_REG_ARM_32_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U32      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_64_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U64      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_TIMER_CTL		(KVM_REG_ARM_32_CP15                 | \
+					( 3ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					(14ULL << KVM_REG_ARM_32_CRN_SHIFT)  | \
+					( 0ULL << KVM_REG_ARM_OPC1_SHIFT)    | \
+					( 1ULL << KVM_REG_ARM_32_OPC2_SHIFT))
+#define KVM_REG_ARM_TIMER_CNT		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 1ULL << KVM_REG_ARM_OPC1_SHIFT))
+#define KVM_REG_ARM_TIMER_CVAL		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 3ULL << KVM_REG_ARM_OPC1_SHIFT))
+
 /* Normal registers are mapped as coprocessor 16. */
 #define KVM_REG_ARM_CORE		(0x0010 << KVM_REG_ARM_COPROC_SHIFT)
 #define KVM_REG_ARM_CORE_REG(name)	(offsetof(struct kvm_regs, name) / 4)

arch/arm/kvm/arch_timer.c +6 -0

index c55b608..8d709eb 100644
@@ -18,6 +18,7 @@
 
 #include <linux/cpu.h>
 #include <linux/of_irq.h>
+#include <linux/uaccess.h>
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>

@@ -171,6 +172,70 @@ static void kvm_timer_init_interrupt(void *info)
 	enable_percpu_irq(timer_irq.irq, 0);
 }
 
+int kvm_arm_num_timer_regs(void)
+{
+	return 3;
+}
+

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 189

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+int kvm_arm_copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	if (put_user(KVM_REG_ARM_TIMER_CTL, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CNT, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices))
+		return -EFAULT;

So these macros are going to break arm64. Any chance you could introduce them at the same time on both platforms? The rest of the work can be delayed, but you shouldn't break arm64 (you'd expect me to say that, wouldn't you? ;-).

Also, I'd like to keep userspace access out of the timer code itself. Low level code shouldn't have to know about that. Can you create proper accessors instead, and move whole userspace access to coproc.c?

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+	return 0;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int ret;
+

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 210

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
+	if (ret != 0)
+		return ret;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = val;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;

I just realized what bothers me here: You're computing cntvoff on a per vcpu basis, while this is a VM property. Which means that as you're restoring vcpus, you'll be changing cntvoff - sounds like a bad idea to me.

The counter is really global. Do we have a way to handle VM-wide registers? I think Christoffer was trying to some a similar thing with the GIC...

Quoted 98 lines

arch/arm/kvm/arch_timer.c +28 -0

+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		timer->cntv_cval = val;
+		break;
+	}
+
+	return 0;
+}
+
+int kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		val = timer->cntv_ctl;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		val = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		val = timer->cntv_cval;
+		break;
+	}
+	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
+}
 
 static int kvm_timer_cpu_notify(struct notifier_block *self,
 				unsigned long action, void *cpu)

arch/arm/kvm/guest.c +25 -1

index 152d036..a50ffb6 100644
@@ -121,7 +121,8 @@ static unsigned long num_core_regs(void)
  */
 unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
 {
-	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu);
+	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu)
+		+ kvm_arm_num_timer_regs();
 }
 
 /**

@@ -133,6 +134,7 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
 	unsigned int i;
 	const u64 core_reg = KVM_REG_ARM | KVM_REG_SIZE_U32 | KVM_REG_ARM_CORE;
+	int ret;
 
 	for (i = 0; i < sizeof(struct kvm_regs)/sizeof(u32); i++) {
 		if (put_user(core_reg | i, uindices))

@@ -140,9 +142,25 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
+	ret = kvm_arm_copy_timer_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += kvm_arm_num_timer_regs();
+
 	return kvm_arm_copy_coproc_indices(vcpu, uindices);
 }
 
+static bool is_timer_reg(u64 index)
+{
+	switch (index) {
+	case KVM_REG_ARM_TIMER_CTL:
+	case KVM_REG_ARM_TIMER_CNT:
+	case KVM_REG_ARM_TIMER_CVAL:
+		return true;
+	}
+	return false;
+}
+
 int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	/* We currently use nothing arch-specific in upper 32 bits */

@@ -153,6 +171,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return get_core_reg(vcpu, reg);
 
+	if (is_timer_reg(reg->id))
+		return kvm_arm_timer_get_reg(vcpu, reg);
+
 	return kvm_arm_coproc_get_reg(vcpu, reg);
 }
 

@@ -166,6 +187,9 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return set_core_reg(vcpu, reg);
 
+	if (is_timer_reg(reg->id))
+		return kvm_arm_timer_set_reg(vcpu, reg);
+
 	return kvm_arm_coproc_set_reg(vcpu, reg);
 }

Quoted 101 lines · Marc Zyngier

Quoted 84 lines · Andre Przywara

For migration to work we need to save (and later restore) the state of each core's virtual generic timer.
Since this is per VCPU, we can use the [gs]et_one_reg ioctl and export the three needed registers (control, counter, compare value). Though they live in cp15 space, we don't use the existing list, since they need special accessor functions and also the arch timer is optional.

Changes from v1:
- move code out of coproc.c and into guest.c and arch_timer.c
- present the registers with their native CP15 addresses, but without using space in the VCPU's cp15 array
- do the user space copying in the accessor functions

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/kvm_host.h |  5 ++++
 arch/arm/include/uapi/asm/kvm.h | 16 ++++++++++
 arch/arm/kvm/arch_timer.c       | 65 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/guest.c            | 26 ++++++++++++++++-
 4 files changed, 111 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_host.h +5 -0

index 57cb786..1096e33 100644
@@ -224,4 +224,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+int kvm_arm_num_timer_regs(void);
+int kvm_arm_copy_timer_indices(struct kvm_vcpu *, u64 __user *);
+int kvm_arm_timer_get_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+int kvm_arm_timer_set_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
+
 #endif /* __ARM_KVM_HOST_H__ */

arch/arm/include/uapi/asm/kvm.h +16 -0

index c1ee007..e3b0115 100644
@@ -118,6 +118,22 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
 #define KVM_REG_ARM_32_CRN_SHIFT	11
 
+#define KVM_REG_ARM_32_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U32      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_64_CP15		(KVM_REG_ARM | KVM_REG_SIZE_U64      | \
+					(15ULL << KVM_REG_ARM_COPROC_SHIFT))
+#define KVM_REG_ARM_TIMER_CTL		(KVM_REG_ARM_32_CP15                 | \
+					( 3ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					(14ULL << KVM_REG_ARM_32_CRN_SHIFT)  | \
+					( 0ULL << KVM_REG_ARM_OPC1_SHIFT)    | \
+					( 1ULL << KVM_REG_ARM_32_OPC2_SHIFT))
+#define KVM_REG_ARM_TIMER_CNT		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 1ULL << KVM_REG_ARM_OPC1_SHIFT))
+#define KVM_REG_ARM_TIMER_CVAL		(KVM_REG_ARM_64_CP15 | \
+					(14ULL << KVM_REG_ARM_CRM_SHIFT)     | \
+					( 3ULL << KVM_REG_ARM_OPC1_SHIFT))
+
 /* Normal registers are mapped as coprocessor 16. */
 #define KVM_REG_ARM_CORE		(0x0010 << KVM_REG_ARM_COPROC_SHIFT)
 #define KVM_REG_ARM_CORE_REG(name)	(offsetof(struct kvm_regs, name) / 4)

arch/arm/kvm/arch_timer.c +6 -0

index c55b608..8d709eb 100644
@@ -18,6 +18,7 @@
 
 #include <linux/cpu.h>
 #include <linux/of_irq.h>
+#include <linux/uaccess.h>
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>

@@ -171,6 +172,70 @@ static void kvm_timer_init_interrupt(void *info)
 	enable_percpu_irq(timer_irq.irq, 0);
 }
 
+int kvm_arm_num_timer_regs(void)
+{
+	return 3;
+}
+

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 189

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+int kvm_arm_copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	if (put_user(KVM_REG_ARM_TIMER_CTL, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CNT, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices))
+		return -EFAULT;

So these macros are going to break arm64. Any chance you could introduce them at the same time on both platforms? The rest of the work can be delayed, but you shouldn't break arm64 (you'd expect me to say that, wouldn't you? ;-).

Quoted 4 lines

Also, I'd like to keep userspace access out of the timer code itself. Low level code shouldn't have to know about that. Can you create proper accessors instead, and move whole userspace access to coproc.c?

I'm fine with this, coproc.c or guest.c - either way.

Quoted 25 lines

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+	return 0;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int ret;
+

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 210

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
+	if (ret != 0)
+		return ret;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = val;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;

I just realized what bothers me here: You're computing cntvoff on a per vcpu basis, while this is a VM property. Which means that as you're restoring vcpus, you'll be changing cntvoff - sounds like a bad idea to me.

Quoted 4 lines

The counter is really global. Do we have a way to handle VM-wide registers? I think Christoffer was trying to some a similar thing with the GIC...

We do have a way, but it requires user space to create a device and keep track of the device fd just to set/get a single register, which seems like overkill to me.

I suggest you do one of two things:
1. Whenever this value is written, make sure it's written across all vcpus, so guests always have a consistent view of time (be careful about synchronization here).
2. Move the cntvoff value to the vm struct instead, so there's only one offset and a consistent view of time. This may have an adverse effect on the world-switch code performance, but I suspect it would completely disappear in the noise.

I dont' feel strongly about either approach.

Quoted 40 lines

Quoted 22 lines

Quoted 7 lines

arch/arm/kvm/arch_timer.c +7 -0

+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int ret;
+

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 210

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
+	if (ret != 0)
+		return ret;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = val;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;

I just realized what bothers me here: You're computing cntvoff on a per vcpu basis, while this is a VM property. Which means that as you're restoring vcpus, you'll be changing cntvoff - sounds like a bad idea to me.

Quoted 4 lines

The counter is really global. Do we have a way to handle VM-wide registers? I think Christoffer was trying to some a similar thing with the GIC...

We do have a way, but it requires user space to create a device and keep track of the device fd just to set/get a single register, which seems like overkill to me.

I suggest you do one of two things:
1. Whenever this value is written, make sure it's written across all vcpus, so guests always have a consistent view of time (be careful about synchronization here).
2. Move the cntvoff value to the vm struct instead, so there's only one offset and a consistent view of time. This may have an adverse effect on the world-switch code performance, but I suspect it would completely disappear in the noise.

Quoted 1 line

I dont' feel strongly about either approach.

So it turns out I've completely forgotten about that - cntvoff is already per-VM (the indirection shows it). Doh.

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Quoted 6 lines

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Do we need to? User space is free to modify time and all sort of other registers at any point during VM execution - it will just break the guest that it's running.

I think the key here is that we expect the VM to be stopped for all save/restore operations (we can enforce it if we want to, which I am going to for the VGIC state, because we don't want to interfere with consistent state being written to the hardware).

Quoted 5 lines · Marc

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Do we need to? User space is free to modify time and all sort of other registers at any point during VM execution - it will just break the guest that it's running.

Quoted 10 lines · Christoffer Dall

Quoted 5 lines · Marc

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Do we need to? User space is free to modify time and all sort of other registers at any point during VM execution - it will just break the guest that it's running.

Note that QEMU will stop all CPUs before doing a migration or similar operation. However there is a monitor command to query the current CPU registers etc which won't try to stop the VM first. So we might try to read vcpu registers (though I hope we don't allow writing them).

Quoted 12 lines · Peter Maydell

Quoted 10 lines · Christoffer Dall

Quoted 5 lines · Marc

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Do we need to? User space is free to modify time and all sort of other registers at any point during VM execution - it will just break the guest that it's running.

Quoted 6 lines

Note that QEMU will stop all CPUs before doing a migration or similar operation. However there is a monitor command to query the current CPU registers etc which won't try to stop the VM first. So we might try to read vcpu registers (though I hope we don't allow writing them).

Sounds like we need to add a -EBUSY return on SET_ONE_REG if the VM is running.

Quoted 19 lines · Christoffer Dall

Quoted 18 lines · Peter Maydell

Quoted 10 lines · Christoffer Dall

Quoted 5 lines · Marc

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Do we need to? User space is free to modify time and all sort of other registers at any point during VM execution - it will just break the guest that it's running.

Note that QEMU will stop all CPUs before doing a migration or similar operation. However there is a monitor command to query the current CPU registers etc which won't try to stop the VM first. So we might try to read vcpu registers (though I hope we don't allow writing them).

Quoted 2 lines

Sounds like we need to add a -EBUSY return on SET_ONE_REG if the VM is running.

The ONE_REG API should already be protected here, as it does vcpu_load() in kvm_vcpu_ioctl(). So a separate thread can't possibly do ONE_REG accesses while another thread has the same vcpu running.

Quoted 25 lines · Alexander Graf

Quoted 21 lines · Christoffer Dall

Quoted 12 lines · Peter Maydell

Quoted 10 lines · Christoffer Dall

Quoted 5 lines · Marc

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Do we need to? User space is free to modify time and all sort of other registers at any point during VM execution - it will just break the guest that it's running.

Quoted 6 lines

Note that QEMU will stop all CPUs before doing a migration or similar operation. However there is a monitor command to query the current CPU registers etc which won't try to stop the VM first. So we might try to read vcpu registers (though I hope we don't allow writing them).

Sounds like we need to add a -EBUSY return on SET_ONE_REG if the VM is running.

Quoted 3 lines

The ONE_REG API should already be protected here, as it does vcpu_load() in kvm_vcpu_ioctl(). So a separate thread can't possibly do ONE_REG accesses while another thread has the same vcpu running.

Doesn't protect you against confusion due to another thread running a different vcpu in the same vm, though.

Quoted 26 lines · Peter Maydell

Quoted 25 lines · Alexander Graf

Quoted 21 lines · Christoffer Dall

Quoted 12 lines · Peter Maydell

Quoted 10 lines · Christoffer Dall

Quoted 5 lines · Marc

So there is just one thing we absolutely need to make sure here: no vcpu can run before they've all had their timer restored, and hence a stable cntvoff. Otherwise two vcpus will have a different view of time.

Can we guarantee this?

Do we need to? User space is free to modify time and all sort of other registers at any point during VM execution - it will just break the guest that it's running.

Quoted 6 lines

Note that QEMU will stop all CPUs before doing a migration or similar operation. However there is a monitor command to query the current CPU registers etc which won't try to stop the VM first. So we might try to read vcpu registers (though I hope we don't allow writing them).

Sounds like we need to add a -EBUSY return on SET_ONE_REG if the VM is running.

Quoted 6 lines

Quoted 3 lines

The ONE_REG API should already be protected here, as it does vcpu_load() in kvm_vcpu_ioctl(). So a separate thread can't possibly do ONE_REG accesses while another thread has the same vcpu running.

Doesn't protect you against confusion due to another thread running a different vcpu in the same vm, though.

Ah, different ONE_REG API. Can't you just notify all vcpus to exit and refresh their timers? That's what kvm_make_request() is there for, no?

Quoted 14 lines

...

arch/arm/kvm/arch_timer.c +12 -0

+int kvm_arm_num_timer_regs(void)
+{
+	return 3;
+}
+
+int kvm_arm_copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	if (put_user(KVM_REG_ARM_TIMER_CTL, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CNT, uindices))
+		return -EFAULT;

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 189

Quoted 3 lines

arch/arm/kvm/arch_timer.c +3 -0

+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices))
+		return -EFAULT;

So these macros are going to break arm64. Any chance you could introduce them at the same time on both platforms? The rest of the work can be delayed, but you shouldn't break arm64 (you'd expect me to say that, wouldn't you? ;-).

Also, I'd like to keep userspace access out of the timer code itself. Low level code shouldn't have to know about that. Can you create proper accessors instead, and move whole userspace access to coproc.c?

Quoted 11 lines

arch/arm/kvm/arch_timer.c +10 -0

+	return 0;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int ret;
+

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 210

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
+	if (ret != 0)
+		return ret;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = val;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;

I just realized what bothers me here: You're computing cntvoff on a per vcpu basis, while this is a VM property. Which means that as you're restoring vcpus, you'll be changing cntvoff - sounds like a bad idea to me.

The counter is really global. Do we have a way to handle VM-wide registers? I think Christoffer was trying to some a similar thing with the GIC...

Quoted 91 lines

arch/arm/kvm/arch_timer.c +28 -0

+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		timer->cntv_cval = val;
+		break;
+	}
+
+	return 0;
+}
+
+int kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		val = timer->cntv_ctl;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		val = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		val = timer->cntv_cval;
+		break;
+	}
+	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
+}
 
  static int kvm_timer_cpu_notify(struct notifier_block *self,
  				unsigned long action, void *cpu)

arch/arm/kvm/guest.c +22 -1

index 152d036..a50ffb6 100644
@@ -121,7 +121,8 @@ static unsigned long num_core_regs(void)
   */
  unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
  {
-	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu);
+	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu)
+		+ kvm_arm_num_timer_regs();
  }
 
  /**

@@ -133,6 +134,7 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
  {
  	unsigned int i;
  	const u64 core_reg = KVM_REG_ARM | KVM_REG_SIZE_U32 | KVM_REG_ARM_CORE;
+	int ret;
 
  	for (i = 0; i < sizeof(struct kvm_regs)/sizeof(u32); i++) {
  		if (put_user(core_reg | i, uindices))

@@ -140,9 +142,25 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
  		uindices++;
  	}
 
+	ret = kvm_arm_copy_timer_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += kvm_arm_num_timer_regs();
+
  	return kvm_arm_copy_coproc_indices(vcpu, uindices);
  }
 
+static bool is_timer_reg(u64 index)
+{
+	switch (index) {
+	case KVM_REG_ARM_TIMER_CTL:
+	case KVM_REG_ARM_TIMER_CNT:
+	case KVM_REG_ARM_TIMER_CVAL:
+		return true;
+	}
+	return false;
+}
+
  int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
  {
  	/* We currently use nothing arch-specific in upper 32 bits */

@@ -153,6 +171,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
  		return get_core_reg(vcpu, reg);
 
+	if (is_timer_reg(reg->id))
+		return kvm_arm_timer_get_reg(vcpu, reg);
+
  	return kvm_arm_coproc_get_reg(vcpu, reg);
  }
 

arch/arm/kvm/guest.c int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) · line 194

Quoted 8 lines

arch/arm/kvm/guest.c +3 -0

@@ -166,6 +187,9 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
  		return set_core_reg(vcpu, reg);
 
+	if (is_timer_reg(reg->id))
+		return kvm_arm_timer_set_reg(vcpu, reg);
+
  	return kvm_arm_coproc_set_reg(vcpu, reg);
  }

This is otherwise moving in the right direction.

Thanks,

	M.

Hi Marc,

Quoted 14 lines

Quoted 14 lines

...

arch/arm/kvm/arch_timer.c +12 -0

+int kvm_arm_num_timer_regs(void)
+{
+	return 3;
+}
+
+int kvm_arm_copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	if (put_user(KVM_REG_ARM_TIMER_CTL, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CNT, uindices))
+		return -EFAULT;

Quoted 8 lines

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 189

Quoted 3 lines

arch/arm/kvm/arch_timer.c +3 -0

+	uindices++;
+	if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices))
+		return -EFAULT;

So these macros are going to break arm64. Any chance you could introduce them at the same time on both platforms? The rest of the work can be delayed, but you shouldn't break arm64 (you'd expect me to say that, wouldn't you? ;-).

Is that just due to KVM_REG_ARM instead of KVM_REG_ARM64?
Or do you expect the numbering to be completely different since there is no mrc/mcr anymore (IIRC)?

Is put_user an issue here (should not, right?)

Is there already a document describing arch timer access on AARCH64?

If I am thinking in a totally wrong direction, please bear with me and feel free to point me to the right one ;-)
/me is now looking at getting a cross compiler to see what you mean...

Quoted 3 lines

Also, I'd like to keep userspace access out of the timer code itself. Low level code shouldn't have to know about that. Can you create proper accessors instead, and move whole userspace access to coproc.c?

IIRC Christoffer recommended to keep this code completely out of coproc.c ;-) This also helps to keep coproc.c clean of ARCH_TIMER ifdefs in coproc.c (which I completely forgot in this version, btw, but that's already fixed).

Quoted 26 lines

Quoted 11 lines

arch/arm/kvm/arch_timer.c +10 -0

+	return 0;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int ret;
+

arch/arm/kvm/arch_timer.c static void kvm_timer_init_interrupt(void *info) · line 210

Quoted 10 lines

arch/arm/kvm/arch_timer.c +10 -0

+	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
+	if (ret != 0)
+		return ret;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = val;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;

I just realized what bothers me here: You're computing cntvoff on a per vcpu basis, while this is a VM property. Which means that as you're restoring vcpus, you'll be changing cntvoff - sounds like a bad idea to me.

Quoted 3 lines

The counter is really global. Do we have a way to handle VM-wide registers? I think Christoffer was trying to some a similar thing with the GIC...

So the consensus of this discussion was just to block writing when the VCPU is running, right? Or is there something else?