On Thu, May 29, 2014 at 09:44:42PM -0700, Victor Kamensky wrote:
> On 29 May 2014 08:30, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
> > On Wed, May 28, 2014 at 11:05:42PM -0700, Victor Kamensky wrote:
[...]
> >> +/*
> >> + * Writes a register value to a userspace address from a kernel variable.
> >> + * Note register size must match sizeof(*__val).
> >> + */
> >> +#define reg_to_user(__uaddr, __val, __id) ({ \
> >> + int __ret = 0; \
> >> + unsigned long __regsize = KVM_REG_SIZE(__id); \
> >> + if (sizeof(*(__val)) != __regsize) { \
> >> + __ret = -ENOENT; \
> >> + } else { \
> >> + if (copy_to_user((__uaddr), (__val), __regsize) != 0) \
> >> + __ret = -EFAULT; \
> >> + } \
> >> + __ret; \
> >> +})
> >
> > Ahh, no. Please don't. If you really think we want checks that we know
> > how to program, then do the _u32 and _u64 versions and have a BUG_ON()
> > in there if the KVM_REG_SIZE does not match.
>
> No BUG_ON should be done on data that is driven by userspace.
> I got burn on that once already, but fortunately got caught in
> review :).
That's because it was not sanitized. If you mean to have always checked
prior to calling this function it is fine to put a BUG_ON.
>
> > I don't think this is needed though, because the special (oddball as you
> > call them) cases are so rare.
>
> OK, we can live without defense mechanism for now and
> come back to it if breakage occur. LE will not notice an issue in
> this area, it will come up only in BE case. Testing will catch it.
>
ok.
> >>
> >> 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, ¶ms))
> >> return -ENOENT;
> >> @@ -710,7 +727,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 +742,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, ¶ms))
> >> return -ENOENT;
> >> @@ -726,7 +750,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 +1036,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 +1048,24 @@ 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;
> >> + /* Note: copies two u32 regs */
> >> + memcpy(&val, &(vcpu->arch.cp15[r->reg]), sizeof(val));
> >
> > Are you not creating a wrongly ordered u64 on BE systems here? Surely
> > the least significant word will be at the lowest address.
>
> Good catch. memcpy logic is the same that I had before,
> but I was wondering about it yesterday myself, so I looked
> at both directions (hypervisor/emulator) from this code today.
> Yes, there is an issue in the code.
>
> It is a bit long description, please bear with me,
> I would like to get your opinion on that.
>
> It depends what is canonical form. In current case it is
> pair of u32 registers and their indexes are
>
> #define c2_TTBR0 6 /* Translation Table Base Register 0 */
> #define c2_TTBR0_high 7 /* TTBR0 top 32 bits */
> #define c2_TTBR1 8 /* Translation Table Base Register 1 */
> #define c2_TTBR1_high 9 /* TTBR1 top 32 bits */
>
> i.e high word index follows low word. And guest
> restore/save code matches this view. For example
>
> add r12, vcpu, #CP15_OFFSET(c2_TTBR0)
> ldrd r6, r7, [r12]
> ...
> mcrr p15, 0, r6, r7, c2 @ TTBR 0
>
> ldrd does not swap words (i.e @r12 word goes to r6 (low TTBR0),
> and @r12+4 goes into r7 (high TTBR0) and it matches mcrr low,
> high. So hypervisor save/restore context will get it right.
>
> and in guest emulation program .. does not get it right.
> Current emulator code does not really look at received values it
> just stores it back. That is why it was not noticed.
>
> Now to fix it first option if we keep current canonical form
> (two u32 register low word first followed by high word) we
> can change memcpy like this so BE emulator (userspace)
> will see correct value
>
> val = (vcpu->arch.cp15[r->reg + 1] << 32) |
> vcpu->arch.cp15[r->reg];
> Effectively it will be memcpy in LE case and word byteswap
> in BE case
>
> and for set similar changes example below
>
> With this canonical form userspace client will see
> correct u64 value of TTBR0/TTBR1 and hypervisor
> code would get word in mcrr mrrc instruction in
> right positions. The only issue with this approach if
> some code in kernel will look at &(vcpu->arch.cp15[r->reg]
> address as u64 value it will see incorrectly in BE case.
> But we don't have such code now.
>
> If we decide that canonical form is such that
> &(vcpu->arch.cp15[r->reg] address should hold correct
> u64 value we would need to change indexes, description
> add word swaps somewhere in context restore/save
> code, access_vm_reg etc. Personally I prefer not to
> choose this canonical form.
>
> Looking for your guidance on suggestion how to go
> about it. I'll post patch for my proposal latter tonight.
>
I'm having a bit hard time following this much text, but essentially
what I think you're asking is: What is the ABI that we should
adhere to.
I think the clear answer is, that we're exporting this as a 64-bit
register (see Documentation/virtual/kvm/api.txt) so user space just sees
this as a u64* pointer and the least significant bit in that value is
the least significant bit of the register, example, pseudo-code-c'ish:
void my_user_space_func(void)
{
struct kvm_one_reg r;
u64 val = 0;
unsigned char ttbr0_bit0;
int ret;
r.id = ARM_CP15_C2_TTBR0_ID;
r.addr = &val;
ret = ioctl(vcpu_fd, KVM_GET_ONE_REG, &r);
if (ret) {
log_err(ret, "blah\n");
return;
}
ttbr0_bit0 = val & 1;
}
> > I think you need a function to copy cp15_64_to_u64() thingy which is
> > taking BE/LE into consideration. You had this for one of the other
> > patches that you may be able to reuse.
>
> Those are V8 aarch32 macros, there are no such macros (like
> vcpu_cp15) in V7. I was looking for similar in V7 but could not find. It
> seems V7 code directly works with vcpu->arch.cp15[]. Or did I miss
> it? In context of above description is it important?
>
If you can add this in the header file so it's availabel for both archs
and reuse it then that seems practical. But this is just an
implementation issue, play with it when you're writing the code to get
to the nicest result.
> > I commented on this in the last revision too, am I getting it wrong?
> >
> >> + 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 +1077,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;
> >> + /* Note: copies two u32 regs */
> >> + ret = reg_from_user(&val, uaddr, reg->id);
> >> + if (!ret)
> >> + memcpy(&(vcpu->arch.cp15[r->reg]), &val, sizeof(val));
> >
> > same as above.
>
> If my suggestion, approach 1, to fix followed instead of
> memcpy it would be
>
> if (!ret) {
> vcpu->arch.cp15[r->reg] = val & 0xffffffff;
> vcpu->arch.cp15[r->reg + 1] = val >> 32;
> }
>
yeah, I think this is correct, but it would be nicer if you wrapped it
in a macro or static inline I think.
> >> + } 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)
> >> --
> >> 1.8.1.4
> >>
> >
> > Otherwise, we are certainly moving in the right direction.
>
> As always appreciate your time!
>
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
