From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59595) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7ryC-0007tD-F6 for qemu-devel@nongnu.org; Fri, 09 Aug 2013 15:03:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7ry6-0003NX-Jm for qemu-devel@nongnu.org; Fri, 09 Aug 2013 15:03:28 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:35861) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7ry6-0003N9-6L for qemu-devel@nongnu.org; Fri, 09 Aug 2013 15:03:22 -0400 Received: by mail-wi0-f174.google.com with SMTP id j17so43329wiw.7 for ; Fri, 09 Aug 2013 12:03:20 -0700 (PDT) Message-ID: <52053CF6.3060600@virtualopensystems.com> Date: Fri, 09 Aug 2013 21:03:18 +0200 From: "Mian M. Hamayun" MIME-Version: 1.0 References: <1374571996-9228-1-git-send-email-m.hamayun@virtualopensystems.com> <1374571996-9228-4-git-send-email-m.hamayun@virtualopensystems.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/7] AARCH64: Add aarch64 CPU initialization, get and put registers support Reply-To: m.hamayun@virtualopensystems.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: tech@virtualopensystems.com, qemu-devel@nongnu.org, kvmarm@lists.cs.columbia.edu On 09/08/2013 15:24, Peter Maydell wrote: > On 23 July 2013 10:33, Mian M. Hamayun wrote: >> From: "Mian M. Hamayun" >> >> 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 >> >> 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