[RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Hi all,

        This compiles, completely untested, but it's my attempt to give
Avi (and Alexander) what he asked for in a generic register accessor.

Mingled in these patches is the conversion of the latest KVM ARM code,
which is the first proposed user: by the end, we use these accessors for
*every* register and piece of state.

GET_MULTI/SET_MULTI is an obvious extension which is not yet
implemented.

Comments please!
Rusty.

[RFC 1/5] KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.

[Rusty Russell]

Avi has indicated that this is the future.  For now, make it dependent on
KVM_HAVE_ONE_REG (and define that for PPC and S/390).

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

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index a29e091..7506652 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -49,6 +49,7 @@
 #include <linux/mmu_notifier.h>
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
+#define KVM_HAVE_ONE_REG
 
 struct kvm;
 extern int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 83e929e..8c711f1 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -535,7 +535,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -550,7 +550,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	return r;
 }
 
-int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 05c28f5..add88a9 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -899,7 +899,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -915,7 +915,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	return r;
 }
 
-int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index d25a097..d239e8e 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1219,12 +1219,12 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	return kvmppc_core_set_sregs(vcpu, sregs);
 }
 
-int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	return -EINVAL;
 }
 
-int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	return -EINVAL;
 }
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 879b14a..21cd47b 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -228,7 +228,6 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_PPC_UNSET_IRQ:
 	case KVM_CAP_PPC_IRQ_LEVEL:
 	case KVM_CAP_ENABLE_CAP:
-	case KVM_CAP_ONE_REG:
 		r = 1;
 		break;
 #ifndef CONFIG_KVM_BOOK3S_64_HV
@@ -708,20 +707,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		break;
 	}
 
-	case KVM_SET_ONE_REG:
-	case KVM_GET_ONE_REG:
-	{
-		struct kvm_one_reg reg;
-		r = -EFAULT;
-		if (copy_from_user(&reg, argp, sizeof(reg)))
-			goto out;
-		if (ioctl == KVM_SET_ONE_REG)
-			r = kvm_vcpu_ioctl_set_one_reg(vcpu, &reg);
-		else
-			r = kvm_vcpu_ioctl_get_one_reg(vcpu, &reg);
-		break;
-	}
-
 #if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
 	case KVM_DIRTY_TLB: {
 		struct kvm_dirty_tlb dirty;
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index b784154..9adb19d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -23,6 +23,7 @@
 #define KVM_MEMORY_SLOTS 32
 /* memory slots that does not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 4
+#define KVM_HAVE_ONE_REG
 
 struct sca_entry {
 	atomic_t scn;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e83df7f..916cf1d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -139,7 +139,6 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_S390_UCONTROL:
 #endif
 	case KVM_CAP_SYNC_REGS:
-	case KVM_CAP_ONE_REG:
 		r = 1;
 		break;
 	case KVM_CAP_NR_VCPUS:
@@ -447,8 +446,7 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
-					   struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -476,8 +474,7 @@ static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
 	return r;
 }
 
-static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
-					   struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -839,18 +836,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
-	case KVM_SET_ONE_REG:
-	case KVM_GET_ONE_REG: {
-		struct kvm_one_reg reg;
-		r = -EFAULT;
-		if (copy_from_user(&reg, argp, sizeof(reg)))
-			break;
-		if (ioctl == KVM_SET_ONE_REG)
-			r = kvm_arch_vcpu_ioctl_set_one_reg(vcpu, &reg);
-		else
-			r = kvm_arch_vcpu_ioctl_get_one_reg(vcpu, &reg);
-		break;
-	}
 #ifdef CONFIG_KVM_S390_UCONTROL
 	case KVM_S390_UCAS_MAP: {
 		struct kvm_s390_ucas_mapping ucasmap;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index d2b897e..f3d43c3 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -566,6 +566,12 @@ void kvm_arch_hardware_unsetup(void);
 void kvm_arch_check_processor_compat(void *rtn);
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
+#ifdef KVM_HAVE_ONE_REG
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+#define KVM_REG_LEN(index)						\
+	(1U << (((index) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
+#endif
 
 void kvm_free_physmem(struct kvm *kvm);
 
@@ -962,5 +968,6 @@ static inline bool kvm_vcpu_eligible_for_directed_yield(struct kvm_vcpu *vcpu)
 }
 
 #endif /* CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT */
+
 #endif
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a2e85af..ad67cf4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1935,6 +1935,21 @@ out_free2:
 		r = 0;
 		break;
 	}
+#ifdef KVM_HAVE_ONE_REG
+	case KVM_SET_ONE_REG:
+	case KVM_GET_ONE_REG: {
+		struct kvm_one_reg reg;
+		r = -EFAULT;
+		if (copy_from_user(&reg, argp, sizeof(reg)))
+			goto out;
+		if (ioctl == KVM_SET_ONE_REG)
+			r = kvm_arch_set_reg(vcpu, &reg);
+		else
+			r = kvm_arch_get_reg(vcpu, &reg);
+		break;
+	}
+#endif
+
 	default:
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
 	}
@@ -2246,6 +2261,9 @@ static long kvm_dev_ioctl_check_extension_generic(long arg)
 #ifdef CONFIG_HAVE_KVM_MSI
 	case KVM_CAP_SIGNAL_MSI:
 #endif
+#ifdef KVM_HAVE_ONE_REG
+	case KVM_CAP_ONE_REG:
+#endif
 		return 1;
 #ifdef KVM_CAP_IRQ_ROUTING
 	case KVM_CAP_IRQ_ROUTING:

Re: [RFC 1/5] KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.

[Avi Kivity]

On 08/28/2012 04:45 PM, Rusty Russell wrote:
> Avi has indicated that this is the future.  For now, make it dependent on
> KVM_HAVE_ONE_REG (and define that for PPC and S/390).

I want GET_MULTI, really.  But maybe iterating over ONE_REG isn't so bad
since we do it so rarely.:

Would all register IDs fit in 64 bits?  cpuid (read only) is addressed
using eax (32-bits), ecx (for some values of eax), and yields 4 32-bit
values.  That's 66 bits of addressing, independent of some way to
discriminate between register sets.

I guess we could special case the cases where ecx is needed, or only
allow 8 bits for ecx (always okay so far).

Other x86 state:
  GPRs - ok
  MSRs - ok
  FPU - one register per xmm/st(x)? the entire xsave area?
  APIC  - one register per apic register? the entire 4k page?
  Various non-register state (pending exceptions, run state,
blocked-by-sti/ss) - ok
  SVM/VMX state - ok
  Segments: one register per segment? one per component?
  Control registers: ok.  Should userspace be careful to set registers
in legal ways only? i.e. cannot set cr3[0:11] if cr4.pae=0, or vice
versa, so need three writes?
  IOAPIC/PIC/PIT - not vcpu state
  Debug registers - ok
  xcr - ok
 
Yuch, we have a lot of state in that thing.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [RFC 1/5] KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.

[Peter Maydell]

On 1 September 2012 10:11, Avi Kivity <avi@redhat.com> wrote:
> Other x86 state:
>   Control registers: ok.  Should userspace be careful to set registers
> in legal ways only? i.e. cannot set cr3[0:11] if cr4.pae=0, or vice
> versa, so need three writes?

The principle I'm hoping we can hold to for ARM is that the
kernel only exposes state in such a way that it's always
possible for userspace to migrate it with a simple "read
everything, send to destination, write everything", ie
without needing to know anything of the semantics of any
of these registers.

This means that registers which have access controls (eg
"can't write this unless you wrote to that other one first")
should not enforce those checks for userspace get/set.
More significantly, it means that registers with odd
behaviour, like "write 1 to clear" or "register A selects
which of an array of underlying registers is exposed
in register B" are not directly exposed at all. Instead
the kernel provides some other (ersatz) register indexes
which let userspace do plain get/set on the underlying
state.

The idea is that then migration depends only on whether
the destination kernel supports all the registers the
source kernel does, and we avoid extra dependencies
on the source and destination qemu. (Most of the state
being transferred is of no interest to userspace at all.)
[It also makes write-multiple easier to use.]

-- PMM

Re: [RFC 1/5] KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.

[Avi Kivity]

On 09/01/2012 03:18 AM, Peter Maydell wrote:
> On 1 September 2012 10:11, Avi Kivity <avi@redhat.com> wrote:
> > Other x86 state:
> >   Control registers: ok.  Should userspace be careful to set registers
> > in legal ways only? i.e. cannot set cr3[0:11] if cr4.pae=0, or vice
> > versa, so need three writes?
>
> The principle I'm hoping we can hold to for ARM is that the
> kernel only exposes state in such a way that it's always
> possible for userspace to migrate it with a simple "read
> everything, send to destination, write everything", ie
> without needing to know anything of the semantics of any
> of these registers.
>
> This means that registers which have access controls (eg
> "can't write this unless you wrote to that other one first")
> should not enforce those checks for userspace get/set.
> More significantly, it means that registers with odd
> behaviour, like "write 1 to clear" or "register A selects
> which of an array of underlying registers is exposed
> in register B" are not directly exposed at all. Instead
> the kernel provides some other (ersatz) register indexes
> which let userspace do plain get/set on the underlying
> state.

I came to the same conclusion.  It could be implemented via a
KVM_REQ_REEVALUATE, which causes all registers to be validated and
applied before guest entry.

> The idea is that then migration depends only on whether
> the destination kernel supports all the registers the
> source kernel does, and we avoid extra dependencies
> on the source and destination qemu. (Most of the state
> being transferred is of no interest to userspace at all.)
> [It also makes write-multiple easier to use.]

We also need to support migration to an earlier kernel.  That means new
state is not transferred (of course this state must not be exposed to
the guest).  There is also the hack with subsections: if we discover new
hidden state, then qemu can avoid transferring it to an older kernel,
provided the value matches the default (which should be often, otherwise
the bug would have been discovered much earlier).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[RFC 2/5] KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.

[Rusty Russell]

Instead of overloading x86's KVM_GET_MSRS/KVM_SET_MSRS.  The only basic
difference is that the ids are 64 bit.

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 d040a2a..9838456 100644
--- a/arch/arm/include/asm/kvm.h
+++ b/arch/arm/include/asm/kvm.h
@@ -85,35 +85,21 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
-/* Based on x86, but we use KVM_GET_VCPU_MSR_INDEX_LIST. */
-struct kvm_msr_entry {
-	__u32 index;
-	__u32 reserved;
-	__u64 data;
-};
-
-/* for KVM_GET_MSRS and KVM_SET_MSRS */
-struct kvm_msrs {
-	__u32 nmsrs; /* number of msrs in entries */
-	__u32 pad;
-
-	struct kvm_msr_entry entries[0];
-};
-
 /* for KVM_VCPU_GET_MSR_INDEX_LIST */
 struct kvm_msr_list {
-	__u32 nmsrs; /* number of msrs in entries */
-	__u32 indices[0];
+	__u64 nmsrs; /* number of msrs in entries */
+	__u64 indices[0];
 };
 
-/* If you need to interpret the index values, here's the key. */
-#define KVM_ARM_MSR_COPROC_MASK		0xFFFF0000
-#define KVM_ARM_MSR_64_BIT_MASK		0x00008000
-#define KVM_ARM_MSR_64_OPC1_MASK	0x000000F0
-#define KVM_ARM_MSR_64_CRM_MASK		0x0000000F
-#define KVM_ARM_MSR_32_CRM_MASK		0x0000000F
-#define KVM_ARM_MSR_32_OPC2_MASK	0x00000070
-#define KVM_ARM_MSR_32_CRN_MASK		0x00000780
-#define KVM_ARM_MSR_32_OPC1_MASK	0x00003800
+/* If you need to interpret the index values, here are the bit offsets. */
+#define KVM_REG_ARM_COPROC_START	16	/* Mask: 0xFFFF0000 */
+#define KVM_REG_ARM_32_OPC2_START	0	/* Mask: 0x00000007 */
+#define KVM_REG_ARM_32_OPC2_LEN		3
+#define KVM_REG_ARM_OPC1_START		3	/* Mask: 0x00000078 */
+#define KVM_REG_ARM_OPC1_LEN		4
+#define KVM_REG_ARM_CRM_START		7	/* Mask: 0x00000780 */
+#define KVM_REG_ARM_CRM_LEN		4
+#define KVM_REG_ARM_32_CRN_START	11	/* Mask: 0x00007800 */
+#define KVM_REG_ARM_32_CRN_LEN		4
 
 #endif /* __ARM_KVM_H__ */
diff --git a/arch/arm/include/asm/kvm_coproc.h b/arch/arm/include/asm/kvm_coproc.h
index 894574c..899b3d3 100644
--- a/arch/arm/include/asm/kvm_coproc.h
+++ b/arch/arm/include/asm/kvm_coproc.h
@@ -28,11 +28,7 @@ int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
 
-int kvm_arm_get_msrs(struct kvm_vcpu *vcpu,
-		     struct kvm_msr_entry __user *entries, u32 num);
-int kvm_arm_set_msrs(struct kvm_vcpu *vcpu,
-		     struct kvm_msr_entry __user *entries, u32 num);
 unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu);
-int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u32 __user *uindices);
+int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 void kvm_coproc_table_init(void);
 #endif /* __ARM_KVM_COPROC_H__ */
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 1c0fa75..7548c95 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -25,6 +25,7 @@
 #define KVM_MEMORY_SLOTS 32
 #define KVM_PRIVATE_MEM_SLOTS 4
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
+#define KVM_HAVE_ONE_REG
 
 #define NUM_FEATURES 0
 
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 4248aa1..55a2995 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -726,20 +726,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 			return -E2BIG;
 		return kvm_arm_copy_msrindices(vcpu, user_msr_list->indices);
 	}
