diff mbox

[v2,3/7] AARCH64: Add aarch64 CPU initialization, get and put registers support

Message ID 1374571996-9228-4-git-send-email-m.hamayun@virtualopensystems.com
State New
Headers show

Commit Message

Mian M. Hamayun July 23, 2013, 9:33 a.m. UTC 
From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>

The cpu init function tries to initialize with all possible cpu types, as
KVM does not provide a means to detect the real cpu type and simply refuses
to initialize on cpu type mis-match. By using the loop based init function,
we avoid the need to modify code if the underlying platform is different,
such as Fast Models instead of Foundation Models.

Get and Put Registers deal with the basic state of Aarch64 CPUs for the moment.

Signed-off-by: Mian M. Hamayun <m.hamayun@virtualopensystems.com>

Conflicts:

	target-arm/kvm.c

Conflicts:

	target-arm/cpu.c
---
 linux-headers/linux/kvm.h |    1 +
 target-arm/kvm.c          |  125 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

Comments

Peter Maydell Aug. 9, 2013, 1:24 p.m. UTC | #1 
On 23 July 2013 10:33, Mian M. Hamayun <m.hamayun@virtualopensystems.com> wrote:
> From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>
>
> The cpu init function tries to initialize with all possible cpu types, as
> KVM does not provide a means to detect the real cpu type and simply refuses
> to initialize on cpu type mis-match. By using the loop based init function,
> we avoid the need to modify code if the underlying platform is different,
> such as Fast Models instead of Foundation Models.
>
> Get and Put Registers deal with the basic state of Aarch64 CPUs for the moment.
>
> Signed-off-by: Mian M. Hamayun <m.hamayun@virtualopensystems.com>
>
> Conflicts:
>
>         target-arm/kvm.c
>
> Conflicts:
>
>         target-arm/cpu.c

This sort of Conflicts note shouldn't be in a commit message.

> ---
>  linux-headers/linux/kvm.h |    1 +
>  target-arm/kvm.c          |  125 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index c614070..4df5292 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -783,6 +783,7 @@ struct kvm_dirty_tlb {
>  #define KVM_REG_IA64           0x3000000000000000ULL
>  #define KVM_REG_ARM            0x4000000000000000ULL
>  #define KVM_REG_S390           0x5000000000000000ULL
> +#define KVM_REG_ARM64          0x6000000000000000ULL
>
>  #define KVM_REG_SIZE_SHIFT     52
>  #define KVM_REG_SIZE_MASK      0x00f0000000000000ULL

Updates to the linux-headers/ files need to:
 * be a separate patch
 * be the result of running scripts/update-linux-headers.sh
   on an upstream mainline kernel or kvm-next kernel tree
 * include the kernel tree/commit hash in the commit message

> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index b92e00d..c96b871 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -32,6 +32,11 @@
>  #error mismatch between cpu.h and KVM header definitions
>  #endif
>
> +#ifdef TARGET_AARCH64
> +#define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> +                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
> +#endif
> +
>  const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>      KVM_CAP_LAST_INFO
>  };
> @@ -50,6 +55,33 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>      return cpu->cpu_index;
>  }
>
> +#ifdef TARGET_AARCH64

This set of #ifdefs is pretty messy. I suggest splitting kvm.c
into three:
  target-arm/kvm.c   -- anything common to both 32 and 64 bit
  target-arm/kvm32.c -- non-TARGET_AARCH64 functions
  target-arm/kvm64.c -- TARGET_AARCH64 functions

and have target-arm/Makefile.objs build only one of kvm32.c
or kvm64.c depending on whether TARGET_AARCH64 is set.

> +    /* Find an appropriate target CPU type.
> +     * KVM does not provide means to detect the host CPU type on aarch64,
> +     * and simply refuses to initialize, if the CPU type mis-matches;
> +     * so we try each possible CPU type on aarch64 before giving up! */
> +    for (i = 0; i < KVM_ARM_NUM_TARGETS; ++i) {
> +        init.target = kvm_arm_targets[i];
> +        ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
> +        if (!ret)
> +            break;
> +    }

