Hi Guys,
Here is series that enable KVM in V8 BE image. It is addition on top of previously posted V7 BE KVM support [1].
It was tested on aarch64 fastmodels and APM mustang board. It was tested only with kvmtool at this point. In case of V8 BE KVM host was tested that V8 BE guest runs fine and V8 LE guest runs too. Also V8 LE KVM regression was tested on both V8 LE guest and V8 BE guest. Note for mixed mode Marc's kvmtool was used and guest image had minor change that treats all virtio in LE form.
Note first two patches are similar to V7 BE KVM patches. Last three are new specific for V8 image.
Thanks, Victor
[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-February/009446.html
Victor Kamensky (5): ARM64: KVM: MMIO support BE host running LE code ARM64: KVM: set and get of sys registers in BE case ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word ARM64: KVM: vgic_elrsr and vgic_eisr need to by byteswapped in BE case ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case
arch/arm64/include/asm/kvm_emulate.h | 22 ++++++++++++++++++++ arch/arm64/kvm/hyp.S | 9 ++++++++- arch/arm64/kvm/sys_regs.c | 39 ++++++++++++++++++++++++++++++------ virt/kvm/arm/vgic.c | 27 +++++++++++++++++++++++-- 4 files changed, 88 insertions(+), 9 deletions(-)
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 --- 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 */
On Tue, Feb 11, 2014 at 09:57:19PM -0800, Victor Kamensky wrote:
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
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
Again, given the ABI merge:
Reviewed-by: Christoffer Dall christoffer.dall@linaro.org
This patch fixes issue of reading and writing V8 sys registers in BE case. It is similar to V7 "ARM: kvm one_reg coproc set and get BE fixes" patch.
It changes reg_from_user and reg_to_user functions to have strong typed 'u64 *val' argument. And it uses endian angnostic way to pick up righ word from '*val' in case when register size is 4 bytes.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org --- arch/arm64/kvm/sys_regs.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 02e9d09..e7c3e24 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -701,18 +701,45 @@ 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) + unsigned long regsize = KVM_REG_SIZE(id); + union { + u32 word; + u64 dword; + } tmp = {0}; + + if (copy_from_user(&tmp, uaddr, regsize) != 0) return -EFAULT; + switch (regsize) { + case 4: + *val = tmp.word; + break; + case 8: + *val = tmp.dword; + break; + } return 0; }
-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) + unsigned long regsize = KVM_REG_SIZE(id); + union { + u32 word; + u64 dword; + } tmp; + + switch (regsize) { + case 4: + tmp.word = *val; + break; + case 8: + tmp.dword = *val; + break; + } + + if (copy_to_user(uaddr, &tmp, regsize) != 0) return -EFAULT; return 0; }
On Tue, Feb 11, 2014 at 09:57:20PM -0800, Victor Kamensky wrote:
This patch fixes issue of reading and writing V8 sys registers in BE case. It is similar to V7 "ARM: kvm one_reg coproc set and get BE fixes" patch.
It changes reg_from_user and reg_to_user functions to have strong typed 'u64 *val' argument. And it uses endian angnostic way to pick up righ word from '*val' in case when register size is 4 bytes.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org
arch/arm64/kvm/sys_regs.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 02e9d09..e7c3e24 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -701,18 +701,45 @@ 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)
- unsigned long regsize = KVM_REG_SIZE(id);
- union {
u32 word;u64 dword;- } tmp = {0};
- if (copy_from_user(&tmp, uaddr, regsize) != 0) return -EFAULT;
- switch (regsize) {
- case 4:
*val = tmp.word;break;
This should never happen for arm64, right? IIRC, we expose all system registers, even the aarch32 ones, as 64-bit versions with padded zeros, just like in the ARM ARM...
- case 8:
*val = tmp.dword;break;- } 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)
- unsigned long regsize = KVM_REG_SIZE(id);
- union {
u32 word;u64 dword;- } tmp;
- switch (regsize) {
- case 4:
tmp.word = *val;break;
same
- case 8:
tmp.dword = *val;break;- }
- if (copy_to_user(uaddr, &tmp, regsize) != 0) return -EFAULT; return 0;
}
1.8.1.4
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 --- 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 3b47c36..104216c 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -801,7 +801,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]
On Tue, Feb 11, 2014 at 09:57:21PM -0800, Victor Kamensky wrote:
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
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 3b47c36..104216c 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -801,7 +801,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
Ouch,
Acked-by: Christoffer Dall christoffer.dall@linaro.org
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 104216c..667293f 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 */
On 12.02.2014, at 06:57, Victor Kamensky victor.kamensky@linaro.org 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 104216c..667293f 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
How about you introduce an asm macro like STRW_64(w7, w8, x3, VGIC_CPU_EISR) which could expand to the two respectively swapped instructions?
Alex
On Tue, Feb 11, 2014 at 09:57:22PM -0800, 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 104216c..667293f 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
Isn't it the vgic emulation code that's incorrect then? The GICv2 hardware defines two registers, GICH_ELRSR0 and GICH_ELRSR1 (and GICH_EISR0 and GICH_EISR1) and I would find it most logic that vgic_cpu->elrsr[0] == GICH_ELRSR0, always.
Marc?
-Christoffer
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 | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 7e11458..e56c5f8 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -96,14 +96,37 @@ static u32 vgic_nr_lr;
static unsigned int vgic_maint_irq;
+/* + * struct vgic_bitmap is union that provides two view of + * the same data. In one case it is array of registers of + * u32 type (.reg). And in another it is bitmap, which is + * array of 'unsgined long' (.reg_ul). It works all well in + * case of 32bit (u32 and 'unsigned long' have the same size). + * It works ok in 64bit LE case, where 'unsigned long' + * size is 8 bytes, while u32 is 4 bytes, and least siginificant + * word of 'unsigned long' matches lower index of .reg array. + * It breaks in 64bit BE case. In this case word sized + * register of even index actually resides in least significant + * word of 'unsigned long' which has address at offset plus 4 + * bytes. And word sized register of odd index resides at most + * significant of 'unsigned long' which has offset minus 4 + * bytes. Define REG_OFFSET_SWIZZLE that would help to + * change offset of register in case of BE 64bit system. + */ +#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,
On Tue, Feb 11, 2014 at 09:57:23PM -0800, 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 | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 7e11458..e56c5f8 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -96,14 +96,37 @@ static u32 vgic_nr_lr; static unsigned int vgic_maint_irq; +/*
- struct vgic_bitmap is union that provides two view of
contains untions that provide two views
- the same data. In one case it is array of registers of
an array
- u32 type (.reg). And in another it is bitmap, which is
, and in the other case it is a bitmap, which is an array...
- array of 'unsgined long' (.reg_ul). It works all well in
It all works well on 32-bit
- case of 32bit (u32 and 'unsigned long' have the same size).
- It works ok in 64bit LE case, where 'unsigned long'
It also works well on 64-bit LE, but breaks on 64-bit BE.
- size is 8 bytes, while u32 is 4 bytes, and least siginificant
- word of 'unsigned long' matches lower index of .reg array.
Drop these two lines.
- It breaks in 64bit BE case. In this case word sized
- register of even index actually resides in least significant
- word of 'unsigned long' which has address at offset plus 4
- bytes. And word sized register of odd index resides at most
- significant of 'unsigned long' which has offset minus 4
- bytes. Define REG_OFFSET_SWIZZLE that would help to
- change offset of register in case of BE 64bit system.
- */
+#if defined(CONFIG_CPU_BIG_ENDIAN) && BITS_PER_LONG == 64 +#define REG_OFFSET_SWIZZLE 1 +#else +#define REG_OFFSET_SWIZZLE 0 +#endif
Wondering if it's worth the trouble in this case of having the union; the union is there only for this function to be simpler, but if it doesn't work for BE, then maybe it's not worth it?
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);
you need spaces around the '^' according to CodingStyle.
else
return x->shared.reg + offset - 1;
return x->shared.reg + ((offset - 1)^REG_OFFSET_SWIZZLE);
ditto
} static int vgic_bitmap_get_irq_val(struct vgic_bitmap *x, -- 1.8.1.4
Functionally, this looks correct to me.
-Christoffer
linaro-kernel@lists.linaro.org
-
Alexander Graf -
Christoffer Dall -
Victor Kamensky