-	case KVM_GET_MSRS: {
-		struct kvm_msrs msrs;
-		struct kvm_msrs __user *umsrs = argp;
-		if (copy_from_user(&msrs, umsrs, sizeof(msrs)) != 0)
-			return -EFAULT;
-		return kvm_arm_get_msrs(vcpu, umsrs->entries, msrs.nmsrs);
-	}
-	case KVM_SET_MSRS: {
-		struct kvm_msrs msrs;
-		struct kvm_msrs __user *umsrs = argp;
-		if (copy_from_user(&msrs, umsrs, sizeof(msrs)) != 0)
-			return -EFAULT;
-		return kvm_arm_set_msrs(vcpu, umsrs->entries, msrs.nmsrs);
-	}
 	default:
 		return -EINVAL;
 	}
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 824e5a3..99de71b 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -568,41 +568,53 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
  *****************************************************************************/
 
 /* Given a simple mask, get those bits. */
-static inline u32 get_bits(u32 index, u32 mask)
+static inline u32 get_bits(u32 index, u32 start, u32 len)
 {
-	return (index & mask) >> (ffs(mask) - 1);
+	return (index >> start) & ((1 << len)-1);
 }
 
-static void index_to_params(u32 index, struct coproc_params *params)
+static bool index_to_params(u64 index, struct coproc_params *params)
 {
-	if (get_bits(index, KVM_ARM_MSR_64_BIT_MASK)) {
+	switch (index & KVM_REG_SIZE_MASK) {
+	case KVM_REG_SIZE_U32:
+		params->is_64bit = false;
+		params->CRn = get_bits(index, KVM_REG_ARM_32_CRN_START,
+				       KVM_REG_ARM_32_CRN_LEN);
+		params->CRm = get_bits(index, KVM_REG_ARM_CRM_START,
+				       KVM_REG_ARM_CRM_LEN);
+		params->Op1 = get_bits(index, KVM_REG_ARM_OPC1_START,
+				       KVM_REG_ARM_OPC1_LEN);
+		params->Op2 = get_bits(index, KVM_REG_ARM_32_OPC2_START,
+				       KVM_REG_ARM_32_OPC2_LEN);
+		return true;
+	case KVM_REG_SIZE_U64:
 		params->is_64bit = true;
-		params->CRm = get_bits(index, KVM_ARM_MSR_64_CRM_MASK);
-		params->Op1 = get_bits(index, KVM_ARM_MSR_64_OPC1_MASK);
+		params->CRm = get_bits(index, KVM_REG_ARM_CRM_START,
+				       KVM_REG_ARM_CRM_LEN);
+		params->Op1 = get_bits(index, KVM_REG_ARM_OPC1_START,
+				       KVM_REG_ARM_OPC1_LEN);
 		params->Op2 = 0;
 		params->CRn = 0;
-	} else {
-		params->is_64bit = false;
-		params->CRn = get_bits(index, KVM_ARM_MSR_32_CRN_MASK);
-		params->CRm = get_bits(index, KVM_ARM_MSR_32_CRM_MASK);
-		params->Op1 = get_bits(index, KVM_ARM_MSR_32_OPC1_MASK);
-		params->Op2 = get_bits(index, KVM_ARM_MSR_32_OPC2_MASK);
+		return true;
+	default:
+		return false;
 	}
 }
 
 /* Decode an index value, and find the cp15 coproc_reg entry. */
 static const struct coproc_reg *index_to_coproc_reg(struct kvm_vcpu *vcpu,
-						    u32 index)
+						    u64 index)
 {
 	size_t num;
 	const struct coproc_reg *table, *r;
 	struct coproc_params params;
 
 	/* We only do cp15 for now. */
-	if (get_bits(index, KVM_ARM_MSR_COPROC_MASK != 15))
+	if (get_bits(index, KVM_REG_ARM_COPROC_START, 16) != 15)
 		return NULL;
 
-	index_to_params(index, &params);
+	if (!index_to_params(index, &params))
+		return NULL;
 
 	table = get_target_table(vcpu->arch.target, &num);
 	r = find_reg(&params, table, num);
@@ -689,30 +701,54 @@ static struct coproc_reg invariant_cp15[] = {
 	{ CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
-static int get_invariant_cp15(u32 index, u64 *val)
+static int reg_from_user(void *val, const void __user *uaddr, u64 index)
+{
+	/* This Just Works because we are little endian. */
+	if (copy_from_user(val, uaddr, KVM_REG_LEN(index)) != 0)
+		return -EFAULT;
+	return 0;
+}
+
+static int reg_to_user(void __user *uaddr, const void *val, u64 index)
+{
+	/* This Just Works because we are little endian. */
+	if (copy_to_user(uaddr, val, KVM_REG_LEN(index)) != 0)
+		return -EFAULT;
+	return 0;
+}
+
+static int get_invariant_cp15(u64 index, void __user *uaddr)
 {
 	struct coproc_params params;
 	const struct coproc_reg *r;
 
-	index_to_params(index, &params);
+	if (!index_to_params(index, &params))
+		return -ENOENT;
+
 	r = find_reg(&params, invariant_cp15, ARRAY_SIZE(invariant_cp15));
 	if (!r)
 		return -ENOENT;
 
-	*val = r->val;
-	return 0;
+	return reg_to_user(uaddr, &r->val, index);
 }
 
-static int set_invariant_cp15(u32 index, u64 val)
+static int set_invariant_cp15(u64 index, void __user *uaddr)
 {
 	struct coproc_params params;
 	const struct coproc_reg *r;
+	int err;
+	u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
 
-	index_to_params(index, &params);
+	if (!index_to_params(index, &params))
+		return -ENOENT;
 	r = find_reg(&params, invariant_cp15, ARRAY_SIZE(invariant_cp15));
 	if (!r)
 		return -ENOENT;
 
+	err = reg_from_user(&val, uaddr, index);
+	if (err)
+		return err;
+
 	/* This is what we mean by invariant: you can't change it. */
 	if (r->val != val)
 		return -EINVAL;
@@ -720,95 +756,36 @@ static int set_invariant_cp15(u32 index, u64 val)
 	return 0;
 }
 
-static int get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *val)
+int kvm_arch_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;
 
-	r = index_to_coproc_reg(vcpu, index);
-	if (!r)
-		return get_invariant_cp15(index, val);
-
-	*val = vcpu->arch.cp15[r->reg];
-	if (r->is_64)
-		*val |= ((u64)vcpu->arch.cp15[r->reg+1]) << 32;
-	return 0;
-}
-
-static int set_msr(struct kvm_vcpu *vcpu, u32 index, u64 val)
-{
-	const struct coproc_reg *r;
+	if ((reg->id & KVM_REG_ARCH_MASK) != KVM_REG_ARM)
+		return -EINVAL;
 
-	r = index_to_coproc_reg(vcpu, index);
+	r = index_to_coproc_reg(vcpu, reg->id);
 	if (!r)
-		return set_invariant_cp15(index, val);
+		return get_invariant_cp15(reg->id, uaddr);
 
-	vcpu->arch.cp15[r->reg] = val;
-	if (r->is_64)
-		vcpu->arch.cp15[r->reg+1] = (val >> 32);
-	return 0;
-}
-
-/* Return user adddress to get/set value from. */
-static u64 __user *get_umsr(struct kvm_msr_entry __user *uentry, u32 *idx)
-{
-	struct kvm_msr_entry entry;
-
-	if (copy_from_user(&entry, uentry, sizeof(entry)))
-		return NULL;
-	*idx = entry.index;
-	return &uentry->data;
+	/* Note: copies two regs if size is 64 bit. */
+	return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
-/**
- * kvm_arm_get_msrs - copy one or more special registers to userspace.
- * @vcpu: the vcpu
- * @entries: the array of entries
- * @num: the number of entries
- */
-int kvm_arm_get_msrs(struct kvm_vcpu *vcpu,
-		     struct kvm_msr_entry __user *entries, u32 num)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
-	u32 i, index;
-	u64 val;
-	u64 __user *uval;
-	int ret;
+	const struct coproc_reg *r;
+	void __user *uaddr = (void __user *)(long)reg->addr;
 
-	for (i = 0; i < num; i++) {
-		uval = get_umsr(&entries[i], &index);
-		if (!uval)
-			return -EFAULT;
-		if ((ret = get_msr(vcpu, index, &val)) != 0)
-			return ret;
-		if (put_user(val, uval))
-			return -EFAULT;
-	}
-	return 0;
-}
+	if ((reg->id & KVM_REG_ARCH_MASK) != KVM_REG_ARM)
+		return -EINVAL;
 
-/**
- * kvm_arm_set_msrs - copy one or more special registers from userspace.
- * @vcpu: the vcpu
- * @entries: the array of entries
- * @num: the number of entries
- */
-int kvm_arm_set_msrs(struct kvm_vcpu *vcpu,
-		     struct kvm_msr_entry __user *entries, u32 num)
-{
-	u32 i, index;
-	u64 val;
-	u64 __user *uval;
-	int ret;
+	r = index_to_coproc_reg(vcpu, reg->id);
+	if (!r)
+		return set_invariant_cp15(reg->id, uaddr);
 
-	for (i = 0; i < num; i++) {
-		uval = get_umsr(&entries[i], &index);
-		if (!uval)
-			return -EFAULT;
-		if (copy_from_user(&val, uval, sizeof(val)) != 0)
-			return -EFAULT;
-		if ((ret = set_msr(vcpu, index, val)) != 0)
-			return ret;
-	}
-	return 0;
+	/* Note: copies two regs if size is 64 bit */
+	return copy_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static int cmp_reg(const struct coproc_reg *i1, const struct coproc_reg *i2)
@@ -827,29 +804,24 @@ static int cmp_reg(const struct coproc_reg *i1, const struct coproc_reg *i2)
 	return i1->Op2 - i2->Op2;
 }
 
-/* Puts in the position indicated by mask (assumes val fits in mask) */
-static inline u32 set_bits(u32 val, u32 mask)
-{
-	return val << (ffs(mask)-1);
-}
-
-static u32 cp15_to_index(const struct coproc_reg *reg)
+static u64 cp15_to_index(const struct coproc_reg *reg)
 {
-	u32 val = set_bits(15, KVM_ARM_MSR_COPROC_MASK);
+	u64 val = (15 << KVM_REG_ARM_COPROC_START);
 	if (reg->is_64) {
-		val |= set_bits(1, KVM_ARM_MSR_64_BIT_MASK);
-		val |= set_bits(reg->Op1, KVM_ARM_MSR_64_OPC1_MASK);
-		val |= set_bits(reg->CRm, KVM_ARM_MSR_64_CRM_MASK);
+		val |= KVM_REG_SIZE_U64;
+		val |= (reg->Op1 << KVM_REG_ARM_OPC1_START);
+		val |= (reg->CRm << KVM_REG_ARM_CRM_START);
 	} else {
-		val |= set_bits(reg->Op1, KVM_ARM_MSR_32_OPC1_MASK);
-		val |= set_bits(reg->Op2, KVM_ARM_MSR_32_OPC2_MASK);
-		val |= set_bits(reg->CRm, KVM_ARM_MSR_32_CRM_MASK);
-		val |= set_bits(reg->CRn, KVM_ARM_MSR_32_CRN_MASK);
+		val |= KVM_REG_SIZE_U32;
+		val |= (reg->Op1 << KVM_REG_ARM_OPC1_START);
+		val |= (reg->Op2 << KVM_REG_ARM_32_OPC2_START);
+		val |= (reg->CRm << KVM_REG_ARM_CRM_START);
+		val |= (reg->CRn << KVM_REG_ARM_32_CRN_START);
 	}
 	return val;
 }
 
-static bool copy_reg_to_user(const struct coproc_reg *reg, u32 __user **uind)
+static bool copy_reg_to_user(const struct coproc_reg *reg, u64 __user **uind)
 {
 	if (!*uind)
 		return true;
@@ -862,7 +834,7 @@ static bool copy_reg_to_user(const struct coproc_reg *reg, u32 __user **uind)
 }
 
 /* Assumed ordered tables, see kvm_coproc_table_init. */
-static int walk_msrs(struct kvm_vcpu *vcpu, u32 __user *uind)
+static int walk_msrs(struct kvm_vcpu *vcpu, u64 __user *uind)
 {
 	const struct coproc_reg *i1, *i2, *end1, *end2;
 	unsigned int total = 0;
@@ -911,7 +883,7 @@ static int walk_msrs(struct kvm_vcpu *vcpu, u32 __user *uind)
  */
 unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
 {
-	return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u32 __user *)NULL);
+	return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u64 __user *)NULL);
 }
 
 /**
@@ -919,7 +891,7 @@ unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
  *
  * This is for special registers, particularly cp15.
  */
-int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u32 __user *uindices)
+int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
 	unsigned int i;
 	int err;

Re: [RFC 2/5] KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.

[Christoffer Dall]

