From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932217AbcLLJHk (ORCPT ); Mon, 12 Dec 2016 04:07:40 -0500 Received: from mx5-phx2.redhat.com ([209.132.183.37]:55326 "EHLO mx5-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751799AbcLLJHj (ORCPT ); Mon, 12 Dec 2016 04:07:39 -0500 Date: Mon, 12 Dec 2016 04:07:37 -0500 (EST) From: Paolo Bonzini To: Roman Kagan Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, dvyukov@redhat.com Message-ID: <1857449606.3465312.1481533657884.JavaMail.zimbra@redhat.com> In-Reply-To: <20161212081357.GA27779@rkaganb.sw.ru> References: <20161210074607.30018-1-pbonzini@redhat.com> <20161212081357.GA27779@rkaganb.sw.ru> Subject: Re: [PATCH] KVM: hyperv: split lock to protect struct kvm_hv MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.4.164.1, 10.5.101.130] X-Mailer: Zimbra 8.0.6_GA_5922 (ZimbraWebClient - FF50 (Linux)/8.0.6_GA_5922) Thread-Topic: hyperv: split lock to protect struct kvm_hv Thread-Index: dDhettEQItvp0dLtlf2ETiV/uZ9M5Q== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- Original Message ----- > From: "Roman Kagan" > To: "Paolo Bonzini" > Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, dvyukov@redhat.com > Sent: Monday, December 12, 2016 9:13:57 AM > Subject: Re: [PATCH] KVM: hyperv: split lock to protect struct kvm_hv > > On Sat, Dec 10, 2016 at 08:46:07AM +0100, Paolo Bonzini wrote: > > Otherwise, there is an AB-BA deadlock between kvm->lock and > > vcpu->mutex. > > > > Reported-by: Dmitry Vyukov > > Signed-off-by: Paolo Bonzini > > --- > > Compile-tested only. > > > > Documentation/virtual/kvm/locking.txt | 2 ++ > > arch/x86/include/asm/kvm_host.h | 1 + > > arch/x86/kvm/hyperv.c | 10 +++++----- > > arch/x86/kvm/x86.c | 1 + > > 4 files changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/virtual/kvm/locking.txt > > b/Documentation/virtual/kvm/locking.txt > > index e5dd9f4d6100..5dd06289ce59 100644 > > --- a/Documentation/virtual/kvm/locking.txt > > +++ b/Documentation/virtual/kvm/locking.txt > > @@ -16,6 +16,8 @@ The acquisition orders for mutexes are as follows: > > For spinlocks, kvm_lock is taken outside kvm->mmu_lock. Everything > > else is a leaf: no other lock is taken inside the critical sections. > > > > +In particular, on x86, vcpu->mutex is taken outside > > kvm->arch.hyperv.hv_lock. > > + > > 2: Exception > > ------------ > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > b/arch/x86/include/asm/kvm_host.h > > index 7892530cbacf..2e25038dbd93 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -704,6 +704,7 @@ struct kvm_apic_map { > > > > /* Hyper-V emulation context */ > > struct kvm_hv { > > + struct mutex hv_lock; > > u64 hv_guest_os_id; > > u64 hv_hypercall; > > u64 hv_tsc_page; > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > > index 99cde5220e07..021abafabc12 100644 > > --- a/arch/x86/kvm/hyperv.c > > +++ b/arch/x86/kvm/hyperv.c > > @@ -1142,9 +1142,9 @@ int kvm_hv_set_msr_common(struct kvm_vcpu *vcpu, u32 > > msr, u64 data, bool host) > > if (kvm_hv_msr_partition_wide(msr)) { > > int r; > > > > - mutex_lock(&vcpu->kvm->lock); > > + mutex_lock(&vcpu->kvm->arch.hyperv.hv_lock); > > r = kvm_hv_set_msr_pw(vcpu, msr, data, host); > > - mutex_unlock(&vcpu->kvm->lock); > > + mutex_unlock(&vcpu->kvm->arch.hyperv.hv_lock); > > return r; > > } else > > return kvm_hv_set_msr(vcpu, msr, data, host); > > @@ -1155,9 +1155,9 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 > > msr, u64 *pdata) > > if (kvm_hv_msr_partition_wide(msr)) { > > int r; > > > > - mutex_lock(&vcpu->kvm->lock); > > + mutex_lock(&vcpu->kvm->arch.hyperv.hv_lock); > > r = kvm_hv_get_msr_pw(vcpu, msr, pdata); > > - mutex_unlock(&vcpu->kvm->lock); > > + mutex_unlock(&vcpu->kvm->arch.hyperv.hv_lock); > > return r; > > } else > > return kvm_hv_get_msr(vcpu, msr, pdata); > > @@ -1165,7 +1165,7 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 > > msr, u64 *pdata) > > > > bool kvm_hv_hypercall_enabled(struct kvm *kvm) > > { > > - return kvm->arch.hyperv.hv_hypercall & HV_X64_MSR_HYPERCALL_ENABLE; > > + return READ_ONCE(kvm->arch.hyperv.hv_hypercall) & > > HV_X64_MSR_HYPERCALL_ENABLE; > > } > > > > I'm afraid we have a problem with ->hv_tsc_page which can't be solved > with a similar READ_ONCE() in kvm_hv_setup_tsc_page(). I need to > double-check if taking a mutex is ok there; if not we may have to do > srcu... Yes, it can take a mutex. Paolo