From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store() Date: Mon, 21 Sep 2015 07:50:22 +0200 Message-ID: <55FF9A9E.6070205@redhat.com> References: <1442559448-1947-1-git-send-email-thuth@redhat.com> <20150921013708.GF20331@voom.fritz.box> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: kvm-ppc@vger.kernel.org, Paul Mackerras , Alexander Graf , kvm@vger.kernel.org To: David Gibson , Thomas Huth Return-path: Received: from mail-wi0-f172.google.com ([209.85.212.172]:37483 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752501AbbIUFuW (ORCPT ); Mon, 21 Sep 2015 01:50:22 -0400 In-Reply-To: <20150921013708.GF20331@voom.fritz.box> Sender: kvm-owner@vger.kernel.org List-ID: On 21/09/2015 03:37, David Gibson wrote: > On Fri, Sep 18, 2015 at 08:57:28AM +0200, Thomas Huth wrote: >> Access to the kvm->buses (like with the kvm_io_bus_read() and >> -write() functions) has to be protected via the kvm->srcu lock. >> The kvmppc_h_logical_ci_load() and -store() functions are >> missing this lock so far, so let's add it there, too. This fixes >> the problem that the kernel reports "suspicious RCU usage" when >> lock debugging is enabled. >> >> Fixes: 99342cf8044420eebdf9297ca03a14cb6a7085a1 Signed-off-by: >> Thomas Huth > > Nice catch. Looks like I missed this because the places > kvm_io_bus_{read,write}() are called on x86 are buried about 5 > layers below where the srcu lock is taken :/. > > Reviewed-by: David Gibson > >> --- arch/powerpc/kvm/book3s.c | 6 ++++++ 1 file changed, 6 >> insertions(+) >> >> diff --git a/arch/powerpc/kvm/book3s.c >> b/arch/powerpc/kvm/book3s.c index d75bf32..096e5eb 100644 --- >> a/arch/powerpc/kvm/book3s.c +++ b/arch/powerpc/kvm/book3s.c @@ >> -828,12 +828,15 @@ int kvmppc_h_logical_ci_load(struct kvm_vcpu >> *vcpu) unsigned long size = kvmppc_get_gpr(vcpu, 4); unsigned >> long addr = kvmppc_get_gpr(vcpu, 5); u64 buf; + int srcu_idx; int >> ret; >> >> if (!is_power_of_2(size) || (size > sizeof(buf))) return >> H_TOO_HARD; >> >> + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); ret = >> kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, size, &buf); + >> srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); if (ret != 0) >> return H_TOO_HARD; >> >> @@ -868,6 +871,7 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu >> *vcpu) unsigned long addr = kvmppc_get_gpr(vcpu, 5); unsigned >> long val = kvmppc_get_gpr(vcpu, 6); u64 buf; + int srcu_idx; int >> ret; >> >> switch (size) { @@ -891,7 +895,9 @@ int >> kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu) return >> H_TOO_HARD; } >> >> + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); ret = >> kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, size, &buf); + >> srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); if (ret != 0) >> return H_TOO_HARD; >> > Paul, shall I take this directly into my tree for -rc3? Paolo