From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Date: Tue, 28 May 2019 04:17:11 +0000 Subject: Re: [PATCH] KVM: PPC: Book3S HV: XIVE: introduce a KVM device lock Message-Id: <20190528041711.ewohm2pdrya5ompz@oak.ozlabs.ibm.com> List-Id: References: <20190524132030.6349-1-clg@kaod.org> <20190524201621.23eb7c44@bahia.lan> In-Reply-To: <20190524201621.23eb7c44@bahia.lan> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Greg Kurz Cc: =?iso-8859-1?Q?C=E9dric?= Le Goater , Alexey Kardashevskiy , David Gibson , kvm@vger.kernel.org, kvm-ppc@vger.kernel.org Greg, On Fri, May 24, 2019 at 08:16:21PM +0200, Greg Kurz wrote: > On Fri, 24 May 2019 15:20:30 +0200 > C=E9dric Le Goater wrote: >=20 > > The XICS-on-XIVE KVM device needs to allocate XIVE event queues when a > > priority is used by the OS. This is referred as EQ provisioning and it > > is done under the hood when : > >=20 > > 1. a CPU is hot-plugged in the VM > > 2. the "set-xive" is called at VM startup > > 3. sources are restored at VM restore > >=20 > > The kvm->lock mutex is used to protect the different XIVE structures > > being modified but in some contextes, kvm->lock is taken under the > > vcpu->mutex which is a forbidden sequence by KVM. > >=20 > > Introduce a new mutex 'lock' for the KVM devices for them to > > synchronize accesses to the XIVE device structures. > >=20 > > Signed-off-by: C=E9dric Le Goater > > --- > > arch/powerpc/kvm/book3s_xive.h | 1 + > > arch/powerpc/kvm/book3s_xive.c | 23 +++++++++++++---------- > > arch/powerpc/kvm/book3s_xive_native.c | 15 ++++++++------- > > 3 files changed, 22 insertions(+), 17 deletions(-) > >=20 > > diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_x= ive.h > > index 426146332984..862c2c9650ae 100644 > > --- a/arch/powerpc/kvm/book3s_xive.h > > +++ b/arch/powerpc/kvm/book3s_xive.h > > @@ -141,6 +141,7 @@ struct kvmppc_xive { > > struct kvmppc_xive_ops *ops; > > struct address_space *mapping; > > struct mutex mapping_lock; > > + struct mutex lock; > > }; > > =20 > > #define KVMPPC_XIVE_Q_COUNT 8 > > diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_x= ive.c > > index f623451ec0a3..12c8a36dd980 100644 > > --- a/arch/powerpc/kvm/book3s_xive.c > > +++ b/arch/powerpc/kvm/book3s_xive.c > > @@ -271,14 +271,14 @@ static int xive_provision_queue(struct kvm_vcpu *= vcpu, u8 prio) > > return rc; > > } > > =20 > > -/* Called with kvm_lock held */ > > +/* Called with xive->lock held */ > > static int xive_check_provisioning(struct kvm *kvm, u8 prio) > > { > > struct kvmppc_xive *xive =3D kvm->arch.xive; >=20 > Since the kvm_lock isn't protecting kvm->arch anymore, this looks weird. Are you suggesting that something that was protected before now isn't with C=E9dric's patch? > Passing xive instead of kvm and using xive->kvm would make more sense IMH= O. >=20 > Maybe fold the following into your patch ? As far as I can see your delta patch doesn't actually change any locking but just rationalizes the parameters for an internal function. That being so, for 5.2 I am intending to put C=E9dric's original patch in, unless someone comes up with a good reason not to. Paul. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4188CC04AB6 for ; Tue, 28 May 2019 04:17:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0B81E2054F for ; Tue, 28 May 2019 04:17:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=ozlabs.org header.i=@ozlabs.org header.b="ER6OfrX3" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726721AbfE1ERV (ORCPT ); Tue, 28 May 2019 00:17:21 -0400 Received: from ozlabs.org ([203.11.71.1]:46775 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725817AbfE1ERV (ORCPT ); Tue, 28 May 2019 00:17:21 -0400 Received: by ozlabs.org (Postfix, from userid 1003) id 45CgXt22Cwz9s9T; Tue, 28 May 2019 14:17:18 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ozlabs.org; s=201707; t=1559017038; bh=RdOxf3E5tUxsnW9M7gQbLgLEnpJ6TP1HLUx0MdurcxU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ER6OfrX3EWqrh+4q3VkjQffXpfag8PIeUeTA3OWvh6b1sTBIdGGiMqcLTGvIkHOKo Rz9Di6Q8oK05sDQRG417jp99AMEvJ3F3loJoCXX0Nnh2jm24jJJp12AJsrRTQlgbUb nc28cuHE34YPu0u0uhMQB0RkNgGXuwUjl/tUryqwQd/9XqgHv4Dy7iPMWkUN5EVbCm EH9FbIZy4HP/Oihh/NADR2eFAIZHMAAwcffOCH4hMikImB6+/z5GJWgxl3hBae+14F la24vW29vJsmvmt7MU85cP2z7HI2CLNqzl2kvMPZWpdDBre7jKHULL0S9mTHK4U3K1 pi3WWsknb9i4A== Date: Tue, 28 May 2019 14:17:11 +1000 From: Paul Mackerras To: Greg Kurz Cc: =?iso-8859-1?Q?C=E9dric?= Le Goater , Alexey Kardashevskiy , David Gibson , kvm@vger.kernel.org, kvm-ppc@vger.kernel.org Subject: Re: [PATCH] KVM: PPC: Book3S HV: XIVE: introduce a KVM device lock Message-ID: <20190528041711.ewohm2pdrya5ompz@oak.ozlabs.ibm.com> References: <20190524132030.6349-1-clg@kaod.org> <20190524201621.23eb7c44@bahia.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190524201621.23eb7c44@bahia.lan> User-Agent: NeoMutt/20170113 (1.7.2) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Greg, On Fri, May 24, 2019 at 08:16:21PM +0200, Greg Kurz wrote: > On Fri, 24 May 2019 15:20:30 +0200 > Cédric Le Goater wrote: > > > The XICS-on-XIVE KVM device needs to allocate XIVE event queues when a > > priority is used by the OS. This is referred as EQ provisioning and it > > is done under the hood when : > > > > 1. a CPU is hot-plugged in the VM > > 2. the "set-xive" is called at VM startup > > 3. sources are restored at VM restore > > > > The kvm->lock mutex is used to protect the different XIVE structures > > being modified but in some contextes, kvm->lock is taken under the > > vcpu->mutex which is a forbidden sequence by KVM. > > > > Introduce a new mutex 'lock' for the KVM devices for them to > > synchronize accesses to the XIVE device structures. > > > > Signed-off-by: Cédric Le Goater > > --- > > arch/powerpc/kvm/book3s_xive.h | 1 + > > arch/powerpc/kvm/book3s_xive.c | 23 +++++++++++++---------- > > arch/powerpc/kvm/book3s_xive_native.c | 15 ++++++++------- > > 3 files changed, 22 insertions(+), 17 deletions(-) > > > > diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h > > index 426146332984..862c2c9650ae 100644 > > --- a/arch/powerpc/kvm/book3s_xive.h > > +++ b/arch/powerpc/kvm/book3s_xive.h > > @@ -141,6 +141,7 @@ struct kvmppc_xive { > > struct kvmppc_xive_ops *ops; > > struct address_space *mapping; > > struct mutex mapping_lock; > > + struct mutex lock; > > }; > > > > #define KVMPPC_XIVE_Q_COUNT 8 > > diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c > > index f623451ec0a3..12c8a36dd980 100644 > > --- a/arch/powerpc/kvm/book3s_xive.c > > +++ b/arch/powerpc/kvm/book3s_xive.c > > @@ -271,14 +271,14 @@ static int xive_provision_queue(struct kvm_vcpu *vcpu, u8 prio) > > return rc; > > } > > > > -/* Called with kvm_lock held */ > > +/* Called with xive->lock held */ > > static int xive_check_provisioning(struct kvm *kvm, u8 prio) > > { > > struct kvmppc_xive *xive = kvm->arch.xive; > > Since the kvm_lock isn't protecting kvm->arch anymore, this looks weird. Are you suggesting that something that was protected before now isn't with Cédric's patch? > Passing xive instead of kvm and using xive->kvm would make more sense IMHO. > > Maybe fold the following into your patch ? As far as I can see your delta patch doesn't actually change any locking but just rationalizes the parameters for an internal function. That being so, for 5.2 I am intending to put Cédric's original patch in, unless someone comes up with a good reason not to. Paul.