armv7 BE kvm support

Switch hypervisor to run in BE mode if image is compiled with CONFIG_CPU_BIG_ENDIAN.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/init.S | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kvm/init.S +3 -0

index 1b9844d..74f0718 100644
@@ -22,6 +22,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
+#include <asm/assembler.h>
 
 /********************************************************************
  * Hypervisor initialization

@@ -70,6 +71,8 @@ __do_hyp_init:
 	cmp	r0, #0			@ We have a SP?
 	bne	phase2			@ Yes, second stage init
 
+ARM_BE8(setend	be) @ Switch to Big Endian mode if needed
+
 	@ Set the HTTBR to point to the hypervisor PGD pointer passed
 	mcrr	p15, 4, r2, r3, c2
 

Signature · 1 line

1.8.1.4

Quoted 29 lines · Victor Kamensky

Switch hypervisor to run in BE mode if image is compiled with CONFIG_CPU_BIG_ENDIAN.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/init.S | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kvm/init.S +3 -0

index 1b9844d..74f0718 100644
@@ -22,6 +22,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
+#include <asm/assembler.h>
 
 /********************************************************************
  * Hypervisor initialization

@@ -70,6 +71,8 @@ __do_hyp_init:
      cmp     r0, #0                  @ We have a SP?
      bne     phase2                  @ Yes, second stage init
 
+ARM_BE8(setend       be) @ Switch to Big Endian mode if needed
+
      @ Set the HTTBR to point to the hypervisor PGD pointer passed
      mcrr    p15, 4, r2, r3, c2
 

Quoted 3 lines

--
1.8.1.4

Won't splitting up the patches this way break bisectability?

-Christoffer

Quoted 29 lines · Victor Kamensky

Switch hypervisor to run in BE mode if image is compiled with CONFIG_CPU_BIG_ENDIAN.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/init.S | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kvm/init.S +3 -0

index 1b9844d..74f0718 100644
@@ -22,6 +22,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
+#include <asm/assembler.h>
 
 /********************************************************************
  * Hypervisor initialization

@@ -70,6 +71,8 @@ __do_hyp_init:
 	cmp	r0, #0			@ We have a SP?
 	bne	phase2			@ Yes, second stage init
 
+ARM_BE8(setend	be) @ Switch to Big Endian mode if needed
+
 	@ Set the HTTBR to point to the hypervisor PGD pointer passed
 	mcrr	p15, 4, r2, r3, c2
 

Quoted 3 lines

Signature · 1 line

1.8.1.4

Won't splitting up the patches this way break bisectability?

The vgic h/w registers are little endian; when asm code reads/writes

from/to them, it needs to do byteswap after/before. Byteswap code uses ARM_BE8 wrapper to add swap only if CONFIG_CPU_BIG_ENDIAN is configured.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/interrupts_head.S | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

arch/arm/kvm/interrupts_head.S +14 -0

index 6f18695..1e9be2f 100644
@@ -1,4 +1,5 @@
 #include <linux/irqchip/arm-gic.h>
+#include <asm/assembler.h>
 
 #define VCPU_USR_REG(_reg_nr)	(VCPU_USR_REGS + (_reg_nr * 4))
 #define VCPU_USR_SP		(VCPU_USR_REG(13))

@@ -412,6 +413,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	ldr	r8, [r2, #GICH_ELRSR0]
 	ldr	r9, [r2, #GICH_ELRSR1]
 	ldr	r10, [r2, #GICH_APR]
+ARM_BE8(rev	r3, r3	)
+ARM_BE8(rev	r4, r4	)
+ARM_BE8(rev	r5, r5	)
+ARM_BE8(rev	r6, r6	)
+ARM_BE8(rev	r7, r7	)
+ARM_BE8(rev	r8, r8	)
+ARM_BE8(rev	r9, r9	)
+ARM_BE8(rev	r10, r10	)
 
 	str	r3, [r11, #VGIC_CPU_HCR]
 	str	r4, [r11, #VGIC_CPU_VMCR]

@@ -431,6 +440,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	add	r3, r11, #VGIC_CPU_LR
 	ldr	r4, [r11, #VGIC_CPU_NR_LR]
 1:	ldr	r6, [r2], #4
+ARM_BE8(rev	r6, r6	)
 	str	r6, [r3], #4
 	subs	r4, r4, #1
 	bne	1b

@@ -458,6 +468,9 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	ldr	r3, [r11, #VGIC_CPU_HCR]
 	ldr	r4, [r11, #VGIC_CPU_VMCR]
 	ldr	r8, [r11, #VGIC_CPU_APR]
+ARM_BE8(rev	r3, r3	)
+ARM_BE8(rev	r4, r4	)
+ARM_BE8(rev	r8, r8	)
 
 	str	r3, [r2, #GICH_HCR]
 	str	r4, [r2, #GICH_VMCR]

@@ -468,6 +481,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	add	r3, r11, #VGIC_CPU_LR
 	ldr	r4, [r11, #VGIC_CPU_NR_LR]
 1:	ldr	r6, [r3], #4
+ARM_BE8(rev	r6, r6  )
 	str	r6, [r2], #4
 	subs	r4, r4, #1
 	bne	1b

Signature · 1 line

1.8.1.4

In some cases the mcrr and mrrc instructions in combination with the ldrd and strd instructions need to deal with 64bit value in memory. The ldrd and strd instructions already handle endianness within word (register) boundaries but to get effect of the whole 64bit value represented correctly, rr_lo_hi macro is introduced and is used to swap registers positions when the mcrr and mrrc instructions are used. That has the effect of swapping two words.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/include/asm/kvm_asm.h | 23 +++++++++++++++++++++--
 arch/arm/kvm/init.S            |  4 ++--
 arch/arm/kvm/interrupts.S      |  4 ++--
 arch/arm/kvm/interrupts_head.S | 18 +++++++++---------
 4 files changed, 34 insertions(+), 15 deletions(-)

arch/arm/include/asm/kvm_asm.h +2 -2

index 661da11..c6ae937 100644
@@ -26,9 +26,9 @@
 #define c1_ACTLR	4	/* Auxilliary Control Register */
 #define c1_CPACR	5	/* Coprocessor Access Control */
 #define c2_TTBR0	6	/* Translation Table Base Register 0 */
