[PATCH v2 0/7] armv7 BE kvm support

[PATCH v2 0/7] armv7 BE kvm support

[Victor Kamensky]

Hi Guys,

Here is second version of series that enables KVM support for V7 big endian
kernels. Mostly it deals with BE KVM host support. Marc Zyngier showed before 
with his patches how BE guest could run on top LE host. With these patches
BE guest runs on top of BE host. If Marc's kvmtool is used with few
additional changes I tested that BE host could run LE guest. Also I verified
that there were no regressions in BE guest on top of LE host case.

Note that posted series covers only kernel side changes. The changes were
tested inside of bigger setup with additional changes in qemu and kvmtool.
I will post those changes separately in proper aliases but for completeness 
sake Appendix A gives pointers to git repositories and branches with all
needed changes.

Changes since V1:
1) Patch that handles issue of including assembler.h into kvm .S file now
handled separately. It was posted before on mailing list [1]. This series
depend on it.

2) Incorporated most of Christoffer V1 review comments. The only thing 
I did not do wrt Christoffer's suggestion is possible rework of 'one_reg
coproc set and get BE fixes' patch. I think I put better explanation on
what my code does, and if it is still not good we will go from there.

3) Moved rr_lo_hi macro into arch/arm/include/asm/kvm_asm.h along the 
lines Christoffer suggested

4) Split 'fix KVM assembler files to work in BE case' patch into 4 smaller
one each handling one logic issue.

Thanks,
Victor

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/231432.html

Victor Kamensky (7):
  ARM: KVM: switch hypervisor into BE mode in case of BE host
  ARM: KVM: fix vgic V7 assembler code to work in BE image
  ARM: KVM: handle 64bit values passed to mrcc or from mcrr instructions
    in BE case
  ARM: KVM: __kvm_vcpu_run function return result fix in BE case
  ARM: KVM: one_reg coproc set and get BE fixes
  ARM: KVM: vgic mmio should hold data as LE bytes array in BE case
  ARM: KVM: MMIO support BE host running LE code

 arch/arm/include/asm/kvm_asm.h     | 23 +++++++++-
 arch/arm/include/asm/kvm_emulate.h | 22 +++++++--
 arch/arm/kvm/coproc.c              | 94 ++++++++++++++++++++++++++++----------
 arch/arm/kvm/init.S                |  7 ++-
 arch/arm/kvm/interrupts.S          |  9 +++-
 arch/arm/kvm/interrupts_head.S     | 32 +++++++++----
 virt/kvm/arm/vgic.c                |  4 +-
 7 files changed, 145 insertions(+), 46 deletions(-)

-- 
1.8.1.4

      Appendix A: Testing and Full Setup Description
      ----------------------------------------------

I) No mixed mode setup - i.e BE guest on BE host; and LE guest
on LE host tested to make sure no regressions.

KVM host and guest kernels:
  TC2 on top of Linus 3.13 (this patch series):
      git: git://git.linaro.org/people/victor.kamensky/linux-linaro-tracking-be.git
      branch: arm-be-kvm-3.13
      range: 7098f45..78419d0
      
  TC2 and Arndale on top of Linaro BE tree:
      git: git://git.linaro.org/people/victor.kamensky/linux-linaro-tracking-be.git
      branch: llct-be-20140211-kvm
      range: 308a752..ae47fa8

- TC1 kernels used as guests

qemu:
      git: git://git.linaro.org/people/victor.kamensky/qemu-be.git 
      branch: armv7be-v1
      description: changes to run qemu on armeb target; and other
      changes to work with be image on top of be host

kvmtool:
      git: git://git.linaro.org/people/victor.kamensky/linux-linaro-tracking-be.git
      branch: kvmtool-armv7be-v1
      description: minimal changes to build kvmtool for armeb target; and
      tiny change with virtio magic