We definitely don't want to do this -- see my notes on
'-cpu host' in another email thread.  (We should make sure
we coordinate who of you or Linaro is going to do the
-cpu host support, incidentally.)

-- PMM
Mian M. Hamayun Aug. 9, 2013, 7:03 p.m. UTC | #2 
On 09/08/2013 15:24, Peter Maydell wrote:
> On 23 July 2013 10:33, Mian M. Hamayun <m.hamayun@virtualopensystems.com> wrote:
>> From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>
>>
>> The cpu init function tries to initialize with all possible cpu types, as
>> KVM does not provide a means to detect the real cpu type and simply refuses
>> to initialize on cpu type mis-match. By using the loop based init function,
>> we avoid the need to modify code if the underlying platform is different,
>> such as Fast Models instead of Foundation Models.
>>
>> Get and Put Registers deal with the basic state of Aarch64 CPUs for the moment.
>>
>> Signed-off-by: Mian M. Hamayun <m.hamayun@virtualopensystems.com>
>>
>> Conflicts:
>>
>>          target-arm/kvm.c
>>
>> Conflicts:
>>
>>          target-arm/cpu.c
> This sort of Conflicts note shouldn't be in a commit message.
Agreed, will remove it in the next revision.
>
>> ---
>>   linux-headers/linux/kvm.h |    1 +
>>   target-arm/kvm.c          |  125 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 126 insertions(+)
>>
>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>> index c614070..4df5292 100644
>> --- a/linux-headers/linux/kvm.h
>> +++ b/linux-headers/linux/kvm.h
>> @@ -783,6 +783,7 @@ struct kvm_dirty_tlb {
>>   #define KVM_REG_IA64           0x3000000000000000ULL
>>   #define KVM_REG_ARM            0x4000000000000000ULL
>>   #define KVM_REG_S390           0x5000000000000000ULL
>> +#define KVM_REG_ARM64          0x6000000000000000ULL
>>
>>   #define KVM_REG_SIZE_SHIFT     52
>>   #define KVM_REG_SIZE_MASK      0x00f0000000000000ULL
> Updates to the linux-headers/ files need to:
>   * be a separate patch
>   * be the result of running scripts/update-linux-headers.sh
>     on an upstream mainline kernel or kvm-next kernel tree
>   * include the kernel tree/commit hash in the commit message
Agreed, will do it like this.
>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index b92e00d..c96b871 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -32,6 +32,11 @@
>>   #error mismatch between cpu.h and KVM header definitions
>>   #endif
>>
>> +#ifdef TARGET_AARCH64
>> +#define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
>> +                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>> +#endif
>> +
>>   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>       KVM_CAP_LAST_INFO
>>   };
>> @@ -50,6 +55,33 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>>       return cpu->cpu_index;
>>   }
>>
>> +#ifdef TARGET_AARCH64
> This set of #ifdefs is pretty messy. I suggest splitting kvm.c
> into three:
>    target-arm/kvm.c   -- anything common to both 32 and 64 bit
>    target-arm/kvm32.c -- non-TARGET_AARCH64 functions
>    target-arm/kvm64.c -- TARGET_AARCH64 functions
>
> and have target-arm/Makefile.objs build only one of kvm32.c
> or kvm64.c depending on whether TARGET_AARCH64 is set.
My initial intuition was to separate 32 and 64 bit versions, but as
many functions are common, so I decided to use #ifdefs instead.
btw, I have a similar situation in hw/arm/boot.c as well.
>
>> +    /* Find an appropriate target CPU type.
>> +     * KVM does not provide means to detect the host CPU type on aarch64,
>> +     * and simply refuses to initialize, if the CPU type mis-matches;
>> +     * so we try each possible CPU type on aarch64 before giving up! */
>> +    for (i = 0; i < KVM_ARM_NUM_TARGETS; ++i) {
>> +        init.target = kvm_arm_targets[i];
>> +        ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
>> +        if (!ret)
>> +            break;
>> +    }
> We definitely don't want to do this -- see my notes on
> '-cpu host' in another email thread.  (We should make sure
> we coordinate who of you or Linaro is going to do the
> -cpu host support, incidentally.)
I tend to agree with this proposition as well. But the point that I don't
understand is how something is acceptable in kvmtool and not in the
qemu. If this implementation lets us use the 64-bit architecture in the
current state of affairs, then why not use it. We can always replace
this particular part, when the -cpu host support becomes available, right ?

Hamayun
Peter Maydell Aug. 10, 2013, 9:10 a.m. UTC | #3 
On 9 August 2013 20:03, Mian M. Hamayun
<m.hamayun@virtualopensystems.com> wrote:
> On 09/08/2013 15:24, Peter Maydell wrote:
>> This set of #ifdefs is pretty messy. I suggest splitting kvm.c
>> into three:
>>    target-arm/kvm.c   -- anything common to both 32 and 64 bit
>>    target-arm/kvm32.c -- non-TARGET_AARCH64 functions
>>    target-arm/kvm64.c -- TARGET_AARCH64 functions
>>
>> and have target-arm/Makefile.objs build only one of kvm32.c
>> or kvm64.c depending on whether TARGET_AARCH64 is set.
>
> My initial intuition was to separate 32 and 64 bit versions, but as
> many functions are common, so I decided to use #ifdefs instead.

That's why I propose having a common-functions file.
Big "disable whole set of functions" ifdefs make it
harder to read a source file.

> btw, I have a similar situation in hw/arm/boot.c as well.

Yeah, I'm not entirely happy with the boot code either, but I didn't
have any concrete suggestions for improvement there.

>> We definitely don't want to do this -- see my notes on
>> '-cpu host' in another email thread.  (We should make sure
>> we coordinate who of you or Linaro is going to do the
>> -cpu host support, incidentally.)
>
> I tend to agree with this proposition as well. But the point that I don't
> understand is how something is acceptable in kvmtool and not in the
> qemu.

Different projects, different rules. In particular, kvmtool aims
to be a good medium for quick development and testing of new
features, whereas QEMU is trying to be the production-ready
backwards-compatible stable solution. That means that it's
often going to be possible to put code into kvmtool as an
interim "this works and we'll revisit it later" approach but
not really be able to do the same in QEMU.

> If this implementation lets us use the 64-bit architecture in the
> current state of affairs, then why not use it. We can always replace
> this particular part, when the -cpu host support becomes available,
> right ?

Also, the way that '-cpu host' support becomes available
is that we insist that it gets written in order for 64-bit
support to land. It's not really all that complicated.

-- PMM
diff mbox

Patch

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index c614070..4df5292 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -783,6 +783,7 @@  struct kvm_dirty_tlb {
 #define KVM_REG_IA64		0x3000000000000000ULL
 #define KVM_REG_ARM		0x4000000000000000ULL
 #define KVM_REG_S390		0x5000000000000000ULL
+#define KVM_REG_ARM64          0x6000000000000000ULL
 
 #define KVM_REG_SIZE_SHIFT	52
 #define KVM_REG_SIZE_MASK	0x00f0000000000000ULL
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index b92e00d..c96b871 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -32,6 +32,11 @@ 
 #error mismatch between cpu.h and KVM header definitions
 #endif
 
+#ifdef TARGET_AARCH64
+#define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
+                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
+#endif
+
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_LAST_INFO
 };