-#define c2_TTBR0_high	7	/* TTBR0 top 32 bits */
+#define c2_TTBR0_hilo	7	/* TTBR0 top 32 bits in LE case, low 32 bits in BE case */
 #define c2_TTBR1	8	/* Translation Table Base Register 1 */
-#define c2_TTBR1_high	9	/* TTBR1 top 32 bits */
+#define c2_TTBR1_hilo	9	/* TTBR1 top 32 bits in LE case, low 32 bits in BE case */
 #define c2_TTBCR	10	/* Translation Table Base Control R. */
 #define c3_DACR		11	/* Domain Access Control Register */
 #define c5_DFSR		12	/* Data Fault Status Register */

Note
+ * within word (single register) the ldrd/strd instruction already
+ * swap data correctly, only additional manipulation required with order
+ * of register to have effect of 64 bit value beeing effectively
+ * swapped.

+ */

arch/arm/include/asm/kvm_asm.h +6 -0

+#ifdef CONFIG_CPU_ENDIAN_BE8
+#define rr_lo_hi(a1, a2) a2, a1
+#else
+#define rr_lo_hi(a1, a2) a1, a2
+#endif
+
 #ifndef __ASSEMBLY__
 struct kvm;
 struct kvm_vcpu;

arch/arm/kvm/init.S +2 -2

index 74f0718..2d10b2d 100644
@@ -74,7 +74,7 @@ __do_hyp_init:
 ARM_BE8(setend	be) @ Switch to Big Endian mode if needed
 
 	@ Set the HTTBR to point to the hypervisor PGD pointer passed
-	mcrr	p15, 4, r2, r3, c2
+	mcrr	p15, 4, rr_lo_hi(r2, r3), c2
 
 	@ Set the HTCR and VTCR to the same shareability and cacheability
 	@ settings as the non-secure TTBCR and with T0SZ == 0.

@@ -140,7 +140,7 @@ phase2:
 	mov	pc, r0
 
 target:	@ We're now in the trampoline code, switch page tables
-	mcrr	p15, 4, r2, r3, c2
+	mcrr	p15, 4, rr_lo_hi(r2, r3), c2
 	isb
 
 	@ Invalidate the old TLBs

arch/arm/kvm/interrupts.S +2 -2

index ddc1553..f0696bd 100644
@@ -52,7 +52,7 @@ ENTRY(__kvm_tlb_flush_vmid_ipa)
 	dsb	ishst
 	add	r0, r0, #KVM_VTTBR
 	ldrd	r2, r3, [r0]
-	mcrr	p15, 6, r2, r3, c2	@ Write VTTBR
+	mcrr	p15, 6, rr_lo_hi(r2, r3), c2	@ Write VTTBR
 	isb
 	mcr     p15, 0, r0, c8, c3, 0	@ TLBIALLIS (rt ignored)
 	dsb	ish

