* [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support
@ 2014-06-12 16:29 Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 01/14] ARM: KVM: switch hypervisor into BE mode in case of BE host Victor Kamensky
` (13 more replies)
0 siblings, 14 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi Christoffer, Marc,
Could you please look at updated ARM BE KVM series. We worked
separately on specific patches, I believe now they should look
OK.
Changes since previous V3 version of series [1]:
o Updated Review-by Acked-by tags where appropriate
o 'ARM: KVM: __kvm_vcpu_run function return result fix in BE case'
patch changed __ARMEB__ to CONFIG_CPU_ENDIAN_BE8 as Marc suggested
o 'ARM: KVM: one_reg coproc set and get BE fixes' patch was
siginificantly reworked based on separate thread discussion with
Christoffer - In short summry now we fix only oddball places where
reg_[from|to]_user function is used in case target variable
mismatching register size
o 'ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case'
incorporated Christoffer's review comments about code comments
(spelling, grammar, etc)
o 'ARM64: KVM: set and get of sys registers in BE case' changed
patch siginificantly based on separate email thread feedback from
Christoffer and Marc - now patch just removes comment about
reg_[to|form]_user being little endian, since it handles only
64 bit register size current code is endian agnostic.
o 'ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest'
incorporoated Christoffer's review comments - two macros
vcpu_cp15_64_high and vcpu_cp15_64_low now introduced which
defined differently between BE and LE cases
Series were tested on top of v3.15. On V7 4 combinations, BE/LE
host x BE/LE guest, were tested with TC2. On V8 8 combinations,
BE/LE host x BE/LE guest x 32/64 bit guest, were tested on
Fast models.
Thanks,
Victor
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/255849.html
Victor Kamensky (14):
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: vgic mmio should hold data as LE bytes array in BE case
ARM: KVM: MMIO support BE host running LE code
ARM: KVM: one_reg coproc set and get BE fixes
ARM: KVM: enable KVM in Kconfig on big-endian systems
ARM64: KVM: MMIO support BE host running LE code
ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word
ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case
ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
ARM64: KVM: set and get of sys registers in BE case
ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest
arch/arm/include/asm/kvm_asm.h | 18 ++++++++
arch/arm/include/asm/kvm_emulate.h | 22 ++++++++--
arch/arm/kvm/Kconfig | 2 +-
arch/arm/kvm/coproc.c | 84 ++++++++++++++++++++++++++++++++----
arch/arm/kvm/init.S | 7 ++-
arch/arm/kvm/interrupts.S | 9 +++-
arch/arm/kvm/interrupts_head.S | 20 +++++++--
arch/arm64/include/asm/kvm_emulate.h | 22 ++++++++++
arch/arm64/include/asm/kvm_host.h | 8 ++++
arch/arm64/kvm/hyp.S | 9 +++-
arch/arm64/kvm/sys_regs.c | 10 ++---
virt/kvm/arm/vgic.c | 28 ++++++++++--
12 files changed, 207 insertions(+), 32 deletions(-)
--
1.8.1.4
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 01/14] ARM: KVM: switch hypervisor into BE mode in case of BE host
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 02/14] ARM: KVM: fix vgic V7 assembler code to work in BE image Victor Kamensky
` (12 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
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>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 02/14] ARM: KVM: fix vgic V7 assembler code to work in BE image
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 01/14] ARM: KVM: switch hypervisor into BE mode in case of BE host Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 03/14] ARM: KVM: handle 64bit values passed to mrcc or from mcrr instructions in BE case Victor Kamensky
` (11 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
The vgic h/w registers are little endian; when BE asm code
reads/writes from/to them, it needs to do byteswap after/before.
Byteswap code uses ARM_BE8 wrapper to add swap only if
CONFIG_CPU_BIG_ENDIAN is configured.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
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 76af9302..e627858 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))
@@ -420,6 +421,14 @@ vcpu .req r0 @ vcpu pointer always in r0
ldr r8, [r2, #GICH_ELRSR0]
ldr r9, [r2, #GICH_ELRSR1]
ldr r10, [r2, #GICH_APR]
+ARM_BE8(rev r3, r3 )
+ARM_BE8(rev r4, r4 )
+ARM_BE8(rev r5, r5 )
+ARM_BE8(rev r6, r6 )
+ARM_BE8(rev r7, r7 )
+ARM_BE8(rev r8, r8 )
+ARM_BE8(rev r9, r9 )
+ARM_BE8(rev r10, r10 )
str r3, [r11, #VGIC_CPU_HCR]
str r4, [r11, #VGIC_CPU_VMCR]
@@ -439,6 +448,7 @@ vcpu .req r0 @ vcpu pointer always in r0
add r3, r11, #VGIC_CPU_LR
ldr r4, [r11, #VGIC_CPU_NR_LR]
1: ldr r6, [r2], #4
+ARM_BE8(rev r6, r6 )
str r6, [r3], #4
subs r4, r4, #1
bne 1b
@@ -466,6 +476,9 @@ vcpu .req r0 @ vcpu pointer always in r0
ldr r3, [r11, #VGIC_CPU_HCR]
ldr r4, [r11, #VGIC_CPU_VMCR]
ldr r8, [r11, #VGIC_CPU_APR]
+ARM_BE8(rev r3, r3 )
+ARM_BE8(rev r4, r4 )
+ARM_BE8(rev r8, r8 )
str r3, [r2, #GICH_HCR]
str r4, [r2, #GICH_VMCR]
@@ -476,6 +489,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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 03/14] ARM: KVM: handle 64bit values passed to mrcc or from mcrr instructions in BE case
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 01/14] ARM: KVM: switch hypervisor into BE mode in case of BE host Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 02/14] ARM: KVM: fix vgic V7 assembler code to work in BE image Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 04/14] ARM: KVM: __kvm_vcpu_run function return result fix " Victor Kamensky
` (10 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
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>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm/include/asm/kvm_asm.h | 18 ++++++++++++++++++
arch/arm/kvm/init.S | 4 ++--
arch/arm/kvm/interrupts.S | 4 ++--
arch/arm/kvm/interrupts_head.S | 6 +++---
4 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 53b3c4a..3a67bec 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -61,6 +61,24 @@
#define ARM_EXCEPTION_FIQ 6
#define ARM_EXCEPTION_HVC 7
+/*
+ * The rr_lo_hi macro swaps a pair of registers depending on
+ * current endianness. It is used in conjunction with ldrd and strd
+ * instructions that load/store a 64-bit value from/to memory to/from
+ * a pair of registers which are used with the mrrc and mcrr instructions.
+ * If used with the ldrd/strd instructions, the a1 parameter is the first
+ * source/destination register and the a2 parameter is the second
+ * source/destination register. Note that the ldrd/strd instructions
+ * already swap the bytes within the words correctly according to the
+ * endianness setting, but the order of the registers need to be effectively
+ * swapped when used with the mrrc/mcrr instructions.
+ */
+#ifdef CONFIG_CPU_ENDIAN_BE8
+#define rr_lo_hi(a1, a2) a2, a1
+#else
+#define rr_lo_hi(a1, a2) a1, a2
+#endif
+
#ifndef __ASSEMBLY__
struct kvm;
struct kvm_vcpu;
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 0d68d40..24d4e65 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 e627858..104977f 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -520,7 +520,7 @@ ARM_BE8(rev r6, r6 )
mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL
isb
- mrrc p15, 3, r2, r3, c14 @ CNTV_CVAL
+ mrrc p15, 3, rr_lo_hi(r2, r3), c14 @ CNTV_CVAL
ldr r4, =VCPU_TIMER_CNTV_CVAL
add r5, vcpu, r4
strd r2, r3, [r5]
@@ -560,12 +560,12 @@ ARM_BE8(rev r6, r6 )
ldr r2, [r4, #KVM_TIMER_CNTVOFF]
ldr r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
- mcrr p15, 4, r2, r3, c14 @ CNTVOFF
+ 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 04/14] ARM: KVM: __kvm_vcpu_run function return result fix in BE case
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (2 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 03/14] ARM: KVM: handle 64bit values passed to mrcc or from mcrr instructions in BE case Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 05/14] ARM: KVM: vgic mmio should hold data as LE bytes array " Victor Kamensky
` (9 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
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>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
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 24d4e65..01dcb0e 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 CONFIG_CPU_ENDIAN_BE8
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 /* CONFIG_CPU_ENDIAN_BE8 */
bx lr @ return to IOCTL
/********************************************************************
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 05/14] ARM: KVM: vgic mmio should hold data as LE bytes array in BE case
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (3 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 04/14] ARM: KVM: __kvm_vcpu_run function return result fix " Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 06/14] ARM: KVM: MMIO support BE host running LE code Victor Kamensky
` (8 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
According to recent clarifications of mmio.data array meaning -
the mmio.data array should hold bytes as they would appear in
memory. Vgic is little endian device. And in case of BE image
kernel side that emulates vgic, holds data in BE form. So we
need to byteswap cpu<->le32 vgic registers when we read/write them
from mmio.data[].
Change has no effect in LE case because cpu already runs in le32.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
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 56ff9be..529c336 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -241,12 +241,12 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq)
static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
{
- return *((u32 *)mmio->data) & mask;
+ 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 06/14] ARM: KVM: MMIO support BE host running LE code
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (4 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 05/14] ARM: KVM: vgic mmio should hold data as LE bytes array " Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 07/14] ARM: KVM: one_reg coproc set and get BE fixes Victor Kamensky
` (7 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
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>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 07/14] ARM: KVM: one_reg coproc set and get BE fixes
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (5 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 06/14] ARM: KVM: MMIO support BE host running LE code Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-12 16:30 ` [PATCH v4 08/14] ARM: KVM: enable KVM in Kconfig on big-endian systems Victor Kamensky
` (6 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
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.
In order to work in endian agnostic way reg_from_user to reg_to_user functions
should copy register value only to kernel variable with size that matches
register size. In few place where size mismatch existed fix issue on macro
caller side.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
arch/arm/kvm/coproc.c | 84 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 75 insertions(+), 9 deletions(-)
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index c58a351..bcc9a0f 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -44,6 +44,30 @@ static u32 cache_levels;
/* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
#define CSSELR_MAX 12
+/*
+ * kvm_vcpu_arch.cp15 holds cp15 registers as an array of u32, but some
+ * of cp15 registers can be viewed either as couple of two u32 registers
+ * or one u64 register. Current u64 register encoding is that least
+ * significant u32 word is followed by most significant u32 word.
+ */
+static inline void vcpu_cp15_reg64_set(struct kvm_vcpu *vcpu,
+ const struct coproc_reg *r,
+ u64 val)
+{
+ vcpu->arch.cp15[r->reg] = val & 0xffffffff;
+ vcpu->arch.cp15[r->reg + 1] = val >> 32;
+}
+
+static inline u64 vcpu_cp15_reg64_get(struct kvm_vcpu *vcpu,
+ const struct coproc_reg *r)
+{
+ u64 val;
+ val = vcpu->arch.cp15[r->reg + 1];
+ val = val << 32;
+ val = val | vcpu->arch.cp15[r->reg];
+ return val;
+}
+
int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run)
{
kvm_inject_undefined(vcpu);
@@ -682,17 +706,23 @@ static struct coproc_reg invariant_cp15[] = {
{ CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
};
+/*
+ * Reads a register value from a userspace address to a kernel
+ * variable. Make sure that register size matches sizeof(*__val).
+ */
static int reg_from_user(void *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;
}
+/*
+ * Writes a register value to a userspace address from a kernel variable.
+ * Make sure that register size matches sizeof(*__val).
+ */
static int reg_to_user(void __user *uaddr, const void *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;
@@ -702,6 +732,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
{
struct coproc_params params;
const struct coproc_reg *r;
+ int ret;
if (!index_to_params(id, ¶ms))
return -ENOENT;
@@ -710,7 +741,14 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
if (!r)
return -ENOENT;
- return reg_to_user(uaddr, &r->val, id);
+ ret = -ENOENT;
+ if (KVM_REG_SIZE(id) == 4) {
+ u32 val = r->val;
+ ret = reg_to_user(uaddr, &val, id);
+ } else if (KVM_REG_SIZE(id) == 8) {
+ ret = reg_to_user(uaddr, &r->val, id);
+ }
+ return ret;
}
static int set_invariant_cp15(u64 id, void __user *uaddr)
@@ -718,7 +756,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
struct coproc_params params;
const struct coproc_reg *r;
int err;
- u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
+ u64 val;
if (!index_to_params(id, ¶ms))
return -ENOENT;
@@ -726,7 +764,15 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
if (!r)
return -ENOENT;
- err = reg_from_user(&val, uaddr, id);
+ err = -ENOENT;
+ if (KVM_REG_SIZE(id) == 4) {
+ u32 val32;
+ err = reg_from_user(&val32, uaddr, id);
+ if (!err)
+ val = val32;
+ } else if (KVM_REG_SIZE(id) == 8) {
+ err = reg_from_user(&val, uaddr, id);
+ }
if (err)
return err;
@@ -1004,6 +1050,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
const struct coproc_reg *r;
void __user *uaddr = (void __user *)(long)reg->addr;
+ int ret;
if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
return demux_c15_get(reg->id, uaddr);
@@ -1015,14 +1062,23 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
if (!r)
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);
+ ret = -ENOENT;
+ if (KVM_REG_SIZE(reg->id) == 8) {
+ u64 val;
+ val = vcpu_cp15_reg64_get(vcpu, r);
+ ret = reg_to_user(uaddr, &val, reg->id);
+ } else if (KVM_REG_SIZE(reg->id) == 4) {
+ ret = reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
+ }
+
+ return ret;
}
int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
const struct coproc_reg *r;
void __user *uaddr = (void __user *)(long)reg->addr;
+ int ret;
if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
return demux_c15_set(reg->id, uaddr);
@@ -1034,8 +1090,18 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
if (!r)
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);
+ ret = -ENOENT;
+ if (KVM_REG_SIZE(reg->id) == 8) {
+ u64 val;
+ ret = reg_from_user(&val, uaddr, reg->id);
+ if (!ret) {
+ vcpu_cp15_reg64_set(vcpu, r, val);
+ }
+ } else if (KVM_REG_SIZE(reg->id) == 4) {
+ ret = reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
+ }
+
+ return ret;
}
static unsigned int num_demux_regs(void)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 08/14] ARM: KVM: enable KVM in Kconfig on big-endian systems
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (6 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 07/14] ARM: KVM: one_reg coproc set and get BE fixes Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-12 16:30 ` [PATCH v4 09/14] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
` (5 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
Previous patches addresses ARMV7 big-endian virtualiztion,
kvm related issues, so enable ARM_VIRT_EXT for big-endian
now.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
arch/arm/kvm/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 4be5bb1..466bd29 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -23,7 +23,7 @@ config KVM
select HAVE_KVM_CPU_RELAX_INTERCEPT
select KVM_MMIO
select KVM_ARM_HOST
- depends on ARM_VIRT_EXT && ARM_LPAE && !CPU_BIG_ENDIAN
+ depends on ARM_VIRT_EXT && ARM_LPAE
---help---
Support hosting virtualized guest machines. You will also
need to select one or more of the processor modules below.
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 09/14] ARM64: KVM: MMIO support BE host running LE code
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (7 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 08/14] ARM: KVM: enable KVM in Kconfig on big-endian systems Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 10/14] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word Victor Kamensky
` (4 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
In case of guest CPU running in LE mode and host runs in
BE mode we need byteswap data, so read/write is emulated correctly.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm64/include/asm/kvm_emulate.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index dd8ecfc..fdc3e21 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -213,6 +213,17 @@ static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu,
default:
return be64_to_cpu(data);
}
+ } else {
+ switch (len) {
+ case 1:
+ return data & 0xff;
+ case 2:
+ return le16_to_cpu(data & 0xffff);
+ case 4:
+ return le32_to_cpu(data & 0xffffffff);
+ default:
+ return le64_to_cpu(data);
+ }
}
return data; /* Leave LE untouched */
@@ -233,6 +244,17 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
default:
return cpu_to_be64(data);
}
+ } else {
+ switch (len) {
+ case 1:
+ return data & 0xff;
+ case 2:
+ return cpu_to_le16(data & 0xffff);
+ case 4:
+ return cpu_to_le32(data & 0xffffffff);
+ default:
+ return cpu_to_le64(data);
+ }
}
return data; /* Leave LE untouched */
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 10/14] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (8 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 09/14] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 11/14] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
` (3 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
esr_el2 field of struct kvm_vcpu_fault_info has u32 type.
It should be stored as word. Current code works in LE case
because existing puts least significant word of x1 into
esr_el2, and it puts most significant work of x1 into next
field, which accidentally is OK because it is updated again
by next instruction. But existing code breaks in BE case.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm64/kvm/hyp.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 2c56012..0620691 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -824,7 +824,7 @@ el1_trap:
mrs x2, far_el2
2: mrs x0, tpidr_el2
- str x1, [x0, #VCPU_ESR_EL2]
+ str w1, [x0, #VCPU_ESR_EL2]
str x2, [x0, #VCPU_FAR_EL2]
str x3, [x0, #VCPU_HPFAR_EL2]
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 11/14] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (9 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 10/14] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-12 16:30 ` [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
` (2 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
Fix vgic_bitmap_get_reg function to return 'right' word address of
'unsigned long' bitmap value in case of BE 64bit image.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
virt/kvm/arm/vgic.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 529c336..b4ffd82 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -101,14 +101,34 @@ static u32 vgic_nr_lr;
static unsigned int vgic_maint_irq;
+/*
+ * struct vgic_bitmap contains unions that provide two views of
+ * the same data. In one case it is an array of registers of
+ * u32's, and in the other case it is a bitmap of unsigned
+ * longs.
+ *
+ * This does not work on 64-bit BE systems, because the bitmap access
+ * will store two consecutive 32-bit words with the higher-addressed
+ * register's bits at the lower index and the lower-addressed register's
+ * bits at the higher index.
+ *
+ * Therefore, swizzle the register index when accessing the 32-bit word
+ * registers to access the right register's value.
+ */
+#if defined(CONFIG_CPU_BIG_ENDIAN) && BITS_PER_LONG == 64
+#define REG_OFFSET_SWIZZLE 1
+#else
+#define REG_OFFSET_SWIZZLE 0
+#endif
+
static u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
int cpuid, u32 offset)
{
offset >>= 2;
if (!offset)
- return x->percpu[cpuid].reg;
+ return x->percpu[cpuid].reg + (offset ^ REG_OFFSET_SWIZZLE);
else
- return x->shared.reg + offset - 1;
+ return x->shared.reg + ((offset - 1) ^ REG_OFFSET_SWIZZLE);
}
static int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (10 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 11/14] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-12 16:30 ` [PATCH v4 13/14] ARM64: KVM: set and get of sys registers " Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest Victor Kamensky
13 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
one 'unsigned long *' bit fields, which has 64bit size. So we need to
swap least significant word with most significant word when code reads
those registers from h/w.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
arch/arm64/kvm/hyp.S | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 0620691..5035b41 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
str w4, [x3, #VGIC_CPU_HCR]
str w5, [x3, #VGIC_CPU_VMCR]
str w6, [x3, #VGIC_CPU_MISR]
+#ifndef CONFIG_CPU_BIG_ENDIAN
str w7, [x3, #VGIC_CPU_EISR]
str w8, [x3, #(VGIC_CPU_EISR + 4)]
str w9, [x3, #VGIC_CPU_ELRSR]
str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
+#else
+ str w7, [x3, #(VGIC_CPU_EISR + 4)]
+ str w8, [x3, #VGIC_CPU_EISR]
+ str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
+ str w10, [x3, #VGIC_CPU_ELRSR]
+#endif
str w11, [x3, #VGIC_CPU_APR]
/* Clear GICH_HCR */
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 13/14] ARM64: KVM: set and get of sys registers in BE case
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (11 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest Victor Kamensky
13 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
Since size of all sys registers is always 8 bytes. Current
code is actually endian agnostic. Just clean it up a bit.
Removed comment about little endian. Change type of pointer
from 'void *' to 'u64 *' to enforce stronger type checking.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm64/kvm/sys_regs.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0324458..8e65e31 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -776,17 +776,15 @@ static struct sys_reg_desc invariant_sys_regs[] = {
NULL, get_ctr_el0 },
};
-static int reg_from_user(void *val, const void __user *uaddr, u64 id)
+static int reg_from_user(u64 *val, const void __user *uaddr, u64 id)
{
- /* This Just Works because we are little endian. */
if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
return -EFAULT;
return 0;
}
-static int reg_to_user(void __user *uaddr, const void *val, u64 id)
+static int reg_to_user(void __user *uaddr, const u64 *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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
` (12 preceding siblings ...)
2014-06-12 16:30 ` [PATCH v4 13/14] ARM64: KVM: set and get of sys registers " Victor Kamensky
@ 2014-06-12 16:30 ` Victor Kamensky
2014-06-14 15:05 ` Christoffer Dall
13 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-12 16:30 UTC (permalink / raw)
To: linux-arm-kernel
Fix issue with 32bit guests running on top of BE KVM host.
Indexes of high and low words of 64bit cp15 register are
swapped in case of big endian code, since 64bit cp15 state is
restored or saved with double word write or read instruction.
Define helper macros to access high low words of 64bit cp15
register.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
arch/arm64/include/asm/kvm_host.h | 8 ++++++++
arch/arm64/kvm/sys_regs.c | 4 ++--
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0a1d697..e9d2e11 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -140,6 +140,14 @@ struct kvm_vcpu_arch {
#define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)])
#define vcpu_cp15(v,r) ((v)->arch.ctxt.cp15[(r)])
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
+#else
+#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
+#endif
+
struct kvm_vm_stat {
u32 remote_tlb_flush;
};
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8e65e31..71aa9b0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -137,9 +137,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
if (!p->is_aarch32) {
vcpu_sys_reg(vcpu, r->reg) = val;
} else {
- vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
+ vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL;
if (!p->is_32bit)
- vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+ vcpu_cp15_64_high(vcpu, r->reg) = val >> 32;
}
return true;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 07/14] ARM: KVM: one_reg coproc set and get BE fixes
2014-06-12 16:30 ` [PATCH v4 07/14] ARM: KVM: one_reg coproc set and get BE fixes Victor Kamensky
@ 2014-06-14 15:04 ` Christoffer Dall
0 siblings, 0 replies; 29+ messages in thread
From: Christoffer Dall @ 2014-06-14 15:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 12, 2014 at 09:30:06AM -0700, 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.
>
> In order to work in endian agnostic way reg_from_user to reg_to_user functions
> should copy register value only to kernel variable with size that matches
> register size. In few place where size mismatch existed fix issue on macro
> caller side.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
> arch/arm/kvm/coproc.c | 84 +++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 75 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index c58a351..bcc9a0f 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -44,6 +44,30 @@ static u32 cache_levels;
> /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
> #define CSSELR_MAX 12
>
> +/*
> + * kvm_vcpu_arch.cp15 holds cp15 registers as an array of u32, but some
> + * of cp15 registers can be viewed either as couple of two u32 registers
> + * or one u64 register. Current u64 register encoding is that least
> + * significant u32 word is followed by most significant u32 word.
> + */
> +static inline void vcpu_cp15_reg64_set(struct kvm_vcpu *vcpu,
> + const struct coproc_reg *r,
> + u64 val)
> +{
> + vcpu->arch.cp15[r->reg] = val & 0xffffffff;
> + vcpu->arch.cp15[r->reg + 1] = val >> 32;
> +}
> +
> +static inline u64 vcpu_cp15_reg64_get(struct kvm_vcpu *vcpu,
> + const struct coproc_reg *r)
> +{
> + u64 val;
> + val = vcpu->arch.cp15[r->reg + 1];
> + val = val << 32;
> + val = val | vcpu->arch.cp15[r->reg];
> + return val;
> +}
> +
> int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> kvm_inject_undefined(vcpu);
> @@ -682,17 +706,23 @@ static struct coproc_reg invariant_cp15[] = {
> { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
> };
>
> +/*
> + * Reads a register value from a userspace address to a kernel
> + * variable. Make sure that register size matches sizeof(*__val).
> + */
> static int reg_from_user(void *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;
> }
>
> +/*
> + * Writes a register value to a userspace address from a kernel variable.
> + * Make sure that register size matches sizeof(*__val).
> + */
> static int reg_to_user(void __user *uaddr, const void *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;
> @@ -702,6 +732,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
> {
> struct coproc_params params;
> const struct coproc_reg *r;
> + int ret;
>
> if (!index_to_params(id, ¶ms))
> return -ENOENT;
> @@ -710,7 +741,14 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
> if (!r)
> return -ENOENT;
>
> - return reg_to_user(uaddr, &r->val, id);
> + ret = -ENOENT;
> + if (KVM_REG_SIZE(id) == 4) {
> + u32 val = r->val;
> + ret = reg_to_user(uaddr, &val, id);
> + } else if (KVM_REG_SIZE(id) == 8) {
> + ret = reg_to_user(uaddr, &r->val, id);
> + }
> + return ret;
> }
>
> static int set_invariant_cp15(u64 id, void __user *uaddr)
> @@ -718,7 +756,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
> struct coproc_params params;
> const struct coproc_reg *r;
> int err;
> - u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
> + u64 val;
>
> if (!index_to_params(id, ¶ms))
> return -ENOENT;
> @@ -726,7 +764,15 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
> if (!r)
> return -ENOENT;
>
> - err = reg_from_user(&val, uaddr, id);
> + err = -ENOENT;
> + if (KVM_REG_SIZE(id) == 4) {
> + u32 val32;
> + err = reg_from_user(&val32, uaddr, id);
> + if (!err)
> + val = val32;
> + } else if (KVM_REG_SIZE(id) == 8) {
> + err = reg_from_user(&val, uaddr, id);
> + }
> if (err)
> return err;
>
> @@ -1004,6 +1050,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> {
> const struct coproc_reg *r;
> void __user *uaddr = (void __user *)(long)reg->addr;
> + int ret;
>
> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
> return demux_c15_get(reg->id, uaddr);
> @@ -1015,14 +1062,23 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> if (!r)
> 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);
> + ret = -ENOENT;
> + if (KVM_REG_SIZE(reg->id) == 8) {
> + u64 val;
> + val = vcpu_cp15_reg64_get(vcpu, r);
> + ret = reg_to_user(uaddr, &val, reg->id);
> + } else if (KVM_REG_SIZE(reg->id) == 4) {
> + ret = reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
> + }
> +
> + return ret;
> }
>
> int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> {
> const struct coproc_reg *r;
> void __user *uaddr = (void __user *)(long)reg->addr;
> + int ret;
>
> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
> return demux_c15_set(reg->id, uaddr);
> @@ -1034,8 +1090,18 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> if (!r)
> 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);
> + ret = -ENOENT;
> + if (KVM_REG_SIZE(reg->id) == 8) {
> + u64 val;
> + ret = reg_from_user(&val, uaddr, reg->id);
> + if (!ret) {
> + vcpu_cp15_reg64_set(vcpu, r, val);
> + }
> + } else if (KVM_REG_SIZE(reg->id) == 4) {
> + ret = reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
> + }
> +
> + return ret;
> }
>
> static unsigned int num_demux_regs(void)
> --
> 1.8.1.4
>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 08/14] ARM: KVM: enable KVM in Kconfig on big-endian systems
2014-06-12 16:30 ` [PATCH v4 08/14] ARM: KVM: enable KVM in Kconfig on big-endian systems Victor Kamensky
@ 2014-06-14 15:04 ` Christoffer Dall
0 siblings, 0 replies; 29+ messages in thread
From: Christoffer Dall @ 2014-06-14 15:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 12, 2014 at 09:30:07AM -0700, Victor Kamensky wrote:
> Previous patches addresses ARMV7 big-endian virtualiztion,
> kvm related issues, so enable ARM_VIRT_EXT for big-endian
> now.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 11/14] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case
2014-06-12 16:30 ` [PATCH v4 11/14] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
@ 2014-06-14 15:04 ` Christoffer Dall
2014-06-21 9:58 ` Marc Zyngier
0 siblings, 1 reply; 29+ messages in thread
From: Christoffer Dall @ 2014-06-14 15:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 12, 2014 at 09:30:10AM -0700, Victor Kamensky wrote:
> Fix vgic_bitmap_get_reg function to return 'right' word address of
> 'unsigned long' bitmap value in case of BE 64bit image.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
> virt/kvm/arm/vgic.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 529c336..b4ffd82 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -101,14 +101,34 @@ static u32 vgic_nr_lr;
>
> static unsigned int vgic_maint_irq;
>
> +/*
> + * struct vgic_bitmap contains unions that provide two views of
> + * the same data. In one case it is an array of registers of
> + * u32's, and in the other case it is a bitmap of unsigned
> + * longs.
> + *
> + * This does not work on 64-bit BE systems, because the bitmap access
> + * will store two consecutive 32-bit words with the higher-addressed
> + * register's bits at the lower index and the lower-addressed register's
> + * bits at the higher index.
> + *
> + * Therefore, swizzle the register index when accessing the 32-bit word
> + * registers to access the right register's value.
> + */
> +#if defined(CONFIG_CPU_BIG_ENDIAN) && BITS_PER_LONG == 64
> +#define REG_OFFSET_SWIZZLE 1
> +#else
> +#define REG_OFFSET_SWIZZLE 0
> +#endif
> +
> static u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
> int cpuid, u32 offset)
> {
> offset >>= 2;
> if (!offset)
> - return x->percpu[cpuid].reg;
> + return x->percpu[cpuid].reg + (offset ^ REG_OFFSET_SWIZZLE);
> else
> - return x->shared.reg + offset - 1;
> + return x->shared.reg + ((offset - 1) ^ REG_OFFSET_SWIZZLE);
> }
>
> static int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,
> --
> 1.8.1.4
>
I had wished there was a cleaner way to abstract this, but I can't think
of a cleaner solution, and restructuring the whole thing would be very
invasive.
I'm curious to hear from Marc on this one, but you can add:
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
because it looks correct.
Thanks for picking up my suggestion on the commenting.
-Christoffer
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-12 16:30 ` [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
@ 2014-06-14 15:04 ` Christoffer Dall
2014-06-14 15:42 ` Victor Kamensky
0 siblings, 1 reply; 29+ messages in thread
From: Christoffer Dall @ 2014-06-14 15:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
> one 'unsigned long *' bit fields, which has 64bit size. So we need to
> swap least significant word with most significant word when code reads
> those registers from h/w.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
> arch/arm64/kvm/hyp.S | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 0620691..5035b41 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
> str w4, [x3, #VGIC_CPU_HCR]
> str w5, [x3, #VGIC_CPU_VMCR]
> str w6, [x3, #VGIC_CPU_MISR]
> +#ifndef CONFIG_CPU_BIG_ENDIAN
> str w7, [x3, #VGIC_CPU_EISR]
> str w8, [x3, #(VGIC_CPU_EISR + 4)]
> str w9, [x3, #VGIC_CPU_ELRSR]
> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
> +#else
> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
> + str w8, [x3, #VGIC_CPU_EISR]
> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
> + str w10, [x3, #VGIC_CPU_ELRSR]
> +#endif
> str w11, [x3, #VGIC_CPU_APR]
>
> /* Clear GICH_HCR */
> --
> 1.8.1.4
>
I thought Marc had something here which allowed you to deal with the
conversion in the accessor functions and avoid this patch?
-Christoffer
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest
2014-06-12 16:30 ` [PATCH v4 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest Victor Kamensky
@ 2014-06-14 15:05 ` Christoffer Dall
2014-06-19 5:43 ` Victor Kamensky
0 siblings, 1 reply; 29+ messages in thread
From: Christoffer Dall @ 2014-06-14 15:05 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 12, 2014 at 09:30:13AM -0700, Victor Kamensky wrote:
> Fix issue with 32bit guests running on top of BE KVM host.
> Indexes of high and low words of 64bit cp15 register are
> swapped in case of big endian code, since 64bit cp15 state is
> restored or saved with double word write or read instruction.
>
> Define helper macros to access high low words of 64bit cp15
> register.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
> arch/arm64/include/asm/kvm_host.h | 8 ++++++++
> arch/arm64/kvm/sys_regs.c | 4 ++--
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 0a1d697..e9d2e11 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -140,6 +140,14 @@ struct kvm_vcpu_arch {
> #define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)])
> #define vcpu_cp15(v,r) ((v)->arch.ctxt.cp15[(r)])
>
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
> +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
> +#else
> +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
> +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
> +#endif
> +
> struct kvm_vm_stat {
> u32 remote_tlb_flush;
> };
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8e65e31..71aa9b0 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -137,9 +137,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
> if (!p->is_aarch32) {
> vcpu_sys_reg(vcpu, r->reg) = val;
> } else {
> - vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
> + vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL;
> if (!p->is_32bit)
> - vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
> + vcpu_cp15_64_high(vcpu, r->reg) = val >> 32;
> }
> return true;
> }
> --
> 1.8.1.4
>
I thought there was a consensus here about handling 64-bit accesses
through the 64-bit values with the vcpu_sys_reg() interface? Did you
give up on this for a particular reason?
-Christoffer
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-14 15:04 ` Christoffer Dall
@ 2014-06-14 15:42 ` Victor Kamensky
2014-06-14 15:47 ` Christoffer Dall
0 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-14 15:42 UTC (permalink / raw)
To: linux-arm-kernel
On 14 June 2014 08:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
>> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
>> one 'unsigned long *' bit fields, which has 64bit size. So we need to
>> swap least significant word with most significant word when code reads
>> those registers from h/w.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> ---
>> arch/arm64/kvm/hyp.S | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 0620691..5035b41 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
>> str w4, [x3, #VGIC_CPU_HCR]
>> str w5, [x3, #VGIC_CPU_VMCR]
>> str w6, [x3, #VGIC_CPU_MISR]
>> +#ifndef CONFIG_CPU_BIG_ENDIAN
>> str w7, [x3, #VGIC_CPU_EISR]
>> str w8, [x3, #(VGIC_CPU_EISR + 4)]
>> str w9, [x3, #VGIC_CPU_ELRSR]
>> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
>> +#else
>> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
>> + str w8, [x3, #VGIC_CPU_EISR]
>> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
>> + str w10, [x3, #VGIC_CPU_ELRSR]
>> +#endif
>> str w11, [x3, #VGIC_CPU_APR]
>>
>> /* Clear GICH_HCR */
>> --
>> 1.8.1.4
>>
> I thought Marc had something here which allowed you to deal with the
> conversion in the accessor functions and avoid this patch?
Christoffer, I appreciate your review comments.
I think I was missing something. Yes, Marc mentioned in [1] about
his new changes in vgic3 series. But just after rereading it now, I
realized that he was suggesting to pick up his commits and add
them to this series. Is it my right understanding that they should
be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed
as well. I am concerned that I don't understand all dependencies
and impact of those. Wondering about other way around. When vgic3
series introduced could we just back off above change and do it in
new right way?
[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html
[2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
[3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
[4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html
Other question: I was testing all this directly on vanilla v3.15, should I
use some other armkvm specific integration branch to make sure it works
with all other in a queue armkvm changes.
In mean time I will try to pick up [4], [2], and [3] into v3.15 and see
how it goes.
Thanks,
Victor
> -Christoffer
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-14 15:42 ` Victor Kamensky
@ 2014-06-14 15:47 ` Christoffer Dall
2014-06-19 3:46 ` Victor Kamensky
0 siblings, 1 reply; 29+ messages in thread
From: Christoffer Dall @ 2014-06-14 15:47 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jun 14, 2014 at 08:42:58AM -0700, Victor Kamensky wrote:
> On 14 June 2014 08:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
> >> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
> >> one 'unsigned long *' bit fields, which has 64bit size. So we need to
> >> swap least significant word with most significant word when code reads
> >> those registers from h/w.
> >>
> >> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> >> ---
> >> arch/arm64/kvm/hyp.S | 7 +++++++
> >> 1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> >> index 0620691..5035b41 100644
> >> --- a/arch/arm64/kvm/hyp.S
> >> +++ b/arch/arm64/kvm/hyp.S
> >> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
> >> str w4, [x3, #VGIC_CPU_HCR]
> >> str w5, [x3, #VGIC_CPU_VMCR]
> >> str w6, [x3, #VGIC_CPU_MISR]
> >> +#ifndef CONFIG_CPU_BIG_ENDIAN
> >> str w7, [x3, #VGIC_CPU_EISR]
> >> str w8, [x3, #(VGIC_CPU_EISR + 4)]
> >> str w9, [x3, #VGIC_CPU_ELRSR]
> >> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
> >> +#else
> >> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
> >> + str w8, [x3, #VGIC_CPU_EISR]
> >> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
> >> + str w10, [x3, #VGIC_CPU_ELRSR]
> >> +#endif
> >> str w11, [x3, #VGIC_CPU_APR]
> >>
> >> /* Clear GICH_HCR */
> >> --
> >> 1.8.1.4
> >>
> > I thought Marc had something here which allowed you to deal with the
> > conversion in the accessor functions and avoid this patch?
>
> Christoffer, I appreciate your review comments.
>
> I think I was missing something. Yes, Marc mentioned in [1] about
> his new changes in vgic3 series. But just after rereading it now, I
> realized that he was suggesting to pick up his commits and add
> them to this series. Is it my right understanding that they should
> be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed
> as well. I am concerned that I don't understand all dependencies
> and impact of those. Wondering about other way around. When vgic3
> series introduced could we just back off above change and do it in
> new right way?
>
> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html
> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
> [3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
> [4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html
>
> Other question: I was testing all this directly on vanilla v3.15, should I
> use some other armkvm specific integration branch to make sure it works
> with all other in a queue armkvm changes.
>
> In mean time I will try to pick up [4], [2], and [3] into v3.15 and see
> how it goes.
>
ok, thanks. I'm ok with potentially adjusting this later if it turns
out to be a pain, depends on what Marc says.
I can probably fix up any conflicts when I apply the patches, but I do
appreciate getting patches that apply to the next branch in [1]. (But
wait until the next branch merges 3.16-rc1).
-Christoffer
[1]: https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-14 15:47 ` Christoffer Dall
@ 2014-06-19 3:46 ` Victor Kamensky
2014-06-21 9:53 ` Marc Zyngier
0 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-19 3:46 UTC (permalink / raw)
To: linux-arm-kernel
Christoffer, Marc,
Please see inline. I am looking for your opinion/advise on how
we go further about this patch.
On 14 June 2014 08:47, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Sat, Jun 14, 2014 at 08:42:58AM -0700, Victor Kamensky wrote:
>> On 14 June 2014 08:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> > On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
>> >> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
>> >> one 'unsigned long *' bit fields, which has 64bit size. So we need to
>> >> swap least significant word with most significant word when code reads
>> >> those registers from h/w.
>> >>
>> >> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> >> ---
>> >> arch/arm64/kvm/hyp.S | 7 +++++++
>> >> 1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> >> index 0620691..5035b41 100644
>> >> --- a/arch/arm64/kvm/hyp.S
>> >> +++ b/arch/arm64/kvm/hyp.S
>> >> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
>> >> str w4, [x3, #VGIC_CPU_HCR]
>> >> str w5, [x3, #VGIC_CPU_VMCR]
>> >> str w6, [x3, #VGIC_CPU_MISR]
>> >> +#ifndef CONFIG_CPU_BIG_ENDIAN
>> >> str w7, [x3, #VGIC_CPU_EISR]
>> >> str w8, [x3, #(VGIC_CPU_EISR + 4)]
>> >> str w9, [x3, #VGIC_CPU_ELRSR]
>> >> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
>> >> +#else
>> >> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
>> >> + str w8, [x3, #VGIC_CPU_EISR]
>> >> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
>> >> + str w10, [x3, #VGIC_CPU_ELRSR]
>> >> +#endif
>> >> str w11, [x3, #VGIC_CPU_APR]
>> >>
>> >> /* Clear GICH_HCR */
>> >> --
>> >> 1.8.1.4
>> >>
>> > I thought Marc had something here which allowed you to deal with the
>> > conversion in the accessor functions and avoid this patch?
>>
>> Christoffer, I appreciate your review comments.
>>
>> I think I was missing something. Yes, Marc mentioned in [1] about
>> his new changes in vgic3 series. But just after rereading it now, I
>> realized that he was suggesting to pick up his commits and add
>> them to this series. Is it my right understanding that they should
>> be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed
>> as well. I am concerned that I don't understand all dependencies
>> and impact of those. Wondering about other way around. When vgic3
>> series introduced could we just back off above change and do it in
>> new right way?
>>
>> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html
>> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
>> [3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
>> [4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html
>>
>> Other question: I was testing all this directly on vanilla v3.15, should I
>> use some other armkvm specific integration branch to make sure it works
>> with all other in a queue armkvm changes.
>>
>> In mean time I will try to pick up [4], [2], and [3] into v3.15 and see
>> how it goes.
>>
> ok, thanks. I'm ok with potentially adjusting this later if it turns
> out to be a pain, depends on what Marc says.
I've tried BE KVM series along with Marc's vgic3 series
and looked closely at picking up accessors to eisr and elrsr
from the vgic3 series ([1] and [2]). It is not trivial. First of
all, existing patches besides accessors introduce callbacks
in vgic_ops, and that pulls pretty much everything before it.
I did try to split [1] and [2] into couple patches each,
one with accessors and another adding vgic_ops callbacks.
In such way I could pick first part and leave vgic_ops
callback in the series. Split worked OK. I can give example
how it would look. However when I've tried to move accessors
part to top of Marc's vgic3 series I got massive conflicts.
Personally I don't have confidence that I can resolve them
correctly, and I don't think Marc would want to do that
as well. I don't think it is worth it.
Instead I propose let's come back to cleaning it up latter
after vgic3 code gets in. I've tried the following patch in
tree with combined series and it worked OK.
Author: Victor Kamensky <victor.kamensky@linaro.org>
Date: Tue Jun 17 21:20:25 2014 -0700
ARM64: KVM: change vgic2 eisr and elrsr word order in big endian case
Now when code uses eisr and elrsr the accessors, move big endian
related code into the accessors. Now in eisr and elrsr arrays
keep least siginificant word at index 0 and most siginificant
word at index 1. Asm code that stores values in array is the
same for little and big endian cases. Correct endian neutral
access to u64 values provided by accessors functions.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
index d5fc5aa..ae21177 100644
--- a/arch/arm64/kvm/vgic-v2-switch.S
+++ b/arch/arm64/kvm/vgic-v2-switch.S
@@ -67,17 +67,10 @@ CPU_BE( rev w11, w11 )
str w4, [x3, #VGIC_V2_CPU_HCR]
str w5, [x3, #VGIC_V2_CPU_VMCR]
str w6, [x3, #VGIC_V2_CPU_MISR]
-#ifndef CONFIG_CPU_BIG_ENDIAN
str w7, [x3, #VGIC_V2_CPU_EISR]
str w8, [x3, #(VGIC_V2_CPU_EISR + 4)]
str w9, [x3, #VGIC_V2_CPU_ELRSR]
str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
-#else
- str w7, [x3, #(VGIC_V2_CPU_EISR + 4)]
- str w8, [x3, #VGIC_V2_CPU_EISR]
- str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
- str w10, [x3, #VGIC_V2_CPU_ELRSR]
-#endif
str w11, [x3, #VGIC_V2_CPU_APR]
/* Clear GICH_HCR */
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index a55a9a4..a4b6f13 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -79,14 +79,30 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu
*vcpu, int lr,
static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
{
+ u64 ret;
const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
- return *(u64 *)elrsr;
+ /*
+ * vgic v2 elrsr is kept as two words, with least significant
+ * word first. Get its value in endian agnostic way.
+ */
+ ret = *(elrsr + 1);
+ ret = ret << 32;
+ ret = ret | *elrsr;
+ return ret;
}
static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
{
+ u64 ret;
const u32 *eisr = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
- return *(u64 *)eisr;
+ /*
+ * vgic v2 eisr is kept as two words, with least siginificant
+ * word first. Get its value in endian agnostic way.
+ */
+ ret = *(eisr + 1);
+ ret = ret << 32;
+ ret = ret | *eisr;
+ return ret;
}
static u32 vgic_v2_get_interrupt_status(const struct kvm_vcpu *vcpu)
Basically it backoffs this commit and changes accessor to read
values assuming that vgic_v2 eisr and elrsr array holds value of
least siginificant word@index 0, and most significant word at
index 1.
Please let me know what you think.
Thanks,
Victor
[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
[2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
> I can probably fix up any conflicts when I apply the patches, but I do
> appreciate getting patches that apply to the next branch in [1]. (But
> wait until the next branch merges 3.16-rc1).
>
> -Christoffer
>
> [1]: https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest
2014-06-14 15:05 ` Christoffer Dall
@ 2014-06-19 5:43 ` Victor Kamensky
0 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-19 5:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi Christoffer,
Please see inline.
On 14 June 2014 08:05, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Thu, Jun 12, 2014 at 09:30:13AM -0700, Victor Kamensky wrote:
>> Fix issue with 32bit guests running on top of BE KVM host.
>> Indexes of high and low words of 64bit cp15 register are
>> swapped in case of big endian code, since 64bit cp15 state is
>> restored or saved with double word write or read instruction.
>>
>> Define helper macros to access high low words of 64bit cp15
>> register.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> ---
>> arch/arm64/include/asm/kvm_host.h | 8 ++++++++
>> arch/arm64/kvm/sys_regs.c | 4 ++--
>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 0a1d697..e9d2e11 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -140,6 +140,14 @@ struct kvm_vcpu_arch {
>> #define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)])
>> #define vcpu_cp15(v,r) ((v)->arch.ctxt.cp15[(r)])
>>
>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>> +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
>> +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
>> +#else
>> +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
>> +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
>> +#endif
>> +
>> struct kvm_vm_stat {
>> u32 remote_tlb_flush;
>> };
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 8e65e31..71aa9b0 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -137,9 +137,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>> if (!p->is_aarch32) {
>> vcpu_sys_reg(vcpu, r->reg) = val;
>> } else {
>> - vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
>> + vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL;
>> if (!p->is_32bit)
>> - vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
>> + vcpu_cp15_64_high(vcpu, r->reg) = val >> 32;
>> }
>> return true;
>> }
>> --
>> 1.8.1.4
>>
>
> I thought there was a consensus here about handling 64-bit accesses
> through the 64-bit values with the vcpu_sys_reg() interface? Did you
> give up on this for a particular reason?
I think I missed that. Do you want this patch to look like below?
Personally, I find it a little bit less clear, but I am fine with it
if you like it more. Or you meant something different?
commit 2de73290a809ef8dbaed087ef2f86d662a006e36
Author: Victor Kamensky <victor.kamensky@linaro.org>
Date: Mon May 12 13:57:21 2014 -0700
ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest
Fix issue with 32bit guests running on top of BE KVM host.
Indexes of high and low words of 64bit cp15 register are
swapped in case of big endian code, since 64bit cp15 state is
restored or saved with double word write or read instruction.
Define helper macro to access low words of 64bit cp15 register.
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index a10803c..fce74ce 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -140,6 +140,12 @@ struct kvm_vcpu_arch {
#define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)])
#define vcpu_cp15(v,r) ((v)->arch.ctxt.cp15[(r)])
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)])
+#else
+#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)])
+#endif
+
struct kvm_vm_stat {
u32 remote_tlb_flush;
};
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8e65e31..a5aa1d1 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -134,12 +134,10 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
BUG_ON(!p->is_write);
val = *vcpu_reg(vcpu, p->Rt);
- if (!p->is_aarch32) {
+ if (!p->is_aarch32 || !p->is_32bit) {
vcpu_sys_reg(vcpu, r->reg) = val;
} else {
- vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
- if (!p->is_32bit)
- vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
+ vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL;
}
return true;
}
Thanks,
Victor
> -Christoffer
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-19 3:46 ` Victor Kamensky
@ 2014-06-21 9:53 ` Marc Zyngier
2014-06-21 17:19 ` Victor Kamensky
0 siblings, 1 reply; 29+ messages in thread
From: Marc Zyngier @ 2014-06-21 9:53 UTC (permalink / raw)
To: linux-arm-kernel
Hi Victor,
On Thu, Jun 19 2014 at 04:46:14 AM, Victor Kamensky <victor.kamensky@linaro.org> wrote:
> Christoffer, Marc,
>
> Please see inline. I am looking for your opinion/advise on how
> we go further about this patch.
>
> On 14 June 2014 08:47, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> On Sat, Jun 14, 2014 at 08:42:58AM -0700, Victor Kamensky wrote:
>>> On 14 June 2014 08:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>> > On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
>>> >> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
>>> >> one 'unsigned long *' bit fields, which has 64bit size. So we need to
>>> >> swap least significant word with most significant word when code reads
>>> >> those registers from h/w.
>>> >>
>>> >> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>>> >> ---
>>> >> arch/arm64/kvm/hyp.S | 7 +++++++
>>> >> 1 file changed, 7 insertions(+)
>>> >>
>>> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>>> >> index 0620691..5035b41 100644
>>> >> --- a/arch/arm64/kvm/hyp.S
>>> >> +++ b/arch/arm64/kvm/hyp.S
>>> >> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
>>> >> str w4, [x3, #VGIC_CPU_HCR]
>>> >> str w5, [x3, #VGIC_CPU_VMCR]
>>> >> str w6, [x3, #VGIC_CPU_MISR]
>>> >> +#ifndef CONFIG_CPU_BIG_ENDIAN
>>> >> str w7, [x3, #VGIC_CPU_EISR]
>>> >> str w8, [x3, #(VGIC_CPU_EISR + 4)]
>>> >> str w9, [x3, #VGIC_CPU_ELRSR]
>>> >> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
>>> >> +#else
>>> >> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
>>> >> + str w8, [x3, #VGIC_CPU_EISR]
>>> >> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
>>> >> + str w10, [x3, #VGIC_CPU_ELRSR]
>>> >> +#endif
>>> >> str w11, [x3, #VGIC_CPU_APR]
>>> >>
>>> >> /* Clear GICH_HCR */
>>> >> --
>>> >> 1.8.1.4
>>> >>
>>> > I thought Marc had something here which allowed you to deal with the
>>> > conversion in the accessor functions and avoid this patch?
>>>
>>> Christoffer, I appreciate your review comments.
>>>
>>> I think I was missing something. Yes, Marc mentioned in [1] about
>>> his new changes in vgic3 series. But just after rereading it now, I
>>> realized that he was suggesting to pick up his commits and add
>>> them to this series. Is it my right understanding that they should
>>> be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed
>>> as well. I am concerned that I don't understand all dependencies
>>> and impact of those. Wondering about other way around. When vgic3
>>> series introduced could we just back off above change and do it in
>>> new right way?
>>>
>>> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html
>>> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
>>> [3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
>>> [4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html
>>>
>>> Other question: I was testing all this directly on vanilla v3.15, should I
>>> use some other armkvm specific integration branch to make sure it works
>>> with all other in a queue armkvm changes.
>>>
>>> In mean time I will try to pick up [4], [2], and [3] into v3.15 and see
>>> how it goes.
>>>
>> ok, thanks. I'm ok with potentially adjusting this later if it turns
>> out to be a pain, depends on what Marc says.
>
> I've tried BE KVM series along with Marc's vgic3 series
> and looked closely at picking up accessors to eisr and elrsr
> from the vgic3 series ([1] and [2]). It is not trivial. First of
> all, existing patches besides accessors introduce callbacks
> in vgic_ops, and that pulls pretty much everything before it.
> I did try to split [1] and [2] into couple patches each,
> one with accessors and another adding vgic_ops callbacks.
> In such way I could pick first part and leave vgic_ops
> callback in the series. Split worked OK. I can give example
> how it would look. However when I've tried to move accessors
> part to top of Marc's vgic3 series I got massive conflicts.
> Personally I don't have confidence that I can resolve them
> correctly, and I don't think Marc would want to do that
> as well. I don't think it is worth it.
>
> Instead I propose let's come back to cleaning it up latter
> after vgic3 code gets in. I've tried the following patch in
> tree with combined series and it worked OK.
>
> Author: Victor Kamensky <victor.kamensky@linaro.org>
> Date: Tue Jun 17 21:20:25 2014 -0700
>
> ARM64: KVM: change vgic2 eisr and elrsr word order in big endian case
>
> Now when code uses eisr and elrsr the accessors, move big endian
> related code into the accessors. Now in eisr and elrsr arrays
> keep least siginificant word at index 0 and most siginificant
> word at index 1. Asm code that stores values in array is the
> same for little and big endian cases. Correct endian neutral
> access to u64 values provided by accessors functions.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>
> diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
> index d5fc5aa..ae21177 100644
> --- a/arch/arm64/kvm/vgic-v2-switch.S
> +++ b/arch/arm64/kvm/vgic-v2-switch.S
> @@ -67,17 +67,10 @@ CPU_BE( rev w11, w11 )
> str w4, [x3, #VGIC_V2_CPU_HCR]
> str w5, [x3, #VGIC_V2_CPU_VMCR]
> str w6, [x3, #VGIC_V2_CPU_MISR]
> -#ifndef CONFIG_CPU_BIG_ENDIAN
> str w7, [x3, #VGIC_V2_CPU_EISR]
> str w8, [x3, #(VGIC_V2_CPU_EISR + 4)]
> str w9, [x3, #VGIC_V2_CPU_ELRSR]
> str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
> -#else
> - str w7, [x3, #(VGIC_V2_CPU_EISR + 4)]
> - str w8, [x3, #VGIC_V2_CPU_EISR]
> - str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
> - str w10, [x3, #VGIC_V2_CPU_ELRSR]
> -#endif
> str w11, [x3, #VGIC_V2_CPU_APR]
>
> /* Clear GICH_HCR */
> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> index a55a9a4..a4b6f13 100644
> --- a/virt/kvm/arm/vgic-v2.c
> +++ b/virt/kvm/arm/vgic-v2.c
> @@ -79,14 +79,30 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu
> *vcpu, int lr,
>
> static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
> {
> + u64 ret;
> const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
> - return *(u64 *)elrsr;
> + /*
> + * vgic v2 elrsr is kept as two words, with least significant
> + * word first. Get its value in endian agnostic way.
> + */
> + ret = *(elrsr + 1);
> + ret = ret << 32;
> + ret = ret | *elrsr;
> + return ret;
> }
>
> static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
> {
> + u64 ret;
> const u32 *eisr = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
> - return *(u64 *)eisr;
> + /*
> + * vgic v2 eisr is kept as two words, with least siginificant
> + * word first. Get its value in endian agnostic way.
> + */
> + ret = *(eisr + 1);
> + ret = ret << 32;
> + ret = ret | *eisr;
> + return ret;
> }
>
> static u32 vgic_v2_get_interrupt_status(const struct kvm_vcpu *vcpu)
>
> Basically it backoffs this commit and changes accessor to read
> values assuming that vgic_v2 eisr and elrsr array holds value of
> least siginificant word at index 0, and most significant word at
> index 1.
>
> Please let me know what you think.
I very much like this solution. Do you mind if I fold this into the
GICv3 series?
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 11/14] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case
2014-06-14 15:04 ` Christoffer Dall
@ 2014-06-21 9:58 ` Marc Zyngier
0 siblings, 0 replies; 29+ messages in thread
From: Marc Zyngier @ 2014-06-21 9:58 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jun 14 2014 at 04:04:51 PM, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Thu, Jun 12, 2014 at 09:30:10AM -0700, Victor Kamensky wrote:
>> Fix vgic_bitmap_get_reg function to return 'right' word address of
>> 'unsigned long' bitmap value in case of BE 64bit image.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> ---
>> virt/kvm/arm/vgic.c | 24 ++++++++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 529c336..b4ffd82 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -101,14 +101,34 @@ static u32 vgic_nr_lr;
>>
>> static unsigned int vgic_maint_irq;
>>
>> +/*
>> + * struct vgic_bitmap contains unions that provide two views of
>> + * the same data. In one case it is an array of registers of
>> + * u32's, and in the other case it is a bitmap of unsigned
>> + * longs.
>> + *
>> + * This does not work on 64-bit BE systems, because the bitmap access
>> + * will store two consecutive 32-bit words with the higher-addressed
>> + * register's bits at the lower index and the lower-addressed register's
>> + * bits at the higher index.
>> + *
>> + * Therefore, swizzle the register index when accessing the 32-bit word
>> + * registers to access the right register's value.
>> + */
>> +#if defined(CONFIG_CPU_BIG_ENDIAN) && BITS_PER_LONG == 64
>> +#define REG_OFFSET_SWIZZLE 1
>> +#else
>> +#define REG_OFFSET_SWIZZLE 0
>> +#endif
>> +
>> static u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
>> int cpuid, u32 offset)
>> {
>> offset >>= 2;
>> if (!offset)
>> - return x->percpu[cpuid].reg;
>> + return x->percpu[cpuid].reg + (offset ^ REG_OFFSET_SWIZZLE);
>> else
>> - return x->shared.reg + offset - 1;
>> + return x->shared.reg + ((offset - 1) ^ REG_OFFSET_SWIZZLE);
>> }
>>
>> static int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,
>> --
>> 1.8.1.4
>>
>
> I had wished there was a cleaner way to abstract this, but I can't think
> of a cleaner solution, and restructuring the whole thing would be very
> invasive.
>
> I'm curious to hear from Marc on this one, but you can add:
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> because it looks correct.
>
> Thanks for picking up my suggestion on the commenting.
I tried hard to find another solution, but I think this is unfortunately
the best we can have... ;-)
So:
Acked-by: Marc Zyngier <narc.zyngier@arm.com>
I will have to fold in a similar solution for the vgic-dyn series.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-21 9:53 ` Marc Zyngier
@ 2014-06-21 17:19 ` Victor Kamensky
2014-06-23 8:26 ` Marc Zyngier
0 siblings, 1 reply; 29+ messages in thread
From: Victor Kamensky @ 2014-06-21 17:19 UTC (permalink / raw)
To: linux-arm-kernel
On 21 June 2014 02:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Victor,
>
> On Thu, Jun 19 2014 at 04:46:14 AM, Victor Kamensky <victor.kamensky@linaro.org> wrote:
>> Christoffer, Marc,
>>
>> Please see inline. I am looking for your opinion/advise on how
>> we go further about this patch.
>>
>> On 14 June 2014 08:47, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>> On Sat, Jun 14, 2014 at 08:42:58AM -0700, Victor Kamensky wrote:
>>>> On 14 June 2014 08:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>>> > On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
>>>> >> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
>>>> >> one 'unsigned long *' bit fields, which has 64bit size. So we need to
>>>> >> swap least significant word with most significant word when code reads
>>>> >> those registers from h/w.
>>>> >>
>>>> >> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>>>> >> ---
>>>> >> arch/arm64/kvm/hyp.S | 7 +++++++
>>>> >> 1 file changed, 7 insertions(+)
>>>> >>
>>>> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>>>> >> index 0620691..5035b41 100644
>>>> >> --- a/arch/arm64/kvm/hyp.S
>>>> >> +++ b/arch/arm64/kvm/hyp.S
>>>> >> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
>>>> >> str w4, [x3, #VGIC_CPU_HCR]
>>>> >> str w5, [x3, #VGIC_CPU_VMCR]
>>>> >> str w6, [x3, #VGIC_CPU_MISR]
>>>> >> +#ifndef CONFIG_CPU_BIG_ENDIAN
>>>> >> str w7, [x3, #VGIC_CPU_EISR]
>>>> >> str w8, [x3, #(VGIC_CPU_EISR + 4)]
>>>> >> str w9, [x3, #VGIC_CPU_ELRSR]
>>>> >> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
>>>> >> +#else
>>>> >> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
>>>> >> + str w8, [x3, #VGIC_CPU_EISR]
>>>> >> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
>>>> >> + str w10, [x3, #VGIC_CPU_ELRSR]
>>>> >> +#endif
>>>> >> str w11, [x3, #VGIC_CPU_APR]
>>>> >>
>>>> >> /* Clear GICH_HCR */
>>>> >> --
>>>> >> 1.8.1.4
>>>> >>
>>>> > I thought Marc had something here which allowed you to deal with the
>>>> > conversion in the accessor functions and avoid this patch?
>>>>
>>>> Christoffer, I appreciate your review comments.
>>>>
>>>> I think I was missing something. Yes, Marc mentioned in [1] about
>>>> his new changes in vgic3 series. But just after rereading it now, I
>>>> realized that he was suggesting to pick up his commits and add
>>>> them to this series. Is it my right understanding that they should
>>>> be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed
>>>> as well. I am concerned that I don't understand all dependencies
>>>> and impact of those. Wondering about other way around. When vgic3
>>>> series introduced could we just back off above change and do it in
>>>> new right way?
>>>>
>>>> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html
>>>> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
>>>> [3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
>>>> [4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html
>>>>
>>>> Other question: I was testing all this directly on vanilla v3.15, should I
>>>> use some other armkvm specific integration branch to make sure it works
>>>> with all other in a queue armkvm changes.
>>>>
>>>> In mean time I will try to pick up [4], [2], and [3] into v3.15 and see
>>>> how it goes.
>>>>
>>> ok, thanks. I'm ok with potentially adjusting this later if it turns
>>> out to be a pain, depends on what Marc says.
>>
>> I've tried BE KVM series along with Marc's vgic3 series
>> and looked closely at picking up accessors to eisr and elrsr
>> from the vgic3 series ([1] and [2]). It is not trivial. First of
>> all, existing patches besides accessors introduce callbacks
>> in vgic_ops, and that pulls pretty much everything before it.
>> I did try to split [1] and [2] into couple patches each,
>> one with accessors and another adding vgic_ops callbacks.
>> In such way I could pick first part and leave vgic_ops
>> callback in the series. Split worked OK. I can give example
>> how it would look. However when I've tried to move accessors
>> part to top of Marc's vgic3 series I got massive conflicts.
>> Personally I don't have confidence that I can resolve them
>> correctly, and I don't think Marc would want to do that
>> as well. I don't think it is worth it.
>>
>> Instead I propose let's come back to cleaning it up latter
>> after vgic3 code gets in. I've tried the following patch in
>> tree with combined series and it worked OK.
>>
>> Author: Victor Kamensky <victor.kamensky@linaro.org>
>> Date: Tue Jun 17 21:20:25 2014 -0700
>>
>> ARM64: KVM: change vgic2 eisr and elrsr word order in big endian case
>>
>> Now when code uses eisr and elrsr the accessors, move big endian
>> related code into the accessors. Now in eisr and elrsr arrays
>> keep least siginificant word at index 0 and most siginificant
>> word at index 1. Asm code that stores values in array is the
>> same for little and big endian cases. Correct endian neutral
>> access to u64 values provided by accessors functions.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>>
>> diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
>> index d5fc5aa..ae21177 100644
>> --- a/arch/arm64/kvm/vgic-v2-switch.S
>> +++ b/arch/arm64/kvm/vgic-v2-switch.S
>> @@ -67,17 +67,10 @@ CPU_BE( rev w11, w11 )
>> str w4, [x3, #VGIC_V2_CPU_HCR]
>> str w5, [x3, #VGIC_V2_CPU_VMCR]
>> str w6, [x3, #VGIC_V2_CPU_MISR]
>> -#ifndef CONFIG_CPU_BIG_ENDIAN
>> str w7, [x3, #VGIC_V2_CPU_EISR]
>> str w8, [x3, #(VGIC_V2_CPU_EISR + 4)]
>> str w9, [x3, #VGIC_V2_CPU_ELRSR]
>> str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
>> -#else
>> - str w7, [x3, #(VGIC_V2_CPU_EISR + 4)]
>> - str w8, [x3, #VGIC_V2_CPU_EISR]
>> - str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
>> - str w10, [x3, #VGIC_V2_CPU_ELRSR]
>> -#endif
>> str w11, [x3, #VGIC_V2_CPU_APR]
>>
>> /* Clear GICH_HCR */
>> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
>> index a55a9a4..a4b6f13 100644
>> --- a/virt/kvm/arm/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic-v2.c
>> @@ -79,14 +79,30 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu
>> *vcpu, int lr,
>>
>> static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
>> {
>> + u64 ret;
>> const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
>> - return *(u64 *)elrsr;
>> + /*
>> + * vgic v2 elrsr is kept as two words, with least significant
>> + * word first. Get its value in endian agnostic way.
>> + */
>> + ret = *(elrsr + 1);
>> + ret = ret << 32;
>> + ret = ret | *elrsr;
>> + return ret;
>> }
>>
>> static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
>> {
>> + u64 ret;
>> const u32 *eisr = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
>> - return *(u64 *)eisr;
>> + /*
>> + * vgic v2 eisr is kept as two words, with least siginificant
>> + * word first. Get its value in endian agnostic way.
>> + */
>> + ret = *(eisr + 1);
>> + ret = ret << 32;
>> + ret = ret | *eisr;
>> + return ret;
>> }
>>
>> static u32 vgic_v2_get_interrupt_status(const struct kvm_vcpu *vcpu)
>>
>> Basically it backoffs this commit and changes accessor to read
>> values assuming that vgic_v2 eisr and elrsr array holds value of
>> least siginificant word at index 0, and most significant word at
>> index 1.
>>
>> Please let me know what you think.
>
> I very much like this solution. Do you mind if I fold this into the
> GICv3 series?
Sure, absolutely, please go ahead.
I am not sure about order of getting into kvmarm tree between
GICv3 series and BE KVM series, If BE KVM series go first, you can
pickup accessors changes right now and add backout of
asm file change when it sees BE KVM series. Actually you would
need to back them out not from vgic-v2-switch.S but from hyp.S
(that was one of few conflicts when I've tried both series together).
If GICv3 series go first, again accessors changes could be picked
right now and when KVM BE series sees hyp.S change will have
to be dropped.
Thanks,
Victor
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-21 17:19 ` Victor Kamensky
@ 2014-06-23 8:26 ` Marc Zyngier
2014-06-23 16:40 ` Victor Kamensky
0 siblings, 1 reply; 29+ messages in thread
From: Marc Zyngier @ 2014-06-23 8:26 UTC (permalink / raw)
To: linux-arm-kernel
On 21/06/14 18:19, Victor Kamensky wrote:
> On 21 June 2014 02:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> Hi Victor,
>>
>> On Thu, Jun 19 2014 at 04:46:14 AM, Victor Kamensky <victor.kamensky@linaro.org> wrote:
>>> Christoffer, Marc,
>>>
>>> Please see inline. I am looking for your opinion/advise on how
>>> we go further about this patch.
>>>
>>> On 14 June 2014 08:47, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>>> On Sat, Jun 14, 2014 at 08:42:58AM -0700, Victor Kamensky wrote:
>>>>> On 14 June 2014 08:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>>>>> On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
>>>>>>> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
>>>>>>> one 'unsigned long *' bit fields, which has 64bit size. So we need to
>>>>>>> swap least significant word with most significant word when code reads
>>>>>>> those registers from h/w.
>>>>>>>
>>>>>>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>>>>>>> ---
>>>>>>> arch/arm64/kvm/hyp.S | 7 +++++++
>>>>>>> 1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>>>>>>> index 0620691..5035b41 100644
>>>>>>> --- a/arch/arm64/kvm/hyp.S
>>>>>>> +++ b/arch/arm64/kvm/hyp.S
>>>>>>> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
>>>>>>> str w4, [x3, #VGIC_CPU_HCR]
>>>>>>> str w5, [x3, #VGIC_CPU_VMCR]
>>>>>>> str w6, [x3, #VGIC_CPU_MISR]
>>>>>>> +#ifndef CONFIG_CPU_BIG_ENDIAN
>>>>>>> str w7, [x3, #VGIC_CPU_EISR]
>>>>>>> str w8, [x3, #(VGIC_CPU_EISR + 4)]
>>>>>>> str w9, [x3, #VGIC_CPU_ELRSR]
>>>>>>> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
>>>>>>> +#else
>>>>>>> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
>>>>>>> + str w8, [x3, #VGIC_CPU_EISR]
>>>>>>> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
>>>>>>> + str w10, [x3, #VGIC_CPU_ELRSR]
>>>>>>> +#endif
>>>>>>> str w11, [x3, #VGIC_CPU_APR]
>>>>>>>
>>>>>>> /* Clear GICH_HCR */
>>>>>>> --
>>>>>>> 1.8.1.4
>>>>>>>
>>>>>> I thought Marc had something here which allowed you to deal with the
>>>>>> conversion in the accessor functions and avoid this patch?
>>>>>
>>>>> Christoffer, I appreciate your review comments.
>>>>>
>>>>> I think I was missing something. Yes, Marc mentioned in [1] about
>>>>> his new changes in vgic3 series. But just after rereading it now, I
>>>>> realized that he was suggesting to pick up his commits and add
>>>>> them to this series. Is it my right understanding that they should
>>>>> be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed
>>>>> as well. I am concerned that I don't understand all dependencies
>>>>> and impact of those. Wondering about other way around. When vgic3
>>>>> series introduced could we just back off above change and do it in
>>>>> new right way?
>>>>>
>>>>> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html
>>>>> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
>>>>> [3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
>>>>> [4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html
>>>>>
>>>>> Other question: I was testing all this directly on vanilla v3.15, should I
>>>>> use some other armkvm specific integration branch to make sure it works
>>>>> with all other in a queue armkvm changes.
>>>>>
>>>>> In mean time I will try to pick up [4], [2], and [3] into v3.15 and see
>>>>> how it goes.
>>>>>
>>>> ok, thanks. I'm ok with potentially adjusting this later if it turns
>>>> out to be a pain, depends on what Marc says.
>>>
>>> I've tried BE KVM series along with Marc's vgic3 series
>>> and looked closely at picking up accessors to eisr and elrsr
>>> from the vgic3 series ([1] and [2]). It is not trivial. First of
>>> all, existing patches besides accessors introduce callbacks
>>> in vgic_ops, and that pulls pretty much everything before it.
>>> I did try to split [1] and [2] into couple patches each,
>>> one with accessors and another adding vgic_ops callbacks.
>>> In such way I could pick first part and leave vgic_ops
>>> callback in the series. Split worked OK. I can give example
>>> how it would look. However when I've tried to move accessors
>>> part to top of Marc's vgic3 series I got massive conflicts.
>>> Personally I don't have confidence that I can resolve them
>>> correctly, and I don't think Marc would want to do that
>>> as well. I don't think it is worth it.
>>>
>>> Instead I propose let's come back to cleaning it up latter
>>> after vgic3 code gets in. I've tried the following patch in
>>> tree with combined series and it worked OK.
>>>
>>> Author: Victor Kamensky <victor.kamensky@linaro.org>
>>> Date: Tue Jun 17 21:20:25 2014 -0700
>>>
>>> ARM64: KVM: change vgic2 eisr and elrsr word order in big endian case
>>>
>>> Now when code uses eisr and elrsr the accessors, move big endian
>>> related code into the accessors. Now in eisr and elrsr arrays
>>> keep least siginificant word at index 0 and most siginificant
>>> word at index 1. Asm code that stores values in array is the
>>> same for little and big endian cases. Correct endian neutral
>>> access to u64 values provided by accessors functions.
>>>
>>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>>>
>>> diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
>>> index d5fc5aa..ae21177 100644
>>> --- a/arch/arm64/kvm/vgic-v2-switch.S
>>> +++ b/arch/arm64/kvm/vgic-v2-switch.S
>>> @@ -67,17 +67,10 @@ CPU_BE( rev w11, w11 )
>>> str w4, [x3, #VGIC_V2_CPU_HCR]
>>> str w5, [x3, #VGIC_V2_CPU_VMCR]
>>> str w6, [x3, #VGIC_V2_CPU_MISR]
>>> -#ifndef CONFIG_CPU_BIG_ENDIAN
>>> str w7, [x3, #VGIC_V2_CPU_EISR]
>>> str w8, [x3, #(VGIC_V2_CPU_EISR + 4)]
>>> str w9, [x3, #VGIC_V2_CPU_ELRSR]
>>> str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
>>> -#else
>>> - str w7, [x3, #(VGIC_V2_CPU_EISR + 4)]
>>> - str w8, [x3, #VGIC_V2_CPU_EISR]
>>> - str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
>>> - str w10, [x3, #VGIC_V2_CPU_ELRSR]
>>> -#endif
>>> str w11, [x3, #VGIC_V2_CPU_APR]
>>>
>>> /* Clear GICH_HCR */
>>> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
>>> index a55a9a4..a4b6f13 100644
>>> --- a/virt/kvm/arm/vgic-v2.c
>>> +++ b/virt/kvm/arm/vgic-v2.c
>>> @@ -79,14 +79,30 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu
>>> *vcpu, int lr,
>>>
>>> static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
>>> {
>>> + u64 ret;
>>> const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
>>> - return *(u64 *)elrsr;
>>> + /*
>>> + * vgic v2 elrsr is kept as two words, with least significant
>>> + * word first. Get its value in endian agnostic way.
>>> + */
>>> + ret = *(elrsr + 1);
>>> + ret = ret << 32;
>>> + ret = ret | *elrsr;
>>> + return ret;
>>> }
>>>
>>> static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
>>> {
>>> + u64 ret;
>>> const u32 *eisr = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
>>> - return *(u64 *)eisr;
>>> + /*
>>> + * vgic v2 eisr is kept as two words, with least siginificant
>>> + * word first. Get its value in endian agnostic way.
>>> + */
>>> + ret = *(eisr + 1);
>>> + ret = ret << 32;
>>> + ret = ret | *eisr;
>>> + return ret;
>>> }
>>>
>>> static u32 vgic_v2_get_interrupt_status(const struct kvm_vcpu *vcpu)
>>>
>>> Basically it backoffs this commit and changes accessor to read
>>> values assuming that vgic_v2 eisr and elrsr array holds value of
>>> least siginificant word at index 0, and most significant word at
>>> index 1.
>>>
>>> Please let me know what you think.
>>
>> I very much like this solution. Do you mind if I fold this into the
>> GICv3 series?
>
> Sure, absolutely, please go ahead.
>
> I am not sure about order of getting into kvmarm tree between
> GICv3 series and BE KVM series, If BE KVM series go first, you can
> pickup accessors changes right now and add backout of
> asm file change when it sees BE KVM series. Actually you would
> need to back them out not from vgic-v2-switch.S but from hyp.S
> (that was one of few conflicts when I've tried both series together).
> If GICv3 series go first, again accessors changes could be picked
> right now and when KVM BE series sees hyp.S change will have
> to be dropped.
I had a quick look myself, and it feels like having GICv3 first and then
KVM-BE after that is a bit easier. I don't want to put the burden of
that on you though, so I'll probably end up doing the merge myself.
Would you be OK to review it?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
2014-06-23 8:26 ` Marc Zyngier
@ 2014-06-23 16:40 ` Victor Kamensky
0 siblings, 0 replies; 29+ messages in thread
From: Victor Kamensky @ 2014-06-23 16:40 UTC (permalink / raw)
To: linux-arm-kernel
On 23 June 2014 01:26, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 21/06/14 18:19, Victor Kamensky wrote:
>> On 21 June 2014 02:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> Hi Victor,
>>>
>>> On Thu, Jun 19 2014 at 04:46:14 AM, Victor Kamensky <victor.kamensky@linaro.org> wrote:
>>>> Christoffer, Marc,
>>>>
>>>> Please see inline. I am looking for your opinion/advise on how
>>>> we go further about this patch.
>>>>
>>>> On 14 June 2014 08:47, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>>>> On Sat, Jun 14, 2014 at 08:42:58AM -0700, Victor Kamensky wrote:
>>>>>> On 14 June 2014 08:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>>>>>> On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote:
>>>>>>>> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
>>>>>>>> one 'unsigned long *' bit fields, which has 64bit size. So we need to
>>>>>>>> swap least significant word with most significant word when code reads
>>>>>>>> those registers from h/w.
>>>>>>>>
>>>>>>>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>>>>>>>> ---
>>>>>>>> arch/arm64/kvm/hyp.S | 7 +++++++
>>>>>>>> 1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>>>>>>>> index 0620691..5035b41 100644
>>>>>>>> --- a/arch/arm64/kvm/hyp.S
>>>>>>>> +++ b/arch/arm64/kvm/hyp.S
>>>>>>>> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 )
>>>>>>>> str w4, [x3, #VGIC_CPU_HCR]
>>>>>>>> str w5, [x3, #VGIC_CPU_VMCR]
>>>>>>>> str w6, [x3, #VGIC_CPU_MISR]
>>>>>>>> +#ifndef CONFIG_CPU_BIG_ENDIAN
>>>>>>>> str w7, [x3, #VGIC_CPU_EISR]
>>>>>>>> str w8, [x3, #(VGIC_CPU_EISR + 4)]
>>>>>>>> str w9, [x3, #VGIC_CPU_ELRSR]
>>>>>>>> str w10, [x3, #(VGIC_CPU_ELRSR + 4)]
>>>>>>>> +#else
>>>>>>>> + str w7, [x3, #(VGIC_CPU_EISR + 4)]
>>>>>>>> + str w8, [x3, #VGIC_CPU_EISR]
>>>>>>>> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
>>>>>>>> + str w10, [x3, #VGIC_CPU_ELRSR]
>>>>>>>> +#endif
>>>>>>>> str w11, [x3, #VGIC_CPU_APR]
>>>>>>>>
>>>>>>>> /* Clear GICH_HCR */
>>>>>>>> --
>>>>>>>> 1.8.1.4
>>>>>>>>
>>>>>>> I thought Marc had something here which allowed you to deal with the
>>>>>>> conversion in the accessor functions and avoid this patch?
>>>>>>
>>>>>> Christoffer, I appreciate your review comments.
>>>>>>
>>>>>> I think I was missing something. Yes, Marc mentioned in [1] about
>>>>>> his new changes in vgic3 series. But just after rereading it now, I
>>>>>> realized that he was suggesting to pick up his commits and add
>>>>>> them to this series. Is it my right understanding that they should
>>>>>> be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed
>>>>>> as well. I am concerned that I don't understand all dependencies
>>>>>> and impact of those. Wondering about other way around. When vgic3
>>>>>> series introduced could we just back off above change and do it in
>>>>>> new right way?
>>>>>>
>>>>>> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html
>>>>>> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html
>>>>>> [3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html
>>>>>> [4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html
>>>>>>
>>>>>> Other question: I was testing all this directly on vanilla v3.15, should I
>>>>>> use some other armkvm specific integration branch to make sure it works
>>>>>> with all other in a queue armkvm changes.
>>>>>>
>>>>>> In mean time I will try to pick up [4], [2], and [3] into v3.15 and see
>>>>>> how it goes.
>>>>>>
>>>>> ok, thanks. I'm ok with potentially adjusting this later if it turns
>>>>> out to be a pain, depends on what Marc says.
>>>>
>>>> I've tried BE KVM series along with Marc's vgic3 series
>>>> and looked closely at picking up accessors to eisr and elrsr
>>>> from the vgic3 series ([1] and [2]). It is not trivial. First of
>>>> all, existing patches besides accessors introduce callbacks
>>>> in vgic_ops, and that pulls pretty much everything before it.
>>>> I did try to split [1] and [2] into couple patches each,
>>>> one with accessors and another adding vgic_ops callbacks.
>>>> In such way I could pick first part and leave vgic_ops
>>>> callback in the series. Split worked OK. I can give example
>>>> how it would look. However when I've tried to move accessors
>>>> part to top of Marc's vgic3 series I got massive conflicts.
>>>> Personally I don't have confidence that I can resolve them
>>>> correctly, and I don't think Marc would want to do that
>>>> as well. I don't think it is worth it.
>>>>
>>>> Instead I propose let's come back to cleaning it up latter
>>>> after vgic3 code gets in. I've tried the following patch in
>>>> tree with combined series and it worked OK.
>>>>
>>>> Author: Victor Kamensky <victor.kamensky@linaro.org>
>>>> Date: Tue Jun 17 21:20:25 2014 -0700
>>>>
>>>> ARM64: KVM: change vgic2 eisr and elrsr word order in big endian case
>>>>
>>>> Now when code uses eisr and elrsr the accessors, move big endian
>>>> related code into the accessors. Now in eisr and elrsr arrays
>>>> keep least siginificant word at index 0 and most siginificant
>>>> word at index 1. Asm code that stores values in array is the
>>>> same for little and big endian cases. Correct endian neutral
>>>> access to u64 values provided by accessors functions.
>>>>
>>>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
>>>> index d5fc5aa..ae21177 100644
>>>> --- a/arch/arm64/kvm/vgic-v2-switch.S
>>>> +++ b/arch/arm64/kvm/vgic-v2-switch.S
>>>> @@ -67,17 +67,10 @@ CPU_BE( rev w11, w11 )
>>>> str w4, [x3, #VGIC_V2_CPU_HCR]
>>>> str w5, [x3, #VGIC_V2_CPU_VMCR]
>>>> str w6, [x3, #VGIC_V2_CPU_MISR]
>>>> -#ifndef CONFIG_CPU_BIG_ENDIAN
>>>> str w7, [x3, #VGIC_V2_CPU_EISR]
>>>> str w8, [x3, #(VGIC_V2_CPU_EISR + 4)]
>>>> str w9, [x3, #VGIC_V2_CPU_ELRSR]
>>>> str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
>>>> -#else
>>>> - str w7, [x3, #(VGIC_V2_CPU_EISR + 4)]
>>>> - str w8, [x3, #VGIC_V2_CPU_EISR]
>>>> - str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
>>>> - str w10, [x3, #VGIC_V2_CPU_ELRSR]
>>>> -#endif
>>>> str w11, [x3, #VGIC_V2_CPU_APR]
>>>>
>>>> /* Clear GICH_HCR */
>>>> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
>>>> index a55a9a4..a4b6f13 100644
>>>> --- a/virt/kvm/arm/vgic-v2.c
>>>> +++ b/virt/kvm/arm/vgic-v2.c
>>>> @@ -79,14 +79,30 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu
>>>> *vcpu, int lr,
>>>>
>>>> static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
>>>> {
>>>> + u64 ret;
>>>> const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
>>>> - return *(u64 *)elrsr;
>>>> + /*
>>>> + * vgic v2 elrsr is kept as two words, with least significant
>>>> + * word first. Get its value in endian agnostic way.
>>>> + */
>>>> + ret = *(elrsr + 1);
>>>> + ret = ret << 32;
>>>> + ret = ret | *elrsr;
>>>> + return ret;
>>>> }
>>>>
>>>> static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
>>>> {
>>>> + u64 ret;
>>>> const u32 *eisr = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
>>>> - return *(u64 *)eisr;
>>>> + /*
>>>> + * vgic v2 eisr is kept as two words, with least siginificant
>>>> + * word first. Get its value in endian agnostic way.
>>>> + */
>>>> + ret = *(eisr + 1);
>>>> + ret = ret << 32;
>>>> + ret = ret | *eisr;
>>>> + return ret;
>>>> }
>>>>
>>>> static u32 vgic_v2_get_interrupt_status(const struct kvm_vcpu *vcpu)
>>>>
>>>> Basically it backoffs this commit and changes accessor to read
>>>> values assuming that vgic_v2 eisr and elrsr array holds value of
>>>> least siginificant word at index 0, and most significant word at
>>>> index 1.
>>>>
>>>> Please let me know what you think.
>>>
>>> I very much like this solution. Do you mind if I fold this into the
>>> GICv3 series?
>>
>> Sure, absolutely, please go ahead.
>>
>> I am not sure about order of getting into kvmarm tree between
>> GICv3 series and BE KVM series, If BE KVM series go first, you can
>> pickup accessors changes right now and add backout of
>> asm file change when it sees BE KVM series. Actually you would
>> need to back them out not from vgic-v2-switch.S but from hyp.S
>> (that was one of few conflicts when I've tried both series together).
>> If GICv3 series go first, again accessors changes could be picked
>> right now and when KVM BE series sees hyp.S change will have
>> to be dropped.
>
> I had a quick look myself, and it feels like having GICv3 first and then
> KVM-BE after that is a bit easier. I don't want to put the burden of
> that on you though, so I'll probably end up doing the merge myself.
>
> Would you be OK to review it?
Sure, either way. Note, that while working on this thread I just
tried KVM BE series after GICv3 series, so I have my resolutions
version and it was not much. It won't be burden for me to come
up with series that is based on top of GICv3 code.If you want to
do it yourself it is fine too.
In this case, if you will pick up accessors part. This patch
will have to be dropped during series merge.
Note at this point all patches except [1] are reviewed. IMHO [1] is
minor issue. When you or Christoffer have time please take a look
at it.
[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-June/010022.html
Thanks,
Victor
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2014-06-23 16:40 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-12 16:29 [PATCH v4 00/14] ARM/ARM64: KVM: big endian host support Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 01/14] ARM: KVM: switch hypervisor into BE mode in case of BE host Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 02/14] ARM: KVM: fix vgic V7 assembler code to work in BE image Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 03/14] ARM: KVM: handle 64bit values passed to mrcc or from mcrr instructions in BE case Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 04/14] ARM: KVM: __kvm_vcpu_run function return result fix " Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 05/14] ARM: KVM: vgic mmio should hold data as LE bytes array " Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 06/14] ARM: KVM: MMIO support BE host running LE code Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 07/14] ARM: KVM: one_reg coproc set and get BE fixes Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-12 16:30 ` [PATCH v4 08/14] ARM: KVM: enable KVM in Kconfig on big-endian systems Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-12 16:30 ` [PATCH v4 09/14] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 10/14] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 11/14] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-21 9:58 ` Marc Zyngier
2014-06-12 16:30 ` [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
2014-06-14 15:04 ` Christoffer Dall
2014-06-14 15:42 ` Victor Kamensky
2014-06-14 15:47 ` Christoffer Dall
2014-06-19 3:46 ` Victor Kamensky
2014-06-21 9:53 ` Marc Zyngier
2014-06-21 17:19 ` Victor Kamensky
2014-06-23 8:26 ` Marc Zyngier
2014-06-23 16:40 ` Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 13/14] ARM64: KVM: set and get of sys registers " Victor Kamensky
2014-06-12 16:30 ` [PATCH v4 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest Victor Kamensky
2014-06-14 15:05 ` Christoffer Dall
2014-06-19 5:43 ` Victor Kamensky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.