All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] aarch64 BE kvm support
@ 2014-02-12  5:57 Victor Kamensky
  2014-02-12  5:57 ` [PATCH 1/5] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Victor Kamensky @ 2014-02-12  5:57 UTC (permalink / raw)
  To: linux-arm-kernel

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(-)

-- 
1.8.1.4

Thanks,
Victor

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/5] ARM64: KVM: MMIO support BE host running LE code
  2014-02-12  5:57 [PATCH 0/5] aarch64 BE kvm support Victor Kamensky
@ 2014-02-12  5:57 ` Victor Kamensky
  2014-03-20  3:41   ` Christoffer Dall
  2014-02-12  5:57 ` [PATCH 2/5] ARM64: KVM: set and get of sys registers in BE case Victor Kamensky
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Victor Kamensky @ 2014-02-12  5:57 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>
---
 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] 12+ messages in thread

* [PATCH 2/5] ARM64: KVM: set and get of sys registers in BE case
  2014-02-12  5:57 [PATCH 0/5] aarch64 BE kvm support Victor Kamensky
  2014-02-12  5:57 ` [PATCH 1/5] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
@ 2014-02-12  5:57 ` Victor Kamensky
  2014-03-20  3:41   ` Christoffer Dall
  2014-02-12  5:57 ` [PATCH 3/5] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word Victor Kamensky
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Victor Kamensky @ 2014-02-12  5:57 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/5] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word
  2014-02-12  5:57 [PATCH 0/5] aarch64 BE kvm support Victor Kamensky
  2014-02-12  5:57 ` [PATCH 1/5] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
  2014-02-12  5:57 ` [PATCH 2/5] ARM64: KVM: set and get of sys registers in BE case Victor Kamensky
@ 2014-02-12  5:57 ` Victor Kamensky
  2014-03-20  3:41   ` Christoffer Dall
  2014-02-12  5:57 ` [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
  2014-02-12  5:57 ` [PATCH 5/5] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
  4 siblings, 1 reply; 12+ messages in thread
From: Victor Kamensky @ 2014-02-12  5:57 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>
---
 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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
  2014-02-12  5:57 [PATCH 0/5] aarch64 BE kvm support Victor Kamensky
                   ` (2 preceding siblings ...)
  2014-02-12  5:57 ` [PATCH 3/5] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word Victor Kamensky
@ 2014-02-12  5:57 ` Victor Kamensky
  2014-02-12  7:15   ` Alexander Graf
  2014-03-20  3:42   ` Christoffer Dall
  2014-02-12  5:57 ` [PATCH 5/5] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
  4 siblings, 2 replies; 12+ messages in thread
From: Victor Kamensky @ 2014-02-12  5:57 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 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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/5] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case
  2014-02-12  5:57 [PATCH 0/5] aarch64 BE kvm support Victor Kamensky
                   ` (3 preceding siblings ...)
  2014-02-12  5:57 ` [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
@ 2014-02-12  5:57 ` Victor Kamensky
  2014-03-20  3:43   ` Christoffer Dall
  4 siblings, 1 reply; 12+ messages in thread
From: Victor Kamensky @ 2014-02-12  5:57 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 | 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,
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
  2014-02-12  5:57 ` [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
@ 2014-02-12  7:15   ` Alexander Graf
  2014-03-20  3:42   ` Christoffer Dall
  1 sibling, 0 replies; 12+ messages in thread
From: Alexander Graf @ 2014-02-12  7:15 UTC (permalink / raw)
  To: linux-arm-kernel


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/5] ARM64: KVM: MMIO support BE host running LE code
  2014-02-12  5:57 ` [PATCH 1/5] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
@ 2014-03-20  3:41   ` Christoffer Dall
  0 siblings, 0 replies; 12+ messages in thread
From: Christoffer Dall @ 2014-03-20  3:41 UTC (permalink / raw)
  To: linux-arm-kernel

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/5] ARM64: KVM: set and get of sys registers in BE case
  2014-02-12  5:57 ` [PATCH 2/5] ARM64: KVM: set and get of sys registers in BE case Victor Kamensky
@ 2014-03-20  3:41   ` Christoffer Dall
  0 siblings, 0 replies; 12+ messages in thread
From: Christoffer Dall @ 2014-03-20  3:41 UTC (permalink / raw)
  To: linux-arm-kernel

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
> 

-- 
Christoffer

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/5] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word
  2014-02-12  5:57 ` [PATCH 3/5] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word Victor Kamensky
@ 2014-03-20  3:41   ` Christoffer Dall
  0 siblings, 0 replies; 12+ messages in thread
From: Christoffer Dall @ 2014-03-20  3:41 UTC (permalink / raw)
  To: linux-arm-kernel

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case
  2014-02-12  5:57 ` [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
  2014-02-12  7:15   ` Alexander Graf
@ 2014-03-20  3:42   ` Christoffer Dall
  1 sibling, 0 replies; 12+ messages in thread
From: Christoffer Dall @ 2014-03-20  3:42 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 5/5] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case
  2014-02-12  5:57 ` [PATCH 5/5] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
@ 2014-03-20  3:43   ` Christoffer Dall
  0 siblings, 0 replies; 12+ messages in thread
From: Christoffer Dall @ 2014-03-20  3:43 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-03-20  3:43 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-12  5:57 [PATCH 0/5] aarch64 BE kvm support Victor Kamensky
2014-02-12  5:57 ` [PATCH 1/5] ARM64: KVM: MMIO support BE host running LE code Victor Kamensky
2014-03-20  3:41   ` Christoffer Dall
2014-02-12  5:57 ` [PATCH 2/5] ARM64: KVM: set and get of sys registers in BE case Victor Kamensky
2014-03-20  3:41   ` Christoffer Dall
2014-02-12  5:57 ` [PATCH 3/5] ARM64: KVM: store kvm_vcpu_fault_info est_el2 as word Victor Kamensky
2014-03-20  3:41   ` Christoffer Dall
2014-02-12  5:57 ` [PATCH 4/5] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case Victor Kamensky
2014-02-12  7:15   ` Alexander Graf
2014-03-20  3:42   ` Christoffer Dall
2014-02-12  5:57 ` [PATCH 5/5] ARM64: KVM: fix vgic_bitmap_get_reg function for BE 64bit case Victor Kamensky
2014-03-20  3:43   ` Christoffer Dall

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.