II) Mixed mode setup all possible combinations within V7 (LE guest on BE host;
BE guest on LE host as Marc's setup tested to make sure no regressions) only
with kvmtool.

This work is based on Marc Zyngier's work that made BE guest to run on top
of LE host. For this setup special version of kvmtool should be used and
in addition I had to apply patch to guest kernel that would switch reading
virtio configs reads to be LE only, that is made on top of previous Rusty
Russell's changes. Effectively I just had to do very minor addition to make
LE guest to work on BE host, most of heavy lifting was done before by Marc.

KVM host kernels: as in previous setup

Guest TC1 kernels with LE virtio config patch:
      git: git://git.linaro.org/people/victor.kamensky/linux-linaro-tracking-be.git
      branch: virtio-leconfig-3.13-rc4

kvmtool:
      git: git://git.linaro.org/people/victor.kamensky/linux-linaro-tracking-be.git
      branch: kvmtool-mixed-v1
      description: based on git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
      branch kvm-arm64/kvmtool-be-on-le; adds missing include fix; above armeb target
      build patches; and one fix related to BE mode

qemu:
      git: git://git.linaro.org/people/victor.kamensky/qemu-be.git
      branch: armv7be-leconfig-v1
      description: change virtio-blk that so qemu could work with guest image
      where virtio leconfig is made; note it does not work in mixed mode; to do
      so qemu would need bunch of similar changes that Marc did in kvmtool

[PATCH v2 1/7] ARM: KVM: switch hypervisor into BE mode in case of BE host

[Victor Kamensky]

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

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 arch/arm/kvm/init.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
index 1b9844d..74f0718 100644
--- a/arch/arm/kvm/init.S
+++ b/arch/arm/kvm/init.S
@@ -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
 
-- 
1.8.1.4

[PATCH v2 1/7] ARM: KVM: switch hypervisor into BE mode in case of BE host

[Christoffer Dall]

On Tue, Feb 11, 2014 at 09:41:27PM -0800, Victor Kamensky wrote:
> Switch hypervisor to run in BE mode if image is compiled
> with CONFIG_CPU_BIG_ENDIAN.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
>  arch/arm/kvm/init.S | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
> index 1b9844d..74f0718 100644
> --- a/arch/arm/kvm/init.S
> +++ b/arch/arm/kvm/init.S
> @@ -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
>  
> -- 
> 1.8.1.4
> 

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

-Christoffer

[PATCH v2 1/7] ARM: KVM: switch hypervisor into BE mode in case of BE host

[Victor Kamensky]

On 18 March 2014 15:23, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Feb 11, 2014 at 09:41:27PM -0800, Victor Kamensky wrote:
>> Switch hypervisor to run in BE mode if image is compiled
>> with CONFIG_CPU_BIG_ENDIAN.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> ---
>>  arch/arm/kvm/init.S | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
>> index 1b9844d..74f0718 100644
>> --- a/arch/arm/kvm/init.S
>> +++ b/arch/arm/kvm/init.S
>> @@ -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
>>
>> --
>> 1.8.1.4
>>
>
> Won't splitting up the patches this way break bisectability?

I don't think so. I think if just only this patch applied BE image
built with CONFIG_VIRTUALIZATION=y and CONFIG_KVM=y will at
least boot. KVM will not work of course without remaining patches.

Currently for BE image CONFIG_VIRTUALIZATION and CONFIG_KVM
should be explicitly turned off - otherwise image won't boot.

As far as LE image concerned it is NOP change.

Or do you have any specific idea how it could break bisectability?

Thanks,
Victor

> -Christoffer

[PATCH v2 1/7] ARM: KVM: switch hypervisor into BE mode in case of BE host

[Christoffer Dall]

On Tue, Mar 18, 2014 at 03:23:35PM -0700, Christoffer Dall wrote:
> On Tue, Feb 11, 2014 at 09:41:27PM -0800, Victor Kamensky wrote:
> > Switch hypervisor to run in BE mode if image is compiled
> > with CONFIG_CPU_BIG_ENDIAN.
> > 
> > Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> > ---
> >  arch/arm/kvm/init.S | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
> > index 1b9844d..74f0718 100644
> > --- a/arch/arm/kvm/init.S
> > +++ b/arch/arm/kvm/init.S
> > @@ -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
> >  
> > -- 
> > 1.8.1.4
> > 
> 
> Won't splitting up the patches this way break bisectability?
> 
Second thought, scratch that, because BE support is not supposed to work
before applying these patches anyhow I guess.

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

-Christoffer

[PATCH v2 2/7] ARM: KVM: fix vgic V7 assembler code to work in BE image

[Victor Kamensky]

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 <victor.kamensky@linaro.org>
---
 arch/arm/kvm/interrupts_head.S | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 6f18695..1e9be2f 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -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
-- 
1.8.1.4

[PATCH v2 2/7] ARM: KVM: fix vgic V7 assembler code to work in BE image

[Christoffer Dall]

On Tue, Feb 11, 2014 at 09:41:28PM -0800, Victor Kamensky wrote:
> The vgic h/w registers are little endian; when asm code reads/writes

when BE asm code...

> 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 <victor.kamensky@linaro.org>
> ---
>  arch/arm/kvm/interrupts_head.S | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 6f18695..1e9be2f 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -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
> -- 
> 1.8.1.4
> 

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

[PATCH v2 3/7] ARM: KVM: handle 64bit values passed to mrcc or from mcrr instructions in BE case

[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.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 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(-)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 661da11..c6ae937 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -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 */
@@ -59,6 +59,25 @@
 #define ARM_EXCEPTION_FIQ	  6
 #define ARM_EXCEPTION_HVC	  7
 
+/*
+ * The rr_lo_hi macro swap pair of registers positions depending on
+ * current endianness. It is used in conjunction with ldrd and strd
+ * instructions that loads/store 64 bit value from/to memory to/from
+ * pair of registers which are used with the mrrc and mcrr instructions.
+ * The a1 parameter is register that typically holds lower address
+ * word (least significant word in LE, most significant in BE). The
+ * a2 parameter is register that holds higher address word. 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.
+ */
+#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;
diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
index 74f0718..2d10b2d 100644
--- a/arch/arm/kvm/init.S
+++ b/arch/arm/kvm/init.S
@@ -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
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index ddc1553..f0696bd 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -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
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 1e9be2f..3409ed6 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -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]
-- 
1.8.1.4

[PATCH v2 3/7] ARM: KVM: handle 64bit values passed to mrcc or from mcrr instructions in BE case

[Christoffer Dall]

On Tue, Feb 11, 2014 at 09:41:29PM -0800, Victor Kamensky wrote:
> 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 <victor.kamensky@linaro.org>
> ---
>  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(-)
> 
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 661da11..c6ae937 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -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 */
> @@ -59,6 +59,25 @@
>  #define ARM_EXCEPTION_FIQ	  6
>  #define ARM_EXCEPTION_HVC	  7
>  

Thanks for writing this comment below, a few language corrections:

> +/*
> + * The rr_lo_hi macro swap pair of registers positions depending on

swaps a pair of registers depending on

> + * current endianness. It is used in conjunction with ldrd and strd
> + * instructions that loads/store 64 bit value from/to memory to/from

that load/store a 64-bit value

> + * pair of registers which are used with the mrrc and mcrr instructions.

a pair of

> + * The a1 parameter is register that typically holds lower address
> + * word (least significant word in LE, most significant in BE). The
> + * a2 parameter is register that holds higher address word. 

I would rewrite this as:

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
> + * 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.

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;
> diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
> index 74f0718..2d10b2d 100644
> --- a/arch/arm/kvm/init.S
> +++ b/arch/arm/kvm/init.S
> @@ -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
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index ddc1553..f0696bd 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -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
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 1e9be2f..3409ed6 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -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]
> -- 
> 1.8.1.4
> 

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

[PATCH v2 4/7] ARM: KVM: __kvm_vcpu_run function return result fix in BE case

[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 <victor.kamensky@linaro.org>
---
 arch/arm/kvm/interrupts.S | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index f0696bd..5d27f7f 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -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
 
 /********************************************************************
-- 
1.8.1.4

[PATCH v2 4/7] ARM: KVM: __kvm_vcpu_run function return result fix in BE case

[Christoffer Dall]

On Tue, Feb 11, 2014 at 09:41:30PM -0800, Victor Kamensky wrote:
> 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 <victor.kamensky@linaro.org>
> ---
>  arch/arm/kvm/interrupts.S | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index f0696bd..5d27f7f 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -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
>  
>  /********************************************************************
> -- 
> 1.8.1.4
> 

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

[PATCH v2 5/7] ARM: KVM: one_reg coproc set and get BE fixes

[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_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 <victor.kamensky@linaro.org>
---
 arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 25 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 78c0885..64b2b94 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -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;
+	}
+	return 0;
+}
+
+/* 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)
-- 
1.8.1.4

[PATCH v2 5/7] ARM: KVM: one_reg coproc set and get BE fixes

[Alexander Graf]

On 12.02.2014, at 06:41, Victor Kamensky <victor.kamensky@linaro.org> wrote:

> 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 <victor.kamensky@linaro.org>

Do you think it'd be possible to converge to a single solution across PPC and ARM? On PPC we currently have "get_reg_val" and "set_reg_val" helpers that encode/decode reg structs for us. The actual copy_from_user and copy_to_user can be generic because we know the size of the access from the ONE_REG constant.


Alex

[PATCH v2 5/7] ARM: KVM: one_reg coproc set and get BE fixes

[Christoffer Dall]

On Tue, Feb 11, 2014 at 09:41:31PM -0800, Victor Kamensky wrote:
> 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

s/reigster/register/

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

> 
> 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 <victor.kamensky@linaro.org>
> ---
>  arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 69 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 78c0885..64b2b94 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -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;
> +	}
> +	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.

> +
> +/* 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)
> -- 
> 1.8.1.4
> 

Thanks,
-- 
Christoffer

[PATCH v2 5/7] ARM: KVM: one_reg coproc set and get BE fixes

[Victor Kamensky]

Hi Christoffer,

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

On 19 March 2014 18:11, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Feb 11, 2014 at 09:41:31PM -0800, Victor Kamensky wrote:
>> 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
>
> s/reigster/register/
>
>>                   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.
>
>>
>> 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 <victor.kamensky@linaro.org>
>> ---
>>  arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 69 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 78c0885..64b2b94 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -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;
>> +     }
>> +     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.

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.

Thanks,
Victor

>> +
>> +/* 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)
>> --
>> 1.8.1.4
>>
>
> Thanks,
> --
> Christoffer

[PATCH v2 5/7] ARM: KVM: one_reg coproc set and get BE fixes

[Christoffer Dall]

On Wed, Mar 19, 2014 at 06:48:07PM -0700, Victor Kamensky wrote:
> Hi Christoffer,
> 
> Appreciate your time and review comments.
> Please see response inline.
> 
> On 19 March 2014 18:11, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > On Tue, Feb 11, 2014 at 09:41:31PM -0800, Victor Kamensky wrote:
> >> 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
> >
> > s/reigster/register/
> >
> >>                   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.
> >
> >>
> >> 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 <victor.kamensky@linaro.org>
> >> ---
> >>  arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
> >>  1 file changed, 69 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> >> index 78c0885..64b2b94 100644
> >> --- a/arch/arm/kvm/coproc.c
> >> +++ b/arch/arm/kvm/coproc.c
> >> @@ -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;
> >> +     }
> >> +     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.
> 
> 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 ...

yeah, sorry, lots of stuff happening at Linaro Connect always.  I am all
for sharing this code.

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

I think you should have that as a preparatory patch series on which this
series depends.

Thanks,
-Christoffer

[PATCH v2 5/7] ARM: KVM: one_reg coproc set and get BE fixes

[Victor Kamensky]

Hi Christoffer,

In just posted update on ARM BE KVM series. I posted
new version of this patch [1] where I could not address
all below comments. Please see my responses inline

On 19 March 2014 18:11, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Feb 11, 2014 at 09:41:31PM -0800, Victor Kamensky wrote:
>> 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
>
> s/reigster/register/

Done

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

Done

>>
>> 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 <victor.kamensky@linaro.org>
>> ---
>>  arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 69 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 78c0885..64b2b94 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -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;
>> +     }
>> +     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.

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

Example of above is TTBR0 register. It is accessed by user land as 64bit
register but in kernel it is stored as two u32 register TTBR0_low and
TTBR0_high. Note code that stores and restores those does not treat as
one 64bit register instead it is a pair

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

I understand concern. I added comments around reg_to/from_user_xx functions
and I think I picked better name for functions. Hopefully it is enough if
this approach is followed.

Also I could be missing better way to address your comments,
if if you have ideas how it should be implemented please let me know.

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

I'll looked at approach that Alex suggested, I will describe issues with it
in separate email.

Thanks,
Victor

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/255856.html

>> +
>> +/* 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)
>> --
>> 1.8.1.4
>>
>
> Thanks,
> --
> Christoffer

[PATCH v2 5/7] ARM: KVM: one_reg coproc set and get BE fixes

[Christoffer Dall]

On Tue, May 13, 2014 at 01:11:34PM -0700, Victor Kamensky wrote:
> Hi Christoffer,
> 

[...]

> >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> >> index 78c0885..64b2b94 100644
> >> --- a/arch/arm/kvm/coproc.c
> >> +++ b/arch/arm/kvm/coproc.c
> >> @@ -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;
> >> +     }
> >> +     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.

I know what the kernel does internally.  I just think these functions
become conceptually very complicated to cater to one special case.  You
could deal with this in the caller.  Consider get_invariant_cp15:

static int get_invariant_cp15(u64 id, void __user *uaddr)
{
	struct coproc_params params;
	const struct coproc_reg *r;

	if (!index_to_params(id, &params))
		return -ENOENT;

	r = find_reg(&params, invariant_cp15, ARRAY_SIZE(invariant_cp15));
	if (!r)
		return -ENOENT;

	if (KVM_REG_SIZE(id) == 4) {
		u32 val = r->val;
		ret = reg_to_user32(uaddr, &val, id);
	} else if (KVM_REG_SIZE(id) == 8) {
		ret = reg_to_user32(uaddr, &r->val, id);
	}
	return ret;
}

Did you actually try this approach and see how the code looks?

-Christoffer

[PATCH v2 6/7] ARM: KVM: vgic mmio should hold data as LE bytes array in BE case

[Victor Kamensky]

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 <victor.kamensky@linaro.org>
---
 virt/kvm/arm/vgic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 685fc72..7e11458 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -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;
 }
 
 /**
-- 
1.8.1.4

[PATCH v2 6/7] ARM: KVM: vgic mmio should hold data as LE bytes array in BE case

[Christoffer Dall]

On Tue, Feb 11, 2014 at 09:41:32PM -0800, Victor Kamensky wrote:
> 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 <victor.kamensky@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 685fc72..7e11458 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -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;
>  }
>  
>  /**
> -- 
> 1.8.1.4
> 

I hate the fact that we have endianness handling code inside the vgic
emulation, I would strongly prefer that the interface to the vgic code
is a typed union, but, that being said, I haven't looked at how the
changes to the code would look to accomplish that.  You didn't reply to
my same comment last time around, but did you ahve a look?

The code here, with the ABI specification does look correct, so assuming
that ABI specification gets merged:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

[PATCH v2 7/7] ARM: KVM: MMIO support BE host running LE code

[Victor Kamensky]

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 <victor.kamensky@linaro.org>
---
 arch/arm/include/asm/kvm_emulate.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 0fa90c9..69b7469 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -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__ */
-- 
1.8.1.4

[PATCH v2 7/7] ARM: KVM: MMIO support BE host running LE code

[Christoffer Dall]

On Tue, Feb 11, 2014 at 09:41:33PM -0800, Victor Kamensky wrote:
> 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 <victor.kamensky@linaro.org>
> ---
>  arch/arm/include/asm/kvm_emulate.h | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 0fa90c9..69b7469 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -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__ */
> -- 
> 1.8.1.4
> 

Somehow in my head the way I think about this is

if (guest_be != host_be) {
	return swab(data);
}

but it's probably not more clear in terms of the actual code...

Anyway, given that the ABI clarification text gets merged:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>