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.
