On 27 May 2014 08:02, Marc Zyngier marc.zyngier@arm.com wrote:

On 13/05/14 17:13, Victor Kamensky wrote:

...

The __kvm_vcpu_run function returns a 64-bit result in two registers, which has to be adjusted for BE case.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org Acked-by: Christoffer Dall christoffer.dall@linaro.org

---

arch/arm/kvm/interrupts.S | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S index 24d4e65..7dfe9e4 100644 --- a/arch/arm/kvm/interrupts.S +++ b/arch/arm/kvm/interrupts.S @@ -199,8 +199,13 @@ after_vfp_restore:

  restore_host_regs
  clrex                           @ Clear exclusive monitor

+#ifndef __ARMEB__ mov r0, r1 @ Return the return code mov r1, #0 @ Clear upper bits in return value +#else

• @ r1 already has return code
  

• mov     r0, #0                  @ Clear upper bits in return value
  

+#endif /* __ARMEB__ */

Why using __ARMEB__ while the rest of the series is using CONFIG_CPU_ENDIAN_BE8?

It felt like big endian ABI related issue, applicable to both BE8 and BE32. But ARM KVM itself applicable only to V7 BE8 so yes, it would not matter. I'll change it to CONFIG_CPU_ENDIAN_BE8 for consistency.

Thanks, Victor

...

  bx      lr                      @ return to IOCTL

/********************************************************************

Aside from the above nit:

Acked-by: Marc Zyngier marc.zyngier@arm.com

    M.

-- Jazz is not dead. It just smells funny...

On 13/05/14 17:13, Victor Kamensky wrote:

According to recent clarifications of mmio.data array meaning - the mmio.data array should hold bytes as they would appear in memory. Vgic is little endian device. And in case of BE image kernel side that emulates vgic, holds data in BE form. So we need to byteswap cpu<->le32 vgic registers when we read/write them from mmio.data[].

Change has no effect in LE case because cpu already runs in le32.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org

---

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

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 56ff9be..529c336 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -241,12 +241,12 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq) static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask) {

• return *((u32 *)mmio->data) & mask;

• return le32_to_cpu(*((u32 *)mmio->data)) & mask;

} static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value) {

• *((u32 *)mmio->data) = value & mask;

• *((u32 *)mmio->data) = cpu_to_le32(value) & mask;

} /**

Acked-by: Marc Zyngier marc.zyngier@arm.com

M.

On 13/05/14 17:13, Victor Kamensky wrote:

In case of status register E bit is not set (LE mode) and host runs in BE mode we need byteswap data, so read/write is emulated correctly.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org

---

arch/arm/include/asm/kvm_emulate.h | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h index 0fa90c9..69b7469 100644 --- a/arch/arm/include/asm/kvm_emulate.h +++ b/arch/arm/include/asm/kvm_emulate.h @@ -185,9 +185,16 @@ static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu, default: return be32_to_cpu(data); }

• } else {

• switch (len) {
  

• case 1:
  

• 	return data & 0xff;
  

• case 2:
  

• 	return le16_to_cpu(data & 0xffff);
  

• default:
  

• 	return le32_to_cpu(data);
  

• }
  

  }

• return data; /* Leave LE untouched */

} static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu, @@ -203,9 +210,16 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu, default: return cpu_to_be32(data); }

• } else {

• switch (len) {
  

• case 1:
  

• 	return data & 0xff;
  

• case 2:
  

• 	return cpu_to_le16(data & 0xffff);
  

• default:
  

• 	return cpu_to_le32(data);
  

• }
  

  }

• return data; /* Leave LE untouched */

} #endif /* __ARM_KVM_EMULATE_H__ */

Reviewed-by: Marc Zyngier marc.zyngier@arm.com

M.

On Tue, May 13, 2014 at 09:13:59AM -0700, Victor Kamensky wrote:

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

reg_from_user_to_u64 - reads register from user-land to kernel 'u64 *val' address; register size could be 4 or 8 bytes reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val' address; note it could be one or two 4 bytes registers reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to user-land register memory; register size could be 4 or 8 bytes ret_to_user_from_u32 - writes register(s) from kernel 'u32 *val' address to user-land register(s) memory; note it could be one or two 4 bytes registers

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 92 insertions(+), 26 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c index c58a351..5ca6582 100644 --- a/arch/arm/kvm/coproc.c +++ b/arch/arm/kvm/coproc.c @@ -682,17 +682,83 @@ static struct coproc_reg invariant_cp15[] = { { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR }, }; -static int reg_from_user(void *val, const void __user *uaddr, u64 id) +/*

• • Reads register value from user-land uaddr address into kernel u64 value
• • given by val address. Note register size could be 4 bytes, so user-land
• • 4 bytes value will be copied into least significant word. Or register
• • size could be 8 bytes, so function works as regular copy.
• */

Reads a register value from a userspace address to a kernel u64 variable. Note that the id may still encode a register size of 4 bytes, in which case only 4 bytes will be copied from userspace into the least significant word of *val.

