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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8A339C76196 for ; Thu, 23 Mar 2023 19:21:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DARnWYEK4ldf4UKKUS6Wjp/BwUTm3qKFCGG875aPElo=; b=lHr6nnhzFBCWzJ GkM5foiwH4yxOYU135a7BHCm9qp4VocNbP/s0URcqfRDu4q9zyhyjZeEnM0OJNLPEOAPT5Vypqnan i4ZndVSOFZCrpvN8+TyIy+linLkFv+hD4fQEsIKZ/mud+HUhdiC38nxFt+teTb2qmJt3ojmajKten TwO+QWTYWWNbD0HDk0dqn8HJjne3UgKCfcrIdEYAIohHDZC0LSY07eYTHnjmG46cn+9D1I79Axfh9 vey0J4kK1hP9ZpIswayRkIihvXzMBn9rnN+Nkv9jOjCIdfdI2ygEgl/i1YCnmiGAmbbSJKoYOmMFo vJw0k+9jE6m5VNkhaonA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pfQUZ-002owI-2k; Thu, 23 Mar 2023 19:20:55 +0000 Received: from out-50.mta1.migadu.com ([95.215.58.50]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pfQUW-002ovU-0q for linux-arm-kernel@lists.infradead.org; Thu, 23 Mar 2023 19:20:54 +0000 Date: Thu, 23 Mar 2023 19:20:44 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1679599248; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=TU2WH5z9ITX34JPCukkYfI2jUtBDW5taExr40IZx5X0=; b=cTPgx3i2SZdTQYOBBAM7OmEiYetGjyxLVpvZ+8naz1WEgkDjw18HaZq5w42YEmE/OYviVD OCE48ElaeFQgJAlntzrM3D+k+H1B0lmkNQOyboRb2wKnWYbS0C3bvhNCL2T4QBX63DJEpD O4kw/+eltP5sjTdhmow7PRji8h0w22A= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Oliver Upton To: Marc Zyngier Cc: James Morse , Suzuki K Poulose , kvmarm@lists.linux.dev, Zenghui Yu , linux-arm-kernel@lists.infradead.org, Sean Christopherson , Jeremy Linton Subject: Re: [PATCH v2 2/4] KVM: arm64: Avoid lock inversion when setting the VM register width Message-ID: References: <20230316211412.2651555-1-oliver.upton@linux.dev> <20230316211412.2651555-3-oliver.upton@linux.dev> <87lejpgfj3.wl-maz@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87lejpgfj3.wl-maz@kernel.org> X-Migadu-Flow: FLOW_OUT X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230323_122052_445329_C5F06889 X-CRM114-Status: GOOD ( 28.16 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Mar 22, 2023 at 12:02:40PM +0000, Marc Zyngier wrote: > On Thu, 16 Mar 2023 21:14:10 +0000, > Oliver Upton wrote: > > > > kvm->lock must be taken outside of the vcpu->mutex. Of course, the > > locking documentation for KVM makes this abundantly clear. Nonetheless, > > the locking order in KVM/arm64 has been wrong for quite a while; we > > acquire the kvm->lock while holding the vcpu->mutex all over the shop. > > > > All was seemingly fine until commit 42a90008f890 ("KVM: Ensure lockdep > > knows about kvm->lock vs. vcpu->mutex ordering rule") caught us with our > > pants down, leading to lockdep barfing: > > > > ====================================================== > > WARNING: possible circular locking dependency detected > > 6.2.0-rc7+ #19 Not tainted > > ------------------------------------------------------ > > qemu-system-aar/859 is trying to acquire lock: > > ffff5aa69269eba0 (&host_kvm->lock){+.+.}-{3:3}, at: kvm_reset_vcpu+0x34/0x274 > > > > but task is already holding lock: > > ffff5aa68768c0b8 (&vcpu->mutex){+.+.}-{3:3}, at: kvm_vcpu_ioctl+0x8c/0xba0 > > > > which lock already depends on the new lock. > > > > Add a dedicated lock to serialize writes to VM-scoped configuration from > > the context of a vCPU. Protect the register width flags with the new > > lock, thus avoiding the need to grab the kvm->lock while holding > > vcpu->mutex in kvm_reset_vcpu(). > > > > Reported-by: Jeremy Linton > > Link: https://lore.kernel.org/kvmarm/f6452cdd-65ff-34b8-bab0-5c06416da5f6@arm.com/ > > Signed-off-by: Oliver Upton > > --- > > arch/arm64/include/asm/kvm_host.h | 3 +++ > > arch/arm64/kvm/arm.c | 18 ++++++++++++++++++ > > arch/arm64/kvm/reset.c | 6 +++--- > > 3 files changed, 24 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index 917586237a4d..1f4b9708a775 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -185,6 +185,9 @@ struct kvm_protected_vm { > > }; > > > > struct kvm_arch { > > + /* Protects VM-scoped configuration data */ > > + struct mutex config_lock; > > + > > nit: can we move this down into the structure and keep the MM stuff on > its own at the top? Placing it next to the flags would make some > sense, as these flags are definitely related to configuration matters. Sure thing! > > struct kvm_s2_mmu mmu; > > > > /* VTCR_EL2 value for this VM */ > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index 731a78f85915..1478bec52767 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -128,6 +128,16 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > { > > int ret; > > > > + mutex_init(&kvm->arch.config_lock); > > + > > +#ifdef CONFIG_LOCKDEP > > + /* Clue in lockdep that the config_lock must be taken inside kvm->lock */ > > + mutex_lock(&kvm->lock); > > + mutex_lock(&kvm->arch.config_lock); > > + mutex_unlock(&kvm->arch.config_lock); > > + mutex_unlock(&kvm->lock); > > +#endif > > + > > ret = kvm_share_hyp(kvm, kvm + 1); > > if (ret) > > return ret; > > @@ -328,6 +338,14 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > > > > spin_lock_init(&vcpu->arch.mp_state_lock); > > > > +#ifdef CONFIG_LOCKDEP > > + /* Inform lockdep that the config_lock is acquired after vcpu->mutex */ > > + mutex_lock(&vcpu->mutex); > > + mutex_lock(&vcpu->kvm->arch.config_lock); > > + mutex_unlock(&vcpu->kvm->arch.config_lock); > > + mutex_unlock(&vcpu->mutex); > > +#endif > > Shouldn't this hunk be moved to the previous patch? Uh, I don't believe so since this is the patch that actually introduces kvm_arch::config_lock. The last patch was aimed at a separate lock for mp state. -- Thanks, Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel