Re: [RFC PATCH 1/2] KVM: arm64: vgic-v3: Roll back failed redistributor region setup

On Mon, 10 Aug 2026 22:22:37 +0100,
Karl Mehltretter <[email protected]> wrote:
> 
> On Mon, Aug 10, 2026 at 03:03:44PM +0100, Marc Zyngier wrote:
> > 
> > > A later REDIST_REGION attribute can be inserted successfully and then
> > 
> > Later than what?
> 
> I meant a REDIST_REGION write after earlier
> region writes have already assigned RDs to some vCPUs.
> 
> In the selftest (patch 2) 
> - region 0 contains RDs for vCPUs 0 and 1
> - region 1 contains RD for vCPU 2
> - the REDIST_REGION write for region 2 fails while KVM processes vCPU 3
> 
> > 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?
> 
> Yes I guess "hole" is the wrong term. The vCPU still has an RD and a
> base address, but its iodev has been removed from KVM_MMIO_BUS.
> An access to that RD address then causes KVM_RUN to return
> to userspace with KVM_EXIT_MMIO.
> 
> > 
> > > failing vCPU already has its base address and region assigned, but the
> > > old i < c rollback does not include it.
> > 
> > What is "it"?
> 
> I meant the current vCPU redistributor iodev.
> vgic_register_redist_iodev() sets the vCPU's region and base address
> before calling kvm_io_bus_register_dev(). If this call fails for
> vCPU c, the rollback in vgic_register_all_redist_iodevs() processes only
> vCPUs with indices below c.
> 
> > > 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.
> 
> vgic_register_redist_iodev() returns immediately if a vCPU's RD base
> address is already set, so its RD region and address don't change.
> 
> Regions are filled in index order, a vCPU without an RD address can
> use the new region only after older regions are full.
> 
> So freeing the new region and clearing the RD state of vCPUs associated
> with it restores the state before the failed write.
> 
> > Based on what I understand of your earlier description, why isn't this
> > as simple as this untested hack:
> 
> I ran the selftest (patch 2) with this, still failed with:
> 
>   Unexpected MMIO exit at 0x8050008
> 
> When redistributor registration for vCPU 3 fails, the for loop does
> vCPUs 0 to 3. The issue is that the iodevs for vCPUs 0 to 2 were
> registered by earlier successful region writes and should not be
> unregistered.

But that's not something we guarantee. Actually, that's not something
userspace should have any view on. And we're far better off just
resetting everything

> 
> On retry, vgic_register_redist_iodev() sees the set addresses
> and does not re-register those iodevs.

Which address? All addresses have been reset. What's missing in my
original patch is resetting the free_index counter.

> 
> > 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).
> 
> I reworked the change without a new helper (see below).
> Is this closer to what you had in mind?

[snip]

No. I want *all* RDs to be gone on failure, and the next successful
registration to result in all possible RDs to be matched against a
vcpu if there are enough of them. Effectively an atomic behaviour from
the PoV of userspace.

See the patch below for what I came up with.

	M.

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 5913a20d8301..94d2f4c6dc51 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -839,6 +839,13 @@ void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
 	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
 
 	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &rd_dev->dev);
+
+	guard(mutex)(&vcpu->kvm->arch.config_lock);
+	if (vcpu->arch.vgic_cpu.rdreg) {
+		vcpu->arch.vgic_cpu.rdreg->free_index--;
+		vcpu->arch.vgic_cpu.rdreg = NULL;
+	}
+	rd_dev->base_addr = VGIC_ADDR_UNDEF;
 }
 
 static int vgic_register_all_redist_iodevs(struct kvm *kvm)
@@ -859,7 +866,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);
 		}
@@ -953,17 +960,8 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
 
 void vgic_v3_free_redist_region(struct kvm *kvm, struct vgic_redist_region *rdreg)
 {
-	struct kvm_vcpu *vcpu;
-	unsigned long c;
-
 	lockdep_assert_held(&kvm->arch.config_lock);
 
-	/* Garbage collect the region */
-	kvm_for_each_vcpu(c, vcpu, kvm) {
-		if (vcpu->arch.vgic_cpu.rdreg == rdreg)
-			vcpu->arch.vgic_cpu.rdreg = NULL;
-	}
-
 	list_del(&rdreg->list);
 	kfree(rdreg);
 }

-- 
Without deviation from the norm, progress is not possible.