[Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Rusty Russell]

Recent kernels use this to set the cpu and features (currently, only
the A15 is supported).

Note that this causes the registers in the CPU to be initialized, so
it's important that all CPUs are created first (they are, as it turns
out).

This code ignores errors, for backwards compatibility with older
kernels.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index 38ff1d6..988890a 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -66,7 +66,13 @@ struct kvm_regs {
 
 };
 
+/* Supported Processor Types */
+#define KVM_ARM_TARGET_CORTEX_A15	(0xC0F)
+
 struct kvm_sregs {
+	__u32 target;
+	__u32 num_features;
+	__u32 features[14];
 };
 
 struct kvm_fpu {
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 29bb51f..67d005f 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -34,7 +34,13 @@ int kvm_arch_init(KVMState *s)
 
 int kvm_arch_init_vcpu(CPUARMState *env)
 {
-    return 0;
+    struct kvm_sregs sregs;
+
+    sregs.target = KVM_ARM_TARGET_CORTEX_A15;
+    sregs.num_features = 0;
+
+    /* Ignore failure for compatibility with old kvm versions. */
+    return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;
 }
 
 int kvm_arch_put_registers(CPUARMState *env, int level)

[Qemu-devel] [PATCH 2/3] target-arm: kvm: use KVM_GET_MSRS/KVM_SET_MSRS for CP15 registers.

[Rusty Russell]

Recent kernels use this to set the CP15 registers, rather than putting
them in 'struct kvm_regs'.  The changed size of that struct changes the
ioctl number, so we have a temporary hack to try both.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index 988890a..4842e85 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -75,6 +75,37 @@ struct kvm_sregs {
 	__u32 features[14];
 };
 
+/* Exactly like x86. */
+struct kvm_msr_entry {
+	__u32 index;
+	__u32 reserved;
+	__u64 data;
+};
+
+/* for KVM_GET_MSRS and KVM_SET_MSRS */
+struct kvm_msrs {
+	__u32 nmsrs; /* number of msrs in entries */
+	__u32 pad;
+
+	struct kvm_msr_entry entries[0];
+};
+
+/* for KVM_GET_MSR_INDEX_LIST */
+struct kvm_msr_list {
+	__u32 nmsrs; /* number of msrs in entries */
+	__u32 indices[0];
+};
+
+/* If you need to interpret the index values, here's the key. */
+#define KVM_ARM_MSR_COPROC_MASK		0xFFFF0000
+#define KVM_ARM_MSR_64_BIT_MASK		0x00008000
+#define KVM_ARM_MSR_64_OPC1_MASK	0x000000F0
+#define KVM_ARM_MSR_64_CRM_MASK		0x0000000F
+#define KVM_ARM_MSR_32_CRM_MASK		0x0000000F
+#define KVM_ARM_MSR_32_OPC2_MASK	0x00000070
+#define KVM_ARM_MSR_32_CRN_MASK		0x00000780
+#define KVM_ARM_MSR_32_OPC1_MASK	0x00003800
+
 struct kvm_fpu {
 };
 
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 67d005f..2c149bd 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -43,15 +43,28 @@ int kvm_arch_init_vcpu(CPUARMState *env)
     return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;
 }
 
+#define MSR32_INDEX_OF(coproc, crn, opc1, crm, opc2) \
+	(((coproc)<<16) | ((opc1)<<11) | ((crn)<<7) | ((opc2)<<4) | (crm))
+
+/* A modern kernel has a smaller struct kvm_regs, so ioctls differ: */
+#define KVM_GET_REGS_MODERN 2157227649U
+#define KVM_SET_REGS_MODERN 1083485826U
+
 int kvm_arch_put_registers(CPUARMState *env, int level)
 {
     struct kvm_regs regs;
     int mode, bn;
+    struct cp15 {
+	    struct kvm_msrs hdr;
+	    struct kvm_msr_entry e[2];
+    } cp15;
     int ret;
 
     ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
     if (ret < 0)
-        return ret;
+	ret = kvm_vcpu_ioctl(env, KVM_GET_REGS_MODERN, &regs);
+    if (ret < 0)
+	return ret;
 
     /* We make sure the banked regs are properly set */
     mode = env->uncached_cpsr & CPSR_M;
@@ -91,8 +104,18 @@ int kvm_arch_put_registers(CPUARMState *env, int level)
     regs.cp15.c0_midr = env->cp15.c0_cpuid;
     regs.cp15.c1_sys = env->cp15.c1_sys;
 
-    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
+    cp15.hdr.nmsrs = ARRAY_SIZE(cp15.e);
+    cp15.e[0].index = MSR32_INDEX_OF(15, 0, 0, 0, 0); /* MIDR */
+    cp15.e[0].data = env->cp15.c0_cpuid;
+    cp15.e[1].index = MSR32_INDEX_OF(15, 1, 0, 0, 0); /* SCTLR */
+    cp15.e[1].data = env->cp15.c1_sys;
 
+    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
+    if (ret < 0) {
+	ret = kvm_vcpu_ioctl(env, KVM_SET_REGS_MODERN, &regs);
+	if (ret == 0)
+	    ret = kvm_vcpu_ioctl(env, KVM_SET_MSRS, &cp15);
+    }
     return ret;
 }
 
@@ -101,11 +124,27 @@ int kvm_arch_get_registers(CPUARMState *env)
     struct kvm_regs regs;
     int mode, bn;
     int32_t ret;
+    struct cp15 {
+	    struct kvm_msrs hdr;
+	    struct kvm_msr_entry e[6];
+    } cp15;
 
     ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
     if (ret < 0)
+	ret = kvm_vcpu_ioctl(env, KVM_GET_REGS_MODERN, &regs);
+    if (ret < 0)
         return ret;
 
+    cp15.hdr.nmsrs = ARRAY_SIZE(cp15.e);
+    cp15.e[0].index = MSR32_INDEX_OF(15, 0, 0, 0, 0); /* MIDR */
+    cp15.e[1].index = MSR32_INDEX_OF(15, 1, 0, 0, 0); /* SCTLR */
+    cp15.e[2].index = MSR32_INDEX_OF(15, 2, 0, 0, 0); /* TTBR0 */
+    cp15.e[3].index = MSR32_INDEX_OF(15, 2, 0, 0, 1); /* TTBR1 */
+    cp15.e[4].index = MSR32_INDEX_OF(15, 2, 0, 0, 2); /* TTBCR */
+    cp15.e[5].index = MSR32_INDEX_OF(15, 3, 0, 0, 0); /* DACR */
+
+    ret = kvm_vcpu_ioctl(env, KVM_GET_MSRS, &cp15);
+
     /* First, let's transfer the banked state */
     cpsr_write(env, regs.cpsr, 0xFFFFFFFF);
     memcpy(env->regs, regs.regs0_7, sizeof(uint32_t) * 8);
@@ -142,18 +181,33 @@ int kvm_arch_get_registers(CPUARMState *env)
     env->regs[14] = env->banked_r14[bn];
     env->spsr = env->banked_spsr[bn];
 
-    //env->cp15.c0_cpuid = regs.cp15.c0_midr;
-    env->cp15.c1_sys = regs.cp15.c1_sys;
-    env->cp15.c2_base0 = regs.cp15.c2_base0;
-    env->cp15.c2_base1 = regs.cp15.c2_base1;
+    /* Old KVM version. */
+    if (ret != 0) {
+	    //env->cp15.c0_cpuid = regs.cp15.c0_midr;
+	    env->cp15.c1_sys = regs.cp15.c1_sys;
+	    env->cp15.c2_base0 = regs.cp15.c2_base0;
+	    env->cp15.c2_base1 = regs.cp15.c2_base1;
 
-    /* This is ugly, but necessary for GDB compatibility */
-    env->cp15.c2_control = regs.cp15.c2_control;
-    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> regs.cp15.c2_control);
-    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> regs.cp15.c2_control);
+	    /* This is ugly, but necessary for GDB compatibility */
+	    env->cp15.c2_control = regs.cp15.c2_control;
+	    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> regs.cp15.c2_control);
+	    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> regs.cp15.c2_control);
 
-    env->cp15.c3 = regs.cp15.c3_dacr;
+	    env->cp15.c3 = regs.cp15.c3_dacr;
+	    return 0;
+    }
+
+    //env->cp15.c0_cpuid = cp15.e[0].data;
+    env->cp15.c1_sys = cp15.e[1].data;
+    env->cp15.c2_base0 = cp15.e[2].data;
+    env->cp15.c2_base1 = cp15.e[3].data;
+
+    /* This is ugly, but necessary for GDB compatibility */
+    env->cp15.c2_control = cp15.e[4].data;
+    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> cp15.e[4].data);
+    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> cp15.e[4].data);
 
+    env->cp15.c3 = cp15.e[5].data;
     return 0;
 }
 

[Qemu-devel] [PATCH 3/3] target-arm: kvm: remove old kernel support.

[Rusty Russell]

This removes old kernel support for cp15 inside struct kvm_regs, and
assumes we have KVM_SET_SREGS support for setting the target.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index 4842e85..8d255f2 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -55,15 +55,6 @@ struct kvm_regs {
 	__u32 reg15;
 	__u32 cpsr;
 	__u32 spsr[5];		/* Banked SPSR,  indexed by MODE_  */
-	struct {
-		__u32 c0_midr;
-		__u32 c1_sys;
-		__u32 c2_base0;
-		__u32 c2_base1;
-		__u32 c2_control;
-		__u32 c3_dacr;
-	} cp15;
-
 };
 
 /* Supported Processor Types */
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 2c149bd..34741ba 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -39,17 +39,12 @@ int kvm_arch_init_vcpu(CPUARMState *env)
     sregs.target = KVM_ARM_TARGET_CORTEX_A15;
     sregs.num_features = 0;
 
-    /* Ignore failure for compatibility with old kvm versions. */
-    return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;
+    return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
 }
 
 #define MSR32_INDEX_OF(coproc, crn, opc1, crm, opc2) \
 	(((coproc)<<16) | ((opc1)<<11) | ((crn)<<7) | ((opc2)<<4) | (crm))
 
-/* A modern kernel has a smaller struct kvm_regs, so ioctls differ: */
-#define KVM_GET_REGS_MODERN 2157227649U
-#define KVM_SET_REGS_MODERN 1083485826U
-
 int kvm_arch_put_registers(CPUARMState *env, int level)
 {
     struct kvm_regs regs;
@@ -62,8 +57,6 @@ int kvm_arch_put_registers(CPUARMState *env, int level)
 
     ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
     if (ret < 0)
-	ret = kvm_vcpu_ioctl(env, KVM_GET_REGS_MODERN, &regs);
-    if (ret < 0)
 	return ret;
 
     /* We make sure the banked regs are properly set */
@@ -101,9 +94,6 @@ int kvm_arch_put_registers(CPUARMState *env, int level)
     regs.spsr[MODE_ABT] = env->banked_spsr[2];
     regs.spsr[MODE_UND] = env->banked_spsr[3];
 
-    regs.cp15.c0_midr = env->cp15.c0_cpuid;
-    regs.cp15.c1_sys = env->cp15.c1_sys;
-
     cp15.hdr.nmsrs = ARRAY_SIZE(cp15.e);
     cp15.e[0].index = MSR32_INDEX_OF(15, 0, 0, 0, 0); /* MIDR */
     cp15.e[0].data = env->cp15.c0_cpuid;
@@ -111,11 +101,8 @@ int kvm_arch_put_registers(CPUARMState *env, int level)
     cp15.e[1].data = env->cp15.c1_sys;
 
     ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
-    if (ret < 0) {
-	ret = kvm_vcpu_ioctl(env, KVM_SET_REGS_MODERN, &regs);
-	if (ret == 0)
-	    ret = kvm_vcpu_ioctl(env, KVM_SET_MSRS, &cp15);
-    }
+    if (ret < 0)
+	ret = kvm_vcpu_ioctl(env, KVM_SET_MSRS, &cp15);
     return ret;
 }
 
@@ -144,6 +131,8 @@ int kvm_arch_get_registers(CPUARMState *env)
     cp15.e[5].index = MSR32_INDEX_OF(15, 3, 0, 0, 0); /* DACR */
 
     ret = kvm_vcpu_ioctl(env, KVM_GET_MSRS, &cp15);
+    if (ret < 0)
+	return ret;
 
     /* First, let's transfer the banked state */
     cpsr_write(env, regs.cpsr, 0xFFFFFFFF);
@@ -181,22 +170,6 @@ int kvm_arch_get_registers(CPUARMState *env)
     env->regs[14] = env->banked_r14[bn];
     env->spsr = env->banked_spsr[bn];
 
-    /* Old KVM version. */
-    if (ret != 0) {
-	    //env->cp15.c0_cpuid = regs.cp15.c0_midr;
-	    env->cp15.c1_sys = regs.cp15.c1_sys;
-	    env->cp15.c2_base0 = regs.cp15.c2_base0;
-	    env->cp15.c2_base1 = regs.cp15.c2_base1;
-
-	    /* This is ugly, but necessary for GDB compatibility */
-	    env->cp15.c2_control = regs.cp15.c2_control;
-	    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> regs.cp15.c2_control);
-	    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> regs.cp15.c2_control);
-
-	    env->cp15.c3 = regs.cp15.c3_dacr;
-	    return 0;
-    }
-
     //env->cp15.c0_cpuid = cp15.e[0].data;
     env->cp15.c1_sys = cp15.e[1].data;
     env->cp15.c2_base0 = cp15.e[2].data;

Re: [Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Peter Maydell]

On 13 July 2012 04:37, Rusty Russell <rusty.russell@linaro.org> wrote:
> Recent kernels use this to set the cpu and features (currently, only
> the A15 is supported).
>
> Note that this causes the registers in the CPU to be initialized, so
> it's important that all CPUs are created first (they are, as it turns
> out).
>
> This code ignores errors, for backwards compatibility with older
> kernels.
>
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

I haven't actually been posting the ARM KVM patches to qemu-devel
thus far, so this patch is a bit of a non-sequitur for qemu-devel readers.
(We've been posting patches to kvmarm only, like the kernel patches.)

Anyway:
 * updates to qemu's kernel headers should be separate patches
and ideally the result of running the automatic 'update headers'
script. [I'm going to squash all the kernel header changes together
before we submit a patch series properly to qemu-devel, so patches
which combine header and code changes require me to untangle them]
 * any code which includes compatibility workarounds for earlier
versions of in-development kernel code should have a FIXME comment
so we can remember to undo the workaround before submitting.

thanks
-- PMM

Re: [Qemu-devel] [kvmarm] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Alexander Graf]

On 13.07.2012, at 05:37, Rusty Russell <rusty.russell@linaro.org> wrote:

> Recent kernels use this to set the cpu and features (currently, only
> the A15 is supported).
> 
> Note that this causes the registers in the CPU to be initialized, so
> it's important that all CPUs are created first (they are, as it turns
> out).
> 
> This code ignores errors, for backwards compatibility with older
> kernels.
> 
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
> 
> diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
> index 38ff1d6..988890a 100644
> --- a/linux-headers/asm-arm/kvm.h
> +++ b/linux-headers/asm-arm/kvm.h
> @@ -66,7 +66,13 @@ struct kvm_regs {
> 
> };
> 
> +/* Supported Processor Types */
> +#define KVM_ARM_TARGET_CORTEX_A15    (0xC0F)
> +
> struct kvm_sregs {
> +    __u32 target;
> +    __u32 num_features;
> +    __u32 features[14];
> };

