From patchwork Tue May 7 10:00:17 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tiejun Chen X-Patchwork-Id: 242105 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 50C2D2C017D for ; Tue, 7 May 2013 20:00:45 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754611Ab3EGKAi (ORCPT ); Tue, 7 May 2013 06:00:38 -0400 Received: from mail1.windriver.com ([147.11.146.13]:45224 "EHLO mail1.windriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751021Ab3EGKAh (ORCPT ); Tue, 7 May 2013 06:00:37 -0400 Received: from ALA-HCA.corp.ad.wrs.com (ala-hca.corp.ad.wrs.com [147.11.189.40]) by mail1.windriver.com (8.14.5/8.14.3) with ESMTP id r47A0NMY004602 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL); Tue, 7 May 2013 03:00:23 -0700 (PDT) Received: from [128.224.162.168] (128.224.162.168) by ALA-HCA.corp.ad.wrs.com (147.11.189.50) with Microsoft SMTP Server (TLS) id 14.2.342.3; Tue, 7 May 2013 03:00:24 -0700 Message-ID: <5188D0B1.2050403@windriver.com> Date: Tue, 7 May 2013 18:00:17 +0800 From: "tiejun.chen" User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Scott Wood CC: Alexander Graf , , , Mihai Caraman , Benjamin Herrenschmidt Subject: Re: [PATCH] kvm/ppc: interrupt disabling fixes References: <1367897551-8382-1-git-send-email-scottwood@freescale.com> In-Reply-To: <1367897551-8382-1-git-send-email-scottwood@freescale.com> X-Originating-IP: [128.224.162.168] Sender: kvm-ppc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm-ppc@vger.kernel.org On 05/07/2013 11:32 AM, Scott Wood wrote: > booke64 was not maintaing consistent lazy ee state when exiting the One typo ;-) s/maintaing/maintaining > guest, leading to warnings and worse. > > booke32 was less affected due to the absence of lazy ee, but it was > still feeding bad information into trace_hardirqs_off/on -- we don't > want guest execution to be seen as an "IRQs off" interval. book3s_pr > also has this problem. > > book3s_pr and booke both used kvmppc_lazy_ee_enable() without > hard-disabling EE first, which could lead to races when irq_happened is > cleared, or if an interrupt happens after kvmppc_lazy_ee_enable(), and > possibly other issues. > > Now, on book3s_pr and booke, always hard-disable interrupts before > kvmppc_prepare_to_enter(), but leave them soft-enabled. On book3s, > this should results in the right lazy EE state when the asm code > hard-enables on an exit. On booke, we call hard_irq_disable() rather > than hard-enable immediately. Looks we always need to call hard_irq_disable() before kvmppc_prepare_to_enter(), so I think we can add hard_irq_disable() directly into kvmppc_prepare_to_enater() since this can avoid forgetting to call hard_irq_disable() when call kvmppc_prepare_to_enater() somewhere in the future. Here I assume Ben's fix to hard_irq_disable() is before this :) So what about this? diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h index d7339df..e4e2120 100644 --- a/arch/powerpc/include/asm/kvm_ppc.h +++ b/arch/powerpc/include/asm/kvm_ppc.h @@ -397,6 +397,13 @@ static inline void kvmppc_mmu_flush_icache(pfn_t pfn) static inline void kvmppc_lazy_ee_enable(void) { #ifdef CONFIG_PPC64 + /* + * To avoid races, the caller must have gone directly from having + * interrupts fully-enabled to hard-disabled. + */ + WARN_ON(local_paca->irq_happened != PACA_IRQ_HARD_DIS); + trace_hardirqs_on(); + /* Only need to enable IRQs by hard enabling them after this */ local_paca->irq_happened = 0; local_paca->soft_enabled = 1; diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c index d09baf1..1ea65cd 100644 --- a/arch/powerpc/kvm/book3s_pr.c +++ b/arch/powerpc/kvm/book3s_pr.c @@ -884,7 +884,6 @@ program_interrupt: * and if we really did time things so badly, then we just exit * again due to a host external interrupt. */ - local_irq_disable(); s = kvmppc_prepare_to_enter(vcpu); if (s <= 0) { local_irq_enable(); @@ -1121,7 +1120,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) * really did time things so badly, then we just exit again due to * a host external interrupt. */ - local_irq_disable(); ret = kvmppc_prepare_to_enter(vcpu); if (ret <= 0) { local_irq_enable(); diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index dc1f590..d412749 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -666,7 +666,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) return -EINVAL; } - local_irq_disable(); s = kvmppc_prepare_to_enter(vcpu); if (s <= 0) { local_irq_enable(); @@ -832,6 +831,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, { int r = RESUME_HOST; int s; +#ifdef CONFIG_PPC64 + WARN_ON(local_paca->irq_happened != 0); +#endif + hard_irq_disable(); /* update before a new last_exit_type is rewritten */ kvmppc_update_timing_stats(vcpu); @@ -1143,7 +1146,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, * aren't already exiting to userspace for some other reason. */ if (!(r & RESUME_HOST)) { - local_irq_disable(); s = kvmppc_prepare_to_enter(vcpu); if (s <= 0) { local_irq_enable(); diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 31084c6..147ac0e 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -64,7 +64,8 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) { int r = 1; - WARN_ON_ONCE(!irqs_disabled()); + hard_irq_disable(); + while (true) { if (need_resched()) { local_irq_enable();