@@ -50,6 +55,33 @@  unsigned long kvm_arch_vcpu_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+#ifdef TARGET_AARCH64
+static uint32_t kvm_arm_targets[KVM_ARM_NUM_TARGETS] = {
+    KVM_ARM_TARGET_AEM_V8,
+    KVM_ARM_TARGET_FOUNDATION_V8,
+    KVM_ARM_TARGET_CORTEX_A57
+};
+
+int kvm_arch_init_vcpu(CPUState *cs)
+{
+    struct kvm_vcpu_init init;
+    int ret, i;
+
+    memset(init.features, 0, sizeof(init.features));
+    /* Find an appropriate target CPU type.
+     * KVM does not provide means to detect the host CPU type on aarch64,
+     * and simply refuses to initialize, if the CPU type mis-matches;
+     * so we try each possible CPU type on aarch64 before giving up! */
+    for (i = 0; i < KVM_ARM_NUM_TARGETS; ++i) {
+        init.target = kvm_arm_targets[i];
+        ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
+        if (!ret)
+            break;
+    }
+
+    return ret;
+}
+#else
 static bool reg_syncs_via_tuple_list(uint64_t regidx)
 {
     /* Return true if the regidx is a register we should synchronize
@@ -173,6 +205,7 @@  out:
     g_free(rlp);
     return ret;
 }
+#endif
 
 /* We track all the KVM devices which need their memory addresses
  * passing to the kernel in a list of these structures.
@@ -339,6 +372,7 @@  typedef struct Reg {
     int offset;
 } Reg;
 
+#ifndef TARGET_AARCH64
 #define COREREG(KERNELNAME, QEMUFIELD)                       \
     {                                                        \
         KVM_REG_ARM | KVM_REG_SIZE_U32 |                     \
@@ -402,7 +436,52 @@  static const Reg regs[] = {
     VFPSYSREG(FPINST),
     VFPSYSREG(FPINST2),
 };
+#endif
 
+#ifdef TARGET_AARCH64
+int kvm_arch_put_registers(CPUState *cs, int level)
+{
+    struct kvm_one_reg reg;
+    int i;
+    int ret;
+
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    for (i = 0; i < ARRAY_SIZE(env->xregs); i++) {
+        reg.id = AARCH64_CORE_REG(regs.regs[i]);
+        reg.addr = (uintptr_t) &env->xregs[i];
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    reg.id = AARCH64_CORE_REG(regs.sp);
+    reg.addr = (uintptr_t) &env->xregs[31];
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    reg.id = AARCH64_CORE_REG(regs.pstate);
+    reg.addr = (uintptr_t) &env->pstate;
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    reg.id = AARCH64_CORE_REG(regs.pc);
+    reg.addr = (uintptr_t) &env->pc;
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    /* TODO: Set Rest of Registers */
+    return ret;
+}
+#else
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -488,7 +567,52 @@  int kvm_arch_put_registers(CPUState *cs, int level)
 
     return ret;
 }
+#endif
 
+#ifdef TARGET_AARCH64
+int kvm_arch_get_registers(CPUState *cs)
+{
+    struct kvm_one_reg reg;
+    int i;
+    int ret;
+
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    for (i = 0; i < ARRAY_SIZE(env->xregs); i++) {
+        reg.id = AARCH64_CORE_REG(regs.regs[i]);
+        reg.addr = (uintptr_t) &env->xregs[i];
+        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    reg.id = AARCH64_CORE_REG(regs.sp);
+    reg.addr = (uintptr_t) &env->xregs[31];
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    reg.id = AARCH64_CORE_REG(regs.pstate);
+    reg.addr = (uintptr_t) &env->pstate;
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    reg.id = AARCH64_CORE_REG(regs.pc);
+    reg.addr = (uintptr_t) &env->pc;
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    /* TODO: Set Rest of Registers */
+    return ret;
+}
+#else
 int kvm_arch_get_registers(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -559,6 +683,7 @@  int kvm_arch_get_registers(CPUState *cs)
 
     return 0;
 }
+#endif
 
 void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
 {