Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.

[Rusty Russell <rusty.russell@linaro.org>]

Rusty Russell <rusty.russell@linaro.org> writes:
> No structures at all any more.
>
> Note: the encoding of general registers makes sense, but it's not
> completely logical so the code is quite messy.  It would actually be
> far neater to expose the struct kvm_vcpu_regs, and use offsets into
> that as the ABI.

That approach is much nicer.  This version just exposes the internal
kvm_vcpu_regs (as kvm_regs) and uses the offsets in that as our register
ids.

It will have to become a mapping table if we change our internal
representation, but that's still straightforward.

Cheers,
Rusty.
PS.  Also use 16, not 0 as coprocessor number.

Subject: KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.

We use the offset within the structure (which, conveniently, is used
internally as well) to supply unique ids for the core registers.

This makes it trivially extensible by appending fields to the struct.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
index a3daa67..ba35a15 100644
--- a/arch/arm/include/asm/kvm.h
+++ b/arch/arm/include/asm/kvm.h
@@ -32,31 +32,15 @@ enum KVM_ARM_IRQ_LINE_TYPE {
 	KVM_ARM_FIQ_LINE = 1,
 };
 
-/*
- * Modes used for short-hand mode determinition in the world-switch code and
- * in emulation code.
- *
- * Note: These indices do NOT correspond to the value of the CPSR mode bits!
- */
-enum vcpu_mode {
-	MODE_FIQ = 0,
-	MODE_IRQ,
-	MODE_SVC,
-	MODE_ABT,
-	MODE_UND,
-	MODE_USR,
-	MODE_SYS
-};
-
 struct kvm_regs {
-	__u32 regs0_7[8];	/* Unbanked regs. (r0 - r7)	   */
-	__u32 fiq_regs8_12[5];	/* Banked fiq regs. (r8 - r12)	   */
-	__u32 usr_regs8_12[5];	/* Banked usr registers (r8 - r12) */
-	__u32 reg13[6];		/* Banked r13, indexed by MODE_	   */
-	__u32 reg14[6];		/* Banked r13, indexed by MODE_	   */
-	__u32 reg15;
-	__u32 cpsr;
-	__u32 spsr[5];		/* Banked SPSR,  indexed by MODE_  */
+	__u32 usr_regs[15];	/* R0_usr - R14_usr */
+	__u32 svc_regs[3];	/* SP_svc, LR_svc, SPSR_svc */
+	__u32 abt_regs[3];	/* SP_abt, LR_abt, SPSR_abt */
+	__u32 und_regs[3];	/* SP_und, LR_und, SPSR_und */
+	__u32 irq_regs[3];	/* SP_irq, LR_irq, SPSR_irq */
+	__u32 fiq_regs[8];	/* R8_fiq - R14_fiq, SPSR_fiq */
+	__u32 pc;		/* The program counter (r15) */
+	__u32 cpsr;		/* The guest CPSR */
 };
 
 /* Supported Processor Types */
@@ -96,4 +80,8 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_32_CRN_START	11	/* Mask: 0x00007800 */
 #define KVM_REG_ARM_32_CRN_LEN		4
 
+/* Normal registers are mapped as "coprocessor" 16. */
+#define KVM_REG_ARM_CORE		(0x0010 << KVM_REG_ARM_COPROC_START)
+#define KVM_REG_ARM_CORE_REG(name)	(offsetof(struct kvm_regs, name) / 4)
+
 #endif /* __ARM_KVM_H__ */
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 64fa65f..4e67f94 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -20,6 +20,7 @@
 #define __ARM_KVM_HOST_H__
 
 #include <asm/fpstate.h>
+#include <asm/kvm.h>
 
 #define KVM_MAX_VCPUS 4
 #define KVM_MEMORY_SLOTS 32
@@ -75,16 +76,21 @@ struct kvm_mmu_memory_cache {
 	void *objects[KVM_NR_MEM_OBJS];
 };
 
