Hi Victor,
On Fri, Dec 20, 2013 at 08:48:43AM -0800, Victor Kamensky wrote:
> This patch fixes issue of reading and writing
interesting line break.
an issue with
> ARM V7 registers values from/to user land. Existing code was designed to
> work only in LE case.
The existing code...
'LE case'? 'little-endian'?
>
> struct kvm_one_reg
> ------------------
>
> registers value passed through kvm_one_reg structure. It is used by
registers value passed through? What are you trying to say?
> KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls. Note by looking at structure
by the KVM...
the structure
> itself we cannot tell what is size of register. Note that structure carries
the size of the register
> address of user memory, 'addr' where register should be read or written
I'm a little confused as to the value of this Section of the commit
text. I believe the ONE_REG interface is quite well documented
already...
>
> Setting register (from user-land to kvm)
> ----------------------------------------
>
> kvm_arm_set_reg takes vcpu and pointer to struct kvm_one_reg which already
> read from user space
I think you could ditch this first sentence
>
> kvm_arm_set_reg calls set_core_reg or kvm_arm_coproc_set_reg
nit: adding kvm_arm_set_reg() makes it clear that this is the function
you're refering to, and not the ioctl as a concept.
>
> set_core_reg deals only with 4 bytes registers, it just reads 4 bytes from
> user space and store it properly into vcpu->arch.regs
stores
>
> kvm_arm_coproc_set_reg deals with registers of different size. At certain
different sizes
At a certain point
> point code reaches phase where it retrieves description of register by id
the description of a register
> and it knows register size, which could be either 4 or 8 bytes. Kernel code
s/could be/is/
Kernel code is ready?
> is ready to read values from user space, but destination type may vary. It
> could be pointer to 32 bit integer or it could be pointer to 64 bit
> integer. And all possible permutation of size and destination pointer are
permutations
> possible. Depending on destination pointer type, 4 bytes or 8 bytes, two
the destination pointer type
> new helper functions are introduced - reg_from_user32 and reg_from_user64.
> They are used instead of reg_from_user function which could work only in
> LE case.
which only worked in
>
> Size sizeof(*DstInt) Function used to read from user
> 4 4 reg_from_user32
> 8 4 reg_from_user32 - read two registers
> 4 8 reg_from_user64 - need special handling for BE
> 8 8 reg_from_user64
>
> Getting register (to user-land from kvm)
> ----------------------------------------
>
> Situation with reading registers is similar to writing. Integer pointer
The situation
> type of register to be copied could be 4 or 8 bytes. And size passed in
The integer pointer
pointer to be copied? Please clarify what you are referring to.
> struct kvm_one_reg could be 4 or 8. And any permutation is possible.
Any permutation of source pointer type and size is possible.
> Depending on src pointer type, 4 bytes or 8 bytes, two new helper functions
> are introduced - reg_from_user32 and reg_to_user64. They are used instead
reg_to_user32?
> of reg_to_user function, which could work only in LE case.
the reg_to_user, which worked only for LE.
>
> Size sizeof(*SrcInt) Function used to write to user
> 4 4 reg_to_user32
> 8 4 reg_to_user32 - writes two registers
> 4 8 reg_to_user64 - need special handleing for BE
> 8 8 reg_to_user64
I think it could be slightly more helpful to put a comment on the
functions, like "Write to 32-bit user pointer" on reg_to_user32, but
it's up to you.
>
> Note code does assume that it can only deals with 4 or 8 byte registers.
Note: We only support register sizes of 4 or 8 bytes.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@xxxxxxxxxx>
I don't mean to hammer on your commit message with all my comments. I
really do appreciate you taking the time to document your changes.
However, with the level of detail you are providing in the commit, I
think you have to be slightly more careful with the language, so that it
doesn't end up being misleading instead of helpful. I think you could
sum this up much shorter to simply say that core register handling is
already endian-safe, but coprocessors and vfpregs use reg_to_user which
is not endian-safe, and therefore needs changing.
The motivation about the pointer types and register sizes being
arbitrarily different is important though, so I appreciate you listing
that.
> ---
> arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 69 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 78c0885..64b2b94 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -634,17 +634,61 @@ 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)
> +static int reg_from_user64(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;
> +}
You stated in the commit message that any permutation of
KVM_REG_SIZE(id) and sizeof(*val) is possible.
So doesn't this totally mess up the the kernel if I pass a 32-bit
pointer to reg_from_user64? Or is that not really the case and that's
an exception to all of the permutations?
Basically you KVM_REG_SIZE(id) and sizeof your destination pointer type
should always match, but we abuse this slightly so far. I don't think
you should cater to that, but just require callers to always provide a
consistent size/type pair (you could also add a union you use as a
parameter instead, or have two typed parameter) and simplify into a
single function.
The only special cases you now have to deal with are in:
set_invariant_cp15(): declare two temp variables of u32 and u64 sizes
get_invariant_cp15(): either have temporary values or change val in
corproc_reg to be a union
The current scheme is pretty hard to understand and to make sure we're
not breaking anything...
> +
> +/* Note it may really copy two u32 registers */
> +static int reg_from_user32(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)
> +static int reg_to_user64(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;
> +}
> +
> +/* Note it may really copy two u32 registers */
> +static int reg_to_user32(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;
> @@ -662,7 +706,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_user64(uaddr, &r->val, id);
> }
>
> static int set_invariant_cp15(u64 id, void __user *uaddr)
> @@ -678,7 +722,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_user64(&val, uaddr, id);
> if (err)
> return err;
>
> @@ -846,7 +890,7 @@ 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],
> + return reg_to_user64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
> id);
> }
>
> @@ -856,22 +900,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_user32(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_user32(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_user32(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_user32(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_user32(uaddr, &val, id);
> case KVM_REG_ARM_VFP_MVFR1:
> val = fmrx(MVFR1);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user32(uaddr, &val, id);
> case KVM_REG_ARM_VFP_FPSID:
> val = fmrx(FPSID);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user32(uaddr, &val, id);
> default:
> return -ENOENT;
> }
> @@ -890,8 +934,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_user64(&vcpu->arch.vfp_guest.fpregs[vfpid],
> + uaddr, id);
> }
>
> /* FP control registers are all 32 bit. */
> @@ -900,28 +944,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_user32(&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_user32(&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_user32(&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_user32(&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_user32(&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_user32(&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_user32(&val, uaddr, id))
> return -EFAULT;
> if (val != fmrx(FPSID))
> return -EINVAL;
> @@ -968,7 +1012,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_user32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
> }
>
> int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> @@ -987,7 +1031,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_user32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
> }
>
> static unsigned int num_demux_regs(void)
> --
> 1.8.1.4
>
Thanks,
--
Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
