From patchwork Wed May 28 06:11:57 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Victor Kamensky X-Patchwork-Id: 4252771 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 17EC6BF90B for ; Wed, 28 May 2014 06:14:19 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 265BC202EB for ; Wed, 28 May 2014 06:14:18 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2A4B2202E9 for ; Wed, 28 May 2014 06:14:17 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1WpX67-0007EC-GS; Wed, 28 May 2014 06:12:23 +0000 Received: from mail-qg0-f54.google.com ([209.85.192.54]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WpX64-00074E-OI for linux-arm-kernel@lists.infradead.org; Wed, 28 May 2014 06:12:21 +0000 Received: by mail-qg0-f54.google.com with SMTP id q108so16417342qgd.27 for ; Tue, 27 May 2014 23:11:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=mkEarOi3aKofhAwazbwtJCcQdpIAnsrUAZy+Out/tlA=; b=CjDOMBMEsBAOI0SYZkix6wWi26GXClKviaLlEm8oZGxOkExpl+CnVbYIYoHebehVPM pzNL6Opr2NjjQicOin5/g5CAQpQLahl7S6Fq/G6tFbM5oU02BkwRvA1nJthSv1JBRvHC c7K6ZKVeeBq+H+SGtOsjQE9GgIQNHiFP22hrsnssIatLSx8qpc5b/t/gZ1PC56p1pGYO ggjGkmT4+wOYN838ZDgxSyNumD65A34QvHCEG2TNVWQzh/wIlLTjye/ZQvdtRcplax3W 3gHwn1+KdOpxdyYKWwFUpoh+SZTFn0VDdXA31thu8wl5qP99ABwC/Z3MB+mlhVocu2A8 K1Aw== X-Gm-Message-State: ALoCoQk24NRGcdtHfpUa9nZPg+SDIbWSejU7H175B9yREcE0PebzboAXj/D7ivjc5c5Hz7L11vJT MIME-Version: 1.0 X-Received: by 10.224.21.1 with SMTP id h1mr22435020qab.103.1401257517711; Tue, 27 May 2014 23:11:57 -0700 (PDT) Received: by 10.229.19.70 with HTTP; Tue, 27 May 2014 23:11:57 -0700 (PDT) In-Reply-To: <20140526175225.GF31431@lvm> References: <1399997646-4716-1-git-send-email-victor.kamensky@linaro.org> <1399997646-4716-15-git-send-email-victor.kamensky@linaro.org> <20140526175225.GF31431@lvm> Date: Tue, 27 May 2014 23:11:57 -0700 Message-ID: Subject: Re: [PATCH v3 14/14] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest From: Victor Kamensky To: Christoffer Dall X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140527_231220_870362_FD3E6A1B X-CRM114-Status: GOOD ( 25.71 ) X-Spam-Score: -0.0 (/) Cc: "linaro-kernel@lists.linaro.org" , Marc Zyngier , Taras Kondratiuk , Alexander Graf , "kvmarm@lists.cs.columbia.edu" , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 26 May 2014 10:52, Christoffer Dall wrote: > On Tue, May 13, 2014 at 09:14:06AM -0700, Victor Kamensky wrote: >> Fix isssue with 32bit guests running on top of BE KVM host. Guest >> state is retored with double word read operations. Within the high > > restored (spell check should catch this). > > "Guest state is restored with double word read operations." I don't > know what this sentence is supposed to tell me. > >> and low word data is already byteswap. This code effectively swaps > > "data is already byteswap" is not English. data is already byteswapped? > >> two words within 64bit value. >> >> Signed-off-by: Victor Kamensky >> --- >> arch/arm64/kvm/sys_regs.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 060c3a9..4438b47 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -51,6 +51,16 @@ static u32 cache_levels; >> /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */ >> #define CSSELR_MAX 12 >> >> +/* Word access indexes for 64bit vm accessor */ >> +#ifdef CONFIG_CPU_BIG_ENDIAN >> +#define CP15_REG_MSW_INDEX 0 >> +#define CP15_REG_LSW_INDEX 1 >> +#else >> +#define CP15_REG_MSW_INDEX 1 >> +#define CP15_REG_LSW_INDEX 0 >> +#endif >> + >> + >> /* Which cache CCSIDR represents depends on CSSELR value. */ >> static u32 get_ccsidr(u32 csselr) >> { >> @@ -137,9 +147,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu, >> if (!p->is_aarch32) { >> vcpu_sys_reg(vcpu, r->reg) = val; >> } else { >> - vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL; >> + vcpu_cp15(vcpu, r->reg + CP15_REG_LSW_INDEX) = val & 0xffffffffUL; >> if (!p->is_32bit) >> - vcpu_cp15(vcpu, r->reg + 1) = val >> 32; >> + vcpu_cp15(vcpu, r->reg + CP15_REG_MSW_INDEX) = val >> 32; >> } >> return true; >> } >> -- >> 1.8.1.4 >> > I really don't like this. If anything I feel like it should be > abstracted inside vcpu_cp15, good point, please see revised proposal below > but wouldn't it be cleaner to do something > along the lines of: > > u64 *regstore = (u64 *)vcpu->arch.ctxt.cp15[r->reg]; > if (p->is_32bit) > val &= 0xffffffffUL; > *regstore = val; I don't think above will be correct. The way I read it the following hypothetical instructions sequence mcrr p15, 0, r6, r7, c2 @ TTBR 0 mcr p15, 0, r6, c2, c0, 0 @ 32bit TTBR 0 will get TTBR 0 high word to 0, when mrc instruction executes, but it should be left unchanged in case of 'p->is_32bit' How about something like following? Is it move into right direction? From b0a7793b03d9c62f7b9c53a317cb2d19a75c935b Mon Sep 17 00:00:00 2001 From: Victor Kamensky Date: Mon, 12 May 2014 13:57:21 -0700 Subject: [PATCH] ARM64: KVM: fix big endian issue in access_vm_reg for 32bit guest Fix issue with 32bit guests running on top of BE KVM host. Indexes of high and low words of 64bit cp15 register are swapped in case of big endian code, since 64bit cp15 state is restored or saved with double word write or read instruction. Define helper macros to access high low words of 64bit cp15 register. Signed-off-by: Victor Kamensky --- arch/arm64/include/asm/kvm_host.h | 8 ++++++++ arch/arm64/kvm/sys_regs.c | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 0a1d697..e9d2e11 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -140,6 +140,14 @@ struct kvm_vcpu_arch { #define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)]) #define vcpu_cp15(v,r) ((v)->arch.ctxt.cp15[(r)]) +#ifdef CONFIG_CPU_BIG_ENDIAN +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 0)]) +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 1)]) +#else +#define vcpu_cp15_64_high(v,r) ((v)->arch.ctxt.cp15[((r) + 1)]) +#define vcpu_cp15_64_low(v,r) ((v)->arch.ctxt.cp15[((r) + 0)]) +#endif + struct kvm_vm_stat { u32 remote_tlb_flush; }; diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index a13e7e7..b243e07 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -137,9 +137,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu, if (!p->is_aarch32) { vcpu_sys_reg(vcpu, r->reg) = val; } else { - vcpu_cp15(vcpu, r->reg) = val & 0xffffffffUL; + vcpu_cp15_64_low(vcpu, r->reg) = val & 0xffffffffUL; if (!p->is_32bit) - vcpu_cp15(vcpu, r->reg + 1) = val >> 32; + vcpu_cp15_64_high(vcpu, r->reg) = val >> 32; } return true; }