Are you sure you want to use sregs? We did the mistake of reusing it on ppc, but that doesn't mean you need to repeat the same one :)

Basically sregs are an x86 specific struct for its segment register information. I'm quite sure that this is not what your use of them is here.

In general you have 3 sane options for API additions:

  1) ONE_REG

If you have information that is syncable in register granularity, this is what you want. We will (once necessary) add an API that allows us to sync multiple ONE_REG items at once, so it'll be like a big dynamic kvm_regs implementation. I don't think this applies here though.

2) new ioctl

Just define an ARM specific ioctl that sets the compat features. Believe me, it will make your code easier to read.

3) ENABLE_CAP

If you only need to enable a feature and care about backwards compatibility of the API (which you don't yet), this is a good one. it basically allows you to enable new features in newer kernel versions which would otherwise break compatibility. You can also pass arbitrary data to ENABLE_CAP to pass in additional information.


Usually in released KVM versions, I'd use ENABLE_CAP for a feature like this. Since you can still define and break the API as you wish, option 2 works as well :).

Alex

> 
> struct kvm_fpu {
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 29bb51f..67d005f 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -34,7 +34,13 @@ int kvm_arch_init(KVMState *s)
> 
> int kvm_arch_init_vcpu(CPUARMState *env)
> {
> -    return 0;
> +    struct kvm_sregs sregs;
> +
> +    sregs.target = KVM_ARM_TARGET_CORTEX_A15;
> +    sregs.num_features = 0;
> +
> +    /* Ignore failure for compatibility with old kvm versions. */
> +    return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;
> }
> 
> int kvm_arch_put_registers(CPUARMState *env, int level)
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Re: [Qemu-devel] [PATCH 2/3] target-arm: kvm: use KVM_GET_MSRS/KVM_SET_MSRS for CP15 registers.

[Blue Swirl]

On Fri, Jul 13, 2012 at 3:43 AM, Rusty Russell <rusty.russell@linaro.org> wrote:
> Recent kernels use this to set the CP15 registers, rather than putting
> them in 'struct kvm_regs'.  The changed size of that struct changes the
> ioctl number, so we have a temporary hack to try both.
>
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
>
> diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
> index 988890a..4842e85 100644
> --- a/linux-headers/asm-arm/kvm.h
> +++ b/linux-headers/asm-arm/kvm.h
> @@ -75,6 +75,37 @@ struct kvm_sregs {
>         __u32 features[14];
>  };
>
> +/* Exactly like x86. */
> +struct kvm_msr_entry {
> +       __u32 index;
> +       __u32 reserved;
> +       __u64 data;
> +};
> +
> +/* for KVM_GET_MSRS and KVM_SET_MSRS */
> +struct kvm_msrs {
> +       __u32 nmsrs; /* number of msrs in entries */
> +       __u32 pad;
> +
> +       struct kvm_msr_entry entries[0];
> +};
> +
> +/* for KVM_GET_MSR_INDEX_LIST */
> +struct kvm_msr_list {
> +       __u32 nmsrs; /* number of msrs in entries */
> +       __u32 indices[0];
> +};
> +
> +/* If you need to interpret the index values, here's the key. */
> +#define KVM_ARM_MSR_COPROC_MASK                0xFFFF0000
> +#define KVM_ARM_MSR_64_BIT_MASK                0x00008000
> +#define KVM_ARM_MSR_64_OPC1_MASK       0x000000F0
> +#define KVM_ARM_MSR_64_CRM_MASK                0x0000000F
> +#define KVM_ARM_MSR_32_CRM_MASK                0x0000000F
> +#define KVM_ARM_MSR_32_OPC2_MASK       0x00000070
> +#define KVM_ARM_MSR_32_CRN_MASK                0x00000780
> +#define KVM_ARM_MSR_32_OPC1_MASK       0x00003800
> +
>  struct kvm_fpu {
>  };
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 67d005f..2c149bd 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -43,15 +43,28 @@ int kvm_arch_init_vcpu(CPUARMState *env)
>      return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;
>  }
>
> +#define MSR32_INDEX_OF(coproc, crn, opc1, crm, opc2) \
> +       (((coproc)<<16) | ((opc1)<<11) | ((crn)<<7) | ((opc2)<<4) | (crm))
> +
> +/* A modern kernel has a smaller struct kvm_regs, so ioctls differ: */
> +#define KVM_GET_REGS_MODERN 2157227649U
> +#define KVM_SET_REGS_MODERN 1083485826U
> +
>  int kvm_arch_put_registers(CPUARMState *env, int level)
>  {
>      struct kvm_regs regs;
>      int mode, bn;
> +    struct cp15 {
> +           struct kvm_msrs hdr;
> +           struct kvm_msr_entry e[2];
> +    } cp15;
>      int ret;
>
>      ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
>      if (ret < 0)
> -        return ret;
> +       ret = kvm_vcpu_ioctl(env, KVM_GET_REGS_MODERN, &regs);
> +    if (ret < 0)
> +       return ret;

Braces.

>
>      /* We make sure the banked regs are properly set */
>      mode = env->uncached_cpsr & CPSR_M;
> @@ -91,8 +104,18 @@ int kvm_arch_put_registers(CPUARMState *env, int level)
>      regs.cp15.c0_midr = env->cp15.c0_cpuid;
>      regs.cp15.c1_sys = env->cp15.c1_sys;
>
> -    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
> +    cp15.hdr.nmsrs = ARRAY_SIZE(cp15.e);
> +    cp15.e[0].index = MSR32_INDEX_OF(15, 0, 0, 0, 0); /* MIDR */
> +    cp15.e[0].data = env->cp15.c0_cpuid;
> +    cp15.e[1].index = MSR32_INDEX_OF(15, 1, 0, 0, 0); /* SCTLR */
> +    cp15.e[1].data = env->cp15.c1_sys;
>
> +    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
> +    if (ret < 0) {
> +       ret = kvm_vcpu_ioctl(env, KVM_SET_REGS_MODERN, &regs);
> +       if (ret == 0)
> +           ret = kvm_vcpu_ioctl(env, KVM_SET_MSRS, &cp15);

Again. Please use checkpatch.pl to avoid these issues.

> +    }
>      return ret;
>  }
>
> @@ -101,11 +124,27 @@ int kvm_arch_get_registers(CPUARMState *env)
>      struct kvm_regs regs;
>      int mode, bn;
>      int32_t ret;
> +    struct cp15 {
> +           struct kvm_msrs hdr;
> +           struct kvm_msr_entry e[6];
> +    } cp15;
>
>      ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
>      if (ret < 0)
> +       ret = kvm_vcpu_ioctl(env, KVM_GET_REGS_MODERN, &regs);
> +    if (ret < 0)

Ditto.

>          return ret;
>
> +    cp15.hdr.nmsrs = ARRAY_SIZE(cp15.e);
> +    cp15.e[0].index = MSR32_INDEX_OF(15, 0, 0, 0, 0); /* MIDR */
> +    cp15.e[1].index = MSR32_INDEX_OF(15, 1, 0, 0, 0); /* SCTLR */
> +    cp15.e[2].index = MSR32_INDEX_OF(15, 2, 0, 0, 0); /* TTBR0 */
> +    cp15.e[3].index = MSR32_INDEX_OF(15, 2, 0, 0, 1); /* TTBR1 */
> +    cp15.e[4].index = MSR32_INDEX_OF(15, 2, 0, 0, 2); /* TTBCR */
> +    cp15.e[5].index = MSR32_INDEX_OF(15, 3, 0, 0, 0); /* DACR */
> +
> +    ret = kvm_vcpu_ioctl(env, KVM_GET_MSRS, &cp15);
> +
>      /* First, let's transfer the banked state */
>      cpsr_write(env, regs.cpsr, 0xFFFFFFFF);
>      memcpy(env->regs, regs.regs0_7, sizeof(uint32_t) * 8);
> @@ -142,18 +181,33 @@ int kvm_arch_get_registers(CPUARMState *env)
>      env->regs[14] = env->banked_r14[bn];
>      env->spsr = env->banked_spsr[bn];
>
> -    //env->cp15.c0_cpuid = regs.cp15.c0_midr;
> -    env->cp15.c1_sys = regs.cp15.c1_sys;
> -    env->cp15.c2_base0 = regs.cp15.c2_base0;
> -    env->cp15.c2_base1 = regs.cp15.c2_base1;
> +    /* Old KVM version. */
> +    if (ret != 0) {
> +           //env->cp15.c0_cpuid = regs.cp15.c0_midr;

I see that this is code movement, but please change the C99 comment to C89.

> +           env->cp15.c1_sys = regs.cp15.c1_sys;
> +           env->cp15.c2_base0 = regs.cp15.c2_base0;
> +           env->cp15.c2_base1 = regs.cp15.c2_base1;
>
> -    /* This is ugly, but necessary for GDB compatibility */
> -    env->cp15.c2_control = regs.cp15.c2_control;
> -    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> regs.cp15.c2_control);
> -    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> regs.cp15.c2_control);
> +           /* This is ugly, but necessary for GDB compatibility */
> +           env->cp15.c2_control = regs.cp15.c2_control;
> +           env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> regs.cp15.c2_control);
> +           env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> regs.cp15.c2_control);
>
> -    env->cp15.c3 = regs.cp15.c3_dacr;
> +           env->cp15.c3 = regs.cp15.c3_dacr;
> +           return 0;
> +    }
> +
> +    //env->cp15.c0_cpuid = cp15.e[0].data;

