From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34904) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xj4Kw-00089d-S1 for qemu-devel@nongnu.org; Tue, 28 Oct 2014 06:49:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xj4Ks-000254-BI for qemu-devel@nongnu.org; Tue, 28 Oct 2014 06:49:14 -0400 Received: from mail-lb0-f182.google.com ([209.85.217.182]:36683) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xj4Ks-00024y-1C for qemu-devel@nongnu.org; Tue, 28 Oct 2014 06:49:10 -0400 Received: by mail-lb0-f182.google.com with SMTP id f15so349157lbj.41 for ; Tue, 28 Oct 2014 03:49:07 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1414478281-5956-1-git-send-email-pranavkumar@linaro.org> References: <1414478281-5956-1-git-send-email-pranavkumar@linaro.org> From: Peter Maydell Date: Tue, 28 Oct 2014 10:48:46 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH] target-arm: Add guest cpu endianness determination for virtio in KVM ARM64 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pranavkumar Sawargaonkar Cc: Marc Zyngier , "patches@apm.com" , QEMU Developers , =?UTF-8?B?QWxleCBCZW5uw6ll?= , "kvmarm@lists.cs.columbia.edu" , Christoffer Dall On 28 October 2014 06:38, Pranavkumar Sawargaonkar wrote: > This patch implements a fucntion pointer virtio_is_big_endian() > from "CPUClass" structure for arm64. > Function aarch64_cpu_virtio_endianness() is added to determine and > returns the guest cpu endianness to virtio. > This is required for running cross endian guests with virtio on ARM64. Thanks for this patch (and the associated testing). Is this the only thing we need for big-endian guest kernels, or are there other patches to follow? I thought we needed something in the kernel loader code too... > Signed-off-by: Pranavkumar Sawargaonkar > --- > include/hw/virtio/virtio-access.h | 2 ++ > target-arm/cpu64.c | 41 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+) > > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h > index 46456fd..84fa701 100644 > --- a/include/hw/virtio/virtio-access.h > +++ b/include/hw/virtio/virtio-access.h > @@ -23,6 +23,8 @@ static inline bool virtio_access_is_big_endian(VirtIODevice *vdev) > return virtio_is_big_endian(vdev); > #elif defined(TARGET_WORDS_BIGENDIAN) > return true; > +#elif defined(TARGET_AARCH64) > + return virtio_is_big_endian(vdev); As Greg says, we can just have the ARM cpu.h define TARGET_BIENDIAN. > #else > return false; > #endif > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c > index c30f47e..789f886 100644 > --- a/target-arm/cpu64.c > +++ b/target-arm/cpu64.c > @@ -192,6 +192,43 @@ static void aarch64_cpu_set_pc(CPUState *cs, vaddr value) > } > } > > +#ifndef CONFIG_USER_ONLY > + > +#define KVM_REG_ARM64_SCTLR_EL1 3, 0, 1, 0, 0 > + > +static bool aarch64_cpu_virtio_endianness(CPUState *cs) > +{ > + ARMCPU *cpu = ARM_CPU(cs); > + CPUARMState *env = &cpu->env; > + struct kvm_one_reg reg; > + uint64_t sctlr; > + > + cpu_synchronize_state(cs); > + > + /* Check if we are running 32bit guest or not */ > + if (!is_a64(env)) > + return (env->pstate & CPSR_E) ? 1 : 0; If you run your patches through checkpatch.pl you'll find they don't correspond with QEMU's coding style here. > + > + /* Ideally we do not need to call IOCTL again to read SCTLR_EL1 value. > + * cpu_synchronize_state() should fill the env->cp15.c1_sys > + * to get this value but this path is currently not implemented for arm64. > + * Hence this is a temporary fix. > + */ > + > + reg.id = ARM64_SYS_REG(KVM_REG_ARM64_SCTLR_EL1); (This won't compile on non-linux hosts, incidentally, because the ARM64_SYS_REG macro is in the kvm headers.) > + reg.addr = (uint64_t) &sctlr; > + kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); As you say really we need to implement sysreg state synchronization properly (IIRC Alex B had some patches for this). I don't think we want to take this patch as-is, though it's very helpful for demonstrating that everything else works. > + > + if ((env->pstate & 0xf) == PSTATE_MODE_EL0t) > + sctlr &= (1U <<24); > + else > + sctlr &= (1U <<25); We have SCTLR_E0E and SCTLR_EE defines in cpu.h for these bits now. > + > + /* If BIG-ENDIAN return 1 */ > + return sctlr ? 1 : 0; > +} > +#endif > + > static void aarch64_cpu_class_init(ObjectClass *oc, void *data) > { > CPUClass *cc = CPU_CLASS(oc); > @@ -203,6 +240,10 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data) > cc->gdb_write_register = aarch64_cpu_gdb_write_register; > cc->gdb_num_core_regs = 34; > cc->gdb_core_xml_file = "aarch64-core.xml"; > +#ifndef CONFIG_USER_ONLY > + cc->virtio_is_big_endian = aarch64_cpu_virtio_endianness; An implementation that doesn't need to work around our lack of aarch64 sysreg syncing could be implemented in the ARM cpu class so it worked for both 32 bit and 64 bit CPUs, I think. > +#endif > + > } > > static void aarch64_cpu_register(const ARMCPUInfo *info) thanks -- PMM