On Wed, Jul 25, 2012 at 4:25 AM, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
> On Thu, 12 Jul 2012 15:24:44 +0930, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
>> Hi all,
>>
>> This turned out to be a longer patch series than I expected.
>> The idea is that qemu tells us exactly what CPU (and cpu features) the
>> guest should have, and we tell it exactly what CP15 registers we have.
>
> OK, I've pushed an updated series:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git msr-ops
>
> Rather than re-post all 17, here are the differences from the first
> patches (see inter-diff below):
>
> 1) We now add a 'struct kvm_vcpu_init' rather than overload kvm_sregs
> (thanks Alexander), and added the ioctl KVM_ARM_VCPU_INIT.
>
> 2) I add a new KVM_VCPU_GET_MSR_INDEX_LIST rather than abusing the
> KVM_GET_VCPU_MSR_INDEX_LIST which x86 uses: ours is per-vcpu.
>
> 3) We return ENOENT for unknown CP registers, EINVAL for invalid
> combinations.
>
> 4) WI/RAZ -> RAW/WI (Thanks Peter).
>
> 5) Remove num_features bit (it's redundant: just zero them all), and
> drop to 7*32 feature bits.
>
> Cheers,
> Rusty.
>
> diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
> index 6838210..d040a2a 100644
> --- a/arch/arm/include/asm/kvm.h
> +++ b/arch/arm/include/asm/kvm.h
> @@ -62,10 +62,12 @@ struct kvm_regs {
> /* Supported Processor Types */
> #define KVM_ARM_TARGET_CORTEX_A15 (0xC0F)
>
> -struct kvm_sregs {
> +struct kvm_vcpu_init {
> __u32 target;
> - __u32 num_features;
> - __u32 features[14];
> + __u32 features[7];
> +};
> +
> +struct kvm_sregs {
> };
>
> struct kvm_fpu {
> @@ -83,7 +85,7 @@ struct kvm_sync_regs {
> struct kvm_arch_memory_slot {
> };
>
> -/* Exactly like x86. */
> +/* Based on x86, but we use KVM_GET_VCPU_MSR_INDEX_LIST. */
> struct kvm_msr_entry {
> __u32 index;
> __u32 reserved;
> @@ -98,7 +100,7 @@ struct kvm_msrs {
> struct kvm_msr_entry entries[0];
> };
>
> -/* for KVM_GET_MSR_INDEX_LIST */
> +/* for KVM_VCPU_GET_MSR_INDEX_LIST */
> struct kvm_msr_list {
> __u32 nmsrs; /* number of msrs in entries */
> __u32 indices[0];
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index a8dd6a0..bbe5617 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -180,4 +180,6 @@ static inline int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
> struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
> struct kvm_vcpu __percpu **kvm_get_running_vcpus(void);
>
> +int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> + const struct kvm_vcpu_init *init);
> #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 15283c9..c9a3770 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -567,7 +567,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> int exit_reason;
> sigset_t sigsaved;
>
> - /* Make sure they initialize the vcpu with KVM_SET_SREGS */
> + /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
> if (unlikely(!vcpu->arch.target))
> return -ENOEXEC;
>
> @@ -730,7 +730,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> return kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, &irq_event);
> }
> #endif
> - case KVM_GET_MSR_INDEX_LIST: {
> + case KVM_ARM_VCPU_INIT: {
> + struct kvm_vcpu_init init;
> +
> + if (copy_from_user(&init, argp, sizeof init))
> + return -EFAULT;
> +
> + return kvm_vcpu_set_target(vcpu, &init);
> + }
> + case KVM_VCPU_GET_MSR_INDEX_LIST: {
> struct kvm_msr_list __user *user_msr_list = argp;
> struct kvm_msr_list msr_list;
> unsigned n;
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index e58d712..79c16c5 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -232,7 +232,7 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,
> * NAKed, so it will read the PMCR anyway.
> *
> * Therefore we tell the guest we have 0 counters. Unfortunately, we
> - * must always support PMCCNTR (the cycle counter): we just WI/RAZ for
> + * must always support PMCCNTR (the cycle counter): we just RAZ/WI for
> * all PM registers, which doesn't crash the guest kernel at least.
> */
> static bool pm_fake(struct kvm_vcpu *vcpu,
> @@ -683,7 +683,7 @@ static int get_fixed_cp15(u32 index, u64 *val)
> index_to_params(index, ¶ms);
> r = find_reg(¶ms, fixed_cp15, ARRAY_SIZE(fixed_cp15));
> if (!r)
> - return -EINVAL;
> + return -ENOENT;
>
> *val = r->val;
> return 0;
> @@ -697,7 +697,7 @@ static int set_fixed_cp15(u32 index, u64 val)
> index_to_params(index, ¶ms);
> r = find_reg(¶ms, fixed_cp15, ARRAY_SIZE(fixed_cp15));
> if (!r)
> - return -EINVAL;
> + return -ENOENT;
>
> /* This is what we mean by fixed: you can't change it. */
> if (r->val != val)
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index af26de7..d842d28 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -122,43 +122,34 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
> struct kvm_sregs *sregs)
> {
> - unsigned int i;
> -
> - /* FIXME: Should get_sregs on uninit vcpu return best CPU we can do? */
> - if (!vcpu->arch.target)
> - return -EINVAL;
> -
> - sregs->target = vcpu->arch.target;
> -
> - BUILD_BUG_ON(NUM_FEATURES > sizeof(sregs->features) * 8);
> - sregs->num_features = NUM_FEATURES;
> -
> - /* Note: sregs is already kzalloc'ed, so no need to zero. */
> - for (i = 0; i < NUM_FEATURES; i++) {
> - if (test_bit(i, vcpu->arch.features))
> - sregs->features[i / 32] |= (1 << (i % 32));
> - }
> - return 0;
> + return -EINVAL;
> }
>
> int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> struct kvm_sregs *sregs)
> {
> + return -EINVAL;
> +}
> +
> +int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> + const struct kvm_vcpu_init *init)
> +{
> unsigned int i;
>
> /* We can only do a cortex A15 for now. */
> - if (sregs->target != kvm_target_cpu())
> + if (init->target != kvm_target_cpu())
> return -EINVAL;
>
> - if (sregs->num_features > NUM_FEATURES)
> - return -E2BIG;
> -
> - vcpu->arch.target = sregs->target;
> + vcpu->arch.target = init->target;
> bitmap_zero(vcpu->arch.features, NUM_FEATURES);
>
> - for (i = 0; i < NUM_FEATURES; i++) {
> - if (sregs->features[i / 32] & (1 << (i % 32)))
> + /* -ENOENT for unknown features, -EINVAL for invalid combinations. */
> + for (i = 0; i < ARRAY_SIZE(init->features)*8; i++) {
with the risk of embarrassing myself: doesn't ARRAY_SIZE return number
of elements, so you're only looking at 56 possible features, and not
224? (iow. should 8 be 32 or sizeof(__u32))
nit: spaces around the *
> + if (init->features[i / 32] & (1 << (i % 32))) {
> + if (i >= NUM_FEATURES)
> + return -ENOENT;
> set_bit(i, vcpu->arch.features);
> + }
> }
>
> /* Now we know what it is, we can reset it. */
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index f978447..209b432 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -905,6 +905,8 @@ struct kvm_s390_ucas_mapping {
> #define KVM_SET_ONE_REG _IOW(KVMIO, 0xac, struct kvm_one_reg)
> /* VM is being stopped by host */
> #define KVM_KVMCLOCK_CTRL _IO(KVMIO, 0xad)
> +#define KVM_ARM_VCPU_INIT _IOW(KVMIO, 0xae, struct kvm_vcpu_init)
> +#define KVM_VCPU_GET_MSR_INDEX_LIST _IOWR(KVMIO, 0xaf, struct kvm_msr_list)
>
> #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
> #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
