[PATCH v3 0/5] KVM: arm64: fix VGICv3 redistributor rollback

Karl Mehltretter posted 5 patches 1 month, 1 week ago
There is a newer version of this series
arch/arm64/kvm/vgic/vgic-init.c               |  46 +++----
arch/arm64/kvm/vgic/vgic-kvm-device.c         |  20 +++
arch/arm64/kvm/vgic/vgic-mmio-v3.c            |  83 ++++++++-----
arch/arm64/kvm/vgic/vgic.h                    |   1 +
tools/testing/selftests/kvm/arm64/vgic_init.c | 116 ++++++++++++++++--
5 files changed, 200 insertions(+), 66 deletions(-)
[PATCH v3 0/5] KVM: arm64: fix VGICv3 redistributor rollback
Posted by Karl Mehltretter 1 month, 1 week ago
A failed REDIST_REGION write can remove redistributor iodevs from
KVM_MMIO_BUS while leaving their cached vCPU assignments intact. A
corrected retry then skips those redistributors.

Userspace should instead see a failed region update atomically: no prior
redistributor assignment survives the failure, and the next successful
update rebuilds all possible assignments in region-index order.

Patch 1 fixes a separate accounting bug when an individual MMIO-bus
registration fails. It reserves the selected region slot before
registration and undoes that known-latest assignment if registration fails.

Patch 2 implements the atomic failed-region behavior. It unregisters every
redistributor iodev, clears every cached assignment, resets the region
counters, and frees the newly inserted region. An in-flight vCPU can have
an RD iodev before kvm_for_each_vcpu() can see it, so REDIST and
REDIST_REGION writes are serialized with vCPU creation and return -EBUSY
while the created_vcpus/online_vcpus counts differ.

Patch 3 is independent teardown cleanup. It separates MMIO-bus teardown
from config-locked assignment cleanup, preserves the cleanup required
before a late failed vCPU creation frees the vCPU, and removes the special
conditional from the common vCPU destructor.

Patch 4 keeps the selftest helper aligned with vm_create_with_vcpus(), and
patch 5 adds regression coverage for an overlapping region, retry, and
final GICR_TYPER accesses to all four redistributors. The test exercises
patch 2's final-state behavior; patch 1's MMIO-bus allocation failure is
not fault-injected.

Testing: built the patched kernel and the arm64 vgic_init selftest with
GCC 13.3.0 in an arm64 Linux container. The selftest passed under QEMU
11.0.2 TCG with -machine virt,virtualization=on,gic-version=3 and -cpu max.

---
Changes since v2:
- Patch 1: limit free_index rollback to the immediate registration failure
  under slots_lock instead of generic unregistration. (Sashiko)
- Patch 2: reset all assignments and region counters after a failed region
  update (Marc), and serialize REDIST and REDIST_REGION writes with vCPU
  creation so rollback cannot miss an unpublished assignment.
- Patch 3: add an already-locked unassignment primitive, move failed-vCPU
  cleanup to kvm_vgic_vcpu_destroy(), and remove the redundant base_addr
  reset. (Marc)
- Patch 4: match vm_create_with_vcpus() by using void * for the guest-code
  argument. (Sashiko)
- Patch 5: document how the first three redistributors span regions 0
  and 1; no functional change.

Previous version:
v2: https://lore.kernel.org/r/20260819224229.82948-1-kmehltretter@gmail.com

Karl Mehltretter (5):
  KVM: arm64: vgic-v3: Undo assignment on iodev registration failure
  KVM: arm64: vgic-v3: Reset redistributors after failed region setup
  KVM: arm64: vgic-v3: Separate redistributor teardown from unassignment
  KVM: arm64: selftests: Pass guest code to vm_gic_create_with_vcpus()
  KVM: arm64: selftests: Test VGICv3 redistributor region retry

 arch/arm64/kvm/vgic/vgic-init.c               |  46 +++----
 arch/arm64/kvm/vgic/vgic-kvm-device.c         |  20 +++
 arch/arm64/kvm/vgic/vgic-mmio-v3.c            |  83 ++++++++-----
 arch/arm64/kvm/vgic/vgic.h                    |   1 +
 tools/testing/selftests/kvm/arm64/vgic_init.c | 116 ++++++++++++++++--
 5 files changed, 200 insertions(+), 66 deletions(-)