@@ -135,7 +135,7 @@ ENTRY(__kvm_vcpu_run)
 	ldr	r1, [vcpu, #VCPU_KVM]
 	add	r1, r1, #KVM_VTTBR
 	ldrd	r2, r3, [r1]
-	mcrr	p15, 6, r2, r3, c2	@ Write VTTBR
+	mcrr	p15, 6, rr_lo_hi(r2, r3), c2	@ Write VTTBR
 
 	@ We're all done, just restore the GPRs and go to the guest
 	restore_guest_regs

arch/arm/kvm/interrupts_head.S +9 -9

index 1e9be2f..3409ed6 100644
@@ -252,8 +252,8 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	mrc	p15, 0, r3, c1, c0, 2	@ CPACR
 	mrc	p15, 0, r4, c2, c0, 2	@ TTBCR
 	mrc	p15, 0, r5, c3, c0, 0	@ DACR
-	mrrc	p15, 0, r6, r7, c2	@ TTBR 0
-	mrrc	p15, 1, r8, r9, c2	@ TTBR 1
+	mrrc	p15, 0, rr_lo_hi(r6, r7), c2	@ TTBR 0
+	mrrc	p15, 1, rr_lo_hi(r8, r9), c2	@ TTBR 1
 	mrc	p15, 0, r10, c10, c2, 0	@ PRRR
 	mrc	p15, 0, r11, c10, c2, 1	@ NMRR
 	mrc	p15, 2, r12, c0, c0, 0	@ CSSELR

@@ -303,7 +303,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	.endif
 
 	mrc	p15, 0, r2, c14, c1, 0	@ CNTKCTL
-	mrrc	p15, 0, r4, r5, c7	@ PAR
+	mrrc	p15, 0, rr_lo_hi(r4, r5), c7	@ PAR
 
 	.if \store_to_vcpu == 0
 	push	{r2,r4-r5}

@@ -331,7 +331,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	.endif
 
 	mcr	p15, 0, r2, c14, c1, 0	@ CNTKCTL
-	mcrr	p15, 0, r4, r5, c7	@ PAR
+	mcrr	p15, 0, rr_lo_hi(r4, r5), c7	@ PAR
 
 	.if \read_from_vcpu == 0
 	pop	{r2-r12}

@@ -381,8 +381,8 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	mcr	p15, 0, r3, c1, c0, 2	@ CPACR
 	mcr	p15, 0, r4, c2, c0, 2	@ TTBCR
 	mcr	p15, 0, r5, c3, c0, 0	@ DACR
-	mcrr	p15, 0, r6, r7, c2	@ TTBR 0
-	mcrr	p15, 1, r8, r9, c2	@ TTBR 1
+	mcrr	p15, 0, rr_lo_hi(r6, r7), c2	@ TTBR 0
+	mcrr	p15, 1, rr_lo_hi(r8, r9), c2	@ TTBR 1
 	mcr	p15, 0, r10, c10, c2, 0	@ PRRR
 	mcr	p15, 0, r11, c10, c2, 1	@ NMRR
 	mcr	p15, 2, r12, c0, c0, 0	@ CSSELR

@@ -512,7 +512,7 @@ ARM_BE8(rev	r6, r6  )
 	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
 	isb
 
-	mrrc	p15, 3, r2, r3, c14	@ CNTV_CVAL
+	mrrc	p15, 3, rr_lo_hi(r2, r3), c14	@ CNTV_CVAL
 	ldr	r4, =VCPU_TIMER_CNTV_CVAL
 	add	r5, vcpu, r4
 	strd	r2, r3, [r5]

@@ -552,12 +552,12 @@ ARM_BE8(rev	r6, r6  )
 
 	ldr	r2, [r4, #KVM_TIMER_CNTVOFF]
 	ldr	r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
-	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
+	mcrr	p15, 4, rr_lo_hi(r2, r3), c14	@ CNTVOFF
 
 	ldr	r4, =VCPU_TIMER_CNTV_CVAL
 	add	r5, vcpu, r4
 	ldrd	r2, r3, [r5]
-	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
+	mcrr	p15, 3, rr_lo_hi(r2, r3), c14	@ CNTV_CVAL
 	isb
 
 	ldr	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]

Signature · 1 line

1.8.1.4

The __kvm_vcpu_run function returns a 64-bit result in two registers, which has to be adjusted for BE case.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/interrupts.S | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/kvm/interrupts.S +5 -0

index f0696bd..5d27f7f 100644
@@ -199,8 +199,13 @@ after_vfp_restore:
 
 	restore_host_regs
 	clrex				@ Clear exclusive monitor
+#ifndef __ARMEB__
 	mov	r0, r1			@ Return the return code
 	mov	r1, #0			@ Clear upper bits in return value
+#else
+	@ r1 already has return code
+	mov	r0, #0			@ Clear upper bits in return value
+#endif /* __ARMEB__ */
 	bx	lr			@ return to IOCTL
 
 /********************************************************************

Signature · 1 line

1.8.1.4

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

reg_from_user64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user64 - writes reigster from kernel 'u64 *val' address to
user-land register memory; register size could be 4 or 8 bytes
ret_to_user32 - writes register(s) from kernel 'u32 *val' address to
user-land register(s) memory; note it could be one or two 4 bytes registers

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding 64 or 32 bit variant of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user64 and reg_to_user64 work only with least siginificant word of target/source kernel value.

Signed-off-by Victor Kamensky <[email protected]>

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

reg_from_user64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user64 - writes reigster from kernel 'u64 *val' address to

user-land register memory; register size could be 4 or 8 bytes
ret_to_user32 - writes register(s) from kernel 'u32 *val' address to
user-land register(s) memory; note it could be one or two 4 bytes registers

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding 64 or 32 bit variant of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user64 and reg_to_user64 work only with least siginificant word of target/source kernel value.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 25 deletions(-)

arch/arm/kvm/coproc.c +11 -1

index 78c0885..64b2b94 100644
@@ -634,17 +634,61 @@ static struct coproc_reg invariant_cp15[] = {
 	{ CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user64(u64 *val, const void __user *uaddr, u64 id)
+{
+	unsigned long regsize = KVM_REG_SIZE(id);
+	union {
+		u32	word;
+		u64	dword;
+	} tmp = {0};
+
+	if (copy_from_user(&tmp, uaddr, regsize) != 0)
+		return -EFAULT;
+

arch/arm/kvm/coproc.c +48 -24

+
+/* Note it may really copy two u32 registers */
+static int reg_from_user32(u32 *val, const void __user *uaddr, u64 id)
 {
-	/* This Just Works because we are little endian. */
 	if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
 		return -EFAULT;
 	return 0;
 }
 
-static int reg_to_user(void __user *uaddr, const void *val, u64 id)
+static int reg_to_user64(void __user *uaddr, const u64 *val, u64 id)
+{
+	unsigned long regsize = KVM_REG_SIZE(id);
+	union {
+		u32	word;
+		u64	dword;
+	} tmp;
+
+	switch (regsize) {
+	case 4:
+		tmp.word = *val;
+		break;
+	case 8:
+		tmp.dword = *val;
+		break;
+	}
+
+	if (copy_to_user(uaddr, &tmp, regsize) != 0)
+		return -EFAULT;
+	return 0;
+}
+
+/* Note it may really copy two u32 registers */
+static int reg_to_user32(void __user *uaddr, const u32 *val, u64 id)
 {
-	/* This Just Works because we are little endian. */
 	if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0)
 		return -EFAULT;
 	return 0;

@@ -662,7 +706,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
 	if (!r)
 		return -ENOENT;
 
-	return reg_to_user(uaddr, &r->val, id);
+	return reg_to_user64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -678,7 +722,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
 	if (!r)
 		return -ENOENT;
 
-	err = reg_from_user(&val, uaddr, id);
+	err = reg_from_user64(&val, uaddr, id);
 	if (err)
 		return err;
 

@@ -846,7 +890,7 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
 	if (vfpid < num_fp_regs()) {
 		if (KVM_REG_SIZE(id) != 8)
 			return -ENOENT;
-		return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+		return reg_to_user64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
 				   id);
 	}
 

