ARM/ARM64: KVM: big endian host support

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

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

Reviewed-by Christoffer Dall <[email protected]>

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

arch/arm/kvm/init.S +1 -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

The vgic h/w registers are little endian; when BE 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]>

Reviewed-by Christoffer Dall <[email protected]>

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

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

index 76af9302..e627858 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))

@@ -420,6 +421,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]

@@ -439,6 +448,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

@@ -466,6 +476,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]

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.

Quoted 7 lines · Victor Kamensky

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.

So in this version you decided to keep the ordering of the TTBRX and TTBRX_high bits (high being most significant). I assume this is because of the trapping of VM registers introduced by mark and to maintain access_vm_reg()?

Any other changes since the last version?

If none, then for the content of this patch:

Reviewed-by Christoffer Dall <[email protected]>

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 | 18 ++++++++++++++++++
 arch/arm/kvm/init.S            |  4 ++--
 arch/arm/kvm/interrupts.S      |  4 ++--
 arch/arm/kvm/interrupts_head.S |  6 +++---
 4 files changed, 25 insertions(+), 7 deletions(-)

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

index 53b3c4a..3a67bec 100644
@@ -61,6 +61,24 @@
 #define ARM_EXCEPTION_FIQ	  6
 #define ARM_EXCEPTION_HVC	  7
 
+/*
+ * The rr_lo_hi macro swaps a pair of registers depending on
+ * current endianness. It is used in conjunction with ldrd and strd
+ * instructions that load/store a 64-bit value from/to memory to/from
+ * a pair of registers which are used with the mrrc and mcrr instructions.
+ * If used with the ldrd/strd instructions, the a1 parameter is the first
+ * source/destination register and the a2 parameter is the second
+ * source/destination register. Note that the ldrd/strd instructions
+ * already swap the bytes within the words correctly according to the
+ * endianness setting, but the order of the registers need to be effectively
+ * swapped when used with the mrrc/mcrr instructions.
+ */
+#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 0d68d40..24d4e65 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 +1 -2

index e627858..104977f 100644
@@ -520,7 +520,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]

@@ -560,12 +560,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

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]>

Acked-by Christoffer Dall <[email protected]>

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

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

index 24d4e65..7dfe9e4 100644

Quoted 15 lines · Victor Kamensky

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]>

Acked-by Christoffer Dall <[email protected]>

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

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

index 24d4e65..7dfe9e4 100644
@@ -199,8 +199,13 @@ after_vfp_restore:
 

arch/arm/kvm/interrupts.S after_vfp_restore: · line 208

Quoted 9 lines

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

      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__ */

Why using __ARMEB__ while the rest of the series is using CONFIG_CPU_ENDIAN_BE8?

arch/arm/kvm/interrupts.S after_vfp_restore: · line 211

Quoted 4 lines

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

      bx      lr                      @ return to IOCTL
 
 /********************************************************************

Aside from the above nit:

Acked-by Marc Zyngier <[email protected]>

        M.

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

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]>

Reviewed-by Christoffer Dall <[email protected]>

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

virt/kvm/arm/vgic.c +0 -1

index 56ff9be..529c336 100644
@@ -241,12 +241,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;

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]>

Reviewed-by Christoffer Dall <[email protected]>

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

arch/arm/include/asm/kvm_emulate.h +14 -2

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:

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_user_to_u64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to
user-land register memory; register size could be 4 or 8 bytes
ret_to_user_from_u32 - 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 u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

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

---
 arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 26 deletions(-)

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

index c58a351..5ca6582 100644

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

+static int reg_from_user_to_u64(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 +1 -1

+static int reg_from_user_to_u32(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;

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

+static int reg_to_user_from_u64(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)

arch/arm/kvm/coproc.c +23 -23

+static int reg_to_user_from_u32(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;

@@ -710,7 +776,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_user_from_u64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -726,7 +792,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_user_to_u64(&val, uaddr, id);
 	if (err)
 		return err;
 

@@ -894,8 +960,8 @@ 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],
-				   id);
+		return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+					    id);
 	}
 
 	/* FP control registers are all 32 bit. */

@@ -904,22 +970,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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(uaddr, &val, id);
 	case KVM_REG_ARM_VFP_MVFR1:
 		val = fmrx(MVFR1);
-		return reg_to_user(uaddr, &val, id);
+		return reg_to_user_from_u32(uaddr, &val, id);
 	case KVM_REG_ARM_VFP_FPSID:
 		val = fmrx(FPSID);
-		return reg_to_user(uaddr, &val, id);
+		return reg_to_user_from_u32(uaddr, &val, id);
 	default:
 		return -ENOENT;
 	}

@@ -938,8 +1004,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_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+					    uaddr, id);
 	}
 
 	/* FP control registers are all 32 bit. */

@@ -948,28 +1014,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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&val, uaddr, id))
 			return -EFAULT;
 		if (val != fmrx(FPSID))
 			return -EINVAL;

@@ -1016,7 +1082,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_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