-struct kvm_vcpu_regs {
-	u32 usr_regs[15];	/* R0_usr - R14_usr */
-	u32 svc_regs[3];	/* SP_svc, LR_svc, SPSR_svc */
-	u32 abt_regs[3];	/* SP_abt, LR_abt, SPSR_abt */
-	u32 und_regs[3];	/* SP_und, LR_und, SPSR_und */
-	u32 irq_regs[3];	/* SP_irq, LR_irq, SPSR_irq */
-	u32 fiq_regs[8];	/* R8_fiq - R14_fiq, SPSR_fiq */
-	u32 pc;			/* The program counter (r15) */
-	u32 cpsr;		/* The guest CPSR */
-} __packed;
+/*
+ * Modes used for short-hand mode determinition in the world-switch code and
+ * in emulation code.
+ *
+ * Note: These indices do NOT correspond to the value of the CPSR mode bits!
+ */
+enum vcpu_mode {
+	MODE_FIQ = 0,
+	MODE_IRQ,
+	MODE_SVC,
+	MODE_ABT,
+	MODE_UND,
+	MODE_USR,
+	MODE_SYS
+};
 
 /* 0 is reserved as an invalid value. */
 enum cp15_regs {
@@ -117,7 +123,7 @@ enum cp15_regs {
 };
 
 struct kvm_vcpu_arch {
-	struct kvm_vcpu_regs regs;
+	struct kvm_regs regs;
 
 	u32 target; /* Currently KVM_ARM_TARGET_CORTEX_A15 */
 	DECLARE_BITMAP(features, NUM_FEATURES);
@@ -192,4 +198,10 @@ static inline int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
 {
 	return 0;
 }
+
+int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
+unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
+struct kvm_one_reg;
+int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
+int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
 #endif /* __ARM_KVM_HOST_H__ */
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 0b9e8b4..ed78a66 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -756,7 +756,7 @@ static int set_invariant_cp15(u64 index, void __user *uaddr)
 	return 0;
 }
 
-int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	const struct coproc_reg *r;
 	void __user *uaddr = (void __user *)(long)reg->addr;
@@ -772,7 +772,7 @@ int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
-int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	const struct coproc_reg *r;
 	void __user *uaddr = (void __user *)(long)reg->addr;
@@ -876,27 +876,18 @@ static int walk_msrs(struct kvm_vcpu *vcpu, u64 __user *uind)
 	return total;
 }
 
-/**
- * kvm_arch_num_regs - how many registers do we present via KVM_GET_ONE_REG
- *
- * This is for special registers, particularly cp15.
- */
-unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu)
+unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu)
 {
-	return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u64 __user *)NULL);
+	return ARRAY_SIZE(invariant_cp15)
+		+ walk_msrs(vcpu, (u64 __user *)NULL);
 }
 
-/**
- * kvm_arch_copy_reg_indices - copy a series of coprocessor registers.
- *
- * This is for special registers, particularly cp15.
- */
-int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
 	unsigned int i;
 	int err;
 
-	/* First give them all the invariant registers' indices. */
+	/* Then give them all the invariant registers' indices. */
 	for (i = 0; i < ARRAY_SIZE(invariant_cp15); i++) {
 		if (put_user(cp15_to_index(&invariant_cp15[i]), uindices))
 			return -EFAULT;
diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 6cbdb08..ec33b80 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -25,7 +25,7 @@
 #include "trace.h"
 
 #define REG_OFFSET(_reg) \
-	(offsetof(struct kvm_vcpu_regs, _reg) / sizeof(u32))
+	(offsetof(struct kvm_regs, _reg) / sizeof(u32))
 
 #define USR_REG_OFFSET(_num) REG_OFFSET(usr_regs[_num])
 
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index 53f72a0..86a5ec2 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -38,75 +38,118 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
+static int get_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
-	struct kvm_vcpu_regs *vcpu_regs = &vcpu->arch.regs;
-
-	/*
-	 * GPRs and PSRs
-	 */
-	memcpy(regs->regs0_7, &(vcpu_regs->usr_regs[0]), sizeof(u32) * 8);
-	memcpy(regs->usr_regs8_12, &(vcpu_regs->usr_regs[8]), sizeof(u32) * 5);
-	memcpy(regs->fiq_regs8_12, &(vcpu_regs->fiq_regs[0]), sizeof(u32) * 5);
-	regs->reg13[MODE_FIQ] = vcpu_regs->fiq_regs[5];
-	regs->reg14[MODE_FIQ] = vcpu_regs->fiq_regs[6];
-	regs->reg13[MODE_IRQ] = vcpu_regs->irq_regs[0];
-	regs->reg14[MODE_IRQ] = vcpu_regs->irq_regs[1];
-	regs->reg13[MODE_SVC] = vcpu_regs->svc_regs[0];
-	regs->reg14[MODE_SVC] = vcpu_regs->svc_regs[1];
-	regs->reg13[MODE_ABT] = vcpu_regs->abt_regs[0];
-	regs->reg14[MODE_ABT] = vcpu_regs->abt_regs[1];
-	regs->reg13[MODE_UND] = vcpu_regs->und_regs[0];
-	regs->reg14[MODE_UND] = vcpu_regs->und_regs[1];
-	regs->reg13[MODE_USR] = vcpu_regs->usr_regs[13];
-	regs->reg14[MODE_USR] = vcpu_regs->usr_regs[14];
-
-	regs->spsr[MODE_FIQ]  = vcpu_regs->fiq_regs[7];
-	regs->spsr[MODE_IRQ]  = vcpu_regs->irq_regs[2];
-	regs->spsr[MODE_SVC]  = vcpu_regs->svc_regs[2];
-	regs->spsr[MODE_ABT]  = vcpu_regs->abt_regs[2];
-	regs->spsr[MODE_UND]  = vcpu_regs->und_regs[2];
-
-	regs->reg15 = vcpu_regs->pc;
-	regs->cpsr = vcpu_regs->cpsr;
+	u32 __user *uaddr = (u32 __user *)(long)reg->addr;
+	struct kvm_regs *regs = &vcpu->arch.regs;
+	u64 off;
+
+	if (KVM_REG_LEN(reg->id) != 4)
+		return -ENOENT;
+
+	/* Our ID is an index into the kvm_regs struct. */
+	off = reg->id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK);
+	if (off >= sizeof(*regs) / KVM_REG_LEN(reg->id))
+		return -ENOENT;
+
+	return put_user(((u32 *)regs)[off], uaddr);
+}
+
+static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	u32 __user *uaddr = (u32 __user *)(long)reg->addr;
+	struct kvm_regs *regs = &vcpu->arch.regs;
+	u64 off, val;
+
+	if (KVM_REG_LEN(reg->id) != 4)
+		return -ENOENT;
+
+	/* Our ID is an index into the kvm_regs struct. */
+	off = reg->id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK);
+	if (off >= sizeof(*regs) / KVM_REG_LEN(reg->id))
+		return -ENOENT;
+
+	if (get_user(val, uaddr) != 0)
+		return -EFAULT;
+
+	if (off == KVM_REG_ARM_CORE_REG(cpsr)) {
+		if (__vcpu_mode(val) == 0xf)
+			return -EINVAL;
+	}
 
+	((u32 *)regs)[off] = val;
 	return 0;
 }
 
+int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
+{
+	return -EINVAL;
+}
+
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	struct kvm_vcpu_regs *vcpu_regs = &vcpu->arch.regs;
+	return -EINVAL;
+}
+
+static unsigned long num_core_regs(void)
+{
+	return sizeof(struct kvm_regs) / sizeof(u32);
+}
+
+/**
+ * kvm_arch_num_regs - how many registers do we present via KVM_GET_ONE_REG
+ *
+ * This is for all registers.
+ */
+unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu)
+{
+	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu);
+}
+
+/**
+ * kvm_arch_copy_reg_indices - get indices of all registers.
+ *
+ * We do core registers right here, then we apppend coproc regs.
+ */
+int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	unsigned int i;
+
+	for (i = 0; i < sizeof(struct kvm_regs)/sizeof(u32); i++) {
+		if (put_user(KVM_REG_ARM|KVM_REG_ARM_CORE|i, uindices))
+			return -EFAULT;
+		uindices++;
+	}
+
+	return kvm_arm_copy_coproc_indices(vcpu, uindices);
+}
+
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	/* We currently only use lower 32 bits for arch-specific stuff. */
+	if ((reg->id & KVM_REG_ARCH_MASK & ~KVM_REG_SIZE_MASK & 0xFFFFFFFFUL)
+	    != KVM_REG_ARM)
+		return -EINVAL;
 
