From patchwork Mon Nov 28 18:39:04 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marc Zyngier X-Patchwork-Id: 9450021 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 373EA6074E for ; Mon, 28 Nov 2016 18:39:14 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 24DDC27F9E for ; Mon, 28 Nov 2016 18:39:14 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 17F3027FA7; Mon, 28 Nov 2016 18:39:14 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 90C5C27F9E for ; Mon, 28 Nov 2016 18:39:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752510AbcK1SjJ (ORCPT ); Mon, 28 Nov 2016 13:39:09 -0500 Received: from foss.arm.com ([217.140.101.70]:58574 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750971AbcK1SjI (ORCPT ); Mon, 28 Nov 2016 13:39:08 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CA42E16; Mon, 28 Nov 2016 10:39:07 -0800 (PST) Received: from [10.1.207.16] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F120B3F318; Mon, 28 Nov 2016 10:39:05 -0800 (PST) Subject: Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly To: Jintack Lim , kvmarm@lists.cs.columbia.edu References: <1480351570-11648-1-git-send-email-jintack@cs.columbia.edu> Cc: linux-arm-kernel@lists.infradead.org, christoffer.dall@linaro.org, will.deacon@arm.com, catalin.marinas@arm.com, pbonzini@redhat.com, rkrcmar@redhat.com, linux@armlinux.org.uk, julien.grall@arm.com, andre.przywara@arm.com, kvm@vger.kernel.org From: Marc Zyngier X-Enigmail-Draft-Status: N1110 Organization: ARM Ltd Message-ID: <799d03f5-a929-9547-1ae7-94026b76f116@arm.com> Date: Mon, 28 Nov 2016 18:39:04 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.2.0 MIME-Version: 1.0 In-Reply-To: Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 28/11/16 17:43, Marc Zyngier wrote: > Hi Jintack, > > On 28/11/16 16:46, Jintack Lim wrote: >> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit. >> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they >> are 11th and 10th bits respectively when E2H is set. Current code is >> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which >> may allow guest OS to access physical timer. So, fix it. >> >> Signed-off-by: Jintack Lim >> --- >> arch/arm/include/asm/kvm_timer.h | 33 +++++++++++++++++++ >> arch/arm64/include/asm/kvm_timer.h | 62 ++++++++++++++++++++++++++++++++++++ >> include/clocksource/arm_arch_timer.h | 6 ++-- >> virt/kvm/arm/hyp/timer-sr.c | 8 ++--- >> 4 files changed, 103 insertions(+), 6 deletions(-) >> create mode 100644 arch/arm/include/asm/kvm_timer.h >> create mode 100644 arch/arm64/include/asm/kvm_timer.h >> [...] > We could make it nicer (read "faster") by introducing a > hyp_alternate_select construct that only returns a value instead > of calling a function. I remember writing something like that > at some point, and dropping it... So here's what this could look like (warning, wacky code ahead, though I fixed a stupid bug that was present in the previous patch). The generated code is quite nice (no branch, only an extra mov instruction on the default path)... Of course, completely untested! Thanks, M. diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h index b18e852..d173902 100644 --- a/arch/arm64/include/asm/kvm_hyp.h +++ b/arch/arm64/include/asm/kvm_hyp.h @@ -121,6 +121,17 @@ typeof(orig) * __hyp_text fname(void) \ return val; \ } +#define hyp_alternate_select_const(fname, orig, alt, cond) \ +inline const typeof(orig) __hyp_text fname(void) \ +{ \ + typeof(alt) val = orig; \ + asm volatile(ALTERNATIVE("nop \n", \ + "mov %0, %1 \n", \ + cond) \ + : "+r" (val) : "r" (alt)); \ + return val; \ +} + void __vgic_v2_save_state(struct kvm_vcpu *vcpu); void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu); diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c index 798866a..7a783af 100644 --- a/virt/kvm/arm/hyp/timer-sr.c +++ b/virt/kvm/arm/hyp/timer-sr.c @@ -21,11 +21,29 @@ #include +#ifdef CONFIG_ARM64 +static hyp_alternate_select_const(cnthclt_shift, 0, 10, + ARM64_HAS_VIRT_HOST_EXTN) + +#else +#define cnthclt_shift() (0) +#endif + +static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set) +{ + u32 val; + int shift = cnthclt_shift(); + + val = read_sysreg(cnthctl_el2); + val &= ~(clr << shift); + val |= set << shift; + write_sysreg(val, cnthctl_el2); +} + /* vcpu is already in the HYP VA space */ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu) { struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; - u64 val; if (timer->enabled) { timer->cntv_ctl = read_sysreg_el0(cntv_ctl); @@ -36,9 +54,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu) write_sysreg_el0(0, cntv_ctl); /* Allow physical timer/counter access for the host */ - val = read_sysreg(cnthctl_el2); - val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; - write_sysreg(val, cnthctl_el2); + cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN); /* Clear cntvoff for the host */ write_sysreg(0, cntvoff_el2); @@ -48,16 +64,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu) { struct kvm *kvm = kern_hyp_va(vcpu->kvm); struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; - u64 val; /* * Disallow physical timer access for the guest * Physical counter access is allowed */ - val = read_sysreg(cnthctl_el2); - val &= ~CNTHCTL_EL1PCEN; - val |= CNTHCTL_EL1PCTEN; - write_sysreg(val, cnthctl_el2); + cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN); if (timer->enabled) { write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);