Re: [Qemu-devel] [PATCH v2 3/7] AARCH64: Add aarch64 CPU initialization, get and put registers support
On 9 August 2013 20:03, Mian M. Hamayun m.hama...@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
Re: [Qemu-devel] [PATCH v2 3/7] AARCH64: Add aarch64 CPU initialization, get and put registers support
On 23 July 2013 10:33, Mian M. Hamayun m.hama...@virtualopensystems.com wrote: From: Mian M. Hamayun m.hama...@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.hama...@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 0x3000ULL #define KVM_REG_ARM0x4000ULL #define KVM_REG_S390 0x5000ULL +#define KVM_REG_ARM64 0x6000ULL #define KVM_REG_SIZE_SHIFT 52 #define KVM_REG_SIZE_MASK 0x00f0ULL 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
Re: [Qemu-devel] [PATCH v2 3/7] AARCH64: Add aarch64 CPU initialization, get and put registers support
On 09/08/2013 15:24, Peter Maydell wrote: On 23 July 2013 10:33, Mian M. Hamayun m.hama...@virtualopensystems.com wrote: From: Mian M. Hamayun m.hama...@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.hama...@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 0x3000ULL #define KVM_REG_ARM0x4000ULL #define KVM_REG_S390 0x5000ULL +#define KVM_REG_ARM64 0x6000ULL #define KVM_REG_SIZE_SHIFT 52 #define KVM_REG_SIZE_MASK 0x00f0ULL 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
[Qemu-devel] [PATCH v2 3/7] AARCH64: Add aarch64 CPU initialization, get and put registers support
From: Mian M. Hamayun m.hama...@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.hama...@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(+) 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 0x3000ULL #define KVM_REG_ARM0x4000ULL #define KVM_REG_S390 0x5000ULL +#define KVM_REG_ARM64 0x6000ULL #define KVM_REG_SIZE_SHIFT 52 #define KVM_REG_SIZE_MASK 0x00f0ULL 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)