Ditto.

> +    env->cp15.c1_sys = cp15.e[1].data;
> +    env->cp15.c2_base0 = cp15.e[2].data;
> +    env->cp15.c2_base1 = cp15.e[3].data;
> +
> +    /* This is ugly, but necessary for GDB compatibility */
> +    env->cp15.c2_control = cp15.e[4].data;
> +    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> cp15.e[4].data);
> +    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> cp15.e[4].data);
>
> +    env->cp15.c3 = cp15.e[5].data;
>      return 0;
>  }
>
>

Re: [Qemu-devel] [kvmarm] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Rusty Russell]

On Fri, 13 Jul 2012 12:06:26 +0200, Alexander Graf <agraf@suse.de> wrote:
> > struct kvm_sregs {
> > +    __u32 target;
> > +    __u32 num_features;
> > +    __u32 features[14];
> > };
> 
> Are you sure you want to use sregs? We did the mistake of reusing it
> on ppc, but that doesn't mean you need to repeat the same one :)
> 
> Basically sregs are an x86 specific struct for its segment register
> information. I'm quite sure that this is not what your use of them is
> here.

Since each arch is given a hook already, I just abused it.  I'll change
this to a fresh KVM_ARM_SET_TARGET ioctl.  

> 3) ENABLE_CAP
> 
> If you only need to enable a feature and care about backwards
> compatibility of the API (which you don't yet), this is a good one. it
> basically allows you to enable new features in newer kernel versions
> which would otherwise break compatibility. You can also pass arbitrary
> data to ENABLE_CAP to pass in additional information.

Hmm, it's not quite a clean fit: this bitmap is for guest features, not
kvm ones.  Which ones you can enable depends on the target CPU, at least
in theory.

eg. FP/NEON support and debug register support are (in theory) optional
features for an implementation.  There may be more in future, I guess.

And you really want to initialize this all at the same time; eg. the cpu
identification registers need to be initialized depending on the
presence of various features.  It's also possible that various features
may be related, so you can't turn a single one off at a time.

Currently, it's a bit theoretical, since we don't have any guest
features, but it was suggested that we'll want them in future.

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Rusty Russell]

