[PATCH v4] ARM/KVM: save and restore generic timer registers

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Andre Przywara]

From: Andre Przywara <andre.przywara@calxeda.com>

For migration to work we need to save (and later restore) the state of
each cores 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 the arch timer is optional.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---

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

Changes from v2:
- fix compilation without CONFIG_ARCH_TIMER
- fix compilation for arm64 by defining the appropriate registers there
- move userspace access out of arch_timer.c into coproc.c
- Christoffer: removed whitespace in function declaration

Changes from v3:
- adapted Marc's SYSREG macro magic from kvmtool for nicer looking code

 arch/arm/include/asm/kvm_host.h   |  3 ++
 arch/arm/include/uapi/asm/kvm.h   | 20 +++++++++
 arch/arm/kvm/guest.c              | 92 ++++++++++++++++++++++++++++++++++++++-
 arch/arm64/include/uapi/asm/kvm.h | 19 ++++++++
 virt/kvm/arm/arch_timer.c         | 34 +++++++++++++++
 5 files changed, 167 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 8a6f6db..098f7dd 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -225,4 +225,7 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
+int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
+
 #endif /* __ARM_KVM_HOST_H__ */
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index c498b60..835b867 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -119,6 +119,26 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
 #define KVM_REG_ARM_32_CRN_SHIFT	11
 
