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 09D0AC6FD1F for ; Wed, 22 Mar 2023 12:03:38 +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:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tuXFUZq9q2HEig+pYlqvkKPzJvl6LdGxvCchrRGL1UY=; b=hwxpoT4m9igkx2 GgXt5r7iUd6EMBU7LKbsZZmiRtKpz69Pqni5b/ktuStgTA/G1lOTAmVJ1k1RPw9wfo3z7R9y5mRZt jjdi2vpc1vyzJjqYXO4rM9Eyv3TtUSc1vy2Y+nDm4RfNEniRISdQapQFGCyvgZG9T8ZR6gJwZniRk NqM0uthUAnJM9G0K1JEmdATJ1wZPEvPgHfXaU0DUt3wX2oBil84pOFRx7i5PJaC+vyZvPeACSPEQi gyYA77+N7KoEscpCRIZbwJG7iPE+STddMVqHmIkW1i8l9LP6dpXo+qXZ/pqlmMxjsePJfmZEb1xYY Hyp9gQtxeoCNV78UFiQA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pexAh-00Fr8d-2n; Wed, 22 Mar 2023 12:02:27 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pexAf-00Fr7X-0B for linux-arm-kernel@lists.infradead.org; Wed, 22 Mar 2023 12:02:27 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 8F104B81A12; Wed, 22 Mar 2023 12:02:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3481AC433D2; Wed, 22 Mar 2023 12:02:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1679486539; bh=rgf80NZ6Lf8YlvQh7lNwvfe1vjJ9RqRyEotFA1blxkE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=pUCA1wNnedBOtiY9kskI+rWDvAkH8KIeoi87P7azHYr4TuGcP/QCXPNhRBOd5iqZx 60pgD4fJjOU4AYBv0BoergYs8BlSooUMK48nL0CZKFWsgrZscEOOc+Tk5D/BbSrzYL 9ZPoi1/FkiVZOVrbNSymvu9pB4KsDb75fqZ36IQhVQHrBxgG4GONpXC88MCO8Y8qMp EZKBc7HP/RS7NHftYhlHjqcndFUuR8soJX4vZJsH/NfIGXa4Q3UX+ZskbHUAk+VJXZ d8EbNE7uQf2OebJ4kjyRb8vV+tZAah2Qp54YpbCja1/aProfVr1DjW3TspBJF0D2LL J4iBqBTHIU70g== Received: from [206.0.71.16] (helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1pexAW-002CJ1-D1; Wed, 22 Mar 2023 12:02:16 +0000 Date: Wed, 22 Mar 2023 12:02:15 +0000 Message-ID: <87o7olgfjs.wl-maz@kernel.org> From: Marc Zyngier To: Oliver Upton Cc: James Morse , Suzuki K Poulose , kvmarm@lists.linux.dev, Zenghui Yu , linux-arm-kernel@lists.infradead.org, Sean Christopherson Subject: Re: [PATCH v2 4/4] KVM: arm64: Use config_lock to protect vgic state In-Reply-To: <20230316211412.2651555-5-oliver.upton@linux.dev> References: <20230316211412.2651555-1-oliver.upton@linux.dev> <20230316211412.2651555-5-oliver.upton@linux.dev> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 206.0.71.16 X-SA-Exim-Rcpt-To: oliver.upton@linux.dev, james.morse@arm.com, suzuki.poulose@arm.com, kvmarm@lists.linux.dev, yuzenghui@huawei.com, linux-arm-kernel@lists.infradead.org, seanjc@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230322_050225_416603_783C9B0B X-CRM114-Status: GOOD ( 35.40 ) 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 Thu, 16 Mar 2023 21:14:12 +0000, Oliver Upton wrote: > > Almost all of the vgic state is VM-scoped but accessed from the context > of a vCPU. These accesses were serialized on the kvm->lock which cannot > be nested within a vcpu->mutex critical section. > > Move over the vgic state to using the config_lock. Tweak the lock > ordering where necessary to ensure that the config_lock is acquired > after the vcpu->mutex. Acquire the config_lock in kvm_vgic_create() to > avoid a race between the converted flows and GIC creation. > > Signed-off-by: Oliver Upton > --- > arch/arm64/kvm/vgic/vgic-debug.c | 8 ++-- > arch/arm64/kvm/vgic/vgic-init.c | 33 ++++++++++------- > arch/arm64/kvm/vgic/vgic-its.c | 29 ++++++--------- > arch/arm64/kvm/vgic/vgic-kvm-device.c | 53 ++++++++++++--------------- > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 4 +- > arch/arm64/kvm/vgic/vgic-mmio.c | 12 +++--- > arch/arm64/kvm/vgic/vgic-v4.c | 11 +++--- > arch/arm64/kvm/vgic/vgic.c | 2 +- > 8 files changed, 75 insertions(+), 77 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-debug.c b/arch/arm64/kvm/vgic/vgic-debug.c > index 78cde687383c..07aa0437125a 100644 > --- a/arch/arm64/kvm/vgic/vgic-debug.c > +++ b/arch/arm64/kvm/vgic/vgic-debug.c > @@ -85,7 +85,7 @@ static void *vgic_debug_start(struct seq_file *s, loff_t *pos) > struct kvm *kvm = s->private; > struct vgic_state_iter *iter; > > - mutex_lock(&kvm->lock); > + mutex_lock(&kvm->arch.config_lock); > iter = kvm->arch.vgic.iter; > if (iter) { > iter = ERR_PTR(-EBUSY); > @@ -104,7 +104,7 @@ static void *vgic_debug_start(struct seq_file *s, loff_t *pos) > if (end_of_vgic(iter)) > iter = NULL; > out: > - mutex_unlock(&kvm->lock); > + mutex_unlock(&kvm->arch.config_lock); > return iter; > } > > @@ -132,12 +132,12 @@ static void vgic_debug_stop(struct seq_file *s, void *v) > if (IS_ERR(v)) > return; > > - mutex_lock(&kvm->lock); > + mutex_lock(&kvm->arch.config_lock); > iter = kvm->arch.vgic.iter; > kfree(iter->lpi_array); > kfree(iter); > kvm->arch.vgic.iter = NULL; > - mutex_unlock(&kvm->lock); > + mutex_unlock(&kvm->arch.config_lock); > } > > static void print_dist_state(struct seq_file *s, struct vgic_dist *dist) > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c > index cd134db41a57..b1690063e17d 100644 > --- a/arch/arm64/kvm/vgic/vgic-init.c > +++ b/arch/arm64/kvm/vgic/vgic-init.c > @@ -74,9 +74,6 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) > unsigned long i; > int ret; > > - if (irqchip_in_kernel(kvm)) > - return -EEXIST; > - > /* > * This function is also called by the KVM_CREATE_IRQCHIP handler, > * which had no chance yet to check the availability of the GICv2 > @@ -91,6 +88,13 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) > if (!lock_all_vcpus(kvm)) > return ret; > > + mutex_lock(&kvm->arch.config_lock); > + > + if (irqchip_in_kernel(kvm)) { > + ret = -EEXIST; > + goto out_unlock; > + } > + > kvm_for_each_vcpu(i, vcpu, kvm) { > if (vcpu_has_run_once(vcpu)) > goto out_unlock; > @@ -118,6 +122,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) > INIT_LIST_HEAD(&kvm->arch.vgic.rd_regions); > > out_unlock: > + mutex_unlock(&kvm->arch.config_lock); > unlock_all_vcpus(kvm); > return ret; > } > @@ -227,9 +232,9 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) > * KVM io device for the redistributor that belongs to this VCPU. > */ > if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > - mutex_lock(&vcpu->kvm->lock); > + mutex_lock(&vcpu->kvm->arch.config_lock); > ret = vgic_register_redist_iodev(vcpu); > - mutex_unlock(&vcpu->kvm->lock); > + mutex_unlock(&vcpu->kvm->arch.config_lock); > } > return ret; > } > @@ -250,7 +255,6 @@ static void kvm_vgic_vcpu_enable(struct kvm_vcpu *vcpu) > * The function is generally called when nr_spis has been explicitly set > * by the guest through the KVM DEVICE API. If not nr_spis is set to 256. > * vgic_initialized() returns true when this function has succeeded. > - * Must be called with kvm->lock held! > */ > int vgic_init(struct kvm *kvm) > { > @@ -259,6 +263,8 @@ int vgic_init(struct kvm *kvm) > int ret = 0, i; > unsigned long idx; > > + lockdep_assert_held(&kvm->arch.config_lock); > + > if (vgic_initialized(kvm)) > return 0; > > @@ -373,12 +379,13 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu) > vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF; > } > > -/* To be called with kvm->lock held */ > static void __kvm_vgic_destroy(struct kvm *kvm) > { > struct kvm_vcpu *vcpu; > unsigned long i; > > + lockdep_assert_held(&kvm->arch.config_lock); > + > vgic_debug_destroy(kvm); > > kvm_for_each_vcpu(i, vcpu, kvm) > @@ -389,9 +396,9 @@ static void __kvm_vgic_destroy(struct kvm *kvm) > > void kvm_vgic_destroy(struct kvm *kvm) > { > - mutex_lock(&kvm->lock); > + mutex_lock(&kvm->arch.config_lock); > __kvm_vgic_destroy(kvm); > - mutex_unlock(&kvm->lock); > + mutex_unlock(&kvm->arch.config_lock); > } > > /** > @@ -414,9 +421,9 @@ int vgic_lazy_init(struct kvm *kvm) > if (kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V2) > return -EBUSY; > > - mutex_lock(&kvm->lock); > + mutex_lock(&kvm->arch.config_lock); > ret = vgic_init(kvm); > - mutex_unlock(&kvm->lock); > + mutex_unlock(&kvm->arch.config_lock); > } > > return ret; > @@ -441,7 +448,7 @@ int kvm_vgic_map_resources(struct kvm *kvm) > if (likely(vgic_ready(kvm))) > return 0; > > - mutex_lock(&kvm->lock); > + mutex_lock(&kvm->arch.config_lock); > if (vgic_ready(kvm)) > goto out; > > @@ -459,7 +466,7 @@ int kvm_vgic_map_resources(struct kvm *kvm) > dist->ready = true; > > out: > - mutex_unlock(&kvm->lock); > + mutex_unlock(&kvm->arch.config_lock); > return ret; > } > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index 2642e9ce2819..ca55065102e7 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -2043,7 +2043,10 @@ static int vgic_its_attr_regs_access(struct kvm_device *dev, > if (offset & align) > return -EINVAL; > > - mutex_lock(&dev->kvm->lock); > + if (!lock_all_vcpus(dev->kvm)) > + return -EBUSY; > + > + mutex_lock(&dev->kvm->arch.config_lock); Huh, that's fishy. The whole "lock the VM and the lock the individual vcpus" is there to prevent a concurrent creation of a vcpu while we're doing stuff that affects them all. Allowing a new vcpu to come online while this sequence is happening is ... unexpected. Why do we need to drop this initial lock? I'd expect them to be completely cumulative. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel