From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0D44C3A383C; Mon, 10 Aug 2026 14:03:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786370629; cv=none; b=L/1qjWBZnjfHHlcSOpqWKBWh88ArcT+w4aqogJCOGUJ7DqWkZQlU4n81Q3L6R6C9OMo4yN8TNa3OyNjWXEDoGcEDTyNa3t+ngUxWjH4c7IZCFJaqRT4iRIRVLVLuxZoTDsq6Q//o88Icxj0vSCLxTQIvAgjxgytt3FhbzD+dZvg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786370629; c=relaxed/simple; bh=iKgaQJIjcdWs4LaCjt3vfrTthKiqTPWMkO+qs4P7hfM=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=R6oITKMXLuLqGD1ODboN8ZN1kmFPA3HYcY00n0J9bjahN1UM6RsssrtXWNs1tWnPkCQOfBHaV2e/ojLw/QUgI5xAuo5Th25aKZ8liNFNR8RYHPU16IqYF8YZXzM8SeSUIjIxw/lrf1rgBtxv8no18ZiODCrm2UMsXnCo5ZGZdQE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=SWQRkjKP; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="SWQRkjKP" Received: by smtp.kernel.org (Postfix) with ESMTPSA id AB20F1F000E9; Mon, 10 Aug 2026 14:03:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1786370627; bh=Em+BQBracOSXwokRIimW6WdMSO/3+8vopQcKAyQpy+Y=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=SWQRkjKPpy6Z7XWCAcAQi4AW/srTp0eWQMuAibTB4MPtBtgKWR4V3WXbolzXHvHHY QtzWvJ8JO7R1vg2lzPAN8biSEP+74sl7wOqPCccf47NCJfqwDbsfv0dB/56OBNe5vX eu1ja8fMK5ScNpW/ARrrLswNrpJcvolKzVq2UHmXZmtiiCI3HmoyLh5v5aArKDYdbA Tqvp5deUdQLGmhcizGzCp3y5laEpx9ydGOkrgzcE1L/NPbbZXJR7PfygMJUlTcXO1O kg6k/ut045DscaciSP6kwFJYwm8vVSQln4AKTXDHZ9ulo4a4NZL5mkJUT/L4UJ1Zw9 QRYfhYK5cOEOg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1wtQbR-0000000E8L5-19oa; Mon, 10 Aug 2026 14:03:45 +0000 Date: Mon, 10 Aug 2026 15:03:44 +0100 Message-ID: <86tsp2139b.wl-maz@kernel.org> From: Marc Zyngier To: Karl Mehltretter Cc: Oliver Upton , kvmarm@lists.linux.dev, Fuad Tabba , Joey Gouly , Steffen Eiden , Suzuki K Poulose , Zenghui Yu , Catalin Marinas , Will Deacon , Paolo Bonzini , Shuah Khan , Eric Auger , kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org Subject: Re: [RFC PATCH 1/2] KVM: arm64: vgic-v3: Roll back failed redistributor region setup In-Reply-To: <4e00fc25aa61ec52e6ef033a53588ce3f5550982.1786344511.git.kmehltretter@gmail.com> References: <4e00fc25aa61ec52e6ef033a53588ce3f5550982.1786344511.git.kmehltretter@gmail.com> 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/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: linux-kselftest@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: kmehltretter@gmail.com, oupton@kernel.org, kvmarm@lists.linux.dev, fuad.tabba@linux.dev, joey.gouly@arm.com, seiden@linux.ibm.com, suzuki.poulose@arm.com, yuzenghui@huawei.com, catalin.marinas@arm.com, will@kernel.org, pbonzini@redhat.com, shuah@kernel.org, eric.auger@redhat.com, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Mon, 10 Aug 2026 07:52:16 +0100, Karl Mehltretter wrote: I'm sorry, but I find it difficult understand what you are trying to explain here: > A later REDIST_REGION attribute can be inserted successfully and then Later than what? > fail while registering redistributor iodevs. For example, a region that > overlaps the distributor is caught by vgic_v3_check_base() when the first > unassigned vCPU is processed. > > The existing rollback unregisters all previously registered > redistributor iodevs. It leaves their rd_iodev.base_addr values set, so a > corrected retry skips re-registering those stale vCPUs and succeeds with > holes in the MMIO bus. Holes in the MMIO space are the norm. The IPA space can multi-TB large, and there is no reason why it'd cover everything (where would you place the RAM otherwise?). Is the problem here that you are left with vcpus that seem to have been matched to an RD (base_addr being set), but that really are left unconnected? > > A failure from kvm_io_bus_register_dev() can leave another hole. The MMIO hole? Coverage hole? > failing vCPU already has its base address and region assigned, but the > old i < c rollback does not include it. What is "it"? > > Preserve devices assigned by earlier successful setters. On failure, > unregister only vCPUs associated with the newly inserted region, clear > their cached base addresses, and free that region. This also includes the > current vCPU when iodev registration itself fails. What I don't see here is an argument explaining that doing this doesn't change the guest-visible assignment of RDs, which would be a regression. > > Fixes: c011f4ea106b ("KVM: arm/arm64: Check vcpu redist base before registering an iodev") > Assisted-by: Codex:gpt-5.6-sol > Signed-off-by: Karl Mehltretter > --- > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 42 ++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > index 5913a20d83019..832a7fe65984c 100644 > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > @@ -841,6 +841,31 @@ void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu) > kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &rd_dev->dev); > } > > +static void vgic_v3_rollback_redist_region(struct kvm *kvm, u32 index) > +{ > + struct vgic_redist_region *rdreg; > + struct kvm_vcpu *vcpu; > + unsigned long c; > + > + lockdep_assert_held(&kvm->slots_lock); > + > + rdreg = vgic_v3_rdist_region_from_index(kvm, index); > + > + kvm_for_each_vcpu(c, vcpu, kvm) { > + if (vcpu->arch.vgic_cpu.rdreg == rdreg) > + vgic_unregister_redist_iodev(vcpu); > + } > + > + guard(mutex)(&kvm->arch.config_lock); > + > + kvm_for_each_vcpu(c, vcpu, kvm) { > + if (vcpu->arch.vgic_cpu.rdreg == rdreg) > + vcpu->arch.vgic_cpu.rd_iodev.base_addr = VGIC_ADDR_UNDEF; > + } > + > + vgic_v3_free_redist_region(kvm, rdreg); > +} > + > static int vgic_register_all_redist_iodevs(struct kvm *kvm) > { > struct kvm_vcpu *vcpu; > @@ -855,16 +880,6 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm) > break; > } > > - if (ret) { > - /* The current c failed, so iterate over the previous ones. */ > - int i; > - > - for (i = 0; i < c; i++) { > - vcpu = kvm_get_vcpu(kvm, i); > - vgic_unregister_redist_iodev(vcpu); > - } > - } > - > return ret; > } > > @@ -984,12 +999,7 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count) > */ > ret = vgic_register_all_redist_iodevs(kvm); > if (ret) { > - struct vgic_redist_region *rdreg; > - > - mutex_lock(&kvm->arch.config_lock); > - rdreg = vgic_v3_rdist_region_from_index(kvm, index); > - vgic_v3_free_redist_region(kvm, rdreg); > - mutex_unlock(&kvm->arch.config_lock); > + vgic_v3_rollback_redist_region(kvm, index); > return ret; > } > Based on what I understand of your earlier description, why isn't this as simple as this untested hack: diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c index 5913a20d83019..804fcc69cbd48 100644 --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c @@ -859,7 +859,7 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm) /* The current c failed, so iterate over the previous ones. */ int i; - for (i = 0; i < c; i++) { + for (i = 0; i <= c; i++) { vcpu = kvm_get_vcpu(kvm, i); vgic_unregister_redist_iodev(vcpu); } @@ -960,8 +960,10 @@ void vgic_v3_free_redist_region(struct kvm *kvm, struct vgic_redist_region *rdre /* Garbage collect the region */ kvm_for_each_vcpu(c, vcpu, kvm) { - if (vcpu->arch.vgic_cpu.rdreg == rdreg) + if (vcpu->arch.vgic_cpu.rdreg == rdreg) { vcpu->arch.vgic_cpu.rdreg = NULL; + vcpu->arch.vgic_cpu.rd_iodev.base_addr = VGIC_ADDR_UNDEF; + } } list_del(&rdreg->list); I don't mind the cleaning up, but not as part of fixing the issue, which has to be as small as possible (think of the backports). Thanks, M. -- Without deviation from the norm, progress is not possible.