From patchwork Mon Jan 14 22:08:39 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 1973901 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id EBD673FD86 for ; Mon, 14 Jan 2013 22:08:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757475Ab3ANWIk (ORCPT ); Mon, 14 Jan 2013 17:08:40 -0500 Received: from mail-ie0-f182.google.com ([209.85.223.182]:44520 "EHLO mail-ie0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757454Ab3ANWIj (ORCPT ); Mon, 14 Jan 2013 17:08:39 -0500 Received: by mail-ie0-f182.google.com with SMTP id s9so5936140iec.41 for ; Mon, 14 Jan 2013 14:08:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-originating-ip:in-reply-to:references:date :message-id:subject:from:to:cc:content-type:x-gm-message-state; bh=/Ptm8x6Pcn2+Jt/q/O1y+ewOoSkRhI+LVMi1MxOJzK0=; b=M+whuQ6iWyIIiFdI5ptCxp2zDYFWkWsZVCRHLOXnPQX7LjpQ1u8rePJPv7x2y5zcZp OBM13gnt6BpWvwMdJovIRlsbJOq1g0sb0kzEWlPqM68TjES6MOxsJpa40216q+zyr/9T Tph+gU1Yljcf5q55/L3/eJDdppTW+bxt6HRiO44baY0ZRgoeUjClB5bZWyxeNE9XFg4c TMbDo4/g/7Ltleb/JSCbGRjdGhpnMoMIoq0AaVDiSjRcaR/OmQONnoouFHOxT5602/XV heJCvF08oBsCS+cl0CXEWVjB7X5x+2T2XSy9T7bKIOKnobAMrkz3zxZdOABH1Wk85Thq Ze5Q== MIME-Version: 1.0 Received: by 10.50.36.198 with SMTP id s6mr51831igj.23.1358201319116; Mon, 14 Jan 2013 14:08:39 -0800 (PST) Received: by 10.64.37.70 with HTTP; Mon, 14 Jan 2013 14:08:39 -0800 (PST) X-Originating-IP: [72.80.83.148] In-Reply-To: <50F445BC.6050507@arm.com> References: <20130108184259.46758.17939.stgit@ubuntu> <20130108184327.46758.70599.stgit@ubuntu> <20130114152101.GD18935@mudshark.cambridge.arm.com> <50F445BC.6050507@arm.com> Date: Mon, 14 Jan 2013 17:08:39 -0500 Message-ID: Subject: Re: [PATCH v5 3/4] ARM: KVM: arch_timers: Add timer world switch From: Christoffer Dall To: Marc Zyngier Cc: Will Deacon , "kvm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "kvmarm@lists.cs.columbia.edu" X-Gm-Message-State: ALoCoQkSnqJS3WLcbujDAdS4M3em+K9ZALN5NcDS6pwJ2wLSr3Rr2tZHRumM/ZbISSkkqgK1lWUo Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Mon, Jan 14, 2013 at 12:51 PM, Marc Zyngier wrote: > On 14/01/13 15:21, Will Deacon wrote: >> On Tue, Jan 08, 2013 at 06:43:27PM +0000, Christoffer Dall wrote: >>> From: Marc Zyngier >>> >>> Do the necessary save/restore dance for the timers in the world >>> switch code. In the process, allow the guest to read the physical >>> counter, which is useful for its own clock_event_device. >> >> [...] >> >>> @@ -476,6 +513,7 @@ vcpu .req r0 @ vcpu pointer always in r0 >>> * for the host. >>> * >>> * Assumes vcpu pointer in vcpu reg >>> + * Clobbers r2-r4 >>> */ >>> .macro restore_timer_state >>> @ Disallow physical timer access for the guest >>> @@ -484,6 +522,30 @@ vcpu .req r0 @ vcpu pointer always in r0 >>> orr r2, r2, #CNTHCTL_PL1PCTEN >>> bic r2, r2, #CNTHCTL_PL1PCEN >>> mcr p15, 4, r2, c14, c1, 0 @ CNTHCTL >>> + >>> +#ifdef CONFIG_KVM_ARM_TIMER >>> + ldr r4, [vcpu, #VCPU_KVM] >>> + ldr r2, [r4, #KVM_TIMER_ENABLED] >>> + cmp r2, #0 >>> + beq 1f >>> + >>> + ldr r2, [r4, #KVM_TIMER_CNTVOFF] >>> + ldr r3, [r4, #(KVM_TIMER_CNTVOFF + 4)] >>> + mcrr p15, 4, r2, r3, c14 @ CNTVOFF >>> + isb >>> + >>> + ldr r4, =VCPU_TIMER_CNTV_CVAL >>> + add vcpu, vcpu, r4 >>> + ldrd r2, r3, [vcpu] >>> + sub vcpu, vcpu, r4 >>> + mcrr p15, 3, r2, r3, c14 @ CNTV_CVAL >>> + >>> + ldr r2, [vcpu, #VCPU_TIMER_CNTV_CTL] >>> + and r2, r2, #3 >>> + mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL >>> + isb >> >> How many of these isbs are actually needed, given that we're going to make >> an exception return to the guest? The last one certainly looks redundant and >> I can't see the need for ordering CNTVOFF vs CNTV_CVAL. I can see an >> argument to putting one *before* CNTV_CTL, but you don't have one there! > > CNTVOFF directly influences whether or not CNTV_CVAL will trigger or > not. Maybe I'm just being paranoid and moving the isb after CNTV_CVAL is > enough. > > The last one is definitively superfluous. > can't we also get rid of the isb on the return path then? do you agree with this patch: --- Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S index 57cfa84..7e6eedf 100644 --- a/arch/arm/kvm/interrupts_head.S +++ b/arch/arm/kvm/interrupts_head.S @@ -492,7 +492,6 @@ vcpu .req r0 @ vcpu pointer always in r0 str r2, [vcpu, #VCPU_TIMER_CNTV_CTL] bic r2, #1 @ Clear ENABLE mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL - isb mrrc p15, 3, r2, r3, c14 @ CNTV_CVAL ldr r4, =VCPU_TIMER_CNTV_CVAL @@ -532,18 +531,17 @@ vcpu .req r0 @ vcpu pointer always in r0 ldr r2, [r4, #KVM_TIMER_CNTVOFF] ldr r3, [r4, #(KVM_TIMER_CNTVOFF + 4)] mcrr p15, 4, r2, r3, c14 @ CNTVOFF - isb ldr r4, =VCPU_TIMER_CNTV_CVAL add vcpu, vcpu, r4 ldrd r2, r3, [vcpu] sub vcpu, vcpu, r4 mcrr p15, 3, r2, r3, c14 @ CNTV_CVAL + isb ldr r2, [vcpu, #VCPU_TIMER_CNTV_CTL] and r2, r2, #3 mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL - isb 1: #endif .endm