ARM/ARM64: KVM: big endian host support

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 <[email protected]>

---
 arch/arm/kvm/coproc.c | 84 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 9 deletions(-)

arch/arm/kvm/coproc.c +75 -9

index c58a351..bcc9a0f 100644
@@ -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, &params))
 		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, &params))
 		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)

Signature · 1 line

1.8.1.4

Previous patches addresses ARMV7 big-endian virtualiztion, kvm related issues, so enable ARM_VIRT_EXT for big-endian now.

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

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

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

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

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

index 529c336..b4ffd82 100644
@@ -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,

Signature · 1 line

1.8.1.4

Quoted 52 lines · Victor Kamensky

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

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

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

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

index 529c336..b4ffd82 100644
@@ -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,

Signature · 1 line

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 <[email protected]>

because it looks correct.

Thanks for picking up my suggestion on the commenting.

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

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

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

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

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

Signature · 1 line

1.8.1.4

Quoted 31 lines · Victor Kamensky

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

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

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

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

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

Quoted 4 lines

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

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

Quoted 32 lines · Christoffer Dall

Quoted 31 lines · Victor Kamensky

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

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

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

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

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

Quoted 6 lines

Quoted 4 lines

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

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

Quoted 61 lines · Victor Kamensky

Quoted 32 lines · Christoffer Dall

Quoted 31 lines · Victor Kamensky

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

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

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

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

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

Quoted 6 lines

Quoted 4 lines

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

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

Quoted 3 lines

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/

Christoffer, Marc,

Please see inline. I am looking for your opinion/advise on how we go further about this patch.

Quoted 62 lines · Christoffer Dall

Quoted 61 lines · Victor Kamensky

Quoted 32 lines · Christoffer Dall

Quoted 31 lines · Victor Kamensky

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

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

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

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

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

Quoted 6 lines

Quoted 4 lines

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

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

Quoted 5 lines

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

arch/arm64/kvm/vgic-v2-switch.S +0 -7

index d5fc5aa..ae21177 100644
@@ -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 */

virt/kvm/arm/vgic-v2.c +18 -2

index a55a9a4..a4b6f13 100644
@@ -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.

Hi Victor,

Quoted 173 lines · Victor Kamensky

Christoffer, Marc,

Please see inline. I am looking for your opinion/advise on how we go further about this patch.

Quoted 62 lines · Christoffer Dall

Quoted 61 lines · Victor Kamensky

Quoted 32 lines · Christoffer Dall

Quoted 31 lines · Victor Kamensky

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

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

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

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

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

Quoted 6 lines

Quoted 4 lines

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

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

Quoted 5 lines

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

arch/arm64/kvm/vgic-v2-switch.S +0 -7

index d5fc5aa..ae21177 100644
@@ -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 */

virt/kvm/arm/vgic-v2.c +18 -2

index a55a9a4..a4b6f13 100644
@@ -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.

Quoted 1 line

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.

Quoted 176 lines · Marc Zyngier

Hi Victor,

Quoted 173 lines · Victor Kamensky

Christoffer, Marc,

Please see inline. I am looking for your opinion/advise on how we go further about this patch.

Quoted 62 lines · Christoffer Dall

Quoted 61 lines · Victor Kamensky

Quoted 32 lines · Christoffer Dall

Quoted 31 lines · Victor Kamensky

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

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

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

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

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

Quoted 6 lines

Quoted 4 lines

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

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

Quoted 5 lines

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

arch/arm64/kvm/vgic-v2-switch.S +0 -7

index d5fc5aa..ae21177 100644
@@ -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 */

virt/kvm/arm/vgic-v2.c +18 -2

index a55a9a4..a4b6f13 100644
@@ -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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 184 lines · Victor Kamensky

Quoted 176 lines · Marc Zyngier

Hi Victor,

Quoted 173 lines · Victor Kamensky

Christoffer, Marc,

Please see inline. I am looking for your opinion/advise on how we go further about this patch.

Quoted 62 lines · Christoffer Dall

Quoted 61 lines · Victor Kamensky

Quoted 32 lines · Christoffer Dall

Quoted 31 lines · Victor Kamensky

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

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

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

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

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

Quoted 6 lines

Quoted 4 lines

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

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

Quoted 5 lines

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

arch/arm64/kvm/vgic-v2-switch.S +0 -7

index d5fc5aa..ae21177 100644
@@ -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 */

virt/kvm/arm/vgic-v2.c +18 -2

index a55a9a4..a4b6f13 100644
@@ -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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 9 lines

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

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

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

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

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

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

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

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

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

Signature · 1 line

1.8.1.4

Quoted 48 lines · Victor Kamensky

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

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

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

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

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

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

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

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

Quoted 4 lines

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

 }
--

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