base-commit: 57e7cf13ac26bf1a3dba6cfa601f7b2481811575
-- 
2.39.5 (Apple Git-154)
Re: [PATCH v3 0/5] KVM: arm64: fix VGICv3 redistributor rollback
Posted by Marc Zyngier 4 weeks, 1 day ago
On Sat, 22 Aug 2026 10:53:41 +0100,
Karl Mehltretter <kmehltretter@gmail.com> wrote:
> 
> A failed REDIST_REGION write can remove redistributor iodevs from
> KVM_MMIO_BUS while leaving their cached vCPU assignments intact. A
> corrected retry then skips those redistributors.
> 
> Userspace should instead see a failed region update atomically: no prior
> redistributor assignment survives the failure, and the next successful
> update rebuilds all possible assignments in region-index order.
> 
> Patch 1 fixes a separate accounting bug when an individual MMIO-bus
> registration fails. It reserves the selected region slot before
> registration and undoes that known-latest assignment if registration fails.
> 
> Patch 2 implements the atomic failed-region behavior. It unregisters every
> redistributor iodev, clears every cached assignment, resets the region
> counters, and frees the newly inserted region. An in-flight vCPU can have
> an RD iodev before kvm_for_each_vcpu() can see it, so REDIST and
> REDIST_REGION writes are serialized with vCPU creation and return -EBUSY
> while the created_vcpus/online_vcpus counts differ.
> 
> Patch 3 is independent teardown cleanup. It separates MMIO-bus teardown
> from config-locked assignment cleanup, preserves the cleanup required
> before a late failed vCPU creation frees the vCPU, and removes the special
> conditional from the common vCPU destructor.
> 
> Patch 4 keeps the selftest helper aligned with vm_create_with_vcpus(), and
> patch 5 adds regression coverage for an overlapping region, retry, and
> final GICR_TYPER accesses to all four redistributors. The test exercises
> patch 2's final-state behavior; patch 1's MMIO-bus allocation failure is
> not fault-injected.
> 
> Testing: built the patched kernel and the arm64 vgic_init selftest with
> GCC 13.3.0 in an arm64 Linux container. The selftest passed under QEMU
> 11.0.2 TCG with -machine virt,virtualization=on,gic-version=3 and -cpu max.
> 
> ---
> Changes since v2:
> - Patch 1: limit free_index rollback to the immediate registration failure
>   under slots_lock instead of generic unregistration. (Sashiko)
> - Patch 2: reset all assignments and region counters after a failed region
>   update (Marc), and serialize REDIST and REDIST_REGION writes with vCPU
>   creation so rollback cannot miss an unpublished assignment.
> - Patch 3: add an already-locked unassignment primitive, move failed-vCPU
>   cleanup to kvm_vgic_vcpu_destroy(), and remove the redundant base_addr
>   reset. (Marc)
> - Patch 4: match vm_create_with_vcpus() by using void * for the guest-code
>   argument. (Sashiko)
> - Patch 5: document how the first three redistributors span regions 0
>   and 1; no functional change.

I really don't understand why this is such a massive departure from
v2, which was pretty close to what I wanted to see.

Honestly, you are making things harder for everyone by over-designing
(or more probably under-filtering) things that should be *fixes*, and
just that.

If you want to rework all of the vgic init/destroy, fine by me. Do
that as a separate series. But for fixes that carry a Cc stable and
require backporting to 6 year old kernels, that's not on.

The hack below is what I have against your v2 to make it acceptable.

	M.

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 84e67c23bedc0..85b00849e6154 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -539,8 +539,6 @@ static void __kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 		 */
 		if (kvm_get_vcpu_by_id(vcpu->kvm, vcpu->vcpu_id) != vcpu)
 			vgic_unregister_redist_iodev(vcpu);
-
-		vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
 	}
 }
 
@@ -563,14 +561,13 @@ void kvm_vgic_destroy(struct kvm *kvm)
 
 	vgic_debug_destroy(kvm);
 