@@ -856,22 +900,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
 
 	switch (vfpid) {
 	case KVM_REG_ARM_VFP_FPEXC:
-		return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
+		return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
 	case KVM_REG_ARM_VFP_FPSCR:
-		return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
+		return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
 	case KVM_REG_ARM_VFP_FPINST:
-		return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
+		return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
 	case KVM_REG_ARM_VFP_FPINST2:
-		return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
+		return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
 	case KVM_REG_ARM_VFP_MVFR0:
 		val = fmrx(MVFR0);
-		return reg_to_user(uaddr, &val, id);
+		return reg_to_user32(uaddr, &val, id);
 	case KVM_REG_ARM_VFP_MVFR1:
 		val = fmrx(MVFR1);
-		return reg_to_user(uaddr, &val, id);
+		return reg_to_user32(uaddr, &val, id);
 	case KVM_REG_ARM_VFP_FPSID:
 		val = fmrx(FPSID);
-		return reg_to_user(uaddr, &val, id);
+		return reg_to_user32(uaddr, &val, id);
 	default:
 		return -ENOENT;
 	}

@@ -890,8 +934,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
 	if (vfpid < num_fp_regs()) {
 		if (KVM_REG_SIZE(id) != 8)
 			return -ENOENT;
-		return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
-				     uaddr, id);
+		return reg_from_user64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+				       uaddr, id);
 	}
 
 	/* FP control registers are all 32 bit. */