@@ -1035,7 +1101,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_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static unsigned int num_demux_regs(void)

Signature · 1 line

1.8.1.4

Quoted 48 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:

reg_from_user_to_u64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to
user-land register memory; register size could be 4 or 8 bytes
ret_to_user_from_u32 - 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 u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

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

---
 arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 26 deletions(-)

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

index c58a351..5ca6582 100644

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

Quoted 10 lines

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

@@ -682,17 +682,83 @@ 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)
+/*
+ * Reads register value from user-land uaddr address into kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so user-land
+ * 4 bytes value will be copied into least significant word. Or register
+ * size could be 8 bytes, so function works as regular copy.
+ */

Reads a register value from a userspace address to a kernel u64 variable. Note that the id may still encode a register size of 4 bytes, in which case only 4 bytes will be copied from userspace into the least significant word of *val.

Quoted 18 lines

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

+static int reg_from_user_to_u64(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 718

Quoted 10 lines

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

+     }
+     return 0;
+}
+
+/*
+ * Reads register value from user-land uaddr address to kernel u32 value
+ * or array of two u32 values. I.e. it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */

Reads a register value from a userspace address to a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

Quoted 5 lines

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

+static int reg_from_user_to_u32(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;

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

Quoted 10 lines

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

      return 0;
 }
 
-static int reg_to_user(void __user *uaddr, const void *val, u64 id)
+/*
+ * Writes register value to user-land uaddr address from kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so only 4
+ * bytes from least significant word will by copied into uaddr address.
+ * In case of 8 bytes sized register it works as regular copy.
+ */

Writes a register value to a userspace address from a kernel u64 value. Note that the register size could be only 4 bytes, in which case only the least significant 4 bytes will be copied into to the userspace address.

Quoted 18 lines

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

+static int reg_to_user_from_u64(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)

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

Quoted 10 lines

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

+             return -EFAULT;
+     return 0;
+}
+
+/*
+ * Writes register value to user-land uaddr address from kernel u32 value
+ * or arra of two u32 values. I.e it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */

Writes a register value to a userspace address from a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

Quoted 131 lines

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

+static int reg_to_user_from_u32(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;

@@ -710,7 +776,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_user_from_u64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -726,7 +792,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_user_to_u64(&val, uaddr, id);
      if (err)
              return err;
 

@@ -894,8 +960,8 @@ 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],
-                                id);
+             return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+                                         id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -904,22 +970,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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_MVFR1:
              val = fmrx(MVFR1);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user_from_u32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_FPSID:
              val = fmrx(FPSID);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user_from_u32(uaddr, &val, id);
      default:
              return -ENOENT;
      }

@@ -938,8 +1004,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_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+                                         uaddr, id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -948,28 +1014,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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(FPSID))
                      return -EINVAL;

@@ -1016,7 +1082,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_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

@@ -1035,7 +1101,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_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static unsigned int num_demux_regs(void)
--

Quoted 2 lines

1.8.1.4

It's definitely an improvement with the new naming, and I do appreciate you taking the time to write comments. (Apologies for suggesting a raw rewrite, but there were a number of language issues with the proposed versions and it felt overly burdensome on you to ask you to address all of them).

However, I'm still not crazy about this overall approach (see separate mail on the old thread).

I'm curious, how many special cases are there really, and how many places would we have to introduce a conditional on the calling side?

Thanks,
-Christoffer

Quoted 294 lines · Christoffer Dall

Quoted 48 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:

reg_from_user_to_u64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to
user-land register memory; register size could be 4 or 8 bytes
ret_to_user_from_u32 - 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 u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

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

---
 arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 26 deletions(-)

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

index c58a351..5ca6582 100644

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

Quoted 10 lines

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

@@ -682,17 +682,83 @@ 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)
+/*
+ * Reads register value from user-land uaddr address into kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so user-land
+ * 4 bytes value will be copied into least significant word. Or register
+ * size could be 8 bytes, so function works as regular copy.
+ */

Reads a register value from a userspace address to a kernel u64 variable. Note that the id may still encode a register size of 4 bytes, in which case only 4 bytes will be copied from userspace into the least significant word of *val.

Quoted 18 lines

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

+static int reg_from_user_to_u64(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 718

Quoted 10 lines

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

+     }
+     return 0;
+}
+
+/*
+ * Reads register value from user-land uaddr address to kernel u32 value
+ * or array of two u32 values. I.e. it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */

Reads a register value from a userspace address to a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

Quoted 5 lines

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

+static int reg_from_user_to_u32(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;

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

Quoted 10 lines

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

      return 0;
 }
 
-static int reg_to_user(void __user *uaddr, const void *val, u64 id)
+/*
+ * Writes register value to user-land uaddr address from kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so only 4
+ * bytes from least significant word will by copied into uaddr address.
+ * In case of 8 bytes sized register it works as regular copy.
+ */

Writes a register value to a userspace address from a kernel u64 value. Note that the register size could be only 4 bytes, in which case only the least significant 4 bytes will be copied into to the userspace address.