On Tue, Aug 28, 2012 at 4:46 PM, Rusty Russell <rusty.russell@linaro.org> wrote:
> Instead of overloading x86's KVM_GET_MSRS/KVM_SET_MSRS.  The only basic
> difference is that the ids are 64 bit.
>
> 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 d040a2a..9838456 100644
> --- a/arch/arm/include/asm/kvm.h
> +++ b/arch/arm/include/asm/kvm.h
> @@ -85,35 +85,21 @@ struct kvm_sync_regs {
>  struct kvm_arch_memory_slot {
>  };
>
> -/* Based on x86, but we use KVM_GET_VCPU_MSR_INDEX_LIST. */
> -struct kvm_msr_entry {
> -       __u32 index;
> -       __u32 reserved;
> -       __u64 data;
> -};
> -
> -/* for KVM_GET_MSRS and KVM_SET_MSRS */
> -struct kvm_msrs {
> -       __u32 nmsrs; /* number of msrs in entries */
> -       __u32 pad;
> -
> -       struct kvm_msr_entry entries[0];
> -};
> -
>  /* for KVM_VCPU_GET_MSR_INDEX_LIST */
>  struct kvm_msr_list {
> -       __u32 nmsrs; /* number of msrs in entries */
> -       __u32 indices[0];
> +       __u64 nmsrs; /* number of msrs in entries */
> +       __u64 indices[0];
>  };
>
> -/* If you need to interpret the index values, here's the key. */
> -#define KVM_ARM_MSR_COPROC_MASK                0xFFFF0000
> -#define KVM_ARM_MSR_64_BIT_MASK                0x00008000
> -#define KVM_ARM_MSR_64_OPC1_MASK       0x000000F0
> -#define KVM_ARM_MSR_64_CRM_MASK                0x0000000F
> -#define KVM_ARM_MSR_32_CRM_MASK                0x0000000F
> -#define KVM_ARM_MSR_32_OPC2_MASK       0x00000070
> -#define KVM_ARM_MSR_32_CRN_MASK                0x00000780
> -#define KVM_ARM_MSR_32_OPC1_MASK       0x00003800
> +/* If you need to interpret the index values, here are the bit offsets. */
> +#define KVM_REG_ARM_COPROC_START       16      /* Mask: 0xFFFF0000 */
> +#define KVM_REG_ARM_32_OPC2_START      0       /* Mask: 0x00000007 */
> +#define KVM_REG_ARM_32_OPC2_LEN                3
> +#define KVM_REG_ARM_OPC1_START         3       /* Mask: 0x00000078 */
> +#define KVM_REG_ARM_OPC1_LEN           4
> +#define KVM_REG_ARM_CRM_START          7       /* Mask: 0x00000780 */
> +#define KVM_REG_ARM_CRM_LEN            4
> +#define KVM_REG_ARM_32_CRN_START       11      /* Mask: 0x00007800 */
> +#define KVM_REG_ARM_32_CRN_LEN         4
>
>  #endif /* __ARM_KVM_H__ */
> diff --git a/arch/arm/include/asm/kvm_coproc.h b/arch/arm/include/asm/kvm_coproc.h
> index 894574c..899b3d3 100644
> --- a/arch/arm/include/asm/kvm_coproc.h
> +++ b/arch/arm/include/asm/kvm_coproc.h
> @@ -28,11 +28,7 @@ int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>
> -int kvm_arm_get_msrs(struct kvm_vcpu *vcpu,
> -                    struct kvm_msr_entry __user *entries, u32 num);
> -int kvm_arm_set_msrs(struct kvm_vcpu *vcpu,
> -                    struct kvm_msr_entry __user *entries, u32 num);
>  unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu);
> -int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u32 __user *uindices);
> +int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u64 __user *uindices);
>  void kvm_coproc_table_init(void);
>  #endif /* __ARM_KVM_COPROC_H__ */
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 1c0fa75..7548c95 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -25,6 +25,7 @@
>  #define KVM_MEMORY_SLOTS 32
>  #define KVM_PRIVATE_MEM_SLOTS 4
>  #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
> +#define KVM_HAVE_ONE_REG
>
>  #define NUM_FEATURES 0
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 4248aa1..55a2995 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -726,20 +726,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>                         return -E2BIG;
>                 return kvm_arm_copy_msrindices(vcpu, user_msr_list->indices);
>         }
> -       case KVM_GET_MSRS: {
> -               struct kvm_msrs msrs;
> -               struct kvm_msrs __user *umsrs = argp;
> -               if (copy_from_user(&msrs, umsrs, sizeof(msrs)) != 0)
> -                       return -EFAULT;
> -               return kvm_arm_get_msrs(vcpu, umsrs->entries, msrs.nmsrs);
> -       }
> -       case KVM_SET_MSRS: {
> -               struct kvm_msrs msrs;
> -               struct kvm_msrs __user *umsrs = argp;
> -               if (copy_from_user(&msrs, umsrs, sizeof(msrs)) != 0)
> -                       return -EFAULT;
> -               return kvm_arm_set_msrs(vcpu, umsrs->entries, msrs.nmsrs);
> -       }
>         default:
>                 return -EINVAL;
>         }
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 824e5a3..99de71b 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -568,41 +568,53 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>   *****************************************************************************/
>
>  /* Given a simple mask, get those bits. */
> -static inline u32 get_bits(u32 index, u32 mask)
> +static inline u32 get_bits(u32 index, u32 start, u32 len)
>  {
> -       return (index & mask) >> (ffs(mask) - 1);
> +       return (index >> start) & ((1 << len)-1);
>  }
>
> -static void index_to_params(u32 index, struct coproc_params *params)
> +static bool index_to_params(u64 index, struct coproc_params *params)
>  {
> -       if (get_bits(index, KVM_ARM_MSR_64_BIT_MASK)) {
> +       switch (index & KVM_REG_SIZE_MASK) {
> +       case KVM_REG_SIZE_U32:
> +               params->is_64bit = false;
> +               params->CRn = get_bits(index, KVM_REG_ARM_32_CRN_START,
> +                                      KVM_REG_ARM_32_CRN_LEN);
> +               params->CRm = get_bits(index, KVM_REG_ARM_CRM_START,
> +                                      KVM_REG_ARM_CRM_LEN);
> +               params->Op1 = get_bits(index, KVM_REG_ARM_OPC1_START,
> +                                      KVM_REG_ARM_OPC1_LEN);
> +               params->Op2 = get_bits(index, KVM_REG_ARM_32_OPC2_START,
> +                                      KVM_REG_ARM_32_OPC2_LEN);
> +               return true;
> +       case KVM_REG_SIZE_U64:
>                 params->is_64bit = true;
> -               params->CRm = get_bits(index, KVM_ARM_MSR_64_CRM_MASK);
> -               params->Op1 = get_bits(index, KVM_ARM_MSR_64_OPC1_MASK);
> +               params->CRm = get_bits(index, KVM_REG_ARM_CRM_START,
> +                                      KVM_REG_ARM_CRM_LEN);
> +               params->Op1 = get_bits(index, KVM_REG_ARM_OPC1_START,
> +                                      KVM_REG_ARM_OPC1_LEN);
>                 params->Op2 = 0;
>                 params->CRn = 0;
> -       } else {
> -               params->is_64bit = false;
> -               params->CRn = get_bits(index, KVM_ARM_MSR_32_CRN_MASK);
> -               params->CRm = get_bits(index, KVM_ARM_MSR_32_CRM_MASK);
> -               params->Op1 = get_bits(index, KVM_ARM_MSR_32_OPC1_MASK);
> -               params->Op2 = get_bits(index, KVM_ARM_MSR_32_OPC2_MASK);
> +               return true;
> +       default:
> +               return false;
>         }
>  }
>
>  /* Decode an index value, and find the cp15 coproc_reg entry. */
>  static const struct coproc_reg *index_to_coproc_reg(struct kvm_vcpu *vcpu,
> -                                                   u32 index)
> +                                                   u64 index)
>  {
>         size_t num;
>         const struct coproc_reg *table, *r;
>         struct coproc_params params;
>
>         /* We only do cp15 for now. */
> -       if (get_bits(index, KVM_ARM_MSR_COPROC_MASK != 15))
> +       if (get_bits(index, KVM_REG_ARM_COPROC_START, 16) != 15)
>                 return NULL;
>
> -       index_to_params(index, &params);
> +       if (!index_to_params(index, &params))
> +               return NULL;
>
>         table = get_target_table(vcpu->arch.target, &num);
>         r = find_reg(&params, table, num);
> @@ -689,30 +701,54 @@ static struct coproc_reg invariant_cp15[] = {
>         { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
>  };
>
> -static int get_invariant_cp15(u32 index, u64 *val)
> +static int reg_from_user(void *val, const void __user *uaddr, u64 index)
> +{
> +       /* This Just Works because we are little endian. */
> +       if (copy_from_user(val, uaddr, KVM_REG_LEN(index)) != 0)
> +               return -EFAULT;
> +       return 0;
> +}
> +
> +static int reg_to_user(void __user *uaddr, const void *val, u64 index)
> +{
> +       /* This Just Works because we are little endian. */
> +       if (copy_to_user(uaddr, val, KVM_REG_LEN(index)) != 0)
> +               return -EFAULT;
> +       return 0;
> +}
> +
> +static int get_invariant_cp15(u64 index, void __user *uaddr)
>  {
>         struct coproc_params params;
>         const struct coproc_reg *r;
>
> -       index_to_params(index, &params);
> +       if (!index_to_params(index, &params))
> +               return -ENOENT;
> +
>         r = find_reg(&params, invariant_cp15, ARRAY_SIZE(invariant_cp15));
>         if (!r)
>                 return -ENOENT;
>
> -       *val = r->val;
> -       return 0;
> +       return reg_to_user(uaddr, &r->val, index);
>  }
>
> -static int set_invariant_cp15(u32 index, u64 val)
> +static int set_invariant_cp15(u64 index, void __user *uaddr)
>  {
>         struct coproc_params params;
>         const struct coproc_reg *r;
> +       int err;
> +       u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
>
> -       index_to_params(index, &params);
> +       if (!index_to_params(index, &params))
> +               return -ENOENT;
>         r = find_reg(&params, invariant_cp15, ARRAY_SIZE(invariant_cp15));
>         if (!r)
>                 return -ENOENT;
>
> +       err = reg_from_user(&val, uaddr, index);
> +       if (err)
> +               return err;
> +
>         /* This is what we mean by invariant: you can't change it. */
>         if (r->val != val)
>                 return -EINVAL;
> @@ -720,95 +756,36 @@ static int set_invariant_cp15(u32 index, u64 val)
>         return 0;
>  }
>
> -static int get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *val)
> +int kvm_arch_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;
>
> -       r = index_to_coproc_reg(vcpu, index);
> -       if (!r)
> -               return get_invariant_cp15(index, val);
> -
> -       *val = vcpu->arch.cp15[r->reg];
> -       if (r->is_64)
> -               *val |= ((u64)vcpu->arch.cp15[r->reg+1]) << 32;
> -       return 0;
> -}
> -
> -static int set_msr(struct kvm_vcpu *vcpu, u32 index, u64 val)
> -{
> -       const struct coproc_reg *r;
> +       if ((reg->id & KVM_REG_ARCH_MASK) != KVM_REG_ARM)
> +               return -EINVAL;
>
> -       r = index_to_coproc_reg(vcpu, index);
> +       r = index_to_coproc_reg(vcpu, reg->id);
>         if (!r)
> -               return set_invariant_cp15(index, val);
> +               return get_invariant_cp15(reg->id, uaddr);
>
> -       vcpu->arch.cp15[r->reg] = val;
> -       if (r->is_64)
> -               vcpu->arch.cp15[r->reg+1] = (val >> 32);
> -       return 0;
> -}
> -
> -/* Return user adddress to get/set value from. */
> -static u64 __user *get_umsr(struct kvm_msr_entry __user *uentry, u32 *idx)
> -{
> -       struct kvm_msr_entry entry;
> -
> -       if (copy_from_user(&entry, uentry, sizeof(entry)))
> -               return NULL;
> -       *idx = entry.index;
> -       return &uentry->data;
> +       /* Note: copies two regs if size is 64 bit. */
> +       return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
>  }
>
> -/**
> - * kvm_arm_get_msrs - copy one or more special registers to userspace.
> - * @vcpu: the vcpu
> - * @entries: the array of entries
> - * @num: the number of entries
> - */
> -int kvm_arm_get_msrs(struct kvm_vcpu *vcpu,
> -                    struct kvm_msr_entry __user *entries, u32 num)
> +int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
> -       u32 i, index;
> -       u64 val;
> -       u64 __user *uval;
> -       int ret;
> +       const struct coproc_reg *r;
> +       void __user *uaddr = (void __user *)(long)reg->addr;
>
> -       for (i = 0; i < num; i++) {
> -               uval = get_umsr(&entries[i], &index);
> -               if (!uval)
> -                       return -EFAULT;
> -               if ((ret = get_msr(vcpu, index, &val)) != 0)
> -                       return ret;
> -               if (put_user(val, uval))
> -                       return -EFAULT;
> -       }
> -       return 0;
> -}
> +       if ((reg->id & KVM_REG_ARCH_MASK) != KVM_REG_ARM)
> +               return -EINVAL;
>
> -/**
> - * kvm_arm_set_msrs - copy one or more special registers from userspace.
> - * @vcpu: the vcpu
> - * @entries: the array of entries
> - * @num: the number of entries
> - */
> -int kvm_arm_set_msrs(struct kvm_vcpu *vcpu,
> -                    struct kvm_msr_entry __user *entries, u32 num)
> -{
> -       u32 i, index;
> -       u64 val;
> -       u64 __user *uval;
> -       int ret;
> +       r = index_to_coproc_reg(vcpu, reg->id);
> +       if (!r)
> +               return set_invariant_cp15(reg->id, uaddr);
>
> -       for (i = 0; i < num; i++) {
> -               uval = get_umsr(&entries[i], &index);
> -               if (!uval)
> -                       return -EFAULT;
> -               if (copy_from_user(&val, uval, sizeof(val)) != 0)
> -                       return -EFAULT;
> -               if ((ret = set_msr(vcpu, index, val)) != 0)
> -                       return ret;
> -       }
> -       return 0;
> +       /* Note: copies two regs if size is 64 bit */
> +       return copy_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
>  }
>
>  static int cmp_reg(const struct coproc_reg *i1, const struct coproc_reg *i2)
> @@ -827,29 +804,24 @@ static int cmp_reg(const struct coproc_reg *i1, const struct coproc_reg *i2)
>         return i1->Op2 - i2->Op2;
>  }
>
> -/* Puts in the position indicated by mask (assumes val fits in mask) */
> -static inline u32 set_bits(u32 val, u32 mask)
> -{
> -       return val << (ffs(mask)-1);
> -}
> -
> -static u32 cp15_to_index(const struct coproc_reg *reg)
> +static u64 cp15_to_index(const struct coproc_reg *reg)
>  {
> -       u32 val = set_bits(15, KVM_ARM_MSR_COPROC_MASK);
> +       u64 val = (15 << KVM_REG_ARM_COPROC_START);
>         if (reg->is_64) {
> -               val |= set_bits(1, KVM_ARM_MSR_64_BIT_MASK);
> -               val |= set_bits(reg->Op1, KVM_ARM_MSR_64_OPC1_MASK);
> -               val |= set_bits(reg->CRm, KVM_ARM_MSR_64_CRM_MASK);
> +               val |= KVM_REG_SIZE_U64;
> +               val |= (reg->Op1 << KVM_REG_ARM_OPC1_START);
> +               val |= (reg->CRm << KVM_REG_ARM_CRM_START);
>         } else {
> -               val |= set_bits(reg->Op1, KVM_ARM_MSR_32_OPC1_MASK);
> -               val |= set_bits(reg->Op2, KVM_ARM_MSR_32_OPC2_MASK);
> -               val |= set_bits(reg->CRm, KVM_ARM_MSR_32_CRM_MASK);
> -               val |= set_bits(reg->CRn, KVM_ARM_MSR_32_CRN_MASK);
> +               val |= KVM_REG_SIZE_U32;
> +               val |= (reg->Op1 << KVM_REG_ARM_OPC1_START);
> +               val |= (reg->Op2 << KVM_REG_ARM_32_OPC2_START);
> +               val |= (reg->CRm << KVM_REG_ARM_CRM_START);
> +               val |= (reg->CRn << KVM_REG_ARM_32_CRN_START);
>         }
>         return val;
>  }
>
> -static bool copy_reg_to_user(const struct coproc_reg *reg, u32 __user **uind)
> +static bool copy_reg_to_user(const struct coproc_reg *reg, u64 __user **uind)
>  {
>         if (!*uind)
>                 return true;
> @@ -862,7 +834,7 @@ static bool copy_reg_to_user(const struct coproc_reg *reg, u32 __user **uind)
>  }
>
>  /* Assumed ordered tables, see kvm_coproc_table_init. */
> -static int walk_msrs(struct kvm_vcpu *vcpu, u32 __user *uind)
> +static int walk_msrs(struct kvm_vcpu *vcpu, u64 __user *uind)
>  {
>         const struct coproc_reg *i1, *i2, *end1, *end2;
>         unsigned int total = 0;
> @@ -911,7 +883,7 @@ static int walk_msrs(struct kvm_vcpu *vcpu, u32 __user *uind)
>   */
>  unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
>  {
> -       return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u32 __user *)NULL);
> +       return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u64 __user *)NULL);
>  }
>
>  /**
> @@ -919,7 +891,7 @@ unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
>   *
>   * This is for special registers, particularly cp15.
>   */
> -int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u32 __user *uindices)
> +int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  {
>         unsigned int i;
>         int err;

why do we still call this stuff MSR? Shouldn't we move away from that
term to something more generic?

-Christoffer

[RFC 3/5] KVM: Add KVM_VCPU_GET_REG_LIST.

[Rusty Russell]

This is a generic interface to find out what you can use
KVM_GET_ONE_REG/KVM_SET_ONE_REG on.  Arhs need to define
KVM_HAVE_REG_LIST and then kvm_arch_num_regs() and
kvm_arch_copy_reg_indices() functions.

It's inspired by KVM_GET_MSR_INDEX_LIST, except it's a per-vcpu ioctl,
and uses 64-bit indices.

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

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 209b432..b607f60 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -906,7 +906,7 @@ struct kvm_s390_ucas_mapping {
 #define KVM_KVMCLOCK_CTRL	  _IO(KVMIO,   0xad)
 #define KVM_ARM_VCPU_INIT	  _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
 #define KVM_VCPU_GET_MSR_INDEX_LIST    _IOWR(KVMIO, 0xaf, struct kvm_msr_list)
+#define KVM_VCPU_GET_REG_LIST	  _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
@@ -958,4 +958,9 @@ struct kvm_assigned_msix_entry {
 	__u16 padding[3];
 };
 
+/* For KVM_VCPU_GET_REG_LIST. */
+struct kvm_reg_list {
+	__u64 n; /* number of regs */
+	__u64 reg[0];
+};
 #endif /* __LINUX_KVM_H */
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f3d43c3..daa3838 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -572,7 +572,10 @@ int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 #define KVM_REG_LEN(index)						\
 	(1U << (((index) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
 #endif
-
+#ifdef KVM_HAVE_REG_LIST
+unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu);
+int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
+#endif
 void kvm_free_physmem(struct kvm *kvm);
 
 void *kvm_kvzalloc(unsigned long size);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ad67cf4..b220c74 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1949,6 +1949,23 @@ out_free2:
 		break;
 	}
 #endif
+#ifdef KVM_HAVE_REG_LIST
+	case KVM_VCPU_GET_REG_LIST: {
+		struct kvm_reg_list __user *user_list = argp;
+		struct kvm_reg_list reg_list;
+		unsigned n;
+
+		if (copy_from_user(&reg_list, user_list, sizeof reg_list))
+			return -EFAULT;
+		n = reg_list.n;
+		reg_list.n = kvm_arch_num_regs(vcpu);
+		if (copy_to_user(user_list, &reg_list, sizeof reg_list))
+			return -EFAULT;
+		if (n < reg_list.n)
+			return -E2BIG;
+		return kvm_arch_copy_reg_indices(vcpu, user_list->reg);
+	}
+#endif
 
 	default:
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);

Re: [RFC 3/5] KVM: Add KVM_VCPU_GET_REG_LIST.

[Christoffer Dall]

On Tue, Aug 28, 2012 at 4:47 PM, Rusty Russell <rusty.russell@linaro.org> wrote:
> This is a generic interface to find out what you can use
> KVM_GET_ONE_REG/KVM_SET_ONE_REG on.  Arhs need to define
> KVM_HAVE_REG_LIST and then kvm_arch_num_regs() and
> kvm_arch_copy_reg_indices() functions.
>
> It's inspired by KVM_GET_MSR_INDEX_LIST, except it's a per-vcpu ioctl,
> and uses 64-bit indices.
>
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
>
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 209b432..b607f60 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -906,7 +906,7 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_KVMCLOCK_CTRL        _IO(KVMIO,   0xad)
>  #define KVM_ARM_VCPU_INIT        _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
>  #define KVM_VCPU_GET_MSR_INDEX_LIST    _IOWR(KVMIO, 0xaf, struct kvm_msr_list)
> +#define KVM_VCPU_GET_REG_LIST    _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
>
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU    (1 << 0)
>  #define KVM_DEV_ASSIGN_PCI_2_3         (1 << 1)
> @@ -958,4 +958,9 @@ struct kvm_assigned_msix_entry {
>         __u16 padding[3];
>  };
>
> +/* For KVM_VCPU_GET_REG_LIST. */
> +struct kvm_reg_list {
> +       __u64 n; /* number of regs */
> +       __u64 reg[0];
> +};
>  #endif /* __LINUX_KVM_H */
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f3d43c3..daa3838 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -572,7 +572,10 @@ int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
>  #define KVM_REG_LEN(index)                                             \
>         (1U << (((index) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
>  #endif
> -
> +#ifdef KVM_HAVE_REG_LIST
> +unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu);
> +int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
> +#endif
>  void kvm_free_physmem(struct kvm *kvm);
>
>  void *kvm_kvzalloc(unsigned long size);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ad67cf4..b220c74 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1949,6 +1949,23 @@ out_free2:
>                 break;
>         }
>  #endif
> +#ifdef KVM_HAVE_REG_LIST
> +       case KVM_VCPU_GET_REG_LIST: {
> +               struct kvm_reg_list __user *user_list = argp;
> +               struct kvm_reg_list reg_list;
> +               unsigned n;
> +
> +               if (copy_from_user(&reg_list, user_list, sizeof reg_list))
> +                       return -EFAULT;
> +               n = reg_list.n;
> +               reg_list.n = kvm_arch_num_regs(vcpu);
> +               if (copy_to_user(user_list, &reg_list, sizeof reg_list))
> +                       return -EFAULT;
> +               if (n < reg_list.n)
> +                       return -E2BIG;
> +               return kvm_arch_copy_reg_indices(vcpu, user_list->reg);
> +       }
> +#endif
>
>         default:
>                 r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);

looks fine, but don't you need to update the API documentation?

-Christoffer

[RFC 4/5] KVM: ARM: Use KVM_VCPU_GET_REG_LIST.

[Rusty Russell]

This trivially replaces the arm-specific KVM_VCPU_GET_MSR_INDEX_LIST.

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

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 209b432..b607f60 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -906,6 +906,5 @@ struct kvm_s390_ucas_mapping {
 #define KVM_KVMCLOCK_CTRL	  _IO(KVMIO,   0xad)
 #define KVM_ARM_VCPU_INIT	  _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
-#define KVM_VCPU_GET_MSR_INDEX_LIST    _IOWR(KVMIO, 0xaf, struct kvm_msr_list)
 #define KVM_VCPU_GET_REG_LIST	  _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
index 9838456..a3daa67 100644
--- a/arch/arm/include/asm/kvm.h
+++ b/arch/arm/include/asm/kvm.h
@@ -85,12 +85,6 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
-/* for KVM_VCPU_GET_MSR_INDEX_LIST */
-struct kvm_msr_list {
-	__u64 nmsrs; /* number of msrs in entries */
-	__u64 indices[0];
-};
-
 /* If you need to interpret the index values, here are the bit offsets. */
 #define KVM_REG_ARM_COPROC_START	16	/* Mask: 0xFFFF0000 */
 #define KVM_REG_ARM_32_OPC2_START	0	/* Mask: 0x00000007 */
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 7548c95..64fa65f 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -26,6 +26,7 @@
 #define KVM_PRIVATE_MEM_SLOTS 4
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 #define KVM_HAVE_ONE_REG
+#define KVM_HAVE_REG_LIST
 
 #define NUM_FEATURES 0
 
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 55a2995..fe98b77 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -711,21 +711,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		return kvm_vcpu_set_target(vcpu, &init);
 
 	}
-	case KVM_VCPU_GET_MSR_INDEX_LIST: {
-		struct kvm_msr_list __user *user_msr_list = argp;
-		struct kvm_msr_list msr_list;
-		unsigned n;
-
-		if (copy_from_user(&msr_list, user_msr_list, sizeof msr_list))
-			return -EFAULT;
-		n = msr_list.nmsrs;
-		msr_list.nmsrs = kvm_arm_num_guest_msrs(vcpu);
-		if (copy_to_user(user_msr_list, &msr_list, sizeof msr_list))
-			return -EFAULT;
-		if (n < msr_list.nmsrs)
-			return -E2BIG;
-		return kvm_arm_copy_msrindices(vcpu, user_msr_list->indices);
-	}
 	default:
 		return -EINVAL;
 	}
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 99de71b..0b9e8b4 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -877,21 +877,21 @@ static int walk_msrs(struct kvm_vcpu *vcpu, u64 __user *uind)
 }
 
 /**
- * kvm_arm_num_guest_msrs - how many registers do we present via KVM_GET_MSR
+ * 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_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
+unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu)
 {
 	return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u64 __user *)NULL);
 }
 
 /**
- * kvm_arm_copy_msrindices - copy a series of coprocessor registers.
+ * kvm_arch_copy_reg_indices - copy a series of coprocessor registers.
  *
  * This is for special registers, particularly cp15.
  */