@@ -900,28 +944,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
 
 	switch (vfpid) {
 	case KVM_REG_ARM_VFP_FPEXC:
-		return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
+		return reg_from_user32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
 	case KVM_REG_ARM_VFP_FPSCR:
-		return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
+		return reg_from_user32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
 	case KVM_REG_ARM_VFP_FPINST:
-		return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
+		return reg_from_user32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
 	case KVM_REG_ARM_VFP_FPINST2:
-		return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
+		return reg_from_user32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
 	/* These are invariant. */
 	case KVM_REG_ARM_VFP_MVFR0:
-		if (reg_from_user(&val, uaddr, id))
+		if (reg_from_user32(&val, uaddr, id))
 			return -EFAULT;
 		if (val != fmrx(MVFR0))
 			return -EINVAL;
 		return 0;
 	case KVM_REG_ARM_VFP_MVFR1:
-		if (reg_from_user(&val, uaddr, id))
+		if (reg_from_user32(&val, uaddr, id))
 			return -EFAULT;
 		if (val != fmrx(MVFR1))
 			return -EINVAL;
 		return 0;
 	case KVM_REG_ARM_VFP_FPSID:
-		if (reg_from_user(&val, uaddr, id))
+		if (reg_from_user32(&val, uaddr, id))
 			return -EFAULT;
 		if (val != fmrx(FPSID))
 			return -EINVAL;

@@ -968,7 +1012,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 		return get_invariant_cp15(reg->id, uaddr);
 
 	/* Note: copies two regs if size is 64 bit. */
-	return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
+	return reg_to_user32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

@@ -987,7 +1031,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 		return set_invariant_cp15(reg->id, uaddr);
 
 	/* Note: copies two regs if size is 64 bit */
-	return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
+	return reg_from_user32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static unsigned int num_demux_regs(void)

Signature · 1 line

1.8.1.4

Quoted 16 lines · Victor Kamensky

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

Quoted 5 lines

reg_from_user64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user64 - writes reigster from kernel 'u64 *val' address to

s/reigster/register/

Quoted 5 lines

user-land register memory; register size could be 4 or 8 bytes
ret_to_user32 - writes register(s) from kernel 'u32 *val' address to
user-land register(s) memory; note it could be one or two 4 bytes registers

If we are really going to keep this scheme, then please add these comments to functions themselves.

Quoted 34 lines

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding 64 or 32 bit variant of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user64 and reg_to_user64 work only with least siginificant word of target/source kernel value.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 25 deletions(-)

arch/arm/kvm/coproc.c +11 -1

index 78c0885..64b2b94 100644
@@ -634,17 +634,61 @@ static struct coproc_reg invariant_cp15[] = {
      { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user64(u64 *val, const void __user *uaddr, u64 id)
+{
+     unsigned long regsize = KVM_REG_SIZE(id);
+     union {
+             u32     word;
+             u64     dword;
+     } tmp = {0};
+
+     if (copy_from_user(&tmp, uaddr, regsize) != 0)
+             return -EFAULT;
+

arch/arm/kvm/coproc.c static struct coproc_reg invariant_cp15[] = { · line 657

Quoted 10 lines

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

+     switch (regsize) {
+     case 4:
+             *val = tmp.word;
+             break;
+     case 8:
+             *val = tmp.dword;
+             break;
+     }
+     return 0;
+}

instead of allowing this 'packing 32 bit values in 64 bit values and packing two 32 bit values in 64 bit values' kernel implementation'y stuff to be passed to these user-space ABI catering function, I really think you want to make reg_from_user64 deal with 64-bit registers, that is, BUG_ON(KVM_REG_SIZE(id) != 8.

If the kernel stores what user space sees as a 32-bit register in a 64-bit register, then let the caller deal with this mess.

If the kernel stores what user space sees as a 64-bit register in two 32-bit registers, then let the caller deal with that mess.

Imagine someone who hasn't been through the development of this code seeing these two functions for the first time without having read your commit message, I think the margin for error here is too large.

If you can share these functions like Alex suggests, then that would make for a much cleaner function API as well.

Quoted 163 lines

arch/arm/kvm/coproc.c +48 -25

+
+/* Note it may really copy two u32 registers */
+static int reg_from_user32(u32 *val, const void __user *uaddr, u64 id)
 {
-     /* This Just Works because we are little endian. */
      if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
              return -EFAULT;
      return 0;
 }
 
-static int reg_to_user(void __user *uaddr, const void *val, u64 id)
+static int reg_to_user64(void __user *uaddr, const u64 *val, u64 id)
+{
+     unsigned long regsize = KVM_REG_SIZE(id);
+     union {
+             u32     word;
+             u64     dword;
+     } tmp;
+
+     switch (regsize) {
+     case 4:
+             tmp.word = *val;
+             break;
+     case 8:
+             tmp.dword = *val;
+             break;
+     }
+
+     if (copy_to_user(uaddr, &tmp, regsize) != 0)
+             return -EFAULT;
+     return 0;
+}
+
+/* Note it may really copy two u32 registers */
+static int reg_to_user32(void __user *uaddr, const u32 *val, u64 id)
 {
-     /* This Just Works because we are little endian. */
      if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0)
              return -EFAULT;
      return 0;

@@ -662,7 +706,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
      if (!r)
              return -ENOENT;
 
-     return reg_to_user(uaddr, &r->val, id);
+     return reg_to_user64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -678,7 +722,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
      if (!r)
              return -ENOENT;
 
-     err = reg_from_user(&val, uaddr, id);
+     err = reg_from_user64(&val, uaddr, id);
      if (err)
              return err;
 

@@ -846,7 +890,7 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
      if (vfpid < num_fp_regs()) {
              if (KVM_REG_SIZE(id) != 8)
                      return -ENOENT;
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+             return reg_to_user64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
                                 id);
      }
 

@@ -856,22 +900,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
 
      switch (vfpid) {
      case KVM_REG_ARM_VFP_FPEXC:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
      case KVM_REG_ARM_VFP_FPSCR:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
      case KVM_REG_ARM_VFP_FPINST:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
      case KVM_REG_ARM_VFP_FPINST2:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
      case KVM_REG_ARM_VFP_MVFR0:
              val = fmrx(MVFR0);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_MVFR1:
              val = fmrx(MVFR1);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_FPSID:
              val = fmrx(FPSID);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user32(uaddr, &val, id);
      default:
              return -ENOENT;
      }

@@ -890,8 +934,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
      if (vfpid < num_fp_regs()) {
              if (KVM_REG_SIZE(id) != 8)
                      return -ENOENT;
-             return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
-                                  uaddr, id);
+             return reg_from_user64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+                                    uaddr, id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -900,28 +944,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
 
      switch (vfpid) {
      case KVM_REG_ARM_VFP_FPEXC:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
      case KVM_REG_ARM_VFP_FPSCR:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
      case KVM_REG_ARM_VFP_FPINST:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
      case KVM_REG_ARM_VFP_FPINST2:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
      /* These are invariant. */
      case KVM_REG_ARM_VFP_MVFR0:
-             if (reg_from_user(&val, uaddr, id))
+             if (reg_from_user32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(MVFR0))
                      return -EINVAL;
              return 0;
      case KVM_REG_ARM_VFP_MVFR1:
-             if (reg_from_user(&val, uaddr, id))
+             if (reg_from_user32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(MVFR1))
                      return -EINVAL;
              return 0;
      case KVM_REG_ARM_VFP_FPSID:
-             if (reg_from_user(&val, uaddr, id))
+             if (reg_from_user32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(FPSID))
                      return -EINVAL;

@@ -968,7 +1012,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
              return get_invariant_cp15(reg->id, uaddr);
 
      /* Note: copies two regs if size is 64 bit. */
-     return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
+     return reg_to_user32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

@@ -987,7 +1031,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
              return set_invariant_cp15(reg->id, uaddr);
 
      /* Note: copies two regs if size is 64 bit */
-     return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
+     return reg_from_user32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static unsigned int num_demux_regs(void)
--

Quoted 2 lines

1.8.1.4

Thanks,
--
Christoffer

Hi Christoffer,

Appreciate your time and review comments.
Please see response inline.

Quoted 91 lines · Christoffer Dall

Quoted 16 lines · Victor Kamensky

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

Quoted 5 lines

reg_from_user64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user64 - writes reigster from kernel 'u64 *val' address to

s/reigster/register/

Quoted 5 lines

user-land register memory; register size could be 4 or 8 bytes
ret_to_user32 - writes register(s) from kernel 'u32 *val' address to
user-land register(s) memory; note it could be one or two 4 bytes registers

If we are really going to keep this scheme, then please add these comments to functions themselves.

Quoted 34 lines

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding 64 or 32 bit variant of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user64 and reg_to_user64 work only with least siginificant word of target/source kernel value.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 25 deletions(-)

arch/arm/kvm/coproc.c +11 -1

index 78c0885..64b2b94 100644
@@ -634,17 +634,61 @@ static struct coproc_reg invariant_cp15[] = {
      { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user64(u64 *val, const void __user *uaddr, u64 id)
+{
+     unsigned long regsize = KVM_REG_SIZE(id);
+     union {
+             u32     word;
+             u64     dword;
+     } tmp = {0};
+
+     if (copy_from_user(&tmp, uaddr, regsize) != 0)
+             return -EFAULT;
+

arch/arm/kvm/coproc.c static struct coproc_reg invariant_cp15[] = { · line 657

Quoted 10 lines

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

+     switch (regsize) {
+     case 4:
+             *val = tmp.word;
+             break;
+     case 8:
+             *val = tmp.dword;
+             break;
+     }
+     return 0;
+}

instead of allowing this 'packing 32 bit values in 64 bit values and packing two 32 bit values in 64 bit values' kernel implementation'y stuff to be passed to these user-space ABI catering function, I really think you want to make reg_from_user64 deal with 64-bit registers, that is, BUG_ON(KVM_REG_SIZE(id) != 8.

If the kernel stores what user space sees as a 32-bit register in a 64-bit register, then let the caller deal with this mess.

If the kernel stores what user space sees as a 64-bit register in two 32-bit registers, then let the caller deal with that mess.

Quoted 6 lines

Imagine someone who hasn't been through the development of this code seeing these two functions for the first time without having read your commit message, I think the margin for error here is too large.

If you can share these functions like Alex suggests, then that would make for a much cleaner function API as well.

During LCA14 I had discussion with Alex about changing and sharing one_reg endianness functions with ppc code and with V8 side as well ... Alex outlined how it should be done and showed me ppc side of this code. He asked me to check with you before I start moving into this direction. I could not catch you during remaining LCA14 days. But now it looks you are on the same page ...

Let me try to rework code along Alex's suggestion and see how it will look like. I will take in account your above suggestions and will try to clean 64/32 bit overload. Since it will have bigger scope and shared with ppc side I wonder maybe I should pull this patch out of this series and handle it as later additions.

Quoted 16 lines · Victor Kamensky

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

Quoted 5 lines

reg_from_user64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user64 - writes reigster from kernel 'u64 *val' address to

s/reigster/register/

Quoted 5 lines

user-land register memory; register size could be 4 or 8 bytes
ret_to_user32 - writes register(s) from kernel 'u32 *val' address to
user-land register(s) memory; note it could be one or two 4 bytes registers

If we are really going to keep this scheme, then please add these comments to functions themselves.

Quoted 42 lines

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding 64 or 32 bit variant of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user64 and reg_to_user64 work only with least siginificant word of target/source kernel value.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 25 deletions(-)

arch/arm/kvm/coproc.c +19 -1

index 78c0885..64b2b94 100644
@@ -634,17 +634,61 @@ static struct coproc_reg invariant_cp15[] = {
      { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user64(u64 *val, const void __user *uaddr, u64 id)
+{
+     unsigned long regsize = KVM_REG_SIZE(id);
+     union {
+             u32     word;
+             u64     dword;
+     } tmp = {0};
+
+     if (copy_from_user(&tmp, uaddr, regsize) != 0)
+             return -EFAULT;
+
+     switch (regsize) {
+     case 4:
+             *val = tmp.word;
+             break;
+     case 8:
+             *val = tmp.dword;
+             break;
+     }

arch/arm/kvm/coproc.c static struct coproc_reg invariant_cp15[] = { · line 657

Quoted 2 lines

arch/arm/kvm/coproc.c +2 -0

+     return 0;
+}

instead of allowing this 'packing 32 bit values in 64 bit values and packing two 32 bit values in 64 bit values' kernel implementation'y stuff to be passed to these user-space ABI catering function, I really think you want to make reg_from_user64 deal with 64-bit registers, that is, BUG_ON(KVM_REG_SIZE(id) != 8.

If the kernel stores what user space sees as a 32-bit register in a 64-bit register, then let the caller deal with this mess.

If the kernel stores what user space sees as a 64-bit register in two 32-bit registers, then let the caller deal with that mess.

Imagine someone who hasn't been through the development of this code seeing these two functions for the first time without having read your commit message, I think the margin for error here is too large.

If you can share these functions like Alex suggests, then that would make for a much cleaner function API as well.

Quoted 163 lines

arch/arm/kvm/coproc.c +48 -25

+
+/* Note it may really copy two u32 registers */
+static int reg_from_user32(u32 *val, const void __user *uaddr, u64 id)
 {
-     /* This Just Works because we are little endian. */
      if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
              return -EFAULT;
      return 0;
 }
 
-static int reg_to_user(void __user *uaddr, const void *val, u64 id)
+static int reg_to_user64(void __user *uaddr, const u64 *val, u64 id)
+{
+     unsigned long regsize = KVM_REG_SIZE(id);
+     union {
+             u32     word;
+             u64     dword;
+     } tmp;
+
+     switch (regsize) {
+     case 4:
+             tmp.word = *val;
+             break;
+     case 8:
+             tmp.dword = *val;
+             break;
+     }
+
+     if (copy_to_user(uaddr, &tmp, regsize) != 0)
+             return -EFAULT;
+     return 0;
+}
+
+/* Note it may really copy two u32 registers */
+static int reg_to_user32(void __user *uaddr, const u32 *val, u64 id)
 {
-     /* This Just Works because we are little endian. */
      if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0)
              return -EFAULT;
      return 0;

@@ -662,7 +706,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
      if (!r)
              return -ENOENT;
 
-     return reg_to_user(uaddr, &r->val, id);
+     return reg_to_user64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -678,7 +722,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
      if (!r)
              return -ENOENT;
 
-     err = reg_from_user(&val, uaddr, id);
+     err = reg_from_user64(&val, uaddr, id);
      if (err)
              return err;
 

@@ -846,7 +890,7 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
      if (vfpid < num_fp_regs()) {
              if (KVM_REG_SIZE(id) != 8)
                      return -ENOENT;
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+             return reg_to_user64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
                                 id);
      }
 

@@ -856,22 +900,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
 
      switch (vfpid) {
      case KVM_REG_ARM_VFP_FPEXC:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
      case KVM_REG_ARM_VFP_FPSCR:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
      case KVM_REG_ARM_VFP_FPINST:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
      case KVM_REG_ARM_VFP_FPINST2:
-             return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
+             return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
      case KVM_REG_ARM_VFP_MVFR0:
              val = fmrx(MVFR0);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_MVFR1:
              val = fmrx(MVFR1);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_FPSID:
              val = fmrx(FPSID);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user32(uaddr, &val, id);
      default:
              return -ENOENT;
      }

@@ -890,8 +934,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
      if (vfpid < num_fp_regs()) {
              if (KVM_REG_SIZE(id) != 8)
                      return -ENOENT;
-             return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
-                                  uaddr, id);
+             return reg_from_user64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+                                    uaddr, id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -900,28 +944,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
 
      switch (vfpid) {
      case KVM_REG_ARM_VFP_FPEXC:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
      case KVM_REG_ARM_VFP_FPSCR:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
      case KVM_REG_ARM_VFP_FPINST:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
      case KVM_REG_ARM_VFP_FPINST2:
-             return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
+             return reg_from_user32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
      /* These are invariant. */
      case KVM_REG_ARM_VFP_MVFR0:
-             if (reg_from_user(&val, uaddr, id))
+             if (reg_from_user32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(MVFR0))
                      return -EINVAL;
              return 0;
      case KVM_REG_ARM_VFP_MVFR1:
-             if (reg_from_user(&val, uaddr, id))
+             if (reg_from_user32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(MVFR1))
                      return -EINVAL;
              return 0;
      case KVM_REG_ARM_VFP_FPSID:
-             if (reg_from_user(&val, uaddr, id))
+             if (reg_from_user32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(FPSID))
                      return -EINVAL;

@@ -968,7 +1012,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
              return get_invariant_cp15(reg->id, uaddr);
 
      /* Note: copies two regs if size is 64 bit. */
-     return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
+     return reg_to_user32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

@@ -987,7 +1031,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
              return set_invariant_cp15(reg->id, uaddr);
 
      /* Note: copies two regs if size is 64 bit */
-     return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
+     return reg_from_user32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static unsigned int num_demux_regs(void)
--

Quoted 2 lines

1.8.1.4

Thanks,
--
Christoffer

Hi Christoffer,

Quoted 28 lines

Quoted 28 lines

arch/arm/kvm/coproc.c +19 -1

index 78c0885..64b2b94 100644
@@ -634,17 +634,61 @@ static struct coproc_reg invariant_cp15[] = {
      { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user64(u64 *val, const void __user *uaddr, u64 id)
+{
+     unsigned long regsize = KVM_REG_SIZE(id);
+     union {
+             u32     word;
+             u64     dword;
+     } tmp = {0};
+
+     if (copy_from_user(&tmp, uaddr, regsize) != 0)
+             return -EFAULT;
+
+     switch (regsize) {
+     case 4:
+             *val = tmp.word;
+             break;
+     case 8:
+             *val = tmp.dword;
+             break;
+     }

Quoted 8 lines

arch/arm/kvm/coproc.c static struct coproc_reg invariant_cp15[] = { · line 657

Quoted 2 lines

arch/arm/kvm/coproc.c +2 -0

+     return 0;
+}

instead of allowing this 'packing 32 bit values in 64 bit values and packing two 32 bit values in 64 bit values' kernel implementation'y stuff to be passed to these user-space ABI catering function, I really think you want to make reg_from_user64 deal with 64-bit registers, that is, BUG_ON(KVM_REG_SIZE(id) != 8.

Example of kernel storing value in u64 but register is 32bit is set/get_invariant_cp15. Note this function passes struct coproc_reg val field address into reg_to_user_xxx function. Since struct coproc_reg is generic structure used to describe both 32bit and 64bit coproc registers val type cannot be changed to u32. One may try to describe
val as union of u32 and u64 but I have concern that it would create bunch of 'if (KVM_REG_SIZE(id) == 4) { ... } else { ..}' pieces all over the place. I think it is better to have function that just can read/write 32bit register from/to u64 value in kernel.

According to recent clarifications of mmio.data array meaning - the mmio.data array should hold bytes as they would appear in memory. Vgic is little endian device. And in case of BE image kernel side that emulates vgic, holds data in BE form. So we need to byteswap cpu<->le32 vgic registers when we read/write them from mmio.data[].

Change has no effect in LE case because cpu already runs in le32.

Signed-off-by Victor Kamensky <[email protected]>

---
 virt/kvm/arm/vgic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

virt/kvm/arm/vgic.c +2 -2

index 685fc72..7e11458 100644
@@ -236,12 +236,12 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq)
 
 static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
 {
-	return *((u32 *)mmio->data) & mask;
+	return le32_to_cpu(*((u32 *)mmio->data)) & mask;
 }
 
 static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value)
 {
-	*((u32 *)mmio->data) = value & mask;
+	*((u32 *)mmio->data) = cpu_to_le32(value) & mask;
 }
 
 /**

Signature · 1 line

1.8.1.4

In case of status register E bit is not set (LE mode) and host runs in BE mode we need byteswap data, so read/write is emulated correctly.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm/include/asm/kvm_emulate.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

arch/arm/include/asm/kvm_emulate.h +18 -4

index 0fa90c9..69b7469 100644
@@ -185,9 +185,16 @@ static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu,
 		default:
 			return be32_to_cpu(data);
 		}
+	} else {
+		switch (len) {
+		case 1:
+			return data & 0xff;
+		case 2:
+			return le16_to_cpu(data & 0xffff);
+		default:
+			return le32_to_cpu(data);
+		}
 	}
-
-	return data;		/* Leave LE untouched */
 }
 
 static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,

@@ -203,9 +210,16 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
 		default:
 			return cpu_to_be32(data);
 		}
+	} else {
+		switch (len) {
+		case 1:
+			return data & 0xff;
+		case 2:
+			return cpu_to_le16(data & 0xffff);
+		default:
+			return cpu_to_le32(data);
+		}
 	}
-
-	return data;		/* Leave LE untouched */
 }
 
 #endif /* __ARM_KVM_EMULATE_H__ */

Signature · 1 line

1.8.1.4