-	kvm_for_each_vcpu(i, vcpu, kvm)
+	kvm_for_each_vcpu(i, vcpu, kvm) {
 		__kvm_vgic_vcpu_destroy(vcpu);
-
-	if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
-		mutex_unlock(&kvm->arch.config_lock);
-		kvm_for_each_vcpu(i, vcpu, kvm)
-			vgic_unregister_redist_iodev(vcpu);
-		mutex_lock(&kvm->arch.config_lock);
+		if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+			kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS,
+						  &vcpu->arch.vgic_cpu.rd_iodev.dev);
+			__vgic_unassign_redist_iodev(vcpu);
+		}
 	}
 
 	kvm_vgic_dist_destroy(kvm);

-- 
Jazz isn't dead. It just smells funny.
Re: [PATCH v3 0/5] KVM: arm64: fix VGICv3 redistributor rollback
Posted by Karl Mehltretter 4 weeks ago
On Sun, Aug 30, 2026 at 09:28:05AM +0100, Marc Zyngier wrote:
> I really don't understand why this is such a massive departure from
> v2, which was pretty close to what I wanted to see.
> 
> Honestly, you are making things harder for everyone by over-designing
> (or more probably under-filtering) things that should be *fixes*, and
> just that.
> 
> If you want to rework all of the vgic init/destroy, fine by me. Do
> that as a separate series. But for fixes that carry a Cc stable and
> require backporting to 6 year old kernels, that's not on.
> 
> The hack below is what I have against your v2 to make it acceptable.

Okay, thanks. I’ll adapt your suggestion and send a much more minimal v4.

Karl
Re: [PATCH v3 0/5] KVM: arm64: fix VGICv3 redistributor rollback
Posted by Fuad Tabba 1 month ago
Hi Karl,

On Sat, 22 Aug 2026 at 10:53, Karl Mehltretter <kmehltretter@gmail.com> wrote:
...
> Testing: built the patched kernel and the arm64 vgic_init selftest with
> GCC 13.3.0 in an arm64 Linux container. The selftest passed under QEMU
> 11.0.2 TCG with -machine virt,virtualization=on,gic-version=3 and -cpu max.

Applied this on kvmarm/next: protected and non-protected guest boots
under pKVM, plus VHE and nVHE, and the vgic_init, vgic_irq and
vgic_lpi_stress selftest. Also built the base with only patches 4-5
and confirmed the new test fails without patches 1-3.

Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba <fuad.tabba@linux.dev>

Cheers,
/fuad


> ---
> Changes since v2:
> - Patch 1: limit free_index rollback to the immediate registration failure
>   under slots_lock instead of generic unregistration. (Sashiko)
> - Patch 2: reset all assignments and region counters after a failed region
>   update (Marc), and serialize REDIST and REDIST_REGION writes with vCPU
>   creation so rollback cannot miss an unpublished assignment.
> - Patch 3: add an already-locked unassignment primitive, move failed-vCPU
>   cleanup to kvm_vgic_vcpu_destroy(), and remove the redundant base_addr
>   reset. (Marc)
> - Patch 4: match vm_create_with_vcpus() by using void * for the guest-code
>   argument. (Sashiko)
> - Patch 5: document how the first three redistributors span regions 0
>   and 1; no functional change.
>
> Previous version:
> v2: https://lore.kernel.org/r/20260819224229.82948-1-kmehltretter@gmail.com
>
> Karl Mehltretter (5):
>   KVM: arm64: vgic-v3: Undo assignment on iodev registration failure
>   KVM: arm64: vgic-v3: Reset redistributors after failed region setup
>   KVM: arm64: vgic-v3: Separate redistributor teardown from unassignment
>   KVM: arm64: selftests: Pass guest code to vm_gic_create_with_vcpus()
>   KVM: arm64: selftests: Test VGICv3 redistributor region retry
>
>  arch/arm64/kvm/vgic/vgic-init.c               |  46 +++----
>  arch/arm64/kvm/vgic/vgic-kvm-device.c         |  20 +++
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c            |  83 ++++++++-----
>  arch/arm64/kvm/vgic/vgic.h                    |   1 +
>  tools/testing/selftests/kvm/arm64/vgic_init.c | 116 ++++++++++++++++--
>  5 files changed, 200 insertions(+), 66 deletions(-)
>
>
> base-commit: 57e7cf13ac26bf1a3dba6cfa601f7b2481811575
> --
> 2.39.5 (Apple Git-154)