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: Victor Kamensky X-Patchwork-Id: 4380551 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 1F62DBEEAA for ; Thu, 19 Jun 2014 03:49:32 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 21D5720384 for ; Thu, 19 Jun 2014 03:49:31 +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 1C33A20382 for ; Thu, 19 Jun 2014 03:49:30 +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 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) 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=zdPmxOMteakoolBnCQ3dv5FLpJb8lum2u4Yqa9dpwj4=; b=UcVkeqydk+6vTIlxSXxO1iaPbBqcHbVosvI+RooAyOIxsIu6gdJZ1VDcNiqDR5qzKd pPbIwzPcPq2XPec0ekbMSKAgemJL0LamIYiuG7zfbXkffqtgD0BmMWF0NKmncmtd2P4g nYa31xXbqS7hE7yjDsYwUQOr/2B0YstGaGly5mMOWt90GFckkEtJajlW4Jrzg9GR2Vn+ qDq0IdZENHxrFnChWEVGCllXmyjUem4QKunKh748Ks3LoEJkWTeLgR5LzBgnQybYBERw U8w9mh9PALg9lZqQnN/OPf+hCnfzo2HrcsB1xfqe8Tk/fvWqtpdQcx269BIiBQt8ZZyU FHkQ== X-Gm-Message-State: ALoCoQlsKvJ0os9K+EozvYr70cO6nBa4U78GlGe5dgI9ie45q3eAABmw3tKFOZ3hokuzRk2BZIcG 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 (/) 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+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, T_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 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; }