-int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
 	unsigned int i;
 	int err;

Re: [RFC 4/5] KVM: ARM: Use KVM_VCPU_GET_REG_LIST.

[Christoffer Dall]

On Tue, Aug 28, 2012 at 4:47 PM, Rusty Russell <rusty.russell@linaro.org> wrote:
> This trivially replaces the arm-specific KVM_VCPU_GET_MSR_INDEX_LIST.
>
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
>
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 209b432..b607f60 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -906,6 +906,5 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_KVMCLOCK_CTRL        _IO(KVMIO,   0xad)
>  #define KVM_ARM_VCPU_INIT        _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
> -#define KVM_VCPU_GET_MSR_INDEX_LIST    _IOWR(KVMIO, 0xaf, struct kvm_msr_list)
>  #define KVM_VCPU_GET_REG_LIST    _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
>
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU    (1 << 0)
> diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
> index 9838456..a3daa67 100644
> --- a/arch/arm/include/asm/kvm.h
> +++ b/arch/arm/include/asm/kvm.h
> @@ -85,12 +85,6 @@ struct kvm_sync_regs {
>  struct kvm_arch_memory_slot {
>  };
>
> -/* for KVM_VCPU_GET_MSR_INDEX_LIST */
> -struct kvm_msr_list {
> -       __u64 nmsrs; /* number of msrs in entries */
> -       __u64 indices[0];
> -};
> -
>  /* If you need to interpret the index values, here are the bit offsets. */
>  #define KVM_REG_ARM_COPROC_START       16      /* Mask: 0xFFFF0000 */
>  #define KVM_REG_ARM_32_OPC2_START      0       /* Mask: 0x00000007 */
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 7548c95..64fa65f 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -26,6 +26,7 @@
>  #define KVM_PRIVATE_MEM_SLOTS 4
>  #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
>  #define KVM_HAVE_ONE_REG
> +#define KVM_HAVE_REG_LIST
>
>  #define NUM_FEATURES 0
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 55a2995..fe98b77 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -711,21 +711,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>                 return kvm_vcpu_set_target(vcpu, &init);
>
>         }
> -       case KVM_VCPU_GET_MSR_INDEX_LIST: {
> -               struct kvm_msr_list __user *user_msr_list = argp;
> -               struct kvm_msr_list msr_list;
> -               unsigned n;
> -
> -               if (copy_from_user(&msr_list, user_msr_list, sizeof msr_list))
> -                       return -EFAULT;
> -               n = msr_list.nmsrs;
> -               msr_list.nmsrs = kvm_arm_num_guest_msrs(vcpu);
> -               if (copy_to_user(user_msr_list, &msr_list, sizeof msr_list))
> -                       return -EFAULT;
> -               if (n < msr_list.nmsrs)
> -                       return -E2BIG;
> -               return kvm_arm_copy_msrindices(vcpu, user_msr_list->indices);
> -       }
>         default:
>                 return -EINVAL;
>         }
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 99de71b..0b9e8b4 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -877,21 +877,21 @@ static int walk_msrs(struct kvm_vcpu *vcpu, u64 __user *uind)
>  }
>
>  /**
> - * kvm_arm_num_guest_msrs - how many registers do we present via KVM_GET_MSR
> + * 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_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
> +unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu)
>  {
>         return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u64 __user *)NULL);
>  }
>
>  /**
> - * kvm_arm_copy_msrindices - copy a series of coprocessor registers.
> + * kvm_arch_copy_reg_indices - copy a series of coprocessor registers.
>   *
>   * This is for special registers, particularly cp15.
>   */
> -int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> +int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  {
>         unsigned int i;
>         int err;

yeah ok, forget my comment in the other mail.

-Christoffer

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

[Rusty Russell]

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.

Peter?

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..1f7190b 100644
--- a/arch/arm/include/asm/kvm.h
+++ b/arch/arm/include/asm/kvm.h
@@ -96,4 +96,28 @@ 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 0. */
+#define KVM_REG_ARM_CORE		(0x0000 << KVM_REG_ARM_COPROC_START)
+#define KVM_REG_ARM_CORE_REG_START	0	/* Mask: 0x000000FF */
+#define KVM_REG_ARM_CORE_REG_LEN	8
+#define KVM_REG_ARM_CORE_MODE_START	8	/* Mask: 0x00002F00 */
+#define KVM_REG_ARM_CORE_MODE_LEN	6
+
+/* Registers r0-r15 are just 0 to 15 as expected. */
+#define KVM_REG_ARM_CORE_REG_PC		0x0000000F
+#define KVM_REG_ARM_CORE_REG_SPSR	0x00000010
+#define KVM_REG_ARM_CORE_REG_CPSR	0x00000011
+
+/* Banked registers appear in multiple modes. */
+#define KVM_REG_ARM_CORE_MODE_USR	0x00001000
+#define KVM_REG_ARM_CORE_MODE_FIQ	0x00001100
+#define KVM_REG_ARM_CORE_MODE_IRQ	0x00001200
+#define KVM_REG_ARM_CORE_MODE_SVC	0x00001300
+#define KVM_REG_ARM_CORE_MODE_MON	0x00001600
+#define KVM_REG_ARM_CORE_MODE_ABT	0x00001700
+#define KVM_REG_ARM_CORE_MODE_HYP	0x00001A00
+#define KVM_REG_ARM_CORE_MODE_UND	0x00001B00
+#define KVM_REG_ARM_CORE_MODE_SYS	0x00001F00
+#define KVM_REG_ARM_CORE_MODE_UNBANKED	0x00002000
+
 #endif /* __ARM_KVM_H__ */
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 64fa65f..340fada 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -192,4 +192,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/guest.c b/arch/arm/kvm/guest.c
index 53f72a0..a9a7d72 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -38,75 +38,256 @@ 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)
 {
+	u32 __user *uaddr = (u32 __user *)(long)reg->addr;
 	struct kvm_vcpu_regs *vcpu_regs = &vcpu->arch.regs;
+	u32 mode, r;
 
-	/*
-	 * 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;
+	if (KVM_REG_LEN(reg->id) != 4)
+		return -ENOENT;
 
-	return 0;
+	/* This var deliberately includes the unused bits. */
+	mode = reg->id >> KVM_REG_ARM_CORE_MODE_START;
+	r = (reg->id >> KVM_REG_ARM_CORE_REG_START)
+		& ((1 << KVM_REG_ARM_CORE_REG_LEN) - 1);
+
+	switch (mode) {
+	case KVM_REG_ARM_CORE_MODE_UNBANKED:
+		if (r < 8)
+			return put_user(vcpu_regs->usr_regs[r], uaddr);
+		else if (r == KVM_REG_ARM_CORE_REG_PC)
+			return put_user(vcpu_regs->pc, uaddr);
+		else if (r == KVM_REG_ARM_CORE_REG_CPSR)
+			return put_user(vcpu_regs->cpsr, uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_USR:
+		if (r >= 8 && r <= 14)
+			return put_user(vcpu_regs->usr_regs[r], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_FIQ:
+		if (r >= 8 && r <= 14)
+			return put_user(vcpu_regs->fiq_regs[r - 8], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return put_user(vcpu_regs->fiq_regs[7], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_IRQ:
+		if (r == 13 || r == 14)
+			return put_user(vcpu_regs->irq_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return put_user(vcpu_regs->irq_regs[2], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_SVC:
+		if (r == 13 || r == 14)
+			return put_user(vcpu_regs->svc_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return put_user(vcpu_regs->svc_regs[2], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_ABT:
+		if (r == 13 || r == 14)
+			return put_user(vcpu_regs->abt_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return put_user(vcpu_regs->abt_regs[2], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_UND:
+		if (r == 13 || r == 14)
+			return put_user(vcpu_regs->und_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return put_user(vcpu_regs->und_regs[2], uaddr);
+		break;
+	}
+
+	return -ENOENT;
 }
 
-int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
+static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
+	const u32 __user *uaddr = (u32 __user *)(long)reg->addr;
 	struct kvm_vcpu_regs *vcpu_regs = &vcpu->arch.regs;
+	u32 mode, r;
+
+	if (KVM_REG_LEN(reg->id) != 4)
+		return -ENOENT;
+
+	mode = reg->id >> KVM_REG_ARM_CORE_MODE_START;
+	r = (reg->id >> KVM_REG_ARM_CORE_REG_START)
+		& ((1 << KVM_REG_ARM_CORE_REG_LEN) - 1);
+
+	switch (mode) {
+	case KVM_REG_ARM_CORE_MODE_UNBANKED:
+		if (r < 8)
+			return get_user(vcpu_regs->usr_regs[r], uaddr);
+		else if (r == KVM_REG_ARM_CORE_REG_PC)
+			return get_user(vcpu_regs->pc, uaddr);
+		else if (r == KVM_REG_ARM_CORE_REG_CPSR) {
+			u32 cpsr;
+			if (get_user(cpsr, uaddr))
+				return -EFAULT;
+
+			if (__vcpu_mode(cpsr) == 0xf)
+				return -EINVAL;
+			vcpu_regs->cpsr = cpsr;
+			return 0;
+		}
+		break;
+	case KVM_REG_ARM_CORE_MODE_USR:
+		if (r >= 8 && r <= 14)
+			return get_user(vcpu_regs->usr_regs[r], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_FIQ:
+		if (r >= 8 && r <= 14)
+			return get_user(vcpu_regs->fiq_regs[r - 8], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return get_user(vcpu_regs->fiq_regs[7], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_IRQ:
+		if (r == 13 || r == 14)
+			return get_user(vcpu_regs->irq_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return get_user(vcpu_regs->irq_regs[2], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_SVC:
+		if (r == 13 || r == 14)
+			return get_user(vcpu_regs->svc_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return get_user(vcpu_regs->svc_regs[2], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_ABT:
+		if (r == 13 || r == 14)
+			return get_user(vcpu_regs->abt_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return get_user(vcpu_regs->abt_regs[2], uaddr);
+		break;
+	case KVM_REG_ARM_CORE_MODE_UND:
+		if (r == 13 || r == 14)
+			return get_user(vcpu_regs->und_regs[r - 13], uaddr);
+		if (r == KVM_REG_ARM_CORE_REG_SPSR)
+			return get_user(vcpu_regs->und_regs[2], uaddr);
+		break;
+	}
+
+	return -ENOENT;
+}
+
+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)
+{
+	return -EINVAL;
+}
+
+static unsigned long num_core_regs(void)
+{
+	return sizeof(struct kvm_vcpu_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;
+	u64 __user *start = uindices;
+	u64 mode;
+	const int banked_modes[] = { KVM_REG_ARM_CORE_MODE_USR,
+				     KVM_REG_ARM_CORE_MODE_SVC,
+				     KVM_REG_ARM_CORE_MODE_ABT,
+				     KVM_REG_ARM_CORE_MODE_UND,
+				     KVM_REG_ARM_CORE_MODE_IRQ,
+				     KVM_REG_ARM_CORE_MODE_FIQ,
+	};
+
+	/* R0 - R7 (unbanked) */
+	mode = KVM_REG_ARM|KVM_REG_ARM_CORE_MODE_UNBANKED;
+	for (i = 0; i <= 7; i++) {
+		if (put_user(mode|i, uindices))
+			return -EFAULT;
+		uindices++;
+	}
+	/* PC and CPSR (unbanked) */
+	if (put_user(mode|KVM_REG_ARM_CORE_REG_PC, uindices))
+		return -EFAULT;
+	uindices++;
+	if (put_user(mode|KVM_REG_ARM_CORE_REG_CPSR, uindices))
+		return -EFAULT;
+	uindices++;
+
+	/* USR and FIQ have banked R8-R12. */
+	mode = KVM_REG_ARM|KVM_REG_ARM_CORE_MODE_USR;
+	for (i = 8; i <= 12; i++) {
+		if (put_user(mode|i, uindices))
+			return -EFAULT;
+		uindices++;
+	}
+	mode = KVM_REG_ARM|KVM_REG_ARM_CORE_MODE_FIQ;
+	for (i = 8; i <= 12; i++) {
+		if (put_user(mode|i, uindices))
+			return -EFAULT;
+		uindices++;
+	}
 
-	if (__vcpu_mode(regs->cpsr) == 0xf)
+	/* Banked SP, LR and SPSR for each mode (no SPSR for USR mode though) */
+	for (i = 0; i < ARRAY_SIZE(banked_modes); i++) {
+		mode = KVM_REG_ARM|banked_modes[i];
+		if (put_user(mode|13, uindices))
+			return -EFAULT;
+		uindices++;
+		if (put_user(mode|14, uindices))
+			return -EFAULT;
+		uindices++;
+		if (banked_modes[i] != KVM_REG_ARM_CORE_MODE_USR) {
+			if (put_user(mode|KVM_REG_ARM_CORE_REG_SPSR, uindices))
+				return -EFAULT;
+			uindices++;
+		}
+	}
+
+	BUG_ON(start - uindices != num_core_regs());
+
+	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;
 
-	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 get_core_reg(vcpu, reg);
 
-	return 0;
+	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;
+
+	/* 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 kvm_arm_coproc_set_reg(vcpu, reg);
 }
 
 int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,

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

[Christoffer Dall]

On Tue, Aug 28, 2012 at 4:48 PM, Rusty Russell <rusty.russell@linaro.org> wrote:
> No structures at all any more.
>

I fail to see the great benefit of all this.  The code is certainly
not easier to read and it's certainly not more clear what is going on.

Is this simply so we don't have to copy header files into QEMU when
QEMU needs to support a new architecture? We have to do that anyway
no? Do core registers really often change and often we need new
registers for an existing architecture?

I can see this for cp15 stuff, but core registers?

-Christoffer

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

[Avi Kivity]

On 08/29/2012 08:29 AM, Christoffer Dall wrote:
> On Tue, Aug 28, 2012 at 4:48 PM, Rusty Russell <rusty.russell@linaro.org> wrote:
> > No structures at all any more.
> >
>
> I fail to see the great benefit of all this.  The code is certainly
> not easier to read and it's certainly not more clear what is going on.
>
> Is this simply so we don't have to copy header files into QEMU when
> QEMU needs to support a new architecture? We have to do that anyway
> no? Do core registers really often change and often we need new
> registers for an existing architecture?
>
> I can see this for cp15 stuff, but core registers?
>

The nice thing about it is that the hardware vendors can keep adding
stuff, and we don't need new ioctls.  Just new encodings for register
numbers.  x86 needed 6-7 updates (some due to our missing some hidden
state).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Peter Maydell]

On 29 August 2012 00:48, Rusty Russell <rusty.russell@linaro.org> wrote:
> No structures at all any more.

I'm not fussed whether we use structs for the core regs or
not; they're not exactly going to change in future so it's
purely a question of whether you think it's aesthetically
prettier to have everything funneled through the one ioctl.

> +       /* Coprocessor 0 means we want a core register. */
> +       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
> +               return set_core_reg(vcpu, reg);

...but if we do go this path, you can't use coprocessor 0
to mean core register -- cp0 could be a valid coprocessor
(the ARM ARM reserves cp0..cp7 for "vendor specific features").
Use something outside 0..15.

-- PMM

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

[Rusty Russell]

Peter Maydell <peter.maydell@linaro.org> writes:
> On 29 August 2012 00:48, Rusty Russell <rusty.russell@linaro.org> wrote:
>> No structures at all any more.
>
> I'm not fussed whether we use structs for the core regs or
> not; they're not exactly going to change in future so it's
> purely a question of whether you think it's aesthetically
> prettier to have everything funneled through the one ioctl.

Avi wanted to get rid of the structures.

But I thought "it will never change" was already proven untrue, for your
psuedo-secure mode proposal?

>> +       /* Coprocessor 0 means we want a core register. */
>> +       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
>> +               return set_core_reg(vcpu, reg);
>
> ...but if we do go this path, you can't use coprocessor 0
> to mean core register -- cp0 could be a valid coprocessor
> (the ARM ARM reserves cp0..cp7 for "vendor specific features").
> Use something outside 0..15.

OK, changed that too (16).

Thanks,
Rusty.

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

[Avi Kivity]

On 08/29/2012 11:21 AM, Rusty Russell wrote:
> >> +       /* Coprocessor 0 means we want a core register. */
> >> +       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
> >> +               return set_core_reg(vcpu, reg);
> >
> > ...but if we do go this path, you can't use coprocessor 0
> > to mean core register -- cp0 could be a valid coprocessor
> > (the ARM ARM reserves cp0..cp7 for "vendor specific features").
> > Use something outside 0..15.
>
> OK, changed that too (16).
>
>

And tomorrow they will add 16.

Have the first byte say where the state belongs (core or coprocessor)
second byte refer to the register family (if applicable), the rest
identifies the register within that family.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Peter Maydell]

On 1 September 2012 10:16, Avi Kivity <avi@redhat.com> wrote:
> On 08/29/2012 11:21 AM, Rusty Russell wrote:
>> >> +       /* Coprocessor 0 means we want a core register. */
>> >> +       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
>> >> +               return set_core_reg(vcpu, reg);
>> >
>> > ...but if we do go this path, you can't use coprocessor 0
>> > to mean core register -- cp0 could be a valid coprocessor
>> > (the ARM ARM reserves cp0..cp7 for "vendor specific features").
>> > Use something outside 0..15.
>>
>> OK, changed that too (16).

> And tomorrow they will add 16.

Not possible in the instruction encoding :-) We haven't used
anywhere near all the coprocessors (even given we've let the
vendors have 0..7, ARM itself uses only 10 and 11 for the FPU,
14 for debug/perf and 15 for system control (and 14 and 15 still
have lots of spare space).

-- PMM

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

[Christoffer Dall]

On Sep 1, 2012, at 6:25 AM, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 1 September 2012 10:16, Avi Kivity <avi@redhat.com> wrote:
>> On 08/29/2012 11:21 AM, Rusty Russell wrote:
>>>>> +       /* Coprocessor 0 means we want a core register. */
>>>>> +       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
>>>>> +               return set_core_reg(vcpu, reg);
>>>> 
>>>> ...but if we do go this path, you can't use coprocessor 0
>>>> to mean core register -- cp0 could be a valid coprocessor
>>>> (the ARM ARM reserves cp0..cp7 for "vendor specific features").
>>>> Use something outside 0..15.
>>> 
>>> OK, changed that too (16).
> 
>> And tomorrow they will add 16.
> 
> Not possible in the instruction encoding :-) We haven't used
> anywhere near all the coprocessors (even given we've let the
> vendors have 0..7, ARM itself uses only 10 and 11 for the FPU,
> 14 for debug/perf and 15 for system control (and 14 and 15 still
> have lots of spare space).
> 

Yeah, but folding core registers under coprocessors feels just too fishy, so I think we should have a separate field. 

-Christoffer

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

[Peter Maydell]

On 1 September 2012 20:40, Christoffer Dall
<c.dall@virtualopensystems.com> wrote:
> On Sep 1, 2012, at 6:25 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 September 2012 10:16, Avi Kivity <avi@redhat.com> wrote:
>>> On 08/29/2012 11:21 AM, Rusty Russell wrote:
>>>> Peter Maydell wrote:
>>>>> ...but if we do go this path, you can't use coprocessor 0
>>>>> to mean core register -- cp0 could be a valid coprocessor
>>>>> (the ARM ARM reserves cp0..cp7 for "vendor specific features").
>>>>> Use something outside 0..15.
>>>>
>>>> OK, changed that too (16).
>>
>>> And tomorrow they will add 16.
>>
>> Not possible in the instruction encoding :-) We haven't used
>> anywhere near all the coprocessors (even given we've let the
>> vendors have 0..7, ARM itself uses only 10 and 11 for the FPU,
>> 14 for debug/perf and 15 for system control (and 14 and 15 still
>> have lots of spare space).

> Yeah, but folding core registers under coprocessors feels just
> too fishy, so I think we should have a separate field.

I never really thought of the top half of the index encoding
as being particularly a coprocessor-number specific thing in
the first place. It's just 16 bits of "what is this thing
anyway?", where each coprocessor gets a bit of the space, and
so will the GIC, and the VFP regs, and so on. We just happened
to use 0..15 of the "what is this?" space for cp0..cp15.

(Incidentally, the term "coprocessor" is now basically just a
historical artefact. The bits of the CPU you get at via the
"coprocessor" registers and instruction encoding space are
not separate functional units, they're part of the core.)

-- PMM

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

[Christoffer Dall]

On Tue, Sep 4, 2012 at 9:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 September 2012 20:40, Christoffer Dall
> <c.dall@virtualopensystems.com> wrote:
>> On Sep 1, 2012, at 6:25 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 1 September 2012 10:16, Avi Kivity <avi@redhat.com> wrote:
>>>> On 08/29/2012 11:21 AM, Rusty Russell wrote:
>>>>> Peter Maydell wrote:
>>>>>> ...but if we do go this path, you can't use coprocessor 0
>>>>>> to mean core register -- cp0 could be a valid coprocessor
>>>>>> (the ARM ARM reserves cp0..cp7 for "vendor specific features").
>>>>>> Use something outside 0..15.
>>>>>
>>>>> OK, changed that too (16).
>>>
>>>> And tomorrow they will add 16.
>>>
>>> Not possible in the instruction encoding :-) We haven't used
>>> anywhere near all the coprocessors (even given we've let the
>>> vendors have 0..7, ARM itself uses only 10 and 11 for the FPU,
>>> 14 for debug/perf and 15 for system control (and 14 and 15 still
>>> have lots of spare space).
>
>> Yeah, but folding core registers under coprocessors feels just
>> too fishy, so I think we should have a separate field.
>
> I never really thought of the top half of the index encoding
> as being particularly a coprocessor-number specific thing in
> the first place. It's just 16 bits of "what is this thing
> anyway?", where each coprocessor gets a bit of the space, and
> so will the GIC, and the VFP regs, and so on. We just happened
> to use 0..15 of the "what is this?" space for cp0..cp15.
>
> (Incidentally, the term "coprocessor" is now basically just a
> historical artefact. The bits of the CPU you get at via the
> "coprocessor" registers and instruction encoding space are
> not separate functional units, they're part of the core.)
>
that's fine, but then the #define's shouldn't be called something with
COPROC in their names.

-Christoffer

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

[Rusty Russell]

Christoffer Dall <c.dall@virtualopensystems.com> writes:
> that's fine, but then the #define's shouldn't be called something with
> COPROC in their names.

Sure, feel free to rename it.  I failed to come up with a concise, clear
name, so coproc stuck.

Cheers,
Rusty.

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

[Rusty Russell]

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:

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Peter Maydell]

On 29 August 2012 00:37, Rusty Russell <rusty.russell@linaro.org> wrote:
>         This compiles, completely untested, but it's my attempt to give
> Avi (and Alexander) what he asked for in a generic register accessor.
>
> Mingled in these patches is the conversion of the latest KVM ARM code,
> which is the first proposed user: by the end, we use these accessors for
> *every* register and piece of state.
>
> GET_MULTI/SET_MULTI is an obvious extension which is not yet
> implemented.

Hi Rusty.

I don't see any api.txt patches in here, did I miss them?
(your cover letter doesn't include a diffstat...)

I don't particularly have comments on the implementation but I would
like to see the kernel-userspace ABI clearly described rather than
having to infer it from the code. (including the complete set of
index mappings for the ARM registers that will use this)

thanks
-- PMM

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 29 August 2012 00:37, Rusty Russell <rusty.russell@linaro.org> wrote:
>>         This compiles, completely untested, but it's my attempt to give
>> Avi (and Alexander) what he asked for in a generic register accessor.
>>
>> Mingled in these patches is the conversion of the latest KVM ARM code,
>> which is the first proposed user: by the end, we use these accessors for
>> *every* register and piece of state.
>>
>> GET_MULTI/SET_MULTI is an obvious extension which is not yet
>> implemented.
>
> Hi Rusty.
>
> I don't see any api.txt patches in here, did I miss them?
> (your cover letter doesn't include a diffstat...)
>
> I don't particularly have comments on the implementation but I would
> like to see the kernel-userspace ABI clearly described rather than
> having to infer it from the code. (including the complete set of
> index mappings for the ARM registers that will use this)

It would look something like this:
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 19d8915..c0453d7 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -293,7 +293,7 @@ kvm_run' (see below).
 4.11 KVM_GET_REGS
 
 Capability: basic
-Architectures: all
+Architectures: all except ARM
 Type: vcpu ioctl
 Parameters: struct kvm_regs (out)
 Returns: 0 on success, -1 on error
@@ -314,7 +314,7 @@ struct kvm_regs {
 4.12 KVM_SET_REGS
 
 Capability: basic
-Architectures: all
+Architectures: all except ARM
 Type: vcpu ioctl
 Parameters: struct kvm_regs (in)
 Returns: 0 on success, -1 on error
@@ -1733,6 +1733,17 @@ registers, find a list below:
         |                       |
   PPC   | KVM_REG_PPC_HIOR      | 64
 
+ARM registers are mapped using the lower 32 bits.  The upper 16 of that
+is the coprocessor number (or 16 for core registers):
+
+ARM 32-bit CP15 registers have the following id bit patterns:
+  0x4002 0000 000F <zero:1> <crn:4> <crm:4> <opc1:4> <opc2:3>
+
+ARM 64-bit CP15 registers have the following id bit patterns:
+  0x4003 0000 000F <zero:1> <zero:4> <crm:4> <opc1:4> <zero:3>
+
+ARM core registers have the following id format:
+  0x4003 0000 0010 <offset in struct kvm_regs, divided by 4>
 
 4.69 KVM_GET_ONE_REG
 
@@ -1986,50 +1997,26 @@ the virtualized real-mode area (VRMA) facility, the kernel will
 re-create the VMRA HPTEs on the next KVM_RUN of any vcpu.)
 
 
-4.76 KVM_VCPU_GET_MSR_INDEX_LIST
+4.76 KVM_VCPU_GET_REG_LIST
 
-Capability: basic
+Capability: KVM_CAP_REG_LIST
 Architectures: arm
 Type: vcpu ioctl
-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
 Returns: 0 on success; -1 on error
 Errors:
-  E2BIG:     the msr index list is too big to fit in the array specified by
-             the user.
+  E2BIG:     the reg index list is too big to fit in the array specified by
+             the user (the number required will be written into n).
 
 struct kvm_msr_list {
-	__u32 nmsrs; /* number of msrs in entries */
-	__u32 indices[0];
+	__u64 n; /* number of registers in reg[] */
+	__u64 reg[0];
 };
 
-This ioctl returns the guest special registers that are supported, and
-is only valid after KVM_ARM_VCPU_INIT has been performed to initialize
-the vcpu type and features.  It is otherwise the equivalent of the
-x86-specific KVM_GET_MSR_INDEX_LIST, for arm's coprocessor registers
-and other non-register state.
-
-The numbering for the indices for coprocesors is simple: the upper 16
-bits are the coprocessor number.  If it's > 15, it's something else,
-for future expansion.
-
-Bit 15 indicates a 64-bit register.  For 64 bit registers the bottom 4
-bits are CRm, the next 4 are opc1 (just like the MCRR/MRCC instruction
-encoding).  For 32 bit registers, the bottom 4 bits are CRm, the next
-3 are opc2, the next 4 CRn, and the next 3 opc1 (the same order as the
-MRC/MCR instruction encoding, but not the same bit positions).
-
-64-bit coprocessor register:
-       ...|19 18 17 16|15|14 13 12 11 10  9  8| 7  6  5  4 |3  2  1  0|
-  ...0  0 |  cp num   | 1| 0  0  0  0  0  0  0|   opc1     |   CRm    |
-
-32-bit coprocessor register:
-       ...|19 18 17 16|15|14|13 12 11|10  9  8  7 |6  5  4 |3  2  1  0|
-  ...0  0 |  cp num   | 0| 0|  opc1  |    CRn     | opc2   |   CRm    |
-
-Non-coprocessor register:
-
-   | 32 31 30 29 28 27 26 25 24 23 22 21 20|19 18 17 16 15 ...
-   |     < some non-zero value >           | ...
+This ioctl returns the guest registers that are supported for the
+KVM_GET_ONE_REG/KVM_SET_ONE_REG calls, and is only valid after
+KVM_ARM_VCPU_INIT has been performed to initialize the vcpu type and
+features.
 
 
 4.77 KVM_ARM_VCPU_INIT

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Avi Kivity]

On 08/29/2012 11:39 AM, Rusty Russell wrote:
>  
>  
> -4.76 KVM_VCPU_GET_MSR_INDEX_LIST
> +4.76 KVM_VCPU_GET_REG_LIST



>  
> -Capability: basic
> +Capability: KVM_CAP_REG_LIST
>  Architectures: arm

all

>  Type: vcpu ioctl
> -Parameters: struct kvm_msr_list (in/out)
> +Parameters: struct kvm_reg_list (in/out)
>  Returns: 0 on success; -1 on error
>  Errors:
> -  E2BIG:     the msr index list is too big to fit in the array specified by
> -             the user.
> +  E2BIG:     the reg index list is too big to fit in the array specified by
> +             the user (the number required will be written into n).
>  
>  struct kvm_msr_list {
> -	__u32 nmsrs; /* number of msrs in entries */
> -	__u32 indices[0];
> +	__u64 n; /* number of registers in reg[] */
> +	__u64 reg[0];
>  };
>  

People complain that this interface is hard to use.

How about supplying the address of the array (in addition to n) so you
don't have to deal with variable sized arrays, and dropping E2BIG in
favour of always updating n (n changed to something bigger than you had
-> reallocate and rerun)




-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Avi Kivity <avi@redhat.com> writes:
>> -Capability: basic
>> +Capability: KVM_CAP_REG_LIST
>>  Architectures: arm
>
> all

OK, I guess that's to be true in future.  Fixed.

>>  Type: vcpu ioctl
>> -Parameters: struct kvm_msr_list (in/out)
>> +Parameters: struct kvm_reg_list (in/out)
>>  Returns: 0 on success; -1 on error
>>  Errors:
>> -  E2BIG:     the msr index list is too big to fit in the array specified by
>> -             the user.
>> +  E2BIG:     the reg index list is too big to fit in the array specified by
>> +             the user (the number required will be written into n).
>>  
>>  struct kvm_msr_list {
>> -	__u32 nmsrs; /* number of msrs in entries */
>> -	__u32 indices[0];
>> +	__u64 n; /* number of registers in reg[] */
>> +	__u64 reg[0];
>>  };
>>  
>
> People complain that this interface is hard to use.
>
> How about supplying the address of the array (in addition to n) so you
> don't have to deal with variable sized arrays, and dropping E2BIG in
> favour of always updating n (n changed to something bigger than you had
> -> reallocate and rerun)

We re-write n anyway, *and* return -E2BIG.  Not returning an error is
asking for trouble.

Passing an address in a struct is pretty bad, since it involves
compatibility wrappers.  I don't think that is what makes the API hard
to use.

Cheers,
Rusty.

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Avi Kivity]

On 09/01/2012 03:35 PM, Rusty Russell wrote:
> Avi Kivity <avi@redhat.com> writes:
>>> -Capability: basic
>>> +Capability: KVM_CAP_REG_LIST
>>>  Architectures: arm
>>
>> all
> 
> OK, I guess that's to be true in future.  Fixed.
> 
>>>  Type: vcpu ioctl
>>> -Parameters: struct kvm_msr_list (in/out)
>>> +Parameters: struct kvm_reg_list (in/out)
>>>  Returns: 0 on success; -1 on error
>>>  Errors:
>>> -  E2BIG:     the msr index list is too big to fit in the array specified by
>>> -             the user.
>>> +  E2BIG:     the reg index list is too big to fit in the array specified by
>>> +             the user (the number required will be written into n).
>>>  
>>>  struct kvm_msr_list {
>>> -	__u32 nmsrs; /* number of msrs in entries */
>>> -	__u32 indices[0];
>>> +	__u64 n; /* number of registers in reg[] */
>>> +	__u64 reg[0];
>>>  };
>>>  
>>
>> People complain that this interface is hard to use.
>>
>> How about supplying the address of the array (in addition to n) so you
>> don't have to deal with variable sized arrays, and dropping E2BIG in
>> favour of always updating n (n changed to something bigger than you had
>> -> reallocate and rerun)
> 
> We re-write n anyway, *and* return -E2BIG.  Not returning an error is
> asking for trouble.
> 
> Passing an address in a struct is pretty bad, since it involves
> compatibility wrappers.  

Right, some s390 thing.

> I don't think that is what makes the API hard
> to use.

What is it then?  I forgot what the original complaints/complainers were.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Avi Kivity <avi@redhat.com> writes:
> On 09/01/2012 03:35 PM, Rusty Russell wrote:
>> Passing an address in a struct is pretty bad, since it involves
>> compatibility wrappers.  
>
> Right, some s390 thing.

Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64....

Any time you put a pointer in a structure which is exposed to userspace,
you have to deal with this.

>> I don't think that is what makes the API hard
>> to use.
>
> What is it then?  I forgot what the original complaints/complainers were.

I have no idea, since I didn't hear the complaints.  But any non-fixed
size array has issues in C; there's not much we can do about it.

x86 manages this fine for msrs, and I didn't have a problem using it for
my test programs.  That's the limit of my experience, however.

Cheers,
Rusty.

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Peter Maydell]

On 3 September 2012 13:33, Rusty Russell <rusty@rustcorp.com.au> wrote:
> I have no idea, since I didn't hear the complaints.  But any non-fixed
> size array has issues in C; there's not much we can do about it.
>
> x86 manages this fine for msrs, and I didn't have a problem using it for
> my test programs.  That's the limit of my experience, however.

Yes, I didn't particularly find the ioctls Rusty initially suggested
hard to use (apart from briefly getting confused between struct
kvm_msrs and struct kvm_msr_list; you could collapse those down
into one struct if you wanted I suppose). The nastiest problem
in the target-i386 qemu code that gets the MSR list is the fact
it has to work around some ancient kernel bug where the ioctl
would write beyond the end of the array...

-- PMM

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Avi Kivity]

On 09/03/2012 03:33 PM, Rusty Russell wrote:
> Avi Kivity <avi@redhat.com> writes:
>> On 09/01/2012 03:35 PM, Rusty Russell wrote:
>>> Passing an address in a struct is pretty bad, since it involves
>>> compatibility wrappers.  
>>
>> Right, some s390 thing.
> 
> Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64....
> 
> Any time you put a pointer in a structure which is exposed to userspace,
> you have to deal with this.

Not is you pack the pointer in a __u64, which is what we do to preserve
padding.  Then it is only s390 which needs extra love.

>>> I don't think that is what makes the API hard
>>> to use.
>>
>> What is it then?  I forgot what the original complaints/complainers were.
> 
> I have no idea, since I didn't hear the complaints.  But any non-fixed
> size array has issues in C; there's not much we can do about it.
> 
> x86 manages this fine for msrs, and I didn't have a problem using it for
> my test programs.  That's the limit of my experience, however.

Another option is to use the size parameter from the ioctl.  It just
sits there doing nothing.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Alexander Graf]

On 04.09.2012, at 07:48, Avi Kivity <avi@redhat.com> wrote:

> On 09/03/2012 03:33 PM, Rusty Russell wrote:
>> Avi Kivity <avi@redhat.com> writes:
>>> On 09/01/2012 03:35 PM, Rusty Russell wrote:
>>>> Passing an address in a struct is pretty bad, since it involves
>>>> compatibility wrappers.  
>>> 
>>> Right, some s390 thing.
>> 
>> Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64....
>> 
>> Any time you put a pointer in a structure which is exposed to userspace,
>> you have to deal with this.
> 
> Not is you pack the pointer in a __u64, which is what we do to preserve
> padding.  Then it is only s390 which needs extra love.

I doubt that anyone wants to run 31-bit user space on an s390x system. In fact, I wouldn't be surprised if exactly that case is broken already.

> 
>>>> I don't think that is what makes the API hard
>>>> to use.
>>> 
>>> What is it then?  I forgot what the original complaints/complainers were.
>> 
>> I have no idea, since I didn't hear the complaints.  But any non-fixed
>> size array has issues in C; there's not much we can do about it.
>> 
>> x86 manages this fine for msrs, and I didn't have a problem using it for
>> my test programs.  That's the limit of my experience, however.
> 
> Another option is to use the size parameter from the ioctl.  It just
> sits there doing nothing.

It would require quite a bunch of changes throughout the stack. Even in user space, like strace...

Alex

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Avi Kivity]

On 09/04/2012 04:59 PM, Alexander Graf wrote:
>> 
>> Not is you pack the pointer in a __u64, which is what we do to preserve
>> padding.  Then it is only s390 which needs extra love.
> 
> I doubt that anyone wants to run 31-bit user space on an s390x system. In fact, I wouldn't be surprised if exactly that case is broken already.

Arnd sent patches to fix it long ago.  Of course, something else may be
broken.

> 
>> 
>>>>> I don't think that is what makes the API hard
>>>>> to use.
>>>> 
>>>> What is it then?  I forgot what the original complaints/complainers were.
>>> 
>>> I have no idea, since I didn't hear the complaints.  But any non-fixed
>>> size array has issues in C; there's not much we can do about it.
>>> 
>>> x86 manages this fine for msrs, and I didn't have a problem using it for
>>> my test programs.  That's the limit of my experience, however.
>> 
>> Another option is to use the size parameter from the ioctl.  It just
>> sits there doing nothing.
> 
> It would require quite a bunch of changes throughout the stack. Even in user space, like strace...

I'm sure strace could cope.

I once had a proposal for extensible ioctl using the size parameter.
You want to extend an ioctl, the kernel zero-extends the input struct
for you, and truncates it on the way back.


-- 
error compiling committee.c: too many arguments to function

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Avi Kivity <avi@redhat.com> writes:

> On 09/03/2012 03:33 PM, Rusty Russell wrote:
>> Avi Kivity <avi@redhat.com> writes:
>>> On 09/01/2012 03:35 PM, Rusty Russell wrote:
>>>> Passing an address in a struct is pretty bad, since it involves
>>>> compatibility wrappers.  
>>>
>>> Right, some s390 thing.
>> 
>> Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64....
>> 
>> Any time you put a pointer in a structure which is exposed to userspace,
>> you have to deal with this.
>
> Not is you pack the pointer in a __u64, which is what we do to preserve
> padding.  Then it is only s390 which needs extra love.

OK, yes.  Or skip pointers altogether, like I do.

> Another option is to use the size parameter from the ioctl.  It just
> sits there doing nothing.

Not nothing, it defines the head struct size.  It's redundant, but
proven a useful sanity check over the years.

Perhaps somewhere else does use these 14 bits to represent a variable
size, but it would surprise me a bit to see it.  We'd probably want some
way to tell userspace the size then, so we have a different redundancy.

We're being too clever, that's why I copied the x86 MSR discovery
interface.

Cheers,
Rusty.

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Rusty Russell <rusty.russell@linaro.org> writes:

> Hi all,
>
>         This compiles, completely untested, but it's my attempt to give
> Avi (and Alexander) what he asked for in a generic register accessor.

And here's the tested version: see my new "onereg-abi" branch.
My next step is to demux CSELR, but that won't be until Tuesday.

Cheers,
Rusty.


The following changes since commit bc9cf74d26786dd2063155f9c703b8cb19d4270d:

  KVM: ARM: Add trace and fix prints on guest aborts (2012-08-28 22:35:53 -0700)

are available in the git repository at:

  gitolite@ra.kernel.org:/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git onereg-abi

for you to fetch changes up to fcecd7ddb31cf75d509b7a7bb2df033042b1d6a8:

  KVM ARM: Update api.txt (2012-09-01 21:56:06 +0930)

----------------------------------------------------------------
Rusty Russell (8):
      KVM: ARM: Fix walk_msrs()
      KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
      KVM: Add KVM_REG_SIZE() helper.
      KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
      KVM: Add KVM_VCPU_GET_REG_LIST.
      KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
      KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
      KVM ARM: Update api.txt

 Documentation/virtual/kvm/api.txt   |   61 ++++-----
 arch/arm/include/asm/kvm.h          |   77 ++++--------
 arch/arm/include/asm/kvm_coproc.h   |    6 +-
 arch/arm/include/asm/kvm_host.h     |   35 ++++--
 arch/arm/kvm/arm.c                  |   29 -----
 arch/arm/kvm/coproc.c               |  234 +++++++++++++++--------------------
 arch/arm/kvm/emulate.c              |    2 +-
 arch/arm/kvm/guest.c                |  158 ++++++++++++++---------
 arch/arm/kvm/reset.c                |    4 +-
 arch/powerpc/include/asm/kvm_host.h |    1 +
 arch/powerpc/kvm/book3s_hv.c        |    4 +-
 arch/powerpc/kvm/book3s_pr.c        |    4 +-
 arch/powerpc/kvm/booke.c            |    4 +-
 arch/powerpc/kvm/powerpc.c          |   15 ---
 arch/s390/include/asm/kvm_host.h    |    1 +
 arch/s390/kvm/kvm-s390.c            |   19 +--
 include/linux/kvm.h                 |   10 +-
 include/linux/kvm_host.h            |    9 +-
 virt/kvm/kvm_main.c                 |   38 ++++++
 19 files changed, 340 insertions(+), 371 deletions(-)

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Rusty Russell <rusty@rustcorp.com.au> writes:
> Rusty Russell <rusty.russell@linaro.org> writes:
>
>> Hi all,
>>
>>         This compiles, completely untested, but it's my attempt to give
>> Avi (and Alexander) what he asked for in a generic register accessor.
>
> And here's the tested version: see my new "onereg-abi" branch.
> My next step is to demux CSELR, but that won't be until Tuesday.
>
> Cheers,
> Rusty.
>
>
> The following changes since commit bc9cf74d26786dd2063155f9c703b8cb19d4270d:
>
>   KVM: ARM: Add trace and fix prints on guest aborts (2012-08-28 22:35:53 -0700)
>
> are available in the git repository at:
>
>   gitolite@ra.kernel.org:/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git onereg-abi

ie:
        git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git onereg-abi

Cheers,
Rusty.

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Peter Maydell]

On 1 September 2012 13:28, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Rusty Russell (8):
>       KVM: ARM: Fix walk_msrs()
>       KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
>       KVM: Add KVM_REG_SIZE() helper.
>       KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
>       KVM: Add KVM_VCPU_GET_REG_LIST.
>       KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
>       KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
>       KVM ARM: Update api.txt

So I was thinking about this, and I remembered that the SET_ONE_REG/
GET_ONE_REG API has userspace pass a pointer to the variable the
kernel should read/write (unlike the _MSR x86 ioctls, where the
actual data value is sent back and forth in the struct). Further,
the kernel only writes a data value of the size of the register
(rather than always reading/writing a uint64_t).

This is a problem because it means userspace needs to know the
size of each register, and the kernel doesn't provide any way
to determine the size. This defeats the idea that userspace should
be able to migrate kernel register state without having to know
the semantics of all the registers involved.

Possible solutions:
 * switch GET/SET_ONE_REG to just passing data, same as the MSR ioctls
 * switch GET/SET_ONE_REG to always writing 64 bits regardless of
   actual guest register width
 * make GET_REG_LIST return register width as well as index

Personally I would really prefer the MSR-style "pass the data".
Otherwise I'm going to end up constructing something like
 uint64_t actual_values[]
 struct kvm_one_reg regs[]

where regs[x].addr = &actual_values[x] for all x. Which seems
like unnecessary indirection really :-)

I could live with "always read/write 64 bits". I definitely don't
want to have to deal with matching up register widths to accesses
in userspace, please.

thanks
-- PMM

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Alexander Graf]

On 04.09.2012, at 09:31, Peter Maydell wrote:

> On 1 September 2012 13:28, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Rusty Russell (8):
>>      KVM: ARM: Fix walk_msrs()
>>      KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
>>      KVM: Add KVM_REG_SIZE() helper.
>>      KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
>>      KVM: Add KVM_VCPU_GET_REG_LIST.
>>      KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
>>      KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
>>      KVM ARM: Update api.txt
> 
> So I was thinking about this, and I remembered that the SET_ONE_REG/
> GET_ONE_REG API has userspace pass a pointer to the variable the
> kernel should read/write (unlike the _MSR x86 ioctls, where the
> actual data value is sent back and forth in the struct). Further,
> the kernel only writes a data value of the size of the register
> (rather than always reading/writing a uint64_t).
> 
> This is a problem because it means userspace needs to know the
> size of each register, and the kernel doesn't provide any way
> to determine the size.

It does, as it's encoded in the register ID.

> This defeats the idea that userspace should
> be able to migrate kernel register state without having to know
> the semantics of all the registers involved.
> 
> Possible solutions:
> * switch GET/SET_ONE_REG to just passing data, same as the MSR ioctls
> * switch GET/SET_ONE_REG to always writing 64 bits regardless of
>   actual guest register width
> * make GET_REG_LIST return register width as well as index
> 
> Personally I would really prefer the MSR-style "pass the data".

Well, the reason I put dynamic sizes in there is that we already have very big register sizes on x86 (265 bits iirc), and so far chances are that it'll rather get bigger than smaller over time. So I would really like to keep the size encoding in the register id so that we can support big multimedia registers later on.

> Otherwise I'm going to end up constructing something like
> uint64_t actual_values[]
> struct kvm_one_reg regs[]
> 
> where regs[x].addr = &actual_values[x] for all x. Which seems
> like unnecessary indirection really :-)
> 
> I could live with "always read/write 64 bits". I definitely don't
> want to have to deal with matching up register widths to accesses
> in userspace, please.

If I understood Rusty correctly, he wanted to do exactly that. Just make all the ARM registers be 64-bit wide, so that you can just keep them all as uint64_t in QEMU's env and then put env's pointers into the ONE_REG ioctl.


Alex

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Peter Maydell <peter.maydell@linaro.org> writes:
> On 1 September 2012 13:28, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Rusty Russell (8):
>>       KVM: ARM: Fix walk_msrs()
>>       KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
>>       KVM: Add KVM_REG_SIZE() helper.
>>       KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
>>       KVM: Add KVM_VCPU_GET_REG_LIST.
>>       KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
>>       KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
>>       KVM ARM: Update api.txt
>
> So I was thinking about this, and I remembered that the SET_ONE_REG/
> GET_ONE_REG API has userspace pass a pointer to the variable the
> kernel should read/write (unlike the _MSR x86 ioctls, where the
> actual data value is sent back and forth in the struct). Further,
> the kernel only writes a data value of the size of the register
> (rather than always reading/writing a uint64_t).
>
> This is a problem because it means userspace needs to know the
> size of each register, and the kernel doesn't provide any way
> to determine the size. This defeats the idea that userspace should
> be able to migrate kernel register state without having to know
> the semantics of all the registers involved.

It's there.  There are bits in the id which indicate the size:

#define KVM_REG_SIZE_SHIFT	52
#define KVM_REG_SIZE_MASK	0x00f0000000000000ULL
#define KVM_REG_SIZE_U8		0x0000000000000000ULL
#define KVM_REG_SIZE_U16	0x0010000000000000ULL
#define KVM_REG_SIZE_U32	0x0020000000000000ULL
#define KVM_REG_SIZE_U64	0x0030000000000000ULL
#define KVM_REG_SIZE_U128	0x0040000000000000ULL
#define KVM_REG_SIZE_U256	0x0050000000000000ULL
#define KVM_REG_SIZE_U512	0x0060000000000000ULL
#define KVM_REG_SIZE_U1024	0x0070000000000000ULL

And my patches added a helper:

#define KVM_REG_SIZE(id)						\
	(1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))

> I could live with "always read/write 64 bits". I definitely don't
> want to have to deal with matching up register widths to accesses
> in userspace, please.

I changed my mind about the old scheme when I realized we have to deal
with 128-bit FPU registers.

Cheers,
Rusty.

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Peter Maydell]

On 5 September 2012 07:48, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> This is a problem because it means userspace needs to know the
>> size of each register, and the kernel doesn't provide any way
>> to determine the size. This defeats the idea that userspace should
>> be able to migrate kernel register state without having to know
>> the semantics of all the registers involved.
>
> It's there.  There are bits in the id which indicate the size:

> And my patches added a helper:
>
> #define KVM_REG_SIZE(id)                                                \
>         (1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))

Ah, right, I hadn't realised that was in the exposed-to-userspace
bit of the code.

>> I could live with "always read/write 64 bits". I definitely don't
>> want to have to deal with matching up register widths to accesses
>> in userspace, please.
>
> I changed my mind about the old scheme when I realized we have to deal
> with 128-bit FPU registers.

Mmm, ARM might not have any awkward size registers but there's
x86 weirdisms to consider for a generic ABI I guess.

-- PMM

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 September 2012 07:48, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I could live with "always read/write 64 bits". I definitely don't
>>> want to have to deal with matching up register widths to accesses
>>> in userspace, please.
>>
>> I changed my mind about the old scheme when I realized we have to deal
>> with 128-bit FPU registers.
>
> Mmm, ARM might not have any awkward size registers but there's
> x86 weirdisms to consider for a generic ABI I guess.

Actually, I hadn't realized ARM didn't do 128-bit FP regs already.

But I'd guess they'll arrive one day.

Cheers,
Rusty.

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Peter Maydell]

On 6 September 2012 02:44, Rusty Russell <rusty.russell@linaro.org> wrote:
> Actually, I hadn't realized ARM didn't do 128-bit FP regs already.
>
> But I'd guess they'll arrive one day.

AArch64 has them. I had thought that you'd be able to treat one 128-bit
reg as two 64 bit regs (in the same way that in 32-bit ARM the 64 bit
register d0 is accessible as the pair of 32 bit regs s0,s1). However
AArch64 doesn't overlap its registers in the same way, so userspace
will need to access them as 128 bit registers to get the full state.

-- PMM

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Avi Kivity]

On 09/05/2012 09:48 AM, Rusty Russell wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 1 September 2012 13:28, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Rusty Russell (8):
>>>       KVM: ARM: Fix walk_msrs()
>>>       KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
>>>       KVM: Add KVM_REG_SIZE() helper.
>>>       KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
>>>       KVM: Add KVM_VCPU_GET_REG_LIST.
>>>       KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
>>>       KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
>>>       KVM ARM: Update api.txt
>>
>> So I was thinking about this, and I remembered that the SET_ONE_REG/
>> GET_ONE_REG API has userspace pass a pointer to the variable the
>> kernel should read/write (unlike the _MSR x86 ioctls, where the
>> actual data value is sent back and forth in the struct). Further,
>> the kernel only writes a data value of the size of the register
>> (rather than always reading/writing a uint64_t).
>>
>> This is a problem because it means userspace needs to know the
>> size of each register, and the kernel doesn't provide any way
>> to determine the size. This defeats the idea that userspace should
>> be able to migrate kernel register state without having to know
>> the semantics of all the registers involved.
> 
> It's there.  There are bits in the id which indicate the size:
> 
> #define KVM_REG_SIZE_SHIFT	52
> #define KVM_REG_SIZE_MASK	0x00f0000000000000ULL
> #define KVM_REG_SIZE_U8		0x0000000000000000ULL
> #define KVM_REG_SIZE_U16	0x0010000000000000ULL
> #define KVM_REG_SIZE_U32	0x0020000000000000ULL
> #define KVM_REG_SIZE_U64	0x0030000000000000ULL
> #define KVM_REG_SIZE_U128	0x0040000000000000ULL
> #define KVM_REG_SIZE_U256	0x0050000000000000ULL
> #define KVM_REG_SIZE_U512	0x0060000000000000ULL
> #define KVM_REG_SIZE_U1024	0x0070000000000000ULL
> 

Assumes power-of-two registers.  On x86 IDTR is 10 bytes long (2 byte
limit, 8 byte address).  We could split it into two registers, or add
padding, but it's unnatural.


-- 
error compiling committee.c: too many arguments to function

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Alexander Graf]

On 06.09.2012, at 10:48, Avi Kivity <avi@redhat.com> wrote:

> On 09/05/2012 09:48 AM, Rusty Russell wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> On 1 September 2012 13:28, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> Rusty Russell (8):
>>>>      KVM: ARM: Fix walk_msrs()
>>>>      KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
>>>>      KVM: Add KVM_REG_SIZE() helper.
>>>>      KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
>>>>      KVM: Add KVM_VCPU_GET_REG_LIST.
>>>>      KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
>>>>      KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
>>>>      KVM ARM: Update api.txt
>>> 
>>> So I was thinking about this, and I remembered that the SET_ONE_REG/
>>> GET_ONE_REG API has userspace pass a pointer to the variable the
>>> kernel should read/write (unlike the _MSR x86 ioctls, where the
>>> actual data value is sent back and forth in the struct). Further,
>>> the kernel only writes a data value of the size of the register
>>> (rather than always reading/writing a uint64_t).
>>> 
>>> This is a problem because it means userspace needs to know the
>>> size of each register, and the kernel doesn't provide any way
>>> to determine the size. This defeats the idea that userspace should
>>> be able to migrate kernel register state without having to know
>>> the semantics of all the registers involved.
>> 
>> It's there.  There are bits in the id which indicate the size:
>> 
>> #define KVM_REG_SIZE_SHIFT    52
>> #define KVM_REG_SIZE_MASK    0x00f0000000000000ULL
>> #define KVM_REG_SIZE_U8        0x0000000000000000ULL
>> #define KVM_REG_SIZE_U16    0x0010000000000000ULL
>> #define KVM_REG_SIZE_U32    0x0020000000000000ULL
>> #define KVM_REG_SIZE_U64    0x0030000000000000ULL
>> #define KVM_REG_SIZE_U128    0x0040000000000000ULL
>> #define KVM_REG_SIZE_U256    0x0050000000000000ULL
>> #define KVM_REG_SIZE_U512    0x0060000000000000ULL
>> #define KVM_REG_SIZE_U1024    0x0070000000000000ULL
>> 
> 
> Assumes power-of-two registers.  On x86 IDTR is 10 bytes long (2 byte
> limit, 8 byte address).  We could split it into two registers, or add
> padding, but it's unnatural.

Why is padding bad? How do you model IDTR throughout the stack today? How does QEMU's savevm serialize it?


Alex

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Avi Kivity]

On 09/06/2012 06:08 PM, Alexander Graf wrote:
> 
> 
> On 06.09.2012, at 10:48, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 09/05/2012 09:48 AM, Rusty Russell wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>> On 1 September 2012 13:28, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>> Rusty Russell (8):
>>>>>      KVM: ARM: Fix walk_msrs()
>>>>>      KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
>>>>>      KVM: Add KVM_REG_SIZE() helper.
>>>>>      KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
>>>>>      KVM: Add KVM_VCPU_GET_REG_LIST.
>>>>>      KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
>>>>>      KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
>>>>>      KVM ARM: Update api.txt
>>>> 
>>>> So I was thinking about this, and I remembered that the SET_ONE_REG/
>>>> GET_ONE_REG API has userspace pass a pointer to the variable the
>>>> kernel should read/write (unlike the _MSR x86 ioctls, where the
>>>> actual data value is sent back and forth in the struct). Further,
>>>> the kernel only writes a data value of the size of the register
>>>> (rather than always reading/writing a uint64_t).
>>>> 
>>>> This is a problem because it means userspace needs to know the
>>>> size of each register, and the kernel doesn't provide any way
>>>> to determine the size. This defeats the idea that userspace should
>>>> be able to migrate kernel register state without having to know
>>>> the semantics of all the registers involved.
>>> 
>>> It's there.  There are bits in the id which indicate the size:
>>> 
>>> #define KVM_REG_SIZE_SHIFT    52
>>> #define KVM_REG_SIZE_MASK    0x00f0000000000000ULL
>>> #define KVM_REG_SIZE_U8        0x0000000000000000ULL
>>> #define KVM_REG_SIZE_U16    0x0010000000000000ULL
>>> #define KVM_REG_SIZE_U32    0x0020000000000000ULL
>>> #define KVM_REG_SIZE_U64    0x0030000000000000ULL
>>> #define KVM_REG_SIZE_U128    0x0040000000000000ULL
>>> #define KVM_REG_SIZE_U256    0x0050000000000000ULL
>>> #define KVM_REG_SIZE_U512    0x0060000000000000ULL
>>> #define KVM_REG_SIZE_U1024    0x0070000000000000ULL
>>> 
>> 
>> Assumes power-of-two registers.  On x86 IDTR is 10 bytes long (2 byte
>> limit, 8 byte address).  We could split it into two registers, or add
>> padding, but it's unnatural.

(and the APIC, if treated as one-large-register) is 4k)

> 
> Why is padding bad?

Where does it come? between the 2 byte and the 8 byte element?  After
the 10 bytes?

It means that users must either include the padding in their internal
data structures, or copy to a temporary.

> How do you model IDTR throughout the stack today? 

struct kvm_dtable {
	__u64 base;
	__u16 limit;
	__u16 padding[3];
};

:p

Internally, it's held in hardware registers.

> How does QEMU's savevm serialize it?

Two separate fields (actually four, of which two are ignored).

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Peter Maydell]

On 6 September 2012 16:16, Avi Kivity <avi@redhat.com> wrote:
> (and the APIC, if treated as one-large-register) is 4k)

...so don't do that then. Trying to treat the whole APIC
as a single "register" means you don't get any of the
advantages of "does this kernel support this register?"
etc. Is there some reason I'm not seeing why it would
make sense to do it that way?

-- PMM

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Avi Kivity]

On 09/06/2012 06:23 PM, Peter Maydell wrote:
> On 6 September 2012 16:16, Avi Kivity <avi@redhat.com> wrote:
>> (and the APIC, if treated as one-large-register) is 4k)
> 
> ...so don't do that then. Trying to treat the whole APIC
> as a single "register" means you don't get any of the
> advantages of "does this kernel support this register?"
> etc. Is there some reason I'm not seeing why it would
> make sense to do it that way?

It's just the easiest path forward.

"one large register" is mainly useful if registers have
interdependencies.  That doesn't exist in the APIC AFAIR, but it does
exist elsewhere.  Another way to handle interdependencies is to defer
applying the changes until a KVM_RUN, and then evaluate them as a group.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

[Rusty Russell]

Avi Kivity <avi@redhat.com> writes:
> On 09/06/2012 06:23 PM, Peter Maydell wrote:
>> On 6 September 2012 16:16, Avi Kivity <avi@redhat.com> wrote:
>>> (and the APIC, if treated as one-large-register) is 4k)
>> 
>> ...so don't do that then. Trying to treat the whole APIC
>> as a single "register" means you don't get any of the
>> advantages of "does this kernel support this register?"
>> etc. Is there some reason I'm not seeing why it would
>> make sense to do it that way?
>
> It's just the easiest path forward.
>
> "one large register" is mainly useful if registers have
> interdependencies.  That doesn't exist in the APIC AFAIR, but it does
> exist elsewhere.  Another way to handle interdependencies is to defer
> applying the changes until a KVM_RUN, and then evaluate them as a group.

The other option is to implement KVM_SET_MULTI_REG.  I have enough of an
implmentation to show it's trivial, but it's needless complexity
until/if we need it.

Cheers,
Rusty.