+#define ARM_CP15_REG_SHIFT_MASK(x,n) \
+	(((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK)
+
+#define __ARM_CP15_REG(op1,crn,crm,op2) \
+	(KVM_REG_ARM | (15 << KVM_REG_ARM_COPROC_SHIFT) | \
+	ARM_CP15_REG_SHIFT_MASK(op1, OPC1) | \
+	ARM_CP15_REG_SHIFT_MASK(crn, 32_CRN) | \
+	ARM_CP15_REG_SHIFT_MASK(crm, CRM) | \
+	ARM_CP15_REG_SHIFT_MASK(op2, 32_OPC2))
+
+#define ARM_CP15_REG32(...) (__ARM_CP15_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
+
+#define __ARM_CP15_REG64(op1,crm) \
+	(__ARM_CP15_REG(op1, 0, crm, 0) | KVM_REG_SIZE_U64)
+#define ARM_CP15_REG64(...) __ARM_CP15_REG64(__VA_ARGS__)
+
+#define KVM_REG_ARM_TIMER_CTL		ARM_CP15_REG32(0, 14, 3, 1)
+#define KVM_REG_ARM_TIMER_CNT		ARM_CP15_REG64(1, 14) 
+#define KVM_REG_ARM_TIMER_CVAL		ARM_CP15_REG64(3, 14) 
+
 /* 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)
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index 20f8d97..2786eae 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -109,6 +109,83 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	return -EINVAL;
 }
 
+#ifndef CONFIG_KVM_ARM_TIMER
+
+#define NUM_TIMER_REGS 0
+
+static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	return 0;
+}
+
+static bool is_timer_reg(u64 index)
+{
+	return false;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
+{
+	return 0;
+}
+
+u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
+{
+	return 0;
+}
+
+#else
+
+#define NUM_TIMER_REGS 3
+
+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;
+}
+
+static int 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;
+}
+
+#endif
+
+static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	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;
+
+	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
+}
+
+static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+
+	val = kvm_arm_timer_get_reg(vcpu, reg->id);
+	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
+}
+
 static unsigned long num_core_regs(void)
 {
 	return sizeof(struct kvm_regs) / sizeof(u32);
@@ -121,7 +198,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)
+		+ NUM_TIMER_REGS;
 }
 
 /**
@@ -133,6 +211,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,6 +219,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
+	ret = copy_timer_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += NUM_TIMER_REGS;
+
 	return kvm_arm_copy_coproc_indices(vcpu, uindices);
 }
 
@@ -153,6 +237,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 get_timer_reg(vcpu, reg);
+
 	return kvm_arm_coproc_get_reg(vcpu, reg);
 }
 
@@ -166,6 +253,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 set_timer_reg(vcpu, reg);
+
 	return kvm_arm_coproc_set_reg(vcpu, reg);
 }
 
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 5031f42..32f6ab3 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -129,6 +129,25 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM64_SYSREG_OP2_MASK	0x0000000000000007
 #define KVM_REG_ARM64_SYSREG_OP2_SHIFT	0
 
+#define ARM64_SYS_REG_SHIFT_MASK(x,n) \
+	(((x) << KVM_REG_ARM64_SYSREG_ ## n ## _SHIFT) & \
+	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
+
+#define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
+	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
+	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
+	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
+	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
+	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
+	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
+
+#define ARM64_SYS_REG32(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
+#define ARM64_SYS_REG64(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
+
+#define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG32(3, 3, 14, 3, 1)
+#define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG64(3, 3, 14, 3, 2)
+#define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG64(3, 3, 14, 0, 2)
+
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
 #define KVM_ARM_IRQ_TYPE_MASK		0xff
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index c2e1ef4..5081e80 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -182,6 +182,40 @@ static void kvm_timer_init_interrupt(void *info)
 	enable_percpu_irq(host_vtimer_irq, 0);
 }
 
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	switch (regid) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = value;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		timer->cntv_cval = value;
+		break;
+	default:
+		return -1;
+	}
+	return 0;
+}
+
+u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	switch (regid) {
+	case KVM_REG_ARM_TIMER_CTL:
+		return timer->cntv_ctl;
+	case KVM_REG_ARM_TIMER_CNT:
+		return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
+	case KVM_REG_ARM_TIMER_CVAL:
+		return timer->cntv_cval;
+	}
+	return (u64)-1;
+}
 
 static int kvm_timer_cpu_notify(struct notifier_block *self,
 				unsigned long action, void *cpu)
-- 
1.7.12.1

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Andre Przywara]

From: Andre Przywara <andre.przywara@calxeda.com>

For migration to work we need to save (and later restore) the state of
each cores 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 the arch timer is optional.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---

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

Changes from v2:
- fix compilation without CONFIG_ARCH_TIMER
- fix compilation for arm64 by defining the appropriate registers there
- move userspace access out of arch_timer.c into coproc.c
- Christoffer: removed whitespace in function declaration

Changes from v3:
- adapted Marc's SYSREG macro magic from kvmtool for nicer looking code

 arch/arm/include/asm/kvm_host.h   |  3 ++
 arch/arm/include/uapi/asm/kvm.h   | 20 +++++++++
 arch/arm/kvm/guest.c              | 92 ++++++++++++++++++++++++++++++++++++++-
 arch/arm64/include/uapi/asm/kvm.h | 19 ++++++++
 virt/kvm/arm/arch_timer.c         | 34 +++++++++++++++
 5 files changed, 167 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 8a6f6db..098f7dd 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -225,4 +225,7 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
+int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
+
 #endif /* __ARM_KVM_HOST_H__ */
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index c498b60..835b867 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -119,6 +119,26 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
 #define KVM_REG_ARM_32_CRN_SHIFT	11
 
+#define ARM_CP15_REG_SHIFT_MASK(x,n) \
+	(((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK)
+
+#define __ARM_CP15_REG(op1,crn,crm,op2) \
+	(KVM_REG_ARM | (15 << KVM_REG_ARM_COPROC_SHIFT) | \
+	ARM_CP15_REG_SHIFT_MASK(op1, OPC1) | \
+	ARM_CP15_REG_SHIFT_MASK(crn, 32_CRN) | \
+	ARM_CP15_REG_SHIFT_MASK(crm, CRM) | \
+	ARM_CP15_REG_SHIFT_MASK(op2, 32_OPC2))
+
+#define ARM_CP15_REG32(...) (__ARM_CP15_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
+
+#define __ARM_CP15_REG64(op1,crm) \
+	(__ARM_CP15_REG(op1, 0, crm, 0) | KVM_REG_SIZE_U64)
+#define ARM_CP15_REG64(...) __ARM_CP15_REG64(__VA_ARGS__)
+
+#define KVM_REG_ARM_TIMER_CTL		ARM_CP15_REG32(0, 14, 3, 1)
+#define KVM_REG_ARM_TIMER_CNT		ARM_CP15_REG64(1, 14) 
+#define KVM_REG_ARM_TIMER_CVAL		ARM_CP15_REG64(3, 14) 
+
 /* 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)
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index 20f8d97..2786eae 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -109,6 +109,83 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	return -EINVAL;
 }
 
+#ifndef CONFIG_KVM_ARM_TIMER
+
+#define NUM_TIMER_REGS 0
+
+static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	return 0;
+}
+
+static bool is_timer_reg(u64 index)
+{
+	return false;
+}
+
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
+{
+	return 0;
+}
+
+u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
+{
+	return 0;
+}
+
+#else
+
+#define NUM_TIMER_REGS 3
+
+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;
+}
+
+static int 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;
+}
+
+#endif
+
+static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	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;
+
+	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
+}
+
+static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+
+	val = kvm_arm_timer_get_reg(vcpu, reg->id);
+	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
+}
+
 static unsigned long num_core_regs(void)
 {
 	return sizeof(struct kvm_regs) / sizeof(u32);
@@ -121,7 +198,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)
+		+ NUM_TIMER_REGS;
 }
 
 /**
@@ -133,6 +211,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,6 +219,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
+	ret = copy_timer_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += NUM_TIMER_REGS;
+
 	return kvm_arm_copy_coproc_indices(vcpu, uindices);
 }
 
@@ -153,6 +237,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 get_timer_reg(vcpu, reg);
+
 	return kvm_arm_coproc_get_reg(vcpu, reg);
 }
 
@@ -166,6 +253,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 set_timer_reg(vcpu, reg);
+
 	return kvm_arm_coproc_set_reg(vcpu, reg);
 }
 
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 5031f42..32f6ab3 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -129,6 +129,25 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM64_SYSREG_OP2_MASK	0x0000000000000007
 #define KVM_REG_ARM64_SYSREG_OP2_SHIFT	0
 
+#define ARM64_SYS_REG_SHIFT_MASK(x,n) \
+	(((x) << KVM_REG_ARM64_SYSREG_ ## n ## _SHIFT) & \
+	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
+
+#define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
+	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
+	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
+	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
+	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
+	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
+	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
+
+#define ARM64_SYS_REG32(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
+#define ARM64_SYS_REG64(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
+
+#define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG32(3, 3, 14, 3, 1)
+#define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG64(3, 3, 14, 3, 2)
+#define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG64(3, 3, 14, 0, 2)
+
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
 #define KVM_ARM_IRQ_TYPE_MASK		0xff
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index c2e1ef4..5081e80 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -182,6 +182,40 @@ static void kvm_timer_init_interrupt(void *info)
 	enable_percpu_irq(host_vtimer_irq, 0);
 }
 
+int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	switch (regid) {
+	case KVM_REG_ARM_TIMER_CTL:
+		timer->cntv_ctl = value;
+		break;
+	case KVM_REG_ARM_TIMER_CNT:
+		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
+		break;
+	case KVM_REG_ARM_TIMER_CVAL:
+		timer->cntv_cval = value;
+		break;
+	default:
+		return -1;
+	}
+	return 0;
+}
+
+u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	switch (regid) {
+	case KVM_REG_ARM_TIMER_CTL:
+		return timer->cntv_ctl;
+	case KVM_REG_ARM_TIMER_CNT:
+		return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
+	case KVM_REG_ARM_TIMER_CVAL:
+		return timer->cntv_cval;
+	}
+	return (u64)-1;
+}
 
 static int kvm_timer_cpu_notify(struct notifier_block *self,
 				unsigned long action, void *cpu)
-- 
1.7.12.1

Re: [PATCH v4] ARM/KVM: save and restore generic timer registers

[Andre Przywara]

On 12/10/2013 11:50 AM, Andre Przywara wrote:
> From: Andre Przywara <andre.przywara@calxeda.com>

Ooops, I managed to screw up the authorship :-( Can the committer please 
change this to: "Andre Przywara <andre.przywara@linaro.org>" (as in the 
signed-off-by?)

Thanks,
Andre.

> For migration to work we need to save (and later restore) the state of
> each cores 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 the arch timer is optional.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>
> 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
>
> Changes from v2:
> - fix compilation without CONFIG_ARCH_TIMER
> - fix compilation for arm64 by defining the appropriate registers there
> - move userspace access out of arch_timer.c into coproc.c
> - Christoffer: removed whitespace in function declaration
>
> Changes from v3:
> - adapted Marc's SYSREG macro magic from kvmtool for nicer looking code
>
>   arch/arm/include/asm/kvm_host.h   |  3 ++
>   arch/arm/include/uapi/asm/kvm.h   | 20 +++++++++
>   arch/arm/kvm/guest.c              | 92 ++++++++++++++++++++++++++++++++++++++-
>   arch/arm64/include/uapi/asm/kvm.h | 19 ++++++++
>   virt/kvm/arm/arch_timer.c         | 34 +++++++++++++++
>   5 files changed, 167 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 8a6f6db..098f7dd 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -225,4 +225,7 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
>   int kvm_perf_init(void);
>   int kvm_perf_teardown(void);
>
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
> +
>   #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index c498b60..835b867 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -119,6 +119,26 @@ struct kvm_arch_memory_slot {
>   #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
>   #define KVM_REG_ARM_32_CRN_SHIFT	11
>
> +#define ARM_CP15_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK)
> +
> +#define __ARM_CP15_REG(op1,crn,crm,op2) \
> +	(KVM_REG_ARM | (15 << KVM_REG_ARM_COPROC_SHIFT) | \
> +	ARM_CP15_REG_SHIFT_MASK(op1, OPC1) | \
> +	ARM_CP15_REG_SHIFT_MASK(crn, 32_CRN) | \
> +	ARM_CP15_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM_CP15_REG_SHIFT_MASK(op2, 32_OPC2))
> +
> +#define ARM_CP15_REG32(...) (__ARM_CP15_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +
> +#define __ARM_CP15_REG64(op1,crm) \
> +	(__ARM_CP15_REG(op1, 0, crm, 0) | KVM_REG_SIZE_U64)
> +#define ARM_CP15_REG64(...) __ARM_CP15_REG64(__VA_ARGS__)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM_CP15_REG32(0, 14, 3, 1)
> +#define KVM_REG_ARM_TIMER_CNT		ARM_CP15_REG64(1, 14)
> +#define KVM_REG_ARM_TIMER_CVAL		ARM_CP15_REG64(3, 14)
> +
>   /* 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)
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 20f8d97..2786eae 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -109,6 +109,83 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>   	return -EINVAL;
>   }
>
> +#ifndef CONFIG_KVM_ARM_TIMER
> +
> +#define NUM_TIMER_REGS 0
> +
> +static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> +{
> +	return 0;
> +}
> +
> +static bool is_timer_reg(u64 index)
> +{
> +	return false;
> +}
> +
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	return 0;
> +}
> +
> +#else
> +
> +#define NUM_TIMER_REGS 3
> +
> +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;
> +}
> +
> +static int 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;
> +}
> +
> +#endif
> +
> +static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	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;
> +
> +	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
> +}
> +
> +static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	void __user *uaddr = (void __user *)(long)reg->addr;
> +	u64 val;
> +
> +	val = kvm_arm_timer_get_reg(vcpu, reg->id);
> +	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
> +}
> +
>   static unsigned long num_core_regs(void)
>   {
>   	return sizeof(struct kvm_regs) / sizeof(u32);
> @@ -121,7 +198,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)
> +		+ NUM_TIMER_REGS;
>   }
>
>   /**
> @@ -133,6 +211,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,6 +219,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>   		uindices++;
>   	}
>
> +	ret = copy_timer_indices(vcpu, uindices);
> +	if (ret)
> +		return ret;
> +	uindices += NUM_TIMER_REGS;
> +
>   	return kvm_arm_copy_coproc_indices(vcpu, uindices);
>   }
>
> @@ -153,6 +237,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 get_timer_reg(vcpu, reg);
> +
>   	return kvm_arm_coproc_get_reg(vcpu, reg);
>   }
>
> @@ -166,6 +253,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 set_timer_reg(vcpu, reg);
> +
>   	return kvm_arm_coproc_set_reg(vcpu, reg);
>   }
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 5031f42..32f6ab3 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -129,6 +129,25 @@ struct kvm_arch_memory_slot {
>   #define KVM_REG_ARM64_SYSREG_OP2_MASK	0x0000000000000007
>   #define KVM_REG_ARM64_SYSREG_OP2_SHIFT	0
>
> +#define ARM64_SYS_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM64_SYSREG_ ## n ## _SHIFT) & \
> +	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
> +
> +#define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
> +	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
> +	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
> +
> +#define ARM64_SYS_REG32(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +#define ARM64_SYS_REG64(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG32(3, 3, 14, 3, 1)
> +#define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG64(3, 3, 14, 3, 2)
> +#define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG64(3, 3, 14, 0, 2)
> +
>   /* KVM_IRQ_LINE irq field index values */
>   #define KVM_ARM_IRQ_TYPE_SHIFT		24
>   #define KVM_ARM_IRQ_TYPE_MASK		0xff
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index c2e1ef4..5081e80 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -182,6 +182,40 @@ static void kvm_timer_init_interrupt(void *info)
>   	enable_percpu_irq(host_vtimer_irq, 0);
>   }
>
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		timer->cntv_ctl = value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		timer->cntv_cval = value;
> +		break;
> +	default:
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		return timer->cntv_ctl;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		return timer->cntv_cval;
> +	}
> +	return (u64)-1;
> +}
>
>   static int kvm_timer_cpu_notify(struct notifier_block *self,
>   				unsigned long action, void *cpu)
>

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Andre Przywara]