+static int reg_from_user_to_u64(u64 *val, const void __user *uaddr, u64 id) +{

• unsigned long regsize = KVM_REG_SIZE(id);
• union {

• u32	word;
  

• u64	dword;
  

• } tmp = {0};
• if (copy_from_user(&tmp, uaddr, regsize) != 0)

• return -EFAULT;
  

• switch (regsize) {
• case 4:

• *val = tmp.word;
  

• break;
  

• case 8:

• *val = tmp.dword;
  

• break;
  

• }
• return 0;

+}

+/*

• • Reads register value from user-land uaddr address to kernel u32 value
• • or array of two u32 values. I.e. it may really copy two u32 registers
• • when used with register which size is 8 bytes (for example V7 64
• • bit registers like TTBR0/TTBR1).
• */

Reads a register value from a userspace address to a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

+static int reg_from_user_to_u32(u32 *val, const void __user *uaddr, u64 id) {

• /* This Just Works because we are little endian. */ if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0) return -EFAULT; return 0;

} -static int reg_to_user(void __user *uaddr, const void *val, u64 id) +/*

• • Writes register value to user-land uaddr address from kernel u64 value
• • given by val address. Note register size could be 4 bytes, so only 4
• • bytes from least significant word will by copied into uaddr address.
• • In case of 8 bytes sized register it works as regular copy.
• */

Writes a register value to a userspace address from a kernel u64 value. Note that the register size could be only 4 bytes, in which case only the least significant 4 bytes will be copied into to the userspace address.

+static int reg_to_user_from_u64(void __user *uaddr, const u64 *val, u64 id) +{

• unsigned long regsize = KVM_REG_SIZE(id);
• union {

• u32	word;
  

• u64	dword;
  

• } tmp;
• switch (regsize) {
• case 4:

• tmp.word = *val;
  

• break;
  

• case 8:

• tmp.dword = *val;
  

• break;
  

• }
• if (copy_to_user(uaddr, &tmp, regsize) != 0)

• return -EFAULT;
  

• return 0;

+}

+/*

• • Writes register value to user-land uaddr address from kernel u32 value
• • or arra of two u32 values. I.e it may really copy two u32 registers
• • when used with register which size is 8 bytes (for example V7 64
• • bit registers like TTBR0/TTBR1).
• */

Writes a register value to a userspace address from a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

+static int reg_to_user_from_u32(void __user *uaddr, const u32 *val, u64 id) {

• /* This Just Works because we are little endian. */ if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0) return -EFAULT; return 0;

@@ -710,7 +776,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr) if (!r) return -ENOENT;

• return reg_to_user(uaddr, &r->val, id);

• return reg_to_user_from_u64(uaddr, &r->val, id);

} static int set_invariant_cp15(u64 id, void __user *uaddr) @@ -726,7 +792,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr) if (!r) return -ENOENT;

• err = reg_from_user(&val, uaddr, id);

• err = reg_from_user_to_u64(&val, uaddr, id); if (err) return err;

@@ -894,8 +960,8 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr) if (vfpid < num_fp_regs()) { if (KVM_REG_SIZE(id) != 8) return -ENOENT;

• return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
  

• 		   id);
  

• return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
  

• 			    id);
  

  }

/* FP control registers are all 32 bit. */ @@ -904,22 +970,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr) switch (vfpid) { case KVM_REG_ARM_VFP_FPEXC:

• return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
  

• return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
  

  case KVM_REG_ARM_VFP_FPSCR:

• return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
  

• return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
  

  case KVM_REG_ARM_VFP_FPINST:

• return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
  

• return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
  

  case KVM_REG_ARM_VFP_FPINST2:

• return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
  

• return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
  

  case KVM_REG_ARM_VFP_MVFR0: val = fmrx(MVFR0);

• return reg_to_user(uaddr, &val, id);
  

• return reg_to_user_from_u32(uaddr, &val, id);
  

  case KVM_REG_ARM_VFP_MVFR1: val = fmrx(MVFR1);

• return reg_to_user(uaddr, &val, id);
  

• return reg_to_user_from_u32(uaddr, &val, id);
  

  case KVM_REG_ARM_VFP_FPSID: val = fmrx(FPSID);

• return reg_to_user(uaddr, &val, id);
  

• return reg_to_user_from_u32(uaddr, &val, id);
  

  default: return -ENOENT; }

@@ -938,8 +1004,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr) if (vfpid < num_fp_regs()) { if (KVM_REG_SIZE(id) != 8) return -ENOENT;

• return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
  

• 		     uaddr, id);
  

• return reg_from_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
  

• 			    uaddr, id);
  

  }

/* FP control registers are all 32 bit. */ @@ -948,28 +1014,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr) switch (vfpid) { case KVM_REG_ARM_VFP_FPEXC:

• return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
  

• return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
  

  case KVM_REG_ARM_VFP_FPSCR:

• return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
  

• return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
  

  case KVM_REG_ARM_VFP_FPINST:

• return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
  

• return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
  

  case KVM_REG_ARM_VFP_FPINST2:

• return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
  

• return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
  

  /* These are invariant. */ case KVM_REG_ARM_VFP_MVFR0:

• if (reg_from_user(&val, uaddr, id))
  

• if (reg_from_user_to_u32(&val, uaddr, id))
  return -EFAULT;
  

  if (val != fmrx(MVFR0)) return -EINVAL; return 0; case KVM_REG_ARM_VFP_MVFR1:

• if (reg_from_user(&val, uaddr, id))
  

• if (reg_from_user_to_u32(&val, uaddr, id))
  return -EFAULT;
  

  if (val != fmrx(MVFR1)) return -EINVAL; return 0; case KVM_REG_ARM_VFP_FPSID:

• if (reg_from_user(&val, uaddr, id))
  

• if (reg_from_user_to_u32(&val, uaddr, id))
  return -EFAULT;
  

  if (val != fmrx(FPSID)) return -EINVAL;

@@ -1016,7 +1082,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return get_invariant_cp15(reg->id, uaddr); /* Note: copies two regs if size is 64 bit. */

• return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);

• return reg_to_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);

} int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) @@ -1035,7 +1101,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return set_invariant_cp15(reg->id, uaddr); /* Note: copies two regs if size is 64 bit */

• return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);

• return reg_from_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);

} static unsigned int num_demux_regs(void) -- 1.8.1.4

It's definitely an improvement with the new naming, and I do appreciate you taking the time to write comments. (Apologies for suggesting a raw rewrite, but there were a number of language issues with the proposed versions and it felt overly burdensome on you to ask you to address all of them).

However, I'm still not crazy about this overall approach (see separate mail on the old thread).

I'm curious, how many special cases are there really, and how many places would we have to introduce a conditional on the calling side?

Thanks, -Christoffer

On Tue, May 27, 2014 at 11:19:59PM -0700, Victor Kamensky wrote:

On 25 May 2014 12:14, Christoffer Dall christoffer.dall@linaro.org wrote:

...

On Tue, May 13, 2014 at 09:13:59AM -0700, Victor Kamensky wrote:

...

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

reg_from_user_to_u64 - reads register from user-land to kernel 'u64 *val' address; register size could be 4 or 8 bytes reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val' address; note it could be one or two 4 bytes registers reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to user-land register memory; register size could be 4 or 8 bytes ret_to_user_from_u32 - writes register(s) from kernel 'u32 *val' address to user-land register(s) memory; note it could be one or two 4 bytes registers

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 92 insertions(+), 26 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c index c58a351..5ca6582 100644 --- a/arch/arm/kvm/coproc.c +++ b/arch/arm/kvm/coproc.c @@ -682,17 +682,83 @@ static struct coproc_reg invariant_cp15[] = { { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR }, };

-static int reg_from_user(void *val, const void __user *uaddr, u64 id) +/*

• • Reads register value from user-land uaddr address into kernel u64 value
• • given by val address. Note register size could be 4 bytes, so user-land
• • 4 bytes value will be copied into least significant word. Or register
• • size could be 8 bytes, so function works as regular copy.
• */

Reads a register value from a userspace address to a kernel u64 variable. Note that the id may still encode a register size of 4 bytes, in which case only 4 bytes will be copied from userspace into the least significant word of *val.

...

+static int reg_from_user_to_u64(u64 *val, const void __user *uaddr, u64 id) +{

• unsigned long regsize = KVM_REG_SIZE(id);
  

• union {
  

•         u32     word;
  

•         u64     dword;
  

• } tmp = {0};
  

• if (copy_from_user(&tmp, uaddr, regsize) != 0)
  

•         return -EFAULT;
  

• switch (regsize) {
  

• case 4:
  

•         *val = tmp.word;
  

•         break;
  

• case 8:
  

•         *val = tmp.dword;
  

•         break;
  

• }
  

• return 0;
  

+}

+/*

• • Reads register value from user-land uaddr address to kernel u32 value
• • or array of two u32 values. I.e. it may really copy two u32 registers
• • when used with register which size is 8 bytes (for example V7 64
• • bit registers like TTBR0/TTBR1).
• */

Reads a register value from a userspace address to a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

...

+static int reg_from_user_to_u32(u32 *val, const void __user *uaddr, u64 id) {

• /* This Just Works because we are little endian. */
  if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
          return -EFAULT;
  return 0;
  

}

-static int reg_to_user(void __user *uaddr, const void *val, u64 id) +/*

• • Writes register value to user-land uaddr address from kernel u64 value
• • given by val address. Note register size could be 4 bytes, so only 4
• • bytes from least significant word will by copied into uaddr address.
• • In case of 8 bytes sized register it works as regular copy.
• */

Writes a register value to a userspace address from a kernel u64 value. Note that the register size could be only 4 bytes, in which case only the least significant 4 bytes will be copied into to the userspace address.

...

+static int reg_to_user_from_u64(void __user *uaddr, const u64 *val, u64 id) +{

• unsigned long regsize = KVM_REG_SIZE(id);
  

• union {
  

•         u32     word;
  

•         u64     dword;
  

• } tmp;
  

• switch (regsize) {
  

• case 4:
  

•         tmp.word = *val;
  

•         break;
  

• case 8:
  

•         tmp.dword = *val;
  

•         break;
  

• }
  

• if (copy_to_user(uaddr, &tmp, regsize) != 0)
  

•         return -EFAULT;
  

• return 0;
  

+}

+/*

• • Writes register value to user-land uaddr address from kernel u32 value
• • or arra of two u32 values. I.e it may really copy two u32 registers
• • when used with register which size is 8 bytes (for example V7 64
• • bit registers like TTBR0/TTBR1).
• */

Writes a register value to a userspace address from a kernel u32 variable or a kernel array of two u32 values. That is, it may really copy two u32 registers when used with registers defined by the ABI to be 8 bytes long (e.g. TTBR0/TTBR1).

...

+static int reg_to_user_from_u32(void __user *uaddr, const u32 *val, u64 id) {

• /* This Just Works because we are little endian. */
  if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0)
          return -EFAULT;
  return 0;
  

@@ -710,7 +776,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr) if (!r) return -ENOENT;

• return reg_to_user(uaddr, &r->val, id);
  

• return reg_to_user_from_u64(uaddr, &r->val, id);
  

}

static int set_invariant_cp15(u64 id, void __user *uaddr) @@ -726,7 +792,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr) if (!r) return -ENOENT;

• err = reg_from_user(&val, uaddr, id);
  

• err = reg_from_user_to_u64(&val, uaddr, id);
  if (err)
          return err;
  
  

@@ -894,8 +960,8 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr) if (vfpid < num_fp_regs()) { if (KVM_REG_SIZE(id) != 8) return -ENOENT;

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                            id);
  

•         return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                                     id);
  }
  
  /* FP control registers are all 32 bit. */
  

@@ -904,22 +970,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)

  switch (vfpid) {
  case KVM_REG_ARM_VFP_FPEXC:

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
  case KVM_REG_ARM_VFP_FPSCR:
  

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
  case KVM_REG_ARM_VFP_FPINST:
  

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
  case KVM_REG_ARM_VFP_FPINST2:
  

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
  case KVM_REG_ARM_VFP_MVFR0:
          val = fmrx(MVFR0);
  

•         return reg_to_user(uaddr, &val, id);
  

•         return reg_to_user_from_u32(uaddr, &val, id);
  case KVM_REG_ARM_VFP_MVFR1:
          val = fmrx(MVFR1);
  

•         return reg_to_user(uaddr, &val, id);
  

•         return reg_to_user_from_u32(uaddr, &val, id);
  case KVM_REG_ARM_VFP_FPSID:
          val = fmrx(FPSID);
  

•         return reg_to_user(uaddr, &val, id);
  

•         return reg_to_user_from_u32(uaddr, &val, id);
  default:
          return -ENOENT;
  }
  

@@ -938,8 +1004,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr) if (vfpid < num_fp_regs()) { if (KVM_REG_SIZE(id) != 8) return -ENOENT;

•         return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                              uaddr, id);
  

•         return reg_from_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                                     uaddr, id);
  }
  
  /* FP control registers are all 32 bit. */
  

@@ -948,28 +1014,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)

  switch (vfpid) {
  case KVM_REG_ARM_VFP_FPEXC:

•         return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
  case KVM_REG_ARM_VFP_FPSCR:
  

•         return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
  case KVM_REG_ARM_VFP_FPINST:
  

•         return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
  case KVM_REG_ARM_VFP_FPINST2:
  

•         return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
  /* These are invariant. */
  case KVM_REG_ARM_VFP_MVFR0:
  

•         if (reg_from_user(&val, uaddr, id))
  

•         if (reg_from_user_to_u32(&val, uaddr, id))
                  return -EFAULT;
          if (val != fmrx(MVFR0))
                  return -EINVAL;
          return 0;
  case KVM_REG_ARM_VFP_MVFR1:
  

•         if (reg_from_user(&val, uaddr, id))
  

•         if (reg_from_user_to_u32(&val, uaddr, id))
                  return -EFAULT;
          if (val != fmrx(MVFR1))
                  return -EINVAL;
          return 0;
  case KVM_REG_ARM_VFP_FPSID:
  

•         if (reg_from_user(&val, uaddr, id))
  

•         if (reg_from_user_to_u32(&val, uaddr, id))
                  return -EFAULT;
          if (val != fmrx(FPSID))
                  return -EINVAL;
  

@@ -1016,7 +1082,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return get_invariant_cp15(reg->id, uaddr);

  /* Note: copies two regs if size is 64 bit. */

• return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
  

• return reg_to_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
  

}

int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) @@ -1035,7 +1101,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return set_invariant_cp15(reg->id, uaddr);

  /* Note: copies two regs if size is 64 bit */

• return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
  

• return reg_from_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
  

}

static unsigned int num_demux_regs(void)

1.8.1.4

It's definitely an improvement with the new naming, and I do appreciate you taking the time to write comments. (Apologies for suggesting a raw rewrite, but there were a number of language issues with the proposed versions and it felt overly burdensome on you to ask you to address all of them).

However, I'm still not crazy about this overall approach (see separate mail on the old thread).

I'm curious, how many special cases are there really, and how many places would we have to introduce a conditional on the calling side?

I've just posted as RFC updated version of the patch. And yes, you are right u64 regsize 4 access happens only in one place, I fixed as you suggested and got rid of regsize == 4 in _u64 functions.

Reading/wring array of two u32 values as regsize == 8 still remains I don't know how to handle it more cleanly.

The kernel-side caller has to put the value in a u64 (if it's a write), then call the user space 64-bit copying function, and then (if it's a read) convert that into the two u32s afterwards.

-Christoffer

On 27 May 2014 11:22, Marc Zyngier marc.zyngier@arm.com wrote:

On 13/05/14 17:13, Victor Kamensky wrote:

...

Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE image. Before this fix get/set_one_reg functions worked correctly only in LE case - reg_from_user was taking 'void *' kernel address that actually could be target/source memory of either 4 bytes size or 8 bytes size, and code copied from/to user memory that could hold either 4 bytes register, 8 byte register or pair of 4 bytes registers.

For example note that there was a case when 4 bytes register was read from user-land to kernel target address of 8 bytes value. Because it was working in LE, least significant word was memcpy(ied) and it just worked. In BE code with 'void *' as target/source 'val' type it is impossible to tell whether 4 bytes register from user-land should be copied to 'val' address itself (4 bytes target) or it should be copied to 'val' + 4 (least significant word of 8 bytes value). So first change was to introduce strongly typed functions, where type of target/source 'val' is strongly defined:

reg_from_user_to_u64 - reads register from user-land to kernel 'u64 *val' address; register size could be 4 or 8 bytes reg_from_user_to_u32 - reads register(s) from user-land to kernel 'u32 *val' address; note it could be one or two 4 bytes registers reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to user-land register memory; register size could be 4 or 8 bytes ret_to_user_from_u32 - writes register(s) from kernel 'u32 *val' address to user-land register(s) memory; note it could be one or two 4 bytes registers

All places where reg_from_user, reg_to_user functions were used, were changed to use either corresponding u64 or u32 variants of functions depending on type of source/target kernel memory variable.

In case of 'u64 *val' and register size equals 4 bytes, reg_from_user_to_u64 and reg_to_user_from_u64 work only with least siginificant word of target/source kernel value. It would be nice to deal only with u32 kernel values in _u32 functions and with u64 kernel values in _u64 functions, however it is not always possible because functions like set_invariant_cp15 get_invariant_cp15 store values in u64 types but registers are 32bit.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

arch/arm/kvm/coproc.c | 118 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 92 insertions(+), 26 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c index c58a351..5ca6582 100644 --- a/arch/arm/kvm/coproc.c +++ b/arch/arm/kvm/coproc.c @@ -682,17 +682,83 @@ static struct coproc_reg invariant_cp15[] = { { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR }, };

-static int reg_from_user(void *val, const void __user *uaddr, u64 id) +/*

• • Reads register value from user-land uaddr address into kernel u64 value
• • given by val address. Note register size could be 4 bytes, so user-land
• • 4 bytes value will be copied into least significant word. Or register
• • size could be 8 bytes, so function works as regular copy.
• */

+static int reg_from_user_to_u64(u64 *val, const void __user *uaddr, u64 id) +{

• unsigned long regsize = KVM_REG_SIZE(id);
  

• union {
  

•         u32     word;
  

•         u64     dword;
  

• } tmp = {0};
  

• if (copy_from_user(&tmp, uaddr, regsize) != 0)
  

•         return -EFAULT;
  

• switch (regsize) {
  

• case 4:
  

•         *val = tmp.word;
  

•         break;
  

• case 8:
  

•         *val = tmp.dword;
  

•         break;
  

• }
  

• return 0;
  

+}

+/*

• • Reads register value from user-land uaddr address to kernel u32 value
• • or array of two u32 values. I.e. it may really copy two u32 registers
• • when used with register which size is 8 bytes (for example V7 64
• • bit registers like TTBR0/TTBR1).
• */

+static int reg_from_user_to_u32(u32 *val, const void __user *uaddr, u64 id) {

• /* This Just Works because we are little endian. */
  if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
          return -EFAULT;
  return 0;
  

}

-static int reg_to_user(void __user *uaddr, const void *val, u64 id) +/*

• • Writes register value to user-land uaddr address from kernel u64 value
• • given by val address. Note register size could be 4 bytes, so only 4
• • bytes from least significant word will by copied into uaddr address.
• • In case of 8 bytes sized register it works as regular copy.
• */

+static int reg_to_user_from_u64(void __user *uaddr, const u64 *val, u64 id) +{

• unsigned long regsize = KVM_REG_SIZE(id);
  

• union {
  

•         u32     word;
  

•         u64     dword;
  

• } tmp;
  

• switch (regsize) {
  

• case 4:
  

•         tmp.word = *val;
  

•         break;
  

• case 8:
  

•         tmp.dword = *val;
  

•         break;
  

• }
  

• if (copy_to_user(uaddr, &tmp, regsize) != 0)
  

•         return -EFAULT;
  

• return 0;
  

+}

+/*

• • Writes register value to user-land uaddr address from kernel u32 value
• • or arra of two u32 values. I.e it may really copy two u32 registers
• • when used with register which size is 8 bytes (for example V7 64
• • bit registers like TTBR0/TTBR1).
• */

+static int reg_to_user_from_u32(void __user *uaddr, const u32 *val, u64 id) {

• /* This Just Works because we are little endian. */
  if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0)
          return -EFAULT;
  return 0;
  

@@ -710,7 +776,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr) if (!r) return -ENOENT;

• return reg_to_user(uaddr, &r->val, id);
  

• return reg_to_user_from_u64(uaddr, &r->val, id);
  

}

static int set_invariant_cp15(u64 id, void __user *uaddr) @@ -726,7 +792,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr) if (!r) return -ENOENT;

• err = reg_from_user(&val, uaddr, id);
  

• err = reg_from_user_to_u64(&val, uaddr, id);
  if (err)
          return err;
  
  

@@ -894,8 +960,8 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr) if (vfpid < num_fp_regs()) { if (KVM_REG_SIZE(id) != 8) return -ENOENT;

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                            id);
  

•         return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                                     id);
  }
  
  /* FP control registers are all 32 bit. */
  

@@ -904,22 +970,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)

  switch (vfpid) {
  case KVM_REG_ARM_VFP_FPEXC:

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
  case KVM_REG_ARM_VFP_FPSCR:
  

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
  case KVM_REG_ARM_VFP_FPINST:
  

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
  case KVM_REG_ARM_VFP_FPINST2:
  

•         return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
  

•         return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
  case KVM_REG_ARM_VFP_MVFR0:
          val = fmrx(MVFR0);
  

•         return reg_to_user(uaddr, &val, id);
  

•         return reg_to_user_from_u32(uaddr, &val, id);
  case KVM_REG_ARM_VFP_MVFR1:
          val = fmrx(MVFR1);
  

•         return reg_to_user(uaddr, &val, id);
  

•         return reg_to_user_from_u32(uaddr, &val, id);
  case KVM_REG_ARM_VFP_FPSID:
          val = fmrx(FPSID);
  

•         return reg_to_user(uaddr, &val, id);
  

•         return reg_to_user_from_u32(uaddr, &val, id);
  default:
          return -ENOENT;
  }
  

@@ -938,8 +1004,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr) if (vfpid < num_fp_regs()) { if (KVM_REG_SIZE(id) != 8) return -ENOENT;

•         return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                              uaddr, id);
  

•         return reg_from_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
  

•                                     uaddr, id);
  }
  
  /* FP control registers are all 32 bit. */
  

@@ -948,28 +1014,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)

  switch (vfpid) {
  case KVM_REG_ARM_VFP_FPEXC:

•         return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
  case KVM_REG_ARM_VFP_FPSCR:
  

•         return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
  case KVM_REG_ARM_VFP_FPINST:
  

•         return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
  case KVM_REG_ARM_VFP_FPINST2:
  

•         return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
  

•         return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
  /* These are invariant. */
  case KVM_REG_ARM_VFP_MVFR0:
  

•         if (reg_from_user(&val, uaddr, id))
  

•         if (reg_from_user_to_u32(&val, uaddr, id))
                  return -EFAULT;
          if (val != fmrx(MVFR0))
                  return -EINVAL;
          return 0;
  case KVM_REG_ARM_VFP_MVFR1:
  

•         if (reg_from_user(&val, uaddr, id))
  

•         if (reg_from_user_to_u32(&val, uaddr, id))
                  return -EFAULT;
          if (val != fmrx(MVFR1))
                  return -EINVAL;
          return 0;
  case KVM_REG_ARM_VFP_FPSID:
  

•         if (reg_from_user(&val, uaddr, id))
  

•         if (reg_from_user_to_u32(&val, uaddr, id))
                  return -EFAULT;
          if (val != fmrx(FPSID))
                  return -EINVAL;
  

@@ -1016,7 +1082,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return get_invariant_cp15(reg->id, uaddr);

  /* Note: copies two regs if size is 64 bit. */

• return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
  

• return reg_to_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
  

}

int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) @@ -1035,7 +1101,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return set_invariant_cp15(reg->id, uaddr);

  /* Note: copies two regs if size is 64 bit */

• return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
  

• return reg_from_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
  

}

static unsigned int num_demux_regs(void)

I really dislike this patch because of the asymmetric behaviour. Why can't we always use the "_u64" version?

Kernel target type could be u32. It code passes it as pointer to u64 it will do wrong things.

I've sent the most recent version of BE coproc handling patches as RFC, let's discuss it there. I think I addressed your questions/concerns in cover letter.

Thanks, Victor

Actually, why don't we use a construct similar to what we have for mmio_{read,write}_buf? This would give us a unified access method.

Thanks,

    M.

-- Jazz is not dead. It just smells funny...

On 26/05/14 18:35, Christoffer Dall wrote:

On Tue, May 13, 2014 at 09:14:04AM -0700, Victor Kamensky wrote:

...

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

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

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

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index 0620691..5035b41 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 ) str w4, [x3, #VGIC_CPU_HCR] str w5, [x3, #VGIC_CPU_VMCR] str w6, [x3, #VGIC_CPU_MISR] +#ifndef CONFIG_CPU_BIG_ENDIAN str w7, [x3, #VGIC_CPU_EISR] str w8, [x3, #(VGIC_CPU_EISR + 4)] str w9, [x3, #VGIC_CPU_ELRSR] str w10, [x3, #(VGIC_CPU_ELRSR + 4)] +#else

• str w7, [x3, #(VGIC_CPU_EISR + 4)]
• str w8, [x3, #VGIC_CPU_EISR]
• str w9, [x3, #(VGIC_CPU_ELRSR + 4)]
• str w10, [x3, #VGIC_CPU_ELRSR]

+#endif str w11, [x3, #VGIC_CPU_APR] /* Clear GICH_HCR */ -- 1.8.1.4

Hmm, the data structure doesn't actually clearly define this as bitmap, but the architecture clearly defines this as two registers. It feels like this critical code is getting over complicated to cater for the fact that we can use a convenient for_each_set_bit() function when we process a maintenance interrupt.

Marc, what are your thoughts on this? Change vgic_process_maintenance and clearly document the array on struct vgic_cpu, or go ahead with this change?

So I've already started abstracting access to ELRSR and EISR in my GICv3 patches, and we could perfectly stick the code swapping code in the accessors, letting the world switch alone.

These changes could easily be lifter from the series, and adapted. This would also make the "bitmap-ification" more obvious.

M.

Hi Victor,

On Tue, May 13 2014 at 5:14:05 pm BST, Victor Kamensky victor.kamensky@linaro.org wrote:

This patch addresses issue of reading and writing V8 sys registers in BE case. Since only register size function deals with is 8 bytes, existing code works in both little and big endian cases. Removed comment about little endian. Added BUG_ON that register size should be always 8 bytes.

If these functions would ever need to support both 8 bytes and 4 bytes register sizes to deals with them in endian agnostic way code should do something along these lines:

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

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

arch/arm64/kvm/sys_regs.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 0324458..060c3a9 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -776,18 +776,25 @@ 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);
• BUG_ON(regsize != 8);

I haven't had time to review this series just yet, but this bit just sends chivers down my spine.

regsize is derived from id, which comes from a struct one_reg, which is directly provided by userspace. Here, you're trusting the luser to give you 8 as a size, and panic the kernel if not.

As much as I'd like to qualify this as only being a slightly undesirable effect, I think it deserves a NAK.

Thanks,

M.

On Tue, May 13, 2014 at 09:14:06AM -0700, Victor Kamensky wrote:

Fix isssue with 32bit guests running on top of BE KVM host. Guest state is retored with double word read operations. Within the high

restored (spell check should catch this).

"Guest state is restored with double word read operations." I don't know what this sentence is supposed to tell me.

and low word data is already byteswap. This code effectively swaps

"data is already byteswap" is not English. data is already byteswapped?

two words within 64bit value.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

arch/arm64/kvm/sys_regs.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 060c3a9..4438b47 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -51,6 +51,16 @@ static u32 cache_levels; /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */ #define CSSELR_MAX 12 +/* Word access indexes for 64bit vm accessor */ +#ifdef CONFIG_CPU_BIG_ENDIAN +#define CP15_REG_MSW_INDEX 0 +#define CP15_REG_LSW_INDEX 1 +#else +#define CP15_REG_MSW_INDEX 1 +#define CP15_REG_LSW_INDEX 0 +#endif

/* Which cache CCSIDR represents depends on CSSELR value. */ static u32 get_ccsidr(u32 csselr) { @@ -137,9 +147,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(vcpu, r->reg + CP15_REG_LSW_INDEX) = val & 0xffffffffUL;
  

  if (!p->is_32bit)

• 	vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
  

• 	vcpu_cp15(vcpu, r->reg + CP15_REG_MSW_INDEX) = val >> 32;
  

  } return true;

}

1.8.1.4

I really don't like this. If anything I feel like it should be abstracted inside vcpu_cp15, but wouldn't it be cleaner to do something along the lines of:

u64 *regstore = (u64 *)vcpu->arch.ctxt.cp15[r->reg]; if (p->is_32bit) val &= 0xffffffffUL; *regstore = val;

But I think the thing that bothers me in general with all these patches is that they deal specially with a lot of situations where the data structure was designed specifically to make the code easy to read, and now it just becomes a really complicated mess. Have you carefully considered other options, redesigning the data structure layout etc.?

-Christoffer

On Tue, May 27, 2014 at 11:11:57PM -0700, Victor Kamensky wrote:

On 26 May 2014 10:52, Christoffer Dall christoffer.dall@linaro.org wrote:

...

On Tue, May 13, 2014 at 09:14:06AM -0700, Victor Kamensky wrote:

...

Fix isssue with 32bit guests running on top of BE KVM host. Guest state is retored with double word read operations. Within the high

restored (spell check should catch this).

"Guest state is restored with double word read operations." I don't know what this sentence is supposed to tell me.

...

and low word data is already byteswap. This code effectively swaps

"data is already byteswap" is not English. data is already byteswapped?

...

two words within 64bit value.

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

arch/arm64/kvm/sys_regs.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 060c3a9..4438b47 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -51,6 +51,16 @@ static u32 cache_levels; /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */ #define CSSELR_MAX 12

+/* Word access indexes for 64bit vm accessor */ +#ifdef CONFIG_CPU_BIG_ENDIAN +#define CP15_REG_MSW_INDEX 0 +#define CP15_REG_LSW_INDEX 1 +#else +#define CP15_REG_MSW_INDEX 1 +#define CP15_REG_LSW_INDEX 0 +#endif

/* Which cache CCSIDR represents depends on CSSELR value. */ static u32 get_ccsidr(u32 csselr) { @@ -137,9 +147,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(vcpu, r->reg + CP15_REG_LSW_INDEX) = val & 0xffffffffUL;
          if (!p->is_32bit)
  

•                 vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
  

•                 vcpu_cp15(vcpu, r->reg + CP15_REG_MSW_INDEX) = val >> 32;
  }
  return true;
  

}

1.8.1.4

I really don't like this. If anything I feel like it should be abstracted inside vcpu_cp15,

good point, please see revised proposal below

...

but wouldn't it be cleaner to do something along the lines of:

u64 *regstore = (u64 *)vcpu->arch.ctxt.cp15[r->reg]; if (p->is_32bit) val &= 0xffffffffUL; *regstore = val;

I don't think above will be correct. The way I read it the following hypothetical instructions sequence

mcrr p15, 0, r6, r7, c2 @ TTBR 0 mcr p15, 0, r6, c2, c0, 0 @ 32bit TTBR 0

will get TTBR 0 high word to 0, when mrc instruction executes, but it should be left unchanged in case of 'p->is_32bit'

How about something like following? Is it move into right direction?

From b0a7793b03d9c62f7b9c53a317cb2d19a75c935b Mon Sep 17 00:00:00 2001 From: Victor Kamensky victor.kamensky@linaro.org Date: Mon, 12 May 2014 13:57:21 -0700 Subject: [PATCH] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest

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

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

Signed-off-by: Victor Kamensky victor.kamensky@linaro.org

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

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 0a1d697..e9d2e11 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -140,6 +140,14 @@ struct kvm_vcpu_arch { #define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)]) #define vcpu_cp15(v,r) ((v)->arch.ctxt.cp15[(r)])

+#ifdef CONFIG_CPU_BIG_ENDIAN +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 0)]) +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)]) +#else +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 1)]) +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)]) +#endif

struct kvm_vm_stat { u32 remote_tlb_flush; }; diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index a13e7e7..b243e07 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -137,9 +137,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu, if (!p->is_aarch32) { vcpu_sys_reg(vcpu, r->reg) = val; } else {

•    vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL;
  

•    vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL;
     if (!p->is_32bit)
  

•        vcpu_cp15(vcpu, r->reg + 1) = val >> 32;
  

•        vcpu_cp15_64_high(vcpu, r->reg) = val >> 32;
  

  } return true;

}

1.8.1.4

This is definitely much better than the original patch.

...

But I think the thing that bothers me in general with all these patches is that they deal specially with a lot of situations where the data structure was designed specifically to make the code easy to read, and now it just becomes a really complicated mess. Have you carefully considered other options, redesigning the data structure layout etc.?

I have considered different options and I am open for suggestions. Bytesswaps quite often could be done in different places but achieve the same result. In several cases I initially developed patch that deals with BE issue in one way and reworked to make it more compact less intrusive. For example in this particular case order of high and low words of 64bit cp15 register could be kept the same in BE/LE cases but code that save/restore it could be changed to do byteswap. My opinion that proposed option is more clean.

What I was thinking is to avoid packing 64-bit values as two 32-bit values, so for example having two separate arrays on the kvm_cpu_context struct, one for 32-bit coprocessor registers and one for 64-bit coprocessor registers.

That would sort of follow Benjamin Herrenschmidt's manatra about always using the correctly typed values everywhere to avoid endianness issues.

Did you look at that approach? Does it blow up?

-Christoffer