On Sun, Aug 5, 2012 at 11:12 PM, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
> 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.
>
ok, fine with me.
>> > +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.
>
sounds good
>> > + 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).
>
Actually, I thought checkpatch would complain, but it turns out it doesn't.
>> > +/**
>> > + * 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.
>
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