-	if (__vcpu_mode(regs->cpsr) == 0xf)
+	/* Coprocessor 0 means we want a core register. */
+	if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
+		return get_core_reg(vcpu, reg);
+
+	return kvm_arm_coproc_get_reg(vcpu, reg);
+}
+
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	/* We currently only use lower 32 bits for arch-specific stuff. */
+	if ((reg->id & KVM_REG_ARCH_MASK & ~KVM_REG_SIZE_MASK & 0xFFFFFFFFUL)
+	    != KVM_REG_ARM)
 		return -EINVAL;
 
-	memcpy(&(vcpu_regs->usr_regs[0]), regs->regs0_7, sizeof(u32) * 8);
-	memcpy(&(vcpu_regs->usr_regs[8]), regs->usr_regs8_12, sizeof(u32) * 5);
-	memcpy(&(vcpu_regs->fiq_regs[0]), regs->fiq_regs8_12, sizeof(u32) * 5);
-
-	vcpu_regs->fiq_regs[5] = regs->reg13[MODE_FIQ];
-	vcpu_regs->fiq_regs[6] = regs->reg14[MODE_FIQ];
-	vcpu_regs->irq_regs[0] = regs->reg13[MODE_IRQ];
-	vcpu_regs->irq_regs[1] = regs->reg14[MODE_IRQ];
-	vcpu_regs->svc_regs[0] = regs->reg13[MODE_SVC];
-	vcpu_regs->svc_regs[1] = regs->reg14[MODE_SVC];
-	vcpu_regs->abt_regs[0] = regs->reg13[MODE_ABT];
-	vcpu_regs->abt_regs[1] = regs->reg14[MODE_ABT];
-	vcpu_regs->und_regs[0] = regs->reg13[MODE_UND];
-	vcpu_regs->und_regs[1] = regs->reg14[MODE_UND];
-	vcpu_regs->usr_regs[13] = regs->reg13[MODE_USR];
-	vcpu_regs->usr_regs[14] = regs->reg14[MODE_USR];
-
-	vcpu_regs->fiq_regs[7] = regs->spsr[MODE_FIQ];
-	vcpu_regs->irq_regs[2] = regs->spsr[MODE_IRQ];
-	vcpu_regs->svc_regs[2] = regs->spsr[MODE_SVC];
-	vcpu_regs->abt_regs[2] = regs->spsr[MODE_ABT];
-	vcpu_regs->und_regs[2] = regs->spsr[MODE_UND];
-
-	vcpu_regs->pc = regs->reg15;
-	vcpu_regs->cpsr = regs->cpsr;
+	/* Coprocessor 0 means we want a core register. */
+	if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
+		return set_core_reg(vcpu, reg);
 
-	return 0;
+	return kvm_arm_coproc_set_reg(vcpu, reg);
 }
 
 int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
index 888799f..290a13d 100644
--- a/arch/arm/kvm/reset.c
+++ b/arch/arm/kvm/reset.c
@@ -33,7 +33,7 @@
 
 static const int a15_max_cpu_idx = 3;
 
-static struct kvm_vcpu_regs a15_regs_reset = {
+static struct kvm_regs a15_regs_reset = {
 	.cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
 };
 
@@ -51,7 +51,7 @@ static struct kvm_vcpu_regs a15_regs_reset = {
  */
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
-	struct kvm_vcpu_regs *cpu_reset;
+	struct kvm_regs *cpu_reset;
 
 	switch (vcpu->arch.target) {
 	case KVM_ARM_TARGET_CORTEX_A15: