On Fri, 3 Aug 2012 13:57:00 +0200, Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jul 12, 2012 at 7:54 AM, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
> > Now we have a table for all the cp15 registers, we can drive a generic
> > API. x86 already has one for sparse-numbered registers, so we simply
> > reproduce that. The only difference is that our KVM_GET_MSR_INDEX_LIST
> > is a per-vcpu ioctl; we can't know the MSRs until we know the cpu type.
> >
> > The numbering for the indices for coprocesors is simple, if userspace
> > cares (it might not for simple save and restore): the upper 16 bits
> > are the coprocessor number. If it's > 15, it's something else, for
> > future expansion.
> >
> > Bit 15 indicates a 64-bit register. For 64 bit registers the bottom 4
> > bits are CRm, the next 4 are opc1 (just like the MCRR/MRCC instruction
> > encoding). For 32 bit registers, the bottom 4 bits are CRm, the next
> > 3 are opc2, the next 4 CRn, and the next 3 opc1 (the same order as the
> > MRC/MCR instruction encoding, but not the same bit positions).
> >
> > 64-bit coprocessor register:
> > ...|19 18 17 16|15|14 13 12 11 10 9 8| 7 6 5 4 |3 2 1 0|
> > ...0 0 | cp num | 1| 0 0 0 0 0 0 0| opc1 | CRm |
> >
> > 32-bit coprocessor register:
> > ...|19 18 17 16|15|14|13 12 11|10 9 8 7 |6 5 4 |3 2 1 0|
> > ...0 0 | cp num | 0| 0| opc1 | CRn | opc2 | CRm |
> >
> > Non-coprocessor register:
> >
> > | 32 31 30 29 28 27 26 25 24 23 22 21 20|19 18 17 16 15 ...
> > | < some non-zero value > | ...
> >
> > Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> > ---
> > arch/arm/include/asm/kvm.h | 31 +++++
> > arch/arm/include/asm/kvm_coproc.h | 7 ++
> > arch/arm/kvm/arm.c | 29 +++++
> > arch/arm/kvm/coproc.c | 237 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 304 insertions(+)
> >
> > diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
> > index 09b5be3..d6531e8 100644
> > --- a/arch/arm/include/asm/kvm.h
> > +++ b/arch/arm/include/asm/kvm.h
> > @@ -92,4 +92,35 @@ struct kvm_sync_regs {
> > struct kvm_arch_memory_slot {
> > };
> >
> > +/* Exactly like x86. */
> > +struct kvm_msr_entry {
> > + __u32 index;
> > + __u32 reserved;
> > + __u64 data;
> > +};
>
> makes sense to move this to include/linux/kvm.h ?
x86 didn't, and you can get away with it because it's only used in a
#define.
> > +static int get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *val)
> > +{
> > + const struct coproc_reg *r;
> > + u32 coproc = get_bits(index, KVM_ARM_MSR_COPROC_MASK);
> > +
> > + /* We only do cp15 for now. */
> > + if (coproc != 15)
> > + return -EINVAL;
> > +
> > + r = index_to_cp15(vcpu, index);
> > + if (!r)
> > + return -EINVAL;
> > +
> > + *val = vcpu->arch.cp15[r->reg];
> > + if (r->is_64)
> > + *val |= ((u64)vcpu->arch.cp15[r->reg+1]) << 32;
> > + return 0;
> > +}
> > +
> > +static int set_msr(struct kvm_vcpu *vcpu, u32 index, u64 val)
> > +{
> > + const struct coproc_reg *r;
> > + u32 coproc = get_bits(index, KVM_ARM_MSR_COPROC_MASK);
> > +
> > + /* We only do cp15 for now. */
> > + if (coproc != 15)
> > + return -EINVAL;
> > +
> > + r = index_to_cp15(vcpu, index);
> > + if (!r)
> > + return -EINVAL;
> > +
> > + vcpu->arch.cp15[r->reg] = val;
> > + if (r->is_64)
> > + vcpu->arch.cp15[r->reg+1] = (val >> 32);
> > + return 0;
> > +}
>
> worth combining get_msr and set_msr with a 'bool write' parameter?
Almost, but not quite, IMHO. I renamed index_to_cp15 to
index_to_coproc_reg and put the coproc check in there, so it shares even
more code though.
> > + u32 i, index;
> > + u64 val;
> > + u64 __user *uval;
> > + int ret;
> > +
> > + for (i = 0; i < num; i++) {
> > + uval = get_umsr(&entries[i], &index);
> > + if (!uval)
> > + return -EFAULT;
> > + if (copy_from_user(&val, uval, sizeof(val)) != 0)
> > + return -EFAULT;
>
> get_user ?
Didn't compile with a u64 IIRC.
> > + /* FIXME: Assumes no duplicates! */
>
> If this is a FIXME, shouldn't we just fix it (allocate a table of the
> registers, zero it out, set a bit and only count once)? I prefer not
> merging new FIXMEs if possible.
OK, that makes it a bit more complex. We actually do want
target-specific entries to be able to override generic ones, I think,
though it's not necessary today.
This is the only code which doesn't respect that, so I wrote a function
to walk both and do the Right Thing with duplicates.
> > +/* Puts in the position indicated by mask (assumes val fits in mask) */
> > +static inline u32 set_bits(u32 val, u32 mask)
> > +{
> > + return val << (ffs(mask)-1);
>
> spaces around the - operator iirc.
Fixed (though I prefer it without spaces, as it's clearer).
> > +/**
> > + * kvm_arm_msridx - what's the index value for the nth register.
>
> this is not the name of the function
>
> > + *
> > + * This is for special registers, particularly cp15.
> > + */
> > +int kvm_copy_msrindices(struct kvm_vcpu *vcpu, u32 __user *uindices)
>
> shouldn't this be kvm_arm_copy_msrindicies for consistency?
Yep, fixed.
> ok, this is pretty damn nice. Good argument for driving the cp15
> register reset from here ;)
Yes, I wrote this patch series backwards, starting here.
Cheers,
Rusty.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
