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: vkamensky X-Patchwork-Id: 31033 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ig0-f199.google.com (mail-ig0-f199.google.com [209.85.213.199]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 66D3E2066E for ; Wed, 28 May 2014 06:13:42 +0000 (UTC) Received: by mail-ig0-f199.google.com with SMTP id c1sf6383322igq.2 for ; Tue, 27 May 2014 23:13:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:subject:from:to:cc:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :sender:errors-to:x-original-sender :x-original-authentication-results:mailing-list:content-type :content-transfer-encoding; bh=3Mwy9IHTXPNSNtou4tsiss6Y106L/j/V0gCzAQkfCbs=; b=VOPIT9Yf3OD+3jvktn6NZx/l10gCETFYkJA4PZcvFrvfYkPgSRbBtw01WU9iFQ+u0E pLvRc4FmG0R6711qiRChMrwTxjuYKlsmAvpe54RLBC0xF2ob7vbQUaQQMQSxupgNh+8Y 1I2dsJQixUaJxZR2kWRUFzsnzWj5J7WcPQbxVK894tuibB8bOx05xVTFQXYMZJIgnie4 0MsQcvZymRN7u0We18WR3v+VzfIYninGtlSH+4BieY6nqHKM25PlLZ77vt6O3iwZDoAf u4JbYB5q9Ctay0T3Ygv2ab2X6K4aMN2fZc+yh+HmJQrfHlK5mK6hDG+9hBI4+U01yE8Y LmSQ== X-Gm-Message-State: ALoCoQnVlTNbjRWLN+tUXU4t6IX2J9NV4t7JIkCcX8cHX+NGz9KnNk6zgMqE75qOGlgsCGknc/xz X-Received: by 10.182.125.4 with SMTP id mm4mr8398383obb.49.1401257621821; Tue, 27 May 2014 23:13:41 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.40.209 with SMTP id x75ls3286132qgx.46.gmail; Tue, 27 May 2014 23:13:41 -0700 (PDT) X-Received: by 10.52.135.226 with SMTP id pv2mr26895612vdb.33.1401257621617; Tue, 27 May 2014 23:13:41 -0700 (PDT) Received: from mail-vc0-f169.google.com (mail-vc0-f169.google.com [209.85.220.169]) by mx.google.com with ESMTPS id py4si9849121vcb.49.2014.05.27.23.13.41 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 27 May 2014 23:13:41 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.169 as permitted sender) client-ip=209.85.220.169; Received: by mail-vc0-f169.google.com with SMTP id ij19so11980911vcb.0 for ; Tue, 27 May 2014 23:13:41 -0700 (PDT) X-Received: by 10.220.95.204 with SMTP id e12mr356014vcn.37.1401257621512; Tue, 27 May 2014 23:13:41 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.220.221.72 with SMTP id ib8csp176034vcb; Tue, 27 May 2014 23:13:41 -0700 (PDT) X-Received: by 10.140.107.137 with SMTP id h9mr46667352qgf.30.1401257620825; Tue, 27 May 2014 23:13:40 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id r9si7610256qci.19.2014.05.27.23.13.40 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 May 2014 23:13:40 -0700 (PDT) Received-SPF: none (google.com: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org does not designate permitted sender hosts) client-ip=2001:1868:205::9; 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) 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 (/) X-Spam-Report: SpamAssassin version 3.3.2 on bombadil.infradead.org summary: Content analysis details: (-0.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/, no trust [209.85.192.54 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 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+patch=linaro.org@lists.infradead.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: victor.kamensky@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.169 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 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; }