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

[Marc Zyngier <maz@kernel.org>]

On Mon, 10 Aug 2026 07:52:16 +0100,
Karl Mehltretter <kmehltretter@gmail.com> 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 <kmehltretter@gmail.com>
> ---
>  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.