On 12/10/2013 11:50 AM, Andre Przywara wrote:
> From: Andre Przywara <andre.przywara@calxeda.com>

Ooops, I managed to screw up the authorship :-( Can the committer please 
change this to: "Andre Przywara <andre.przywara@linaro.org>" (as in the 
signed-off-by?)

Thanks,
Andre.

> For migration to work we need to save (and later restore) the state of
> each cores 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 the arch timer is optional.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>
> 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
>
> Changes from v2:
> - fix compilation without CONFIG_ARCH_TIMER
> - fix compilation for arm64 by defining the appropriate registers there
> - move userspace access out of arch_timer.c into coproc.c
> - Christoffer: removed whitespace in function declaration
>
> Changes from v3:
> - adapted Marc's SYSREG macro magic from kvmtool for nicer looking code
>
>   arch/arm/include/asm/kvm_host.h   |  3 ++
>   arch/arm/include/uapi/asm/kvm.h   | 20 +++++++++
>   arch/arm/kvm/guest.c              | 92 ++++++++++++++++++++++++++++++++++++++-
>   arch/arm64/include/uapi/asm/kvm.h | 19 ++++++++
>   virt/kvm/arm/arch_timer.c         | 34 +++++++++++++++
>   5 files changed, 167 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 8a6f6db..098f7dd 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -225,4 +225,7 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
>   int kvm_perf_init(void);
>   int kvm_perf_teardown(void);
>
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
> +
>   #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index c498b60..835b867 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -119,6 +119,26 @@ struct kvm_arch_memory_slot {
>   #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
>   #define KVM_REG_ARM_32_CRN_SHIFT	11
>
> +#define ARM_CP15_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK)
> +
> +#define __ARM_CP15_REG(op1,crn,crm,op2) \
> +	(KVM_REG_ARM | (15 << KVM_REG_ARM_COPROC_SHIFT) | \
> +	ARM_CP15_REG_SHIFT_MASK(op1, OPC1) | \
> +	ARM_CP15_REG_SHIFT_MASK(crn, 32_CRN) | \
> +	ARM_CP15_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM_CP15_REG_SHIFT_MASK(op2, 32_OPC2))
> +
> +#define ARM_CP15_REG32(...) (__ARM_CP15_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +
> +#define __ARM_CP15_REG64(op1,crm) \
> +	(__ARM_CP15_REG(op1, 0, crm, 0) | KVM_REG_SIZE_U64)
> +#define ARM_CP15_REG64(...) __ARM_CP15_REG64(__VA_ARGS__)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM_CP15_REG32(0, 14, 3, 1)
> +#define KVM_REG_ARM_TIMER_CNT		ARM_CP15_REG64(1, 14)
> +#define KVM_REG_ARM_TIMER_CVAL		ARM_CP15_REG64(3, 14)
> +
>   /* 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)
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 20f8d97..2786eae 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -109,6 +109,83 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>   	return -EINVAL;
>   }
>
> +#ifndef CONFIG_KVM_ARM_TIMER
> +
> +#define NUM_TIMER_REGS 0
> +
> +static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> +{
> +	return 0;
> +}
> +
> +static bool is_timer_reg(u64 index)
> +{
> +	return false;
> +}
> +
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	return 0;
> +}
> +
> +#else
> +
> +#define NUM_TIMER_REGS 3
> +
> +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;
> +}
> +
> +static int 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;
> +}
> +
> +#endif
> +
> +static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	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;
> +
> +	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
> +}
> +
> +static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	void __user *uaddr = (void __user *)(long)reg->addr;
> +	u64 val;
> +
> +	val = kvm_arm_timer_get_reg(vcpu, reg->id);
> +	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
> +}
> +
>   static unsigned long num_core_regs(void)
>   {
>   	return sizeof(struct kvm_regs) / sizeof(u32);
> @@ -121,7 +198,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)
> +		+ NUM_TIMER_REGS;
>   }
>
>   /**
> @@ -133,6 +211,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,6 +219,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>   		uindices++;
>   	}
>
> +	ret = copy_timer_indices(vcpu, uindices);
> +	if (ret)
> +		return ret;
> +	uindices += NUM_TIMER_REGS;
> +
>   	return kvm_arm_copy_coproc_indices(vcpu, uindices);
>   }
>
> @@ -153,6 +237,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 get_timer_reg(vcpu, reg);
> +
>   	return kvm_arm_coproc_get_reg(vcpu, reg);
>   }
>
> @@ -166,6 +253,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 set_timer_reg(vcpu, reg);
> +
>   	return kvm_arm_coproc_set_reg(vcpu, reg);
>   }
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 5031f42..32f6ab3 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -129,6 +129,25 @@ struct kvm_arch_memory_slot {
>   #define KVM_REG_ARM64_SYSREG_OP2_MASK	0x0000000000000007
>   #define KVM_REG_ARM64_SYSREG_OP2_SHIFT	0
>
> +#define ARM64_SYS_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM64_SYSREG_ ## n ## _SHIFT) & \
> +	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
> +
> +#define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
> +	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
> +	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
> +
> +#define ARM64_SYS_REG32(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +#define ARM64_SYS_REG64(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG32(3, 3, 14, 3, 1)
> +#define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG64(3, 3, 14, 3, 2)
> +#define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG64(3, 3, 14, 0, 2)
> +
>   /* KVM_IRQ_LINE irq field index values */
>   #define KVM_ARM_IRQ_TYPE_SHIFT		24
>   #define KVM_ARM_IRQ_TYPE_MASK		0xff
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index c2e1ef4..5081e80 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -182,6 +182,40 @@ static void kvm_timer_init_interrupt(void *info)
>   	enable_percpu_irq(host_vtimer_irq, 0);
>   }
>
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		timer->cntv_ctl = value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		timer->cntv_cval = value;
> +		break;
> +	default:
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		return timer->cntv_ctl;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		return timer->cntv_cval;
> +	}
> +	return (u64)-1;
> +}
>
>   static int kvm_timer_cpu_notify(struct notifier_block *self,
>   				unsigned long action, void *cpu)
>

