From patchwork Thu Jun 19 03:46:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: vkamensky X-Patchwork-Id: 32162 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-vc0-f199.google.com (mail-vc0-f199.google.com [209.85.220.199]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id A336D203F3 for ; Thu, 19 Jun 2014 03:48:36 +0000 (UTC) Received: by mail-vc0-f199.google.com with SMTP id ij19sf5260926vcb.10 for ; Wed, 18 Jun 2014 20:48:36 -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=FyEnVWW4qJvJKC6GXaQWNGsjLfdUB85ghprmltFp8UE=; b=hdeaJvsVh5E56QhxFZJDFxrE5l3Bc/31AnjuhLly4soXjzXsXPmVzpurcc3Rl7dCab WwiVlmYhJYLpr1y+Rj2JTRAQqaBHgqGUJ8tKn6Glg2ln0X34Lxjn9mAMkO3aYDxkUchg 9wJMtJ8RidCfgf6Pb+ujsqFIaeWhUsMiQuQxHtlXCZ6n9kxpNA6N6ObltOQhm1y0wa1Y J/SEpxd7BPEh/f694tChS0ea/201fAxuSco2BQ+NgEkB+yZ4thTVnfrxIkviv77glEf2 eqyST4V0UQC7RvTSzo9S47IzTheuXxiepCo/YJ0D6R8yLO0wL5f8LENsAD12MK9QBAu5 nkaQ== X-Gm-Message-State: ALoCoQnMZznigLfLsYgFWOKJo6qHj8yGB013ygs13InQCJyZeMgBQoNLW/N7/k6J5jPbbMOvaJbR X-Received: by 10.58.237.130 with SMTP id vc2mr1273213vec.25.1403149716404; Wed, 18 Jun 2014 20:48:36 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.43.135 with SMTP id e7ls430588qga.70.gmail; Wed, 18 Jun 2014 20:48:36 -0700 (PDT) X-Received: by 10.221.37.1 with SMTP id tc1mr1758416vcb.32.1403149716310; Wed, 18 Jun 2014 20:48:36 -0700 (PDT) Received: from mail-ve0-f172.google.com (mail-ve0-f172.google.com [209.85.128.172]) by mx.google.com with ESMTPS id xw10si1765694vdb.51.2014.06.18.20.48.36 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 18 Jun 2014 20:48:36 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.128.172 as permitted sender) client-ip=209.85.128.172; Received: by mail-ve0-f172.google.com with SMTP id jz11so1759836veb.17 for ; Wed, 18 Jun 2014 20:48:36 -0700 (PDT) X-Received: by 10.58.245.194 with SMTP id xq2mr1769662vec.26.1403149716189; Wed, 18 Jun 2014 20:48:36 -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.221.54.6 with SMTP id vs6csp330527vcb; Wed, 18 Jun 2014 20:48:35 -0700 (PDT) X-Received: by 10.140.81.74 with SMTP id e68mr2725387qgd.77.1403149715481; Wed, 18 Jun 2014 20:48:35 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id 2si4871100qaa.29.2014.06.18.20.48.35 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Jun 2014 20:48:35 -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 1WxTJB-0001L2-4u; Thu, 19 Jun 2014 03:46:41 +0000 Received: from mail-qa0-f50.google.com ([209.85.216.50]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WxTJ7-00017e-A2 for linux-arm-kernel@lists.infradead.org; Thu, 19 Jun 2014 03:46:38 +0000 Received: by mail-qa0-f50.google.com with SMTP id m5so1485214qaj.23 for ; Wed, 18 Jun 2014 20:46:14 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.140.105.183 with SMTP id c52mr3042103qgf.40.1403149574728; Wed, 18 Jun 2014 20:46:14 -0700 (PDT) Received: by 10.229.234.200 with HTTP; Wed, 18 Jun 2014 20:46:14 -0700 (PDT) In-Reply-To: <20140614154747.GA18355@lvm> References: <1402590613-3341-1-git-send-email-victor.kamensky@linaro.org> <1402590613-3341-13-git-send-email-victor.kamensky@linaro.org> <20140614150459.GH14023@lvm> <20140614154747.GA18355@lvm> Date: Wed, 18 Jun 2014 20:46:14 -0700 Message-ID: Subject: Re: [PATCH v4 12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case From: Victor Kamensky To: Christoffer Dall , Marc Zyngier X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140618_204637_521127_DA597EEE X-CRM114-Status: GOOD ( 24.97 ) X-Spam-Score: -0.7 (/) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-0.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.216.50 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RCVD_IN_MSPIKE_H3 RBL: Good reputation (+3) [209.85.216.50 listed in wl.mailspike.net] -0.0 RCVD_IN_MSPIKE_WL Mailspike good senders Cc: Taras Kondratiuk , "linaro-kernel@lists.linaro.org" , "kvmarm@lists.cs.columbia.edu" , "linux-arm-kernel@lists.infradead.org" , Alexander Graf X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 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.128.172 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 Christoffer, Marc, Please see inline. I am looking for your opinion/advise on how we go further about this patch. On 14 June 2014 08:47, Christoffer Dall wrote: > On Sat, Jun 14, 2014 at 08:42:58AM -0700, Victor Kamensky wrote: >> On 14 June 2014 08:04, Christoffer Dall wrote: >> > On Thu, Jun 12, 2014 at 09:30:11AM -0700, Victor Kamensky wrote: >> >> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as >> >> one 'unsigned long *' bit fields, which has 64bit size. So we need to >> >> swap least significant word with most significant word when code reads >> >> those registers from h/w. >> >> >> >> Signed-off-by: Victor Kamensky >> >> --- >> >> arch/arm64/kvm/hyp.S | 7 +++++++ >> >> 1 file changed, 7 insertions(+) >> >> >> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >> >> index 0620691..5035b41 100644 >> >> --- a/arch/arm64/kvm/hyp.S >> >> +++ b/arch/arm64/kvm/hyp.S >> >> @@ -415,10 +415,17 @@ CPU_BE( rev w11, w11 ) >> >> str w4, [x3, #VGIC_CPU_HCR] >> >> str w5, [x3, #VGIC_CPU_VMCR] >> >> str w6, [x3, #VGIC_CPU_MISR] >> >> +#ifndef CONFIG_CPU_BIG_ENDIAN >> >> str w7, [x3, #VGIC_CPU_EISR] >> >> str w8, [x3, #(VGIC_CPU_EISR + 4)] >> >> str w9, [x3, #VGIC_CPU_ELRSR] >> >> str w10, [x3, #(VGIC_CPU_ELRSR + 4)] >> >> +#else >> >> + str w7, [x3, #(VGIC_CPU_EISR + 4)] >> >> + str w8, [x3, #VGIC_CPU_EISR] >> >> + str w9, [x3, #(VGIC_CPU_ELRSR + 4)] >> >> + str w10, [x3, #VGIC_CPU_ELRSR] >> >> +#endif >> >> str w11, [x3, #VGIC_CPU_APR] >> >> >> >> /* Clear GICH_HCR */ >> >> -- >> >> 1.8.1.4 >> >> >> > I thought Marc had something here which allowed you to deal with the >> > conversion in the accessor functions and avoid this patch? >> >> Christoffer, I appreciate your review comments. >> >> I think I was missing something. Yes, Marc mentioned in [1] about >> his new changes in vgic3 series. But just after rereading it now, I >> realized that he was suggesting to pick up his commits and add >> them to this series. Is it my right understanding that they should >> be [2] and [3] ... looking a bit closer to it, it seems that [4] is needed >> as well. I am concerned that I don't understand all dependencies >> and impact of those. Wondering about other way around. When vgic3 >> series introduced could we just back off above change and do it in >> new right way? >> >> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009618.html >> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html >> [3] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html >> [4] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009473.html >> >> Other question: I was testing all this directly on vanilla v3.15, should I >> use some other armkvm specific integration branch to make sure it works >> with all other in a queue armkvm changes. >> >> In mean time I will try to pick up [4], [2], and [3] into v3.15 and see >> how it goes. >> > ok, thanks. I'm ok with potentially adjusting this later if it turns > out to be a pain, depends on what Marc says. I've tried BE KVM series along with Marc's vgic3 series and looked closely at picking up accessors to eisr and elrsr from the vgic3 series ([1] and [2]). It is not trivial. First of all, existing patches besides accessors introduce callbacks in vgic_ops, and that pulls pretty much everything before it. I did try to split [1] and [2] into couple patches each, one with accessors and another adding vgic_ops callbacks. In such way I could pick first part and leave vgic_ops callback in the series. Split worked OK. I can give example how it would look. However when I've tried to move accessors part to top of Marc's vgic3 series I got massive conflicts. Personally I don't have confidence that I can resolve them correctly, and I don't think Marc would want to do that as well. I don't think it is worth it. Instead I propose let's come back to cleaning it up latter after vgic3 code gets in. I've tried the following patch in tree with combined series and it worked OK. Author: Victor Kamensky Date: Tue Jun 17 21:20:25 2014 -0700 ARM64: KVM: change vgic2 eisr and elrsr word order in big endian case Now when code uses eisr and elrsr the accessors, move big endian related code into the accessors. Now in eisr and elrsr arrays keep least siginificant word at index 0 and most siginificant word at index 1. Asm code that stores values in array is the same for little and big endian cases. Correct endian neutral access to u64 values provided by accessors functions. Signed-off-by: Victor Kamensky static u32 vgic_v2_get_interrupt_status(const struct kvm_vcpu *vcpu) Basically it backoffs this commit and changes accessor to read values assuming that vgic_v2 eisr and elrsr array holds value of least siginificant word at index 0, and most significant word at index 1. Please let me know what you think. Thanks, Victor [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009475.html [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009472.html > I can probably fix up any conflicts when I apply the patches, but I do > appreciate getting patches that apply to the next branch in [1]. (But > wait until the next branch merges 3.16-rc1). > > -Christoffer > > [1]: https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/ diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S index d5fc5aa..ae21177 100644 --- a/arch/arm64/kvm/vgic-v2-switch.S +++ b/arch/arm64/kvm/vgic-v2-switch.S @@ -67,17 +67,10 @@ CPU_BE( rev w11, w11 ) str w4, [x3, #VGIC_V2_CPU_HCR] str w5, [x3, #VGIC_V2_CPU_VMCR] str w6, [x3, #VGIC_V2_CPU_MISR] -#ifndef CONFIG_CPU_BIG_ENDIAN str w7, [x3, #VGIC_V2_CPU_EISR] str w8, [x3, #(VGIC_V2_CPU_EISR + 4)] str w9, [x3, #VGIC_V2_CPU_ELRSR] str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)] -#else - str w7, [x3, #(VGIC_V2_CPU_EISR + 4)] - str w8, [x3, #VGIC_V2_CPU_EISR] - str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)] - str w10, [x3, #VGIC_V2_CPU_ELRSR] -#endif str w11, [x3, #VGIC_V2_CPU_APR] /* Clear GICH_HCR */ diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c index a55a9a4..a4b6f13 100644 --- a/virt/kvm/arm/vgic-v2.c +++ b/virt/kvm/arm/vgic-v2.c @@ -79,14 +79,30 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr, static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu) { + u64 ret; const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr; - return *(u64 *)elrsr; + /* + * vgic v2 elrsr is kept as two words, with least significant + * word first. Get its value in endian agnostic way. + */ + ret = *(elrsr + 1); + ret = ret << 32; + ret = ret | *elrsr; + return ret; } static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu) { + u64 ret; const u32 *eisr = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr; - return *(u64 *)eisr; + /* + * vgic v2 eisr is kept as two words, with least siginificant + * word first. Get its value in endian agnostic way. + */ + ret = *(eisr + 1); + ret = ret << 32; + ret = ret | *eisr; + return ret; }