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

[Karl Mehltretter <kmehltretter@gmail.com>]

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.

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

> 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?

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 5913a20d83019..d9a28b983ecca 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -841,7 +841,7 @@ void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
 	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &rd_dev->dev);
 }
 
-static int vgic_register_all_redist_iodevs(struct kvm *kvm)
+static int vgic_register_all_redist_iodevs(struct kvm *kvm, u32 index)
 {
 	struct kvm_vcpu *vcpu;
 	unsigned long c;
@@ -856,12 +856,15 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm)
 	}
 
 	if (ret) {
-		/* The current c failed, so iterate over the previous ones. */
+		struct vgic_redist_region *rdreg;
 		int i;
 
-		for (i = 0; i < c; i++) {
+		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
+
+		for (i = 0; i <= c; i++) {
 			vcpu = kvm_get_vcpu(kvm, i);
-			vgic_unregister_redist_iodev(vcpu);
+			if (vcpu->arch.vgic_cpu.rdreg == rdreg)
+				vgic_unregister_redist_iodev(vcpu);
 		}
 	}
 
@@ -960,8 +963,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);
@@ -982,7 +987,7 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count)
 	 * Register iodevs for each existing VCPU.  Adding more VCPUs
 	 * afterwards will register the iodevs when needed.
 	 */
-	ret = vgic_register_all_redist_iodevs(kvm);
+	ret = vgic_register_all_redist_iodevs(kvm, index);
 	if (ret) {
 		struct vgic_redist_region *rdreg;
 

Thanks,
Karl