On Fri, 13 Jul 2012 09:06:16 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 July 2012 04:37, Rusty Russell <rusty.russell@linaro.org> wrote:
> > Recent kernels use this to set the cpu and features (currently, only
> > the A15 is supported).
> >
> > Note that this causes the registers in the CPU to be initialized, so
> > it's important that all CPUs are created first (they are, as it turns
> > out).
> >
> > This code ignores errors, for backwards compatibility with older
> > kernels.
> >
> > Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
> 
> I haven't actually been posting the ARM KVM patches to qemu-devel
> thus far, so this patch is a bit of a non-sequitur for qemu-devel readers.
> (We've been posting patches to kvmarm only, like the kernel patches.)

OK; given the wider API implications I thought I'd add them.

> Anyway:
>  * updates to qemu's kernel headers should be separate patches
> and ideally the result of running the automatic 'update headers'
> script. [I'm going to squash all the kernel header changes together
> before we submit a patch series properly to qemu-devel, so patches
> which combine header and code changes require me to untangle them]

OK.  This is totally a hack until we know how we're going to transition.

>  * any code which includes compatibility workarounds for earlier
> versions of in-development kernel code should have a FIXME comment
> so we can remember to undo the workaround before submitting.

Sure.  It's up to you whether you want a flag day: tell me and I'll
re-spin the patches?

Thanks,
Rusty.

Re: [Qemu-devel] [kvmarm] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Alexander Graf]

On 16.07.2012, at 09:19, Rusty Russell wrote:

> On Fri, 13 Jul 2012 12:06:26 +0200, Alexander Graf <agraf@suse.de> wrote:
>>> struct kvm_sregs {
>>> +    __u32 target;
>>> +    __u32 num_features;
>>> +    __u32 features[14];
>>> };
>> 
>> Are you sure you want to use sregs? We did the mistake of reusing it
>> on ppc, but that doesn't mean you need to repeat the same one :)
>> 
>> Basically sregs are an x86 specific struct for its segment register
>> information. I'm quite sure that this is not what your use of them is
>> here.
> 
> Since each arch is given a hook already, I just abused it.  I'll change
> this to a fresh KVM_ARM_SET_TARGET ioctl.  
> 
>> 3) ENABLE_CAP
>> 
>> If you only need to enable a feature and care about backwards
>> compatibility of the API (which you don't yet), this is a good one. it
>> basically allows you to enable new features in newer kernel versions
>> which would otherwise break compatibility. You can also pass arbitrary
>> data to ENABLE_CAP to pass in additional information.
> 
> Hmm, it's not quite a clean fit: this bitmap is for guest features, not
> kvm ones.  Which ones you can enable depends on the target CPU, at least
> in theory.

Sure. You'd do (pseudo-code):

  kvm_ioctl(fd, KVM_ENABLE_CAP, KVM_CAP_ARM_CPU_TARGET, ARM_CPU_NEON | ARM_CPU_VFP16 | ARM_CPU_V7);

> 
> eg. FP/NEON support and debug register support are (in theory) optional
> features for an implementation.  There may be more in future, I guess.
> 
> And you really want to initialize this all at the same time; eg. the cpu
> identification registers need to be initialized depending on the
> presence of various features.  It's also possible that various features
> may be related, so you can't turn a single one off at a time.

That argument does hold true. We are in that mess with ppc, where we don't have a proper "init" ioctl. So you'd definitely be better off defining an extensible init handshake now, so you can easily configure the kernel side early on.

Currently for PPC, we just try to either make things stateless or put additional constraints on certain CAPs, like "may only be set before the first vcpu run". Not as pretty as doing it cleanly.

> Currently, it's a bit theoretical, since we don't have any guest
> features, but it was suggested that we'll want them in future.

Yes, hence I'm trying to get you guys to something where you won't be stuck in 6 months from now with a stable ABI that totally doesn't fit you :)


Alex

Re: [Qemu-devel] [kvmarm] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Avi Kivity]

On 07/16/2012 10:19 AM, Rusty Russell wrote:
> On Fri, 13 Jul 2012 12:06:26 +0200, Alexander Graf <agraf@suse.de> wrote:
>> > struct kvm_sregs {
>> > +    __u32 target;
>> > +    __u32 num_features;
>> > +    __u32 features[14];
>> > };
>> 
>> Are you sure you want to use sregs? We did the mistake of reusing it
>> on ppc, but that doesn't mean you need to repeat the same one :)
>> 
>> Basically sregs are an x86 specific struct for its segment register
>> information. I'm quite sure that this is not what your use of them is
>> here.
> 
> Since each arch is given a hook already, I just abused it.  I'll change
> this to a fresh KVM_ARM_SET_TARGET ioctl.  

I guess this is equivalent to KVM_SET_CPUID2 on x86.  Note that's a vcpu
ioctl.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Peter Maydell]

On 13 July 2012 04:37, Rusty Russell <rusty.russell@linaro.org> wrote:
>  int kvm_arch_init_vcpu(CPUARMState *env)
>  {
> -    return 0;
> +    struct kvm_sregs sregs;
> +
> +    sregs.target = KVM_ARM_TARGET_CORTEX_A15;
> +    sregs.num_features = 0;

We need to add a check somewhere that if we're not emulating
an A15 then we either fail noisily or silently drop back to
TCG. Then we should have an assert in here that env refers
to an A15 I guess. [not yet figured out how to do that, I
don't want to look at the cpu->midr, we only just finished
cleaning out all the references to that. Maybe there should
be a field in the struct ARMCPU which gives the KVM_ARM_TARGET_*
value to use?]

Unfortunately I can't find a good place to put the CPU type
check -- ideally we would fail kvm_init() as this would make
us fall back to TCG in the usual way, but in kvm_init() you
don't yet know what guest CPU you're going to be emulating...

> +
> +    /* Ignore failure for compatibility with old kvm versions. */
> +    return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;

I assume this weird "? 0 : 0" construct is going to go away
when we drop the back-compat?

Since this back-compat code is going to go away within a
few weeks, it would make my life easier if all the back
compat code was flagged with ifdefs or something, so we
don't leave it in by mistake (and so I know what not to
worry about reviewing in patches).

-- PMM

Re: [Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Peter Maydell]

On 17 July 2012 18:31, Peter Maydell <peter.maydell@linaro.org> wrote:
> We need to add a check somewhere that if we're not emulating
> an A15 then we either fail noisily or silently drop back to
> TCG. Then we should have an assert in here that env refers
> to an A15 I guess. [not yet figured out how to do that, I
> don't want to look at the cpu->midr, we only just finished
> cleaning out all the references to that. Maybe there should
> be a field in the struct ARMCPU which gives the KVM_ARM_TARGET_*
> value to use?]
>
> Unfortunately I can't find a good place to put the CPU type
> check -- ideally we would fail kvm_init() as this would make
> us fall back to TCG in the usual way, but in kvm_init() you
> don't yet know what guest CPU you're going to be emulating...

Having thought more about this, I don't think there is anywhere
we can do a check that would give the failure semantics we'd
like (ie respecting whatever the user's accel= options were).
We don't know what the guest CPU is until we get to trying to
construct the CPU in the machine. But other devices the machine
constructs might want to know kvm vs tcg (eg irqchip), and we
can't enforce ordering restrictions on device construction really.
(That's ignoring the question of whether we could even postpone
kvm_init() at all, which I'm not convinced wouldn't break something.)

So I can't see anything better than "complain and return failure
from kvm_arch_init_vcpu() if the guest CPU isn't going to work".

-- PMM

Re: [Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Rusty Russell]

On Tue, 17 Jul 2012 19:19:16 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
> So I can't see anything better than "complain and return failure
> from kvm_arch_init_vcpu() if the guest CPU isn't going to work".

OK, here's what I have for kvm.  It matches the kernel patches I just
sent out, with the updated API.

I rewrote all the backwards compat stuff to wrap it in #ifdef
HORRIBLE_COMPAT_HACK, but something subtle broke and it no longer set
cpsr correctly for older kernels.  After wasting an afternoon on it, I
gave up and threw out the backwards compat code in digust.

Nor does it bisect, since we update the headers as you asked, then
update the code.

The following changes since commit 94d477a4017d45222a4a0227857ad9fc3f81fb2d:

  Merge upstream master into qemu-linaro (2012-07-16 15:04:16 +0100)

are available in the git repository at:


  git://git.linaro.org/people/rusty/qemu-linaro.git msr-ops

for you to fetch changes up to caf1e8a68ceb6ffd485e96d2f05102b095dd69bd:

  target-arm: kvm: use KVM_GET_MSRS/KVM_SET_MSRS for CP15 registers. (2012-07-25 15:37:44 +0930)

----------------------------------------------------------------
Rusty Russell (3):
      Local update of linux-headers/asm-arm/kvm.h
      target-arm: kvm: use KVM_ARM_VCPU_INIT to set target to Cortex A15
      target-arm: kvm: use KVM_GET_MSRS/KVM_SET_MSRS for CP15 registers.

 linux-headers/asm-arm/kvm.h |   50 +++++++++++++++++++++++++++++-------
 linux-headers/linux/kvm.h   |    3 +++
 target-arm/kvm.c            |   60 +++++++++++++++++++++++++++++++++----------
 3 files changed, 91 insertions(+), 22 deletions(-)

Re: [Qemu-devel] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Peter Maydell]

On 25 July 2012 07:17, Rusty Russell <rusty.russell@linaro.org> wrote:
> On Tue, 17 Jul 2012 19:19:16 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
>> So I can't see anything better than "complain and return failure
>> from kvm_arch_init_vcpu() if the guest CPU isn't going to work".
>
> OK, here's what I have for kvm.  It matches the kernel patches I just
> sent out, with the updated API.
>
> I rewrote all the backwards compat stuff to wrap it in #ifdef
> HORRIBLE_COMPAT_HACK, but something subtle broke and it no longer set
> cpsr correctly for older kernels.  After wasting an afternoon on it, I
> gave up and threw out the backwards compat code in digust.
>
> Nor does it bisect, since we update the headers as you asked, then
> update the code.

Looks ok. We'll probably rewrite that register read/write code
at some point before upstreaming -- I want to see if we
can do that as a straightforward "iterate through cp15 hash
table and do something for every register" kind of loop,
but this is OK for purposes of staying in sync with the kernel.

I'm going to hold off on putting this in qemu-linaro until Christoffer
has acked the ABI changes on the kernel side, though.

>       Local update of linux-headers/asm-arm/kvm.h

>From the commit message for that:
# Worse, the new arm kvm_para.h includes asm-generic/kvm_para.h,
# which isn't included in qemu.

That we can deal with now, though: update-linux-headers.sh
just needs updating to include it in its non-arch-specific
set of copied headers.

-- PMM

Re: [Qemu-devel] [kvmarm] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Antonios Motakis]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

On Wed, Jul 25, 2012 at 8:17 AM, Rusty Russell <rusty.russell@linaro.org>wrote:

> On Tue, 17 Jul 2012 19:19:16 +0100, Peter Maydell <
> peter.maydell@linaro.org> wrote:
> > So I can't see anything better than "complain and return failure
> > from kvm_arch_init_vcpu() if the guest CPU isn't going to work".
>
> OK, here's what I have for kvm.  It matches the kernel patches I just
> sent out, with the updated API.
>
> I rewrote all the backwards compat stuff to wrap it in #ifdef
> HORRIBLE_COMPAT_HACK, but something subtle broke and it no longer set
> cpsr correctly for older kernels.  After wasting an afternoon on it, I
> gave up and threw out the backwards compat code in digust.
>
> Nor does it bisect, since we update the headers as you asked, then
> update the code.
>
> The following changes since commit
> 94d477a4017d45222a4a0227857ad9fc3f81fb2d:
>
>   Merge upstream master into qemu-linaro (2012-07-16 15:04:16 +0100)
>
> are available in the git repository at:
>
>
>   git://git.linaro.org/people/rusty/qemu-linaro.git msr-ops
>
> for you to fetch changes up to caf1e8a68ceb6ffd485e96d2f05102b095dd69bd:
>
>   target-arm: kvm: use KVM_GET_MSRS/KVM_SET_MSRS for CP15 registers.
> (2012-07-25 15:37:44 +0930)
>
> ----------------------------------------------------------------
> Rusty Russell (3):
>       Local update of linux-headers/asm-arm/kvm.h
>       target-arm: kvm: use KVM_ARM_VCPU_INIT to set target to Cortex A15
>       target-arm: kvm: use KVM_GET_MSRS/KVM_SET_MSRS for CP15 registers.
>
>  linux-headers/asm-arm/kvm.h |   50 +++++++++++++++++++++++++++++-------
>  linux-headers/linux/kvm.h   |    3 +++
>  target-arm/kvm.c            |   60
> +++++++++++++++++++++++++++++++++----------
>  3 files changed, 91 insertions(+), 22 deletions(-)
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
>

Hello,

I've been trying to get your msr-ops linux-kvm & qemu branches to work, but
so far I've not had any luck booting a guest. I'll nuke my setup and start
over tomorrow to make sure I don't have something broken. Are there any
known issues with the code so far?

Cheers,
Antonios

[-- Attachment #2: Type: text/html, Size: 3052 bytes --]

Re: [Qemu-devel] [kvmarm] [PATCH] target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

[Rusty Russell]

On Tue, 31 Jul 2012 23:52:20 +0200, Antonios Motakis <a.motakis@virtualopensystems.com> wrote:
> Hello,
> 
> I've been trying to get your msr-ops linux-kvm & qemu branches to work, but
> so far I've not had any luck booting a guest. I'll nuke my setup and start
> over tomorrow to make sure I don't have something broken. Are there any
> known issues with the code so far?

Hmm, works for me...

I've just moved everything across to a separate linux-kvm-arm tree, and
just pushed the exact version I was using:

     git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git

(branch msr-ops).

If that fails, please describe symtoms...

Thanks,
Rusty.