Quoted 18 lines

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

+static int reg_to_user_from_u64(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)

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

Quoted 10 lines

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

+             return -EFAULT;
+     return 0;
+}
+
+/*
+ * Writes register value to user-land uaddr address from kernel u32 value
+ * or arra of two u32 values. I.e it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */

Writes a register value to a userspace address from a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

Quoted 131 lines

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

+static int reg_to_user_from_u32(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;

@@ -710,7 +776,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_user_from_u64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -726,7 +792,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_user_to_u64(&val, uaddr, id);
      if (err)
              return err;
 

@@ -894,8 +960,8 @@ 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],
-                                id);
+             return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+                                         id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -904,22 +970,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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_MVFR1:
              val = fmrx(MVFR1);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user_from_u32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_FPSID:
              val = fmrx(FPSID);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user_from_u32(uaddr, &val, id);
      default:
              return -ENOENT;
      }

@@ -938,8 +1004,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_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+                                         uaddr, id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -948,28 +1014,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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(FPSID))
                      return -EINVAL;

@@ -1016,7 +1082,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_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

@@ -1035,7 +1101,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_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static unsigned int num_demux_regs(void)
--

Quoted 2 lines

1.8.1.4

It's definitely an improvement with the new naming, and I do appreciate you taking the time to write comments. (Apologies for suggesting a raw rewrite, but there were a number of language issues with the proposed versions and it felt overly burdensome on you to ask you to address all of them).

Quoted 5 lines

However, I'm still not crazy about this overall approach (see separate mail on the old thread).

I'm curious, how many special cases are there really, and how many places would we have to introduce a conditional on the calling side?

I've just posted as RFC updated version of the patch. And yes, you are right u64 regsize 4 access happens only in one place, I fixed as you suggested and got rid of regsize == 4 in _u64 functions.

Reading/wring array of two u32 values as regsize == 8 still remains I don't know how to handle it more cleanly.

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_user_to_u64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to
user-land register memory; register size could be 4 or 8 bytes
ret_to_user_from_u32 - 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 u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

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

---
 arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 26 deletions(-)

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

index c58a351..5ca6582 100644
@@ -682,17 +682,83 @@ 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)
+/*
+ * Reads register value from user-land uaddr address into kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so user-land
+ * 4 bytes value will be copied into least significant word. Or register
+ * size could be 8 bytes, so function works as regular copy.
+ */
+static int reg_from_user_to_u64(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;
+	}
+	return 0;
+}
+
+/*
+ * Reads register value from user-land uaddr address to kernel u32 value
+ * or array of two u32 values. I.e. it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */
+static int reg_from_user_to_u32(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)
+/*
+ * Writes register value to user-land uaddr address from kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so only 4
+ * bytes from least significant word will by copied into uaddr address.
+ * In case of 8 bytes sized register it works as regular copy. 
+ */
+static int reg_to_user_from_u64(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;
+}
+
+/*
+ * Writes register value to user-land uaddr address from kernel u32 value
+ * or arra of two u32 values. I.e it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */
+static int reg_to_user_from_u32(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;

@@ -710,7 +776,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_user_from_u64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -726,7 +792,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_user_to_u64(&val, uaddr, id);
 	if (err)
 		return err;
 

@@ -894,8 +960,8 @@ 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],
-				   id);
+		return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+					    id);
 	}
 
 	/* FP control registers are all 32 bit. */

@@ -904,22 +970,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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(uaddr, &val, id);
 	case KVM_REG_ARM_VFP_MVFR1:
 		val = fmrx(MVFR1);
-		return reg_to_user(uaddr, &val, id);
+		return reg_to_user_from_u32(uaddr, &val, id);
 	case KVM_REG_ARM_VFP_FPSID:
 		val = fmrx(FPSID);
-		return reg_to_user(uaddr, &val, id);
+		return reg_to_user_from_u32(uaddr, &val, id);
 	default:
 		return -ENOENT;
 	}

@@ -938,8 +1004,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_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+					    uaddr, id);
 	}
 
 	/* FP control registers are all 32 bit. */

@@ -948,28 +1014,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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&val, uaddr, id))
 			return -EFAULT;
 		if (val != fmrx(FPSID))
 			return -EINVAL;

@@ -1016,7 +1082,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_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

Quoted 258 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:

reg_from_user_to_u64 - reads register from user-land to kernel 'u64 *val'
address; register size could be 4 or 8 bytes
reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val'
address; note it could be one or two 4 bytes registers
reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to
user-land register memory; register size could be 4 or 8 bytes
ret_to_user_from_u32 - 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 u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

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

---
 arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 26 deletions(-)

arch/arm/kvm/coproc.c +92 -26

index c58a351..5ca6582 100644
@@ -682,17 +682,83 @@ 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)
+/*
+ * Reads register value from user-land uaddr address into kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so user-land
+ * 4 bytes value will be copied into least significant word. Or register
+ * size could be 8 bytes, so function works as regular copy.
+ */
+static int reg_from_user_to_u64(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;
+     }
+     return 0;
+}
+
+/*
+ * Reads register value from user-land uaddr address to kernel u32 value
+ * or array of two u32 values. I.e. it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */
+static int reg_from_user_to_u32(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)
+/*
+ * Writes register value to user-land uaddr address from kernel u64 value
+ * given by val address. Note register size could be 4 bytes, so only 4
+ * bytes from least significant word will by copied into uaddr address.
+ * In case of 8 bytes sized register it works as regular copy.
+ */
+static int reg_to_user_from_u64(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;
+}
+
+/*
+ * Writes register value to user-land uaddr address from kernel u32 value
+ * or arra of two u32 values. I.e it may really copy two u32 registers
+ * when used with register which size is 8 bytes (for example V7 64
+ * bit registers like TTBR0/TTBR1).
+ */
+static int reg_to_user_from_u32(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;

@@ -710,7 +776,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_user_from_u64(uaddr, &r->val, id);
 }
 
 static int set_invariant_cp15(u64 id, void __user *uaddr)

@@ -726,7 +792,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_user_to_u64(&val, uaddr, id);
      if (err)
              return err;
 

@@ -894,8 +960,8 @@ 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],
-                                id);
+             return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
+                                         id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -904,22 +970,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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(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_user_from_u32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_MVFR1:
              val = fmrx(MVFR1);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user_from_u32(uaddr, &val, id);
      case KVM_REG_ARM_VFP_FPSID:
              val = fmrx(FPSID);
-             return reg_to_user(uaddr, &val, id);
+             return reg_to_user_from_u32(uaddr, &val, id);
      default:
              return -ENOENT;
      }

@@ -938,8 +1004,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_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
+                                         uaddr, id);
      }
 
      /* FP control registers are all 32 bit. */

@@ -948,28 +1014,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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&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_user_to_u32(&val, uaddr, id))
                      return -EFAULT;
              if (val != fmrx(FPSID))
                      return -EINVAL;

@@ -1016,7 +1082,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_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
 int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)

@@ -1035,7 +1101,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_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 

arch/arm/kvm/coproc.c int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) · line 1107

Quoted 2 lines

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

 static unsigned int num_demux_regs(void)

I really dislike this patch because of the asymmetric behaviour. Why can't we always use the "_u64" version?

Actually, why don't we use a construct similar to what we have for mmio_{read,write}_buf? This would give us a unified access method.

Thanks,

        M.

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

In case of guest CPU running in 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]>

Reviewed-by Christoffer Dall <[email protected]>

---
 arch/arm64/include/asm/kvm_emulate.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

arch/arm64/include/asm/kvm_emulate.h +15 -0

index dd8ecfc..fdc3e21 100644
@@ -213,6 +213,17 @@ static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu,
 		default:
 			return be64_to_cpu(data);
 		}
+	} else {
+		switch (len) {
+		case 1:
+			return data & 0xff;
+		case 2:
+			return le16_to_cpu(data & 0xffff);
+		case 4:
+			return le32_to_cpu(data & 0xffffffff);
+		default:
+			return le64_to_cpu(data);
+		}
 	}
 
 	return data;		/* Leave LE untouched */

@@ -233,6 +244,17 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
 		default:
 			return cpu_to_be64(data);
 		}