Re: [PATCH v4] ARM/KVM: save and restore generic timer registers

[Christoffer Dall]

On Tue, Dec 10, 2013 at 11:50:25AM +0100, Andre Przywara wrote:
> From: Andre Przywara <andre.przywara@calxeda.com>
> 
> For migration to work we need to save (and later restore) the state of
> each cores 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 the arch timer is optional.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> 
> 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
> 
> Changes from v2:
> - fix compilation without CONFIG_ARCH_TIMER
> - fix compilation for arm64 by defining the appropriate registers there
> - move userspace access out of arch_timer.c into coproc.c
> - Christoffer: removed whitespace in function declaration
> 
> Changes from v3:
> - adapted Marc's SYSREG macro magic from kvmtool for nicer looking code
> 
>  arch/arm/include/asm/kvm_host.h   |  3 ++
>  arch/arm/include/uapi/asm/kvm.h   | 20 +++++++++
>  arch/arm/kvm/guest.c              | 92 ++++++++++++++++++++++++++++++++++++++-
>  arch/arm64/include/uapi/asm/kvm.h | 19 ++++++++
>  virt/kvm/arm/arch_timer.c         | 34 +++++++++++++++
>  5 files changed, 167 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 8a6f6db..098f7dd 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -225,4 +225,7 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
>  int kvm_perf_init(void);
>  int kvm_perf_teardown(void);
>  
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
> +
>  #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index c498b60..835b867 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -119,6 +119,26 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
>  #define KVM_REG_ARM_32_CRN_SHIFT	11
>  
> +#define ARM_CP15_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK)
> +
> +#define __ARM_CP15_REG(op1,crn,crm,op2) \
> +	(KVM_REG_ARM | (15 << KVM_REG_ARM_COPROC_SHIFT) | \
> +	ARM_CP15_REG_SHIFT_MASK(op1, OPC1) | \
> +	ARM_CP15_REG_SHIFT_MASK(crn, 32_CRN) | \
> +	ARM_CP15_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM_CP15_REG_SHIFT_MASK(op2, 32_OPC2))
> +
> +#define ARM_CP15_REG32(...) (__ARM_CP15_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +
> +#define __ARM_CP15_REG64(op1,crm) \
> +	(__ARM_CP15_REG(op1, 0, crm, 0) | KVM_REG_SIZE_U64)
> +#define ARM_CP15_REG64(...) __ARM_CP15_REG64(__VA_ARGS__)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM_CP15_REG32(0, 14, 3, 1)
> +#define KVM_REG_ARM_TIMER_CNT		ARM_CP15_REG64(1, 14) 
> +#define KVM_REG_ARM_TIMER_CVAL		ARM_CP15_REG64(3, 14) 
> +
>  /* 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)
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 20f8d97..2786eae 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -109,6 +109,83 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>  	return -EINVAL;
>  }
>  
> +#ifndef CONFIG_KVM_ARM_TIMER
> +
> +#define NUM_TIMER_REGS 0
> +
> +static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> +{
> +	return 0;
> +}
> +
> +static bool is_timer_reg(u64 index)
> +{
> +	return false;
> +}
> +
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	return 0;
> +}
> +
> +#else
> +
> +#define NUM_TIMER_REGS 3
> +
> +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;
> +}
> +
> +static int 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;
> +}
> +
> +#endif
> +
> +static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	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;
> +
> +	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
> +}
> +
> +static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	void __user *uaddr = (void __user *)(long)reg->addr;
> +	u64 val;
> +
> +	val = kvm_arm_timer_get_reg(vcpu, reg->id);
> +	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
> +}
> +
>  static unsigned long num_core_regs(void)
>  {
>  	return sizeof(struct kvm_regs) / sizeof(u32);
> @@ -121,7 +198,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)
> +		+ NUM_TIMER_REGS;
>  }
>  
>  /**
> @@ -133,6 +211,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,6 +219,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  		uindices++;
>  	}
>  
> +	ret = copy_timer_indices(vcpu, uindices);
> +	if (ret)
> +		return ret;
> +	uindices += NUM_TIMER_REGS;
> +
>  	return kvm_arm_copy_coproc_indices(vcpu, uindices);
>  }
>  
> @@ -153,6 +237,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 get_timer_reg(vcpu, reg);
> +
>  	return kvm_arm_coproc_get_reg(vcpu, reg);
>  }
>  
> @@ -166,6 +253,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 set_timer_reg(vcpu, reg);
> +
>  	return kvm_arm_coproc_set_reg(vcpu, reg);
>  }

Don't you need similar changes for arm64 as well?

>  
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 5031f42..32f6ab3 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -129,6 +129,25 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM64_SYSREG_OP2_MASK	0x0000000000000007
>  #define KVM_REG_ARM64_SYSREG_OP2_SHIFT	0
>  
> +#define ARM64_SYS_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM64_SYSREG_ ## n ## _SHIFT) & \
> +	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
> +
> +#define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
> +	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
> +	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
> +
> +#define ARM64_SYS_REG32(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +#define ARM64_SYS_REG64(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG32(3, 3, 14, 3, 1)

So this looks a bit strange to me when comparing with
Documentation/virtual/kvm/api.txt.

That clearly specifies that arm64 system registers have the id bit
patterns with (13 << 16) implying a reg size of 64 bit, but does not
specify any valid encoding for (13 << 16) KVM_REG_SIZE_U64 with a reg
size of 32 bit (KVM_REG_SIZE_U32).

So it seems to me the api documentation should be tweaked to add a note
about 32-bit sized arm64 registers.  It doesn't break the API because we
add something new (although from an extremely strict point of view the
existing wording could be understood to mean *all arm64 system
registers*, but I think we can ignore this).

Something like this:

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index a30035d..9565e6a 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1889,9 +1889,12 @@ value in the kvm_regs structure seen as a 32bit array.
 arm64 CCSIDR registers are demultiplexed by CSSELR value:
   0x6020 0000 0011 00 <csselr:8>
 
-arm64 system registers have the following id bit patterns:
+arm64 64-bit system registers have the following id bit patterns:
   0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
 
+arm64 32-bit system registers have the following id bit patterns:
+  0x6020 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
+
 4.69 KVM_GET_ONE_REG
 
 Capability: KVM_CAP_ONE_REG

> +#define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG64(3, 3, 14, 3, 2)
> +#define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG64(3, 3, 14, 0, 2)
> +
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
>  #define KVM_ARM_IRQ_TYPE_MASK		0xff
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index c2e1ef4..5081e80 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -182,6 +182,40 @@ static void kvm_timer_init_interrupt(void *info)
>  	enable_percpu_irq(host_vtimer_irq, 0);
>  }
>  
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		timer->cntv_ctl = value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		timer->cntv_cval = value;
> +		break;
> +	default:
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		return timer->cntv_ctl;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		return timer->cntv_cval;
> +	}
> +	return (u64)-1;
> +}
>  
>  static int kvm_timer_cpu_notify(struct notifier_block *self,
>  				unsigned long action, void *cpu)
> -- 
> 1.7.12.1
> 

-- 
Christoffer

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Christoffer Dall]

On Tue, Dec 10, 2013 at 11:50:25AM +0100, Andre Przywara wrote:
> From: Andre Przywara <andre.przywara@calxeda.com>
> 
> For migration to work we need to save (and later restore) the state of
> each cores 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 the arch timer is optional.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> 
> 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
> 
> Changes from v2:
> - fix compilation without CONFIG_ARCH_TIMER
> - fix compilation for arm64 by defining the appropriate registers there
> - move userspace access out of arch_timer.c into coproc.c
> - Christoffer: removed whitespace in function declaration
> 
> Changes from v3:
> - adapted Marc's SYSREG macro magic from kvmtool for nicer looking code
> 
>  arch/arm/include/asm/kvm_host.h   |  3 ++
>  arch/arm/include/uapi/asm/kvm.h   | 20 +++++++++
>  arch/arm/kvm/guest.c              | 92 ++++++++++++++++++++++++++++++++++++++-
>  arch/arm64/include/uapi/asm/kvm.h | 19 ++++++++
>  virt/kvm/arm/arch_timer.c         | 34 +++++++++++++++
>  5 files changed, 167 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 8a6f6db..098f7dd 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -225,4 +225,7 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
>  int kvm_perf_init(void);
>  int kvm_perf_teardown(void);
>  
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
> +
>  #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index c498b60..835b867 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -119,6 +119,26 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM_32_CRN_MASK		0x0000000000007800
>  #define KVM_REG_ARM_32_CRN_SHIFT	11
>  
> +#define ARM_CP15_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK)
> +
> +#define __ARM_CP15_REG(op1,crn,crm,op2) \
> +	(KVM_REG_ARM | (15 << KVM_REG_ARM_COPROC_SHIFT) | \
> +	ARM_CP15_REG_SHIFT_MASK(op1, OPC1) | \
> +	ARM_CP15_REG_SHIFT_MASK(crn, 32_CRN) | \
> +	ARM_CP15_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM_CP15_REG_SHIFT_MASK(op2, 32_OPC2))
> +
> +#define ARM_CP15_REG32(...) (__ARM_CP15_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +
> +#define __ARM_CP15_REG64(op1,crm) \
> +	(__ARM_CP15_REG(op1, 0, crm, 0) | KVM_REG_SIZE_U64)
> +#define ARM_CP15_REG64(...) __ARM_CP15_REG64(__VA_ARGS__)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM_CP15_REG32(0, 14, 3, 1)
> +#define KVM_REG_ARM_TIMER_CNT		ARM_CP15_REG64(1, 14) 
> +#define KVM_REG_ARM_TIMER_CVAL		ARM_CP15_REG64(3, 14) 
> +
>  /* 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)
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 20f8d97..2786eae 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -109,6 +109,83 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>  	return -EINVAL;
>  }
>  
> +#ifndef CONFIG_KVM_ARM_TIMER
> +
> +#define NUM_TIMER_REGS 0
> +
> +static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> +{
> +	return 0;
> +}
> +
> +static bool is_timer_reg(u64 index)
> +{
> +	return false;
> +}
> +
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	return 0;
> +}
> +
> +#else
> +
> +#define NUM_TIMER_REGS 3
> +
> +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;
> +}
> +
> +static int 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;
> +}
> +
> +#endif
> +
> +static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	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;
> +
> +	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
> +}
> +
> +static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	void __user *uaddr = (void __user *)(long)reg->addr;
> +	u64 val;
> +
> +	val = kvm_arm_timer_get_reg(vcpu, reg->id);
> +	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id));
> +}
> +
>  static unsigned long num_core_regs(void)
>  {
>  	return sizeof(struct kvm_regs) / sizeof(u32);
> @@ -121,7 +198,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)
> +		+ NUM_TIMER_REGS;
>  }
>  
>  /**
> @@ -133,6 +211,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,6 +219,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  		uindices++;
>  	}
>  
> +	ret = copy_timer_indices(vcpu, uindices);
> +	if (ret)
> +		return ret;
> +	uindices += NUM_TIMER_REGS;
> +
>  	return kvm_arm_copy_coproc_indices(vcpu, uindices);
>  }
>  
> @@ -153,6 +237,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 get_timer_reg(vcpu, reg);
> +
>  	return kvm_arm_coproc_get_reg(vcpu, reg);
>  }
>  
> @@ -166,6 +253,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 set_timer_reg(vcpu, reg);
> +
>  	return kvm_arm_coproc_set_reg(vcpu, reg);
>  }

Don't you need similar changes for arm64 as well?

>  
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 5031f42..32f6ab3 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -129,6 +129,25 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM64_SYSREG_OP2_MASK	0x0000000000000007
>  #define KVM_REG_ARM64_SYSREG_OP2_SHIFT	0
>  
> +#define ARM64_SYS_REG_SHIFT_MASK(x,n) \
> +	(((x) << KVM_REG_ARM64_SYSREG_ ## n ## _SHIFT) & \
> +	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
> +
> +#define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
> +	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
> +	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
> +	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
> +	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
> +
> +#define ARM64_SYS_REG32(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U32)
> +#define ARM64_SYS_REG64(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
> +
> +#define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG32(3, 3, 14, 3, 1)

So this looks a bit strange to me when comparing with
Documentation/virtual/kvm/api.txt.

That clearly specifies that arm64 system registers have the id bit
patterns with (13 << 16) implying a reg size of 64 bit, but does not
specify any valid encoding for (13 << 16) KVM_REG_SIZE_U64 with a reg
size of 32 bit (KVM_REG_SIZE_U32).

So it seems to me the api documentation should be tweaked to add a note
about 32-bit sized arm64 registers.  It doesn't break the API because we
add something new (although from an extremely strict point of view the
existing wording could be understood to mean *all arm64 system
registers*, but I think we can ignore this).

Something like this:

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index a30035d..9565e6a 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1889,9 +1889,12 @@ value in the kvm_regs structure seen as a 32bit array.
 arm64 CCSIDR registers are demultiplexed by CSSELR value:
   0x6020 0000 0011 00 <csselr:8>
 
-arm64 system registers have the following id bit patterns:
+arm64 64-bit system registers have the following id bit patterns:
   0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
 
+arm64 32-bit system registers have the following id bit patterns:
+  0x6020 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
+
 4.69 KVM_GET_ONE_REG
 
 Capability: KVM_CAP_ONE_REG

> +#define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG64(3, 3, 14, 3, 2)
> +#define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG64(3, 3, 14, 0, 2)
> +
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
>  #define KVM_ARM_IRQ_TYPE_MASK		0xff
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index c2e1ef4..5081e80 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -182,6 +182,40 @@ static void kvm_timer_init_interrupt(void *info)
>  	enable_percpu_irq(host_vtimer_irq, 0);
>  }
>  
> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		timer->cntv_ctl = value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
> +		break;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		timer->cntv_cval = value;
> +		break;
> +	default:
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	switch (regid) {
> +	case KVM_REG_ARM_TIMER_CTL:
> +		return timer->cntv_ctl;
> +	case KVM_REG_ARM_TIMER_CNT:
> +		return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> +	case KVM_REG_ARM_TIMER_CVAL:
> +		return timer->cntv_cval;
> +	}
> +	return (u64)-1;
> +}
>  
>  static int kvm_timer_cpu_notify(struct notifier_block *self,
>  				unsigned long action, void *cpu)
> -- 
> 1.7.12.1
> 

-- 
Christoffer

Re: [PATCH v4] ARM/KVM: save and restore generic timer registers

[Peter Maydell]

On 12 December 2013 02:28, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index a30035d..9565e6a 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1889,9 +1889,12 @@ value in the kvm_regs structure seen as a 32bit array.
>  arm64 CCSIDR registers are demultiplexed by CSSELR value:
>    0x6020 0000 0011 00 <csselr:8>
>
> -arm64 system registers have the following id bit patterns:
> +arm64 64-bit system registers have the following id bit patterns:
>    0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>
> +arm64 32-bit system registers have the following id bit patterns:
> +  0x6020 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>

What does it mean to say that a system register for AArch64
is "32 bits" given that MRS/MSR always operate on a 64 bit
register? We have the distinction in AArch32 because the
instructions (and whether the input/output is in one register
or a register pair) are different, but I can't see the need for
AArch64.

(The code I've just written for QEMU to handle sysregs says
"they're all 64 bit"...)

thanks
-- PMM

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Peter Maydell]

On 12 December 2013 02:28, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index a30035d..9565e6a 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1889,9 +1889,12 @@ value in the kvm_regs structure seen as a 32bit array.
>  arm64 CCSIDR registers are demultiplexed by CSSELR value:
>    0x6020 0000 0011 00 <csselr:8>
>
> -arm64 system registers have the following id bit patterns:
> +arm64 64-bit system registers have the following id bit patterns:
>    0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>
> +arm64 32-bit system registers have the following id bit patterns:
> +  0x6020 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>

What does it mean to say that a system register for AArch64
is "32 bits" given that MRS/MSR always operate on a 64 bit
register? We have the distinction in AArch32 because the
instructions (and whether the input/output is in one register
or a register pair) are different, but I can't see the need for
AArch64.

(The code I've just written for QEMU to handle sysregs says
"they're all 64 bit"...)

thanks
-- PMM

Re: [PATCH v4] ARM/KVM: save and restore generic timer registers

[Andre Przywara]

On 12/12/2013 10:23 AM, Peter Maydell wrote:
> On 12 December 2013 02:28, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index a30035d..9565e6a 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1889,9 +1889,12 @@ value in the kvm_regs structure seen as a 32bit array.
>>   arm64 CCSIDR registers are demultiplexed by CSSELR value:
>>     0x6020 0000 0011 00 <csselr:8>
>>
>> -arm64 system registers have the following id bit patterns:
>> +arm64 64-bit system registers have the following id bit patterns:
>>     0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>>
>> +arm64 32-bit system registers have the following id bit patterns:
>> +  0x6020 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>
> What does it mean to say that a system register for AArch64
> is "32 bits" given that MRS/MSR always operate on a 64 bit
> register? We have the distinction in AArch32 because the
> instructions (and whether the input/output is in one register
> or a register pair) are different, but I can't see the need for
> AArch64.

But ARMv8 ARM still defines these registers as 32-bit:
D8.5.14: CNTV_CTL_EL0
Attributes
            CNTV_CTL_EL0 is a 32-bit register.
But indeed the MSR/MRS instruction references a Xt register, and the 
documentation does not seem to tell how this is handled, so I assume 
this is zero-extended.
Would be great to have this clarified, though.

Regards,
Andre.


>
> (The code I've just written for QEMU to handle sysregs says
> "they're all 64 bit"...)
>
> thanks
> -- PMM
>

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Andre Przywara]

On 12/12/2013 10:23 AM, Peter Maydell wrote:
> On 12 December 2013 02:28, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index a30035d..9565e6a 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1889,9 +1889,12 @@ value in the kvm_regs structure seen as a 32bit array.
>>   arm64 CCSIDR registers are demultiplexed by CSSELR value:
>>     0x6020 0000 0011 00 <csselr:8>
>>
>> -arm64 system registers have the following id bit patterns:
>> +arm64 64-bit system registers have the following id bit patterns:
>>     0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>>
>> +arm64 32-bit system registers have the following id bit patterns:
>> +  0x6020 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>
> What does it mean to say that a system register for AArch64
> is "32 bits" given that MRS/MSR always operate on a 64 bit
> register? We have the distinction in AArch32 because the
> instructions (and whether the input/output is in one register
> or a register pair) are different, but I can't see the need for
> AArch64.

But ARMv8 ARM still defines these registers as 32-bit:
D8.5.14: CNTV_CTL_EL0
Attributes
            CNTV_CTL_EL0 is a 32-bit register.
But indeed the MSR/MRS instruction references a Xt register, and the 
documentation does not seem to tell how this is handled, so I assume 
this is zero-extended.
Would be great to have this clarified, though.

Regards,
Andre.


>
> (The code I've just written for QEMU to handle sysregs says
> "they're all 64 bit"...)
>
> thanks
> -- PMM
>

Re: [PATCH v4] ARM/KVM: save and restore generic timer registers

[Peter Maydell]

On 12 December 2013 09:32, Andre Przywara <andre.przywara@linaro.org> wrote:
> On 12/12/2013 10:23 AM, Peter Maydell wrote:
>> What does it mean to say that a system register for AArch64
>> is "32 bits" given that MRS/MSR always operate on a 64 bit
>> register?

> But ARMv8 ARM still defines these registers as 32-bit:
> D8.5.14: CNTV_CTL_EL0
> Attributes
>            CNTV_CTL_EL0 is a 32-bit register.
> But indeed the MSR/MRS instruction references a Xt register, and the
> documentation does not seem to tell how this is handled, so I assume this is
> zero-extended.

I checked, and for AArch64 registers, "32 bits" is just
a shorthand for "64 bit register where the top 32 bits are
RAZ/WI" (and I suspect it's not totally impossible that some
future architecture revision might define new bits in the
top half).

So I would suggest that we should make the KVM user<->kernel
interface just consistently make all the sysregs 64 bit.

(There is actually precedent of a sort here in that the
kernel claims the PSTATE register is 64 bits wide despite
it really being a 32 bit SPSR format value under the hood.)

thanks
-- PMM

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Peter Maydell]

On 12 December 2013 09:32, Andre Przywara <andre.przywara@linaro.org> wrote:
> On 12/12/2013 10:23 AM, Peter Maydell wrote:
>> What does it mean to say that a system register for AArch64
>> is "32 bits" given that MRS/MSR always operate on a 64 bit
>> register?

> But ARMv8 ARM still defines these registers as 32-bit:
> D8.5.14: CNTV_CTL_EL0
> Attributes
>            CNTV_CTL_EL0 is a 32-bit register.
> But indeed the MSR/MRS instruction references a Xt register, and the
> documentation does not seem to tell how this is handled, so I assume this is
> zero-extended.

I checked, and for AArch64 registers, "32 bits" is just
a shorthand for "64 bit register where the top 32 bits are
RAZ/WI" (and I suspect it's not totally impossible that some
future architecture revision might define new bits in the
top half).

So I would suggest that we should make the KVM user<->kernel
interface just consistently make all the sysregs 64 bit.

(There is actually precedent of a sort here in that the
kernel claims the PSTATE register is 64 bits wide despite
it really being a 32 bit SPSR format value under the hood.)

thanks
-- PMM

Re: [PATCH v4] ARM/KVM: save and restore generic timer registers

[Marc Zyngier]

On 12/12/13 11:36, Peter Maydell wrote:
> On 12 December 2013 09:32, Andre Przywara <andre.przywara@linaro.org> wrote:
>> On 12/12/2013 10:23 AM, Peter Maydell wrote:
>>> What does it mean to say that a system register for AArch64
>>> is "32 bits" given that MRS/MSR always operate on a 64 bit
>>> register?
> 
>> But ARMv8 ARM still defines these registers as 32-bit:
>> D8.5.14: CNTV_CTL_EL0
>> Attributes
>>            CNTV_CTL_EL0 is a 32-bit register.
>> But indeed the MSR/MRS instruction references a Xt register, and the
>> documentation does not seem to tell how this is handled, so I assume this is
>> zero-extended.
> 
> I checked, and for AArch64 registers, "32 bits" is just
> a shorthand for "64 bit register where the top 32 bits are
> RAZ/WI" (and I suspect it's not totally impossible that some
> future architecture revision might define new bits in the
> top half).

Indeed. Actually, there isn't an instruction to access these 32bit
registers with a 'W' register. You really have to use a 'X'.

> So I would suggest that we should make the KVM user<->kernel
> interface just consistently make all the sysregs 64 bit.
> 
> (There is actually precedent of a sort here in that the
> kernel claims the PSTATE register is 64 bits wide despite
> it really being a 32 bit SPSR format value under the hood.)

I definitely agree with Peter here. I'd like to keep the ABI 64bit for
the sysregs. It makes the whole thing much nicer.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Marc Zyngier]

On 12/12/13 11:36, Peter Maydell wrote:
> On 12 December 2013 09:32, Andre Przywara <andre.przywara@linaro.org> wrote:
>> On 12/12/2013 10:23 AM, Peter Maydell wrote:
>>> What does it mean to say that a system register for AArch64
>>> is "32 bits" given that MRS/MSR always operate on a 64 bit
>>> register?
> 
>> But ARMv8 ARM still defines these registers as 32-bit:
>> D8.5.14: CNTV_CTL_EL0
>> Attributes
>>            CNTV_CTL_EL0 is a 32-bit register.
>> But indeed the MSR/MRS instruction references a Xt register, and the
>> documentation does not seem to tell how this is handled, so I assume this is
>> zero-extended.
> 
> I checked, and for AArch64 registers, "32 bits" is just
> a shorthand for "64 bit register where the top 32 bits are
> RAZ/WI" (and I suspect it's not totally impossible that some
> future architecture revision might define new bits in the
> top half).

Indeed. Actually, there isn't an instruction to access these 32bit
registers with a 'W' register. You really have to use a 'X'.

> So I would suggest that we should make the KVM user<->kernel
> interface just consistently make all the sysregs 64 bit.
> 
> (There is actually precedent of a sort here in that the
> kernel claims the PSTATE register is 64 bits wide despite
> it really being a 32 bit SPSR format value under the hood.)

I definitely agree with Peter here. I'd like to keep the ABI 64bit for
the sysregs. It makes the whole thing much nicer.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4] ARM/KVM: save and restore generic timer registers

[Christoffer Dall]

On Thu, Dec 12, 2013 at 05:15:36PM +0000, Marc Zyngier wrote:
> On 12/12/13 11:36, Peter Maydell wrote:
> > On 12 December 2013 09:32, Andre Przywara <andre.przywara@linaro.org> wrote:
> >> On 12/12/2013 10:23 AM, Peter Maydell wrote:
> >>> What does it mean to say that a system register for AArch64
> >>> is "32 bits" given that MRS/MSR always operate on a 64 bit
> >>> register?
> > 
> >> But ARMv8 ARM still defines these registers as 32-bit:
> >> D8.5.14: CNTV_CTL_EL0
> >> Attributes
> >>            CNTV_CTL_EL0 is a 32-bit register.
> >> But indeed the MSR/MRS instruction references a Xt register, and the
> >> documentation does not seem to tell how this is handled, so I assume this is
> >> zero-extended.
> > 
> > I checked, and for AArch64 registers, "32 bits" is just
> > a shorthand for "64 bit register where the top 32 bits are
> > RAZ/WI" (and I suspect it's not totally impossible that some
> > future architecture revision might define new bits in the
> > top half).
> 
> Indeed. Actually, there isn't an instruction to access these 32bit
> registers with a 'W' register. You really have to use a 'X'.
> 
> > So I would suggest that we should make the KVM user<->kernel
> > interface just consistently make all the sysregs 64 bit.
> > 
> > (There is actually precedent of a sort here in that the
> > kernel claims the PSTATE register is 64 bits wide despite
> > it really being a 32 bit SPSR format value under the hood.)
> 
> I definitely agree with Peter here. I'd like to keep the ABI 64bit for
> the sysregs. It makes the whole thing much nicer.
> 
OK, makes perfect sense, so we just need to change the patch to export
the system registers as 64 bit in size, and then actually export them on
arm64.

-Christoffer

[PATCH v4] ARM/KVM: save and restore generic timer registers

[Christoffer Dall]

On Thu, Dec 12, 2013 at 05:15:36PM +0000, Marc Zyngier wrote:
> On 12/12/13 11:36, Peter Maydell wrote:
> > On 12 December 2013 09:32, Andre Przywara <andre.przywara@linaro.org> wrote:
> >> On 12/12/2013 10:23 AM, Peter Maydell wrote:
> >>> What does it mean to say that a system register for AArch64
> >>> is "32 bits" given that MRS/MSR always operate on a 64 bit
> >>> register?
> > 
> >> But ARMv8 ARM still defines these registers as 32-bit:
> >> D8.5.14: CNTV_CTL_EL0
> >> Attributes
> >>            CNTV_CTL_EL0 is a 32-bit register.
> >> But indeed the MSR/MRS instruction references a Xt register, and the
> >> documentation does not seem to tell how this is handled, so I assume this is
> >> zero-extended.
> > 
> > I checked, and for AArch64 registers, "32 bits" is just
> > a shorthand for "64 bit register where the top 32 bits are
> > RAZ/WI" (and I suspect it's not totally impossible that some
> > future architecture revision might define new bits in the
> > top half).
> 
> Indeed. Actually, there isn't an instruction to access these 32bit
> registers with a 'W' register. You really have to use a 'X'.
> 
> > So I would suggest that we should make the KVM user<->kernel
> > interface just consistently make all the sysregs 64 bit.
> > 
> > (There is actually precedent of a sort here in that the
> > kernel claims the PSTATE register is 64 bits wide despite
> > it really being a 32 bit SPSR format value under the hood.)
> 
> I definitely agree with Peter here. I'd like to keep the ABI 64bit for
> the sysregs. It makes the whole thing much nicer.
> 
OK, makes perfect sense, so we just need to change the patch to export
the system registers as 64 bit in size, and then actually export them on
arm64.

-Christoffer