+	} else {
+		switch (len) {
+		case 1:
+			return data & 0xff;

esr_el2 field of struct kvm_vcpu_fault_info has u32 type. It should be stored as word. Current code works in LE case because existing puts least significant word of x1 into esr_el2, and it puts most significant work of x1 into next

field, which accidentally is OK because it is updated again by next instruction. But existing code breaks in BE case.

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

Acked-by Christoffer Dall <[email protected]>

---
 arch/arm64/kvm/hyp.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kvm/hyp.S +0 -0

index 2c56012..0620691 100644

Fix vgic_bitmap_get_reg function to return 'right' word address of 'unsigned long' bitmap value in case of BE 64bit image.

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

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

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

index 529c336..d0a1a24 100644

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

+#if defined(CONFIG_CPU_BIG_ENDIAN) && BITS_PER_LONG == 64
+#define REG_OFFSET_SWIZZLE	1
+#else
+#define REG_OFFSET_SWIZZLE	0
+#endif
+
 static u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
 				int cpuid, u32 offset)
 {
 	offset >>= 2;
 	if (!offset)
-		return x->percpu[cpuid].reg;
+		return x->percpu[cpuid].reg + (offset ^ REG_OFFSET_SWIZZLE);
 	else
-		return x->shared.reg + offset - 1;
+		return x->shared.reg + ((offset - 1) ^ REG_OFFSET_SWIZZLE);
 }
 
 static int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,

Signature · 1 line

1.8.1.4

On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as one 'unsigned long *' bit fields, which has 64bit size. So we need to swap least significant word with most significant word when code reads those registers from h/w.

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

---
 arch/arm64/kvm/hyp.S | 7 +++++++
 1 file changed, 7 insertions(+)

arch/arm64/kvm/hyp.S +7 -0

index 0620691..5035b41 100644
@@ -415,10 +415,17 @@ CPU_BE(	rev	w11, w11 )
 	str	w4, [x3, #VGIC_CPU_HCR]
 	str	w5, [x3, #VGIC_CPU_VMCR]
 	str	w6, [x3, #VGIC_CPU_MISR]
+#ifndef CONFIG_CPU_BIG_ENDIAN
 	str	w7, [x3, #VGIC_CPU_EISR]
 	str	w8, [x3, #(VGIC_CPU_EISR + 4)]
 	str	w9, [x3, #VGIC_CPU_ELRSR]
 	str	w10, [x3, #(VGIC_CPU_ELRSR + 4)]
+#else
+	str	w7, [x3, #(VGIC_CPU_EISR + 4)]
+	str	w8, [x3, #VGIC_CPU_EISR]
+	str	w9, [x3, #(VGIC_CPU_ELRSR + 4)]
+	str	w10, [x3, #VGIC_CPU_ELRSR]
+#endif
 	str	w11, [x3, #VGIC_CPU_APR]
 
 	/* Clear GICH_HCR */

Signature · 1 line

1.8.1.4

Quoted 35 lines · Victor Kamensky

On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as one 'unsigned long *' bit fields, which has 64bit size. So we need to swap least significant word with most significant word when code reads those registers from h/w.

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

---
 arch/arm64/kvm/hyp.S | 7 +++++++
 1 file changed, 7 insertions(+)

arch/arm64/kvm/hyp.S +7 -0

index 0620691..5035b41 100644
@@ -415,10 +415,17 @@ CPU_BE(	rev	w11, w11 )
 	str	w4, [x3, #VGIC_CPU_HCR]
 	str	w5, [x3, #VGIC_CPU_VMCR]
 	str	w6, [x3, #VGIC_CPU_MISR]
+#ifndef CONFIG_CPU_BIG_ENDIAN
 	str	w7, [x3, #VGIC_CPU_EISR]
 	str	w8, [x3, #(VGIC_CPU_EISR + 4)]
 	str	w9, [x3, #VGIC_CPU_ELRSR]
 	str	w10, [x3, #(VGIC_CPU_ELRSR + 4)]
+#else
+	str	w7, [x3, #(VGIC_CPU_EISR + 4)]
+	str	w8, [x3, #VGIC_CPU_EISR]
+	str	w9, [x3, #(VGIC_CPU_ELRSR + 4)]
+	str	w10, [x3, #VGIC_CPU_ELRSR]
+#endif
 	str	w11, [x3, #VGIC_CPU_APR]
 
 	/* Clear GICH_HCR */

Signature · 1 line

1.8.1.4

Hmm, the data structure doesn't actually clearly define this as bitmap, but the architecture clearly defines this as two registers. It feels like this critical code is getting over complicated to cater for the fact that we can use a convenient for_each_set_bit() function when we process a maintenance interrupt.

Marc, what are your thoughts on this? Change vgic_process_maintenance and clearly document the array on struct vgic_cpu, or go ahead with this change?

This patch addresses issue of reading and writing V8 sys registers in BE case. Since only register size function deals with is 8 bytes, existing code works in both little and big endian cases. Removed comment about little endian. Added BUG_ON that register size should be always 8 bytes.

If these functions would ever need to support both 8 bytes and 4 bytes register sizes to deals with them in endian agnostic way code should do something along these lines:

       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;
       }

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

---
 arch/arm64/kvm/sys_regs.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

arch/arm64/kvm/sys_regs.c +0 -0

index 0324458..060c3a9 100644
@@ -776,18 +776,25 @@ static struct sys_reg_desc invariant_sys_regs[] = {
 	  NULL, get_ctr_el0 },

Hi Victor,

Quoted 38 lines · Victor Kamensky

This patch addresses issue of reading and writing V8 sys registers in BE case. Since only register size function deals with is 8 bytes, existing code works in both little and big endian cases. Removed comment about little endian. Added BUG_ON that register size should be always 8 bytes.

If these functions would ever need to support both 8 bytes and 4 bytes register sizes to deals with them in endian agnostic way code should do something along these lines:

       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;
       }

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

---
 arch/arm64/kvm/sys_regs.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

arch/arm64/kvm/sys_regs.c +0 -0

index 0324458..060c3a9 100644
@@ -776,18 +776,25 @@ static struct sys_reg_desc invariant_sys_regs[] = {
        NULL, get_ctr_el0 },

arch/arm64/kvm/sys_regs.c static struct sys_reg_desc invariant_sys_regs[] = { · line 783

Quoted 10 lines

arch/arm64/kvm/sys_regs.c +4 -3

 };
 
-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user(u64 *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)
+     unsigned long regsize = KVM_REG_SIZE(id);
+
+     BUG_ON(regsize != 8);

I haven't had time to review this series just yet, but this bit just sends chivers down my spine.

regsize is derived from id, which comes from a struct one_reg, which is directly provided by userspace. Here, you're trusting the luser to give you 8 as a size, and panic the kernel if not.

As much as I'd like to qualify this as only being a slightly undesirable effect, I think it deserves a NAK.

Thanks,

        M.

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

On 14 May 2014 01:45, Marc Zyngier <[email protected]> wrote:

Quoted 13 lines

arch/arm64/kvm/sys_regs.c static struct sys_reg_desc invariant_sys_regs[] = { · line 783

Quoted 9 lines

arch/arm64/kvm/sys_regs.c +4 -3

-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user(u64 *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)
+     unsigned long regsize = KVM_REG_SIZE(id);
+
+     BUG_ON(regsize != 8);

I haven't had time to review this series just yet, but this bit just sends chivers down my spine.

Quoted 6 lines

regsize is derived from id, which comes from a struct one_reg, which is directly provided by userspace. Here, you're trusting the luser to give you 8 as a size, and panic the kernel if not.

As much as I'd like to qualify this as only being a slightly undesirable effect, I think it deserves a NAK.

Fair enough. I agree. Good catch! I was following on Christoffer's comments at [1], but I have not thought it through. Please advise should I come back to previous version as in [2] or just ignore any sizes other than 8 without having BUG_ON?

Thanks,
Victor

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/241815.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/231891.html

Fix isssue with 32bit guests running on top of BE KVM host. Guest state is retored with double word read operations. Within the high

and low word data is already byteswap. This code effectively swaps

two words within 64bit value.

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

---
 arch/arm64/kvm/sys_regs.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/arm64/kvm/sys_regs.c +12 -2

index 060c3a9..4438b47 100644
@@ -51,6 +51,16 @@ static u32 cache_levels;
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
 #define CSSELR_MAX 12
 
+/* Word access indexes for 64bit vm accessor */
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define CP15_REG_MSW_INDEX 0
+#define CP15_REG_LSW_INDEX 1
+#else
+#define CP15_REG_MSW_INDEX 1
+#define CP15_REG_LSW_INDEX 0
+#endif
+
+
 /* Which cache CCSIDR represents depends on CSSELR value. */
 static u32 get_ccsidr(u32 csselr)
 {

@@ -137,9 +147,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
 	if (!p->is_aarch32) {
 		vcpu_sys_reg(vcpu, r->reg) = val;
 	} else {
-		vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
+		vcpu_cp15(vcpu, r->reg + CP15_REG_LSW_INDEX) = val & 0xffffffffUL;
 		if (!p->is_32bit)
-			vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+			vcpu_cp15(vcpu, r->reg + CP15_REG_MSW_INDEX) = val >> 32;
 	}
 	return true;
 }

Signature · 1 line

1.8.1.4

Quoted 2 lines · Victor Kamensky

Fix isssue with 32bit guests running on top of BE KVM host. Guest state is retored with double word read operations. Within the high

restored (spell check should catch this).

"Guest state is restored with double word read operations." I don't know what this sentence is supposed to tell me.

Quoted 1 line

and low word data is already byteswap. This code effectively swaps

"data is already byteswap" is not English. data is already byteswapped?

Quoted 37 lines

two words within 64bit value.

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

---
 arch/arm64/kvm/sys_regs.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/arm64/kvm/sys_regs.c +12 -2

index 060c3a9..4438b47 100644
@@ -51,6 +51,16 @@ static u32 cache_levels;
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
 #define CSSELR_MAX 12
 
+/* Word access indexes for 64bit vm accessor */
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define CP15_REG_MSW_INDEX 0
+#define CP15_REG_LSW_INDEX 1
+#else
+#define CP15_REG_MSW_INDEX 1
+#define CP15_REG_LSW_INDEX 0
+#endif
+
+
 /* Which cache CCSIDR represents depends on CSSELR value. */
 static u32 get_ccsidr(u32 csselr)
 {

@@ -137,9 +147,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
      if (!p->is_aarch32) {
              vcpu_sys_reg(vcpu, r->reg) = val;
      } else {
-             vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
+             vcpu_cp15(vcpu, r->reg + CP15_REG_LSW_INDEX) = val & 0xffffffffUL;
              if (!p->is_32bit)
-                     vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+                     vcpu_cp15(vcpu, r->reg + CP15_REG_MSW_INDEX) = val >> 32;

Quoted 6 lines

arch/arm64/kvm/sys_regs.c +0 -1

      }
      return true;
 }
--

1.8.1.4

I really don't like this. If anything I feel like it should be abstracted inside vcpu_cp15,

but wouldn't it be cleaner to do something
along the lines of:

u64 *regstore = (u64 *)vcpu->arch.ctxt.cp15[r->reg]; if (p->is_32bit)

        val &= 0xffffffffUL;

*regstore = val;

From b0a7793b03d9c62f7b9c53a317cb2d19a75c935b Mon Sep 17 00:00:00 2001

But I think the thing that bothers me in general with all these patches is that they deal specially with a lot of situations where the data structure was designed specifically to make the code easy to read, and now it just becomes a really complicated mess. Have you carefully considered other options, redesigning the data structure layout etc.?

-Christoffer

Quoted 50 lines · Christoffer Dall

Quoted 2 lines · Victor Kamensky

Fix isssue with 32bit guests running on top of BE KVM host. Guest state is retored with double word read operations. Within the high

restored (spell check should catch this).

"Guest state is restored with double word read operations." I don't know what this sentence is supposed to tell me.

Quoted 1 line

and low word data is already byteswap. This code effectively swaps

"data is already byteswap" is not English. data is already byteswapped?

Quoted 37 lines

two words within 64bit value.

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

---
 arch/arm64/kvm/sys_regs.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/arm64/kvm/sys_regs.c +12 -2

index 060c3a9..4438b47 100644
@@ -51,6 +51,16 @@ static u32 cache_levels;
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
 #define CSSELR_MAX 12
 
+/* Word access indexes for 64bit vm accessor */
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define CP15_REG_MSW_INDEX 0
+#define CP15_REG_LSW_INDEX 1
+#else
+#define CP15_REG_MSW_INDEX 1
+#define CP15_REG_LSW_INDEX 0
+#endif
+
+
 /* Which cache CCSIDR represents depends on CSSELR value. */
 static u32 get_ccsidr(u32 csselr)
 {

@@ -137,9 +147,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
      if (!p->is_aarch32) {
              vcpu_sys_reg(vcpu, r->reg) = val;
      } else {
-             vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
+             vcpu_cp15(vcpu, r->reg + CP15_REG_LSW_INDEX) = val & 0xffffffffUL;
              if (!p->is_32bit)
-                     vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+                     vcpu_cp15(vcpu, r->reg + CP15_REG_MSW_INDEX) = val >> 32;

Quoted 8 lines

Quoted 6 lines

arch/arm64/kvm/sys_regs.c +0 -1

      }
      return true;
 }
--

1.8.1.4

I really don't like this. If anything I feel like it should be abstracted inside vcpu_cp15,

good point, please see revised proposal below

Quoted 7 lines

but wouldn't it be cleaner to do something
along the lines of:

u64 *regstore = (u64 *)vcpu->arch.ctxt.cp15[r->reg]; if (p->is_32bit)

        val &= 0xffffffffUL;

*regstore = val;

I don't think above will be correct. The way I read it the following hypothetical instructions sequence

mcrr p15, 0, r6, r7, c2 @ TTBR 0
mcr p15, 0, r6, c2, c0, 0 @ 32bit TTBR 0

will get TTBR 0 high word to 0, when mrc instruction executes, but it should be left unchanged in case of 'p->is_32bit'

How about something like following? Is it move into right direction?

From b0a7793b03d9c62f7b9c53a317cb2d19a75c935b Mon Sep 17 00:00:00 2001
From: Victor Kamensky <[email protected]>
Date: Mon, 12 May 2014 13:57:21 -0700
Subject: [PATCH] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit
guest

Fix issue with 32bit guests running on top of BE KVM host. Indexes of high and low words of 64bit cp15 register are swapped in case of big endian code, since 64bit cp15 state is restored or saved with double word write or read instruction.

Define helper macros to access high low words of 64bit cp15 register.

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

---
 arch/arm64/include/asm/kvm_host.h | 8 ++++++++
 arch/arm64/kvm/sys_regs.c         | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

arch/arm64/include/asm/kvm_host.h +8 -0

index 0a1d697..e9d2e11 100644
@@ -140,6 +140,14 @@ struct kvm_vcpu_arch {
 #define vcpu_sys_reg(v,r)    ((v)->arch.ctxt.sys_regs[(r)])
 #define vcpu_cp15(v,r)        ((v)->arch.ctxt.cp15[(r)])
 
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
+#else
+#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
+#endif
+
 struct kvm_vm_stat {
     u32 remote_tlb_flush;
 };

arch/arm64/kvm/sys_regs.c +2 -2

index a13e7e7..b243e07 100644
@@ -137,9 +137,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
     if (!p->is_aarch32) {
         vcpu_sys_reg(vcpu, r->reg) = val;
     } else {
-        vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
+        vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL;
         if (!p->is_32bit)
-            vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+            vcpu_cp15_64_high(vcpu, r->reg) = val >> 32;
     }
     return true;
 }

Signature · 1 line

1.8.1.4

Quoted 6 lines

But I think the thing that bothers me in general with all these patches is that they deal specially with a lot of situations where the data structure was designed specifically to make the code easy to read, and now it just becomes a really complicated mess. Have you carefully considered other options, redesigning the data structure layout etc.?

I have considered different options and I am open for suggestions. Bytesswaps quite often could be done in different places but achieve the same result. In several cases I initially developed patch that deals with BE issue in one way and reworked to make it more compact less intrusive. For example in this particular case order of high and low words of 64bit cp15 register could be kept the same in BE/LE cases but code that save/restore it could be changed to do byteswap. My opinion that proposed option is more clean.

Quoted 138 lines · Victor Kamensky

Quoted 50 lines · Christoffer Dall

Quoted 2 lines · Victor Kamensky

Fix isssue with 32bit guests running on top of BE KVM host. Guest state is retored with double word read operations. Within the high

restored (spell check should catch this).

"Guest state is restored with double word read operations." I don't know what this sentence is supposed to tell me.

Quoted 1 line

and low word data is already byteswap. This code effectively swaps

"data is already byteswap" is not English. data is already byteswapped?

Quoted 37 lines

two words within 64bit value.

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

---
 arch/arm64/kvm/sys_regs.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/arm64/kvm/sys_regs.c +12 -2

index 060c3a9..4438b47 100644
@@ -51,6 +51,16 @@ static u32 cache_levels;
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
 #define CSSELR_MAX 12
 
+/* Word access indexes for 64bit vm accessor */
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define CP15_REG_MSW_INDEX 0
+#define CP15_REG_LSW_INDEX 1
+#else
+#define CP15_REG_MSW_INDEX 1
+#define CP15_REG_LSW_INDEX 0
+#endif
+
+
 /* Which cache CCSIDR represents depends on CSSELR value. */
 static u32 get_ccsidr(u32 csselr)
 {

@@ -137,9 +147,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
      if (!p->is_aarch32) {
              vcpu_sys_reg(vcpu, r->reg) = val;
      } else {
-             vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
+             vcpu_cp15(vcpu, r->reg + CP15_REG_LSW_INDEX) = val & 0xffffffffUL;
              if (!p->is_32bit)
-                     vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+                     vcpu_cp15(vcpu, r->reg + CP15_REG_MSW_INDEX) = val >> 32;

Quoted 8 lines

Quoted 6 lines

arch/arm64/kvm/sys_regs.c +0 -1

      }
      return true;
 }
--

1.8.1.4

I really don't like this. If anything I feel like it should be abstracted inside vcpu_cp15,

good point, please see revised proposal below

Quoted 7 lines

but wouldn't it be cleaner to do something
along the lines of:

u64 *regstore = (u64 *)vcpu->arch.ctxt.cp15[r->reg]; if (p->is_32bit)

        val &= 0xffffffffUL;

*regstore = val;

I don't think above will be correct. The way I read it the following hypothetical instructions sequence

mcrr p15, 0, r6, r7, c2 @ TTBR 0
mcr p15, 0, r6, c2, c0, 0 @ 32bit TTBR 0

will get TTBR 0 high word to 0, when mrc instruction executes, but it should be left unchanged in case of 'p->is_32bit'

How about something like following? Is it move into right direction?

From b0a7793b03d9c62f7b9c53a317cb2d19a75c935b Mon Sep 17 00:00:00 2001
From: Victor Kamensky <[email protected]>
Date: Mon, 12 May 2014 13:57:21 -0700
Subject: [PATCH] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit
guest

Fix issue with 32bit guests running on top of BE KVM host. Indexes of high and low words of 64bit cp15 register are swapped in case of big endian code, since 64bit cp15 state is restored or saved with double word write or read instruction.

Define helper macros to access high low words of 64bit cp15 register.

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

---
 arch/arm64/include/asm/kvm_host.h | 8 ++++++++
 arch/arm64/kvm/sys_regs.c         | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

arch/arm64/include/asm/kvm_host.h +8 -0

index 0a1d697..e9d2e11 100644
@@ -140,6 +140,14 @@ struct kvm_vcpu_arch {
 #define vcpu_sys_reg(v,r)    ((v)->arch.ctxt.sys_regs[(r)])
 #define vcpu_cp15(v,r)        ((v)->arch.ctxt.cp15[(r)])
 
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
+#else
+#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
+#endif
+
 struct kvm_vm_stat {
     u32 remote_tlb_flush;
 };

arch/arm64/kvm/sys_regs.c +2 -3

index a13e7e7..b243e07 100644
@@ -137,9 +137,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
     if (!p->is_aarch32) {
         vcpu_sys_reg(vcpu, r->reg) = val;
     } else {
-        vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
+        vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL;
         if (!p->is_32bit)
-            vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+            vcpu_cp15_64_high(vcpu, r->reg) = val >> 32;
     }
     return true;
 }
--

Quoted 1 line

1.8.1.4

This is definitely much better than the original patch.

Quoted 7 lines

Quoted 6 lines

But I think the thing that bothers me in general with all these patches is that they deal specially with a lot of situations where the data structure was designed specifically to make the code easy to read, and now it just becomes a really complicated mess. Have you carefully considered other options, redesigning the data structure layout etc.?

Quoted 10 lines

I have considered different options and I am open for suggestions. Bytesswaps quite often could be done in different places but achieve the same result. In several cases I initially developed patch that deals with BE issue in one way and reworked to make it more compact less intrusive. For example in this particular case order of high and low words of 64bit cp15 register could be kept the same in BE/LE cases but code that save/restore it could be changed to do byteswap. My opinion that proposed option is more clean.