Re: [PATCH v4 30/48] KVM: arm64: gic-v5: Mask per-vcpu PPI state in vgic_v5_finalize_ppi_state()

On Fri, 2026-07-24 at 11:52 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> New issues:
> - [High] Restored active software-managed PPIs are not queued to the
> ap_list, leading to lost interrupts on VM restore.
> 
> Pre-existing issues:
> - [High] Data race on the global `vgic_ppi_mask` when `SW_PPI` is not
> implemented in hardware, leading to guest state corruption.
> --
> 
> --- Patch [30]: [PATCH v4 30/48] KVM: arm64: gic-v5: Mask per-vcpu
> PPI state in vgic_v5_finalize_ppi_state() ---
> commit 04a9407ed3605cc393d7dce45456f17c9e1fc2c3
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Mask per-vcpu PPI state in
> vgic_v5_finalize_ppi_state()
> 
> This commit reworks vgic_v5_finalize_ppi_state() to discover exposed
> PPIs and
> mask non-exposed PPI state when VCPUs are first run. It clears state
> for
> non-exposed PPIs to ensure only the intended state is exposed to the
> guest
> after userspace restores the state.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 04392b686c2e2..6af1264ed2371 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> [ ... ]
> > @@ -795,35 +796,65 @@ int vgic_v5_finalize_ppi_state(struct kvm
> > *kvm)
> >  	guard(mutex)(&kvm->arch.config_lock);
> >  
> >  	/*
> > -	 * If SW_PPI has been advertised, then we know we already
> > -	 * initialised the whole thing, and we can return early.
> > Yes,
> > -	 * this is pretty hackish as far as state tracking goes...
> > +	 * Discover the set of PPIs that are exposed to the guest
> > once per VM.
> > +	 * Once known, apply that mask to each VCPU's restored PPI
> > state as the
> > +	 * VCPUs are first run.
> >  	 */
> > -	if (test_bit(GICV5_ARCH_PPI_SW_PPI, kvm-
> > >arch.vgic.gicv5_vm.vgic_ppi_mask))
> > -		return 0;
> > -
> > -	/* The PPI state for all VCPUs should be the same. Pick
> > the first. */
> > -	vcpu0 = kvm_get_vcpu(kvm, 0);
> > +	if (!test_bit(GICV5_ARCH_PPI_SW_PPI, kvm-
> > >arch.vgic.gicv5_vm.vgic_ppi_mask)) {
> > +		bitmap_zero(kvm->arch.vgic.gicv5_vm.vgic_ppi_mask,
> > +			    VGIC_V5_NR_PRIVATE_IRQS);
> > +		bitmap_zero(kvm->arch.vgic.gicv5_vm.vgic_ppi_hmr,
> > +			    VGIC_V5_NR_PRIVATE_IRQS);
> 
> [Severity: High]
> This is a pre-existing issue, but does this conditional in
> vgic_v5_finalize_ppi_state() create a data race on the global
> vgic_ppi_mask
> if the SW_PPI feature is not implemented in hardware?

From the A.a GICv5 spec:

R_MSWFB
When FEAT_GCIE is implemented, PPI 3 (SW_PPI) is implemented.

So, if we have GICv5, we have the SW_PPI. No question.

> 
> In a multi-VCPU VM booting on GICv5 hardware without the optional
> SW_PPI
> feature, when a subsequent VCPU runs for the first time, this test
> will
> evaluate to false because the bit was never set.
> 
> This seems to cause the global mask to be cleared via bitmap_zero()
> while
> earlier VCPUs might already be running in the guest. Could this lead
> to a
> transiently zeroed vgic_ppi_mask during sysreg accesses and silently
> dropped
> interrupts for emulated devices like timers?
> 
> > +
> > +		for_each_set_bit(i, ppi_caps.impl_ppi_mask,
> > VGIC_V5_NR_PRIVATE_IRQS) {
> [ ... ]
> > +	/* Also update the vgic_irqs */
> > +	for (i = 0; i < VGIC_V5_NR_PRIVATE_IRQS; i++) {
> > +		bool visible = test_bit(i, kvm-
> > >arch.vgic.gicv5_vm.vgic_ppi_mask);
> >  		const u32 intid = vgic_v5_make_ppi(i);
> >  		struct vgic_irq *irq;
> >  
> > -		irq = vgic_get_vcpu_irq(vcpu0, intid);
> > +		irq = vgic_get_vcpu_irq(vcpu, intid);
> >  
> > -		/* Expose PPIs with an owner or the SW_PPI, only
> > */
> >  		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> > {
> > -			if (irq->owner || i ==
> > GICV5_ARCH_PPI_SW_PPI) {
> > -				__set_bit(i, kvm-
> > >arch.vgic.gicv5_vm.vgic_ppi_mask);
> > -				__assign_bit(i, kvm-
> > >arch.vgic.gicv5_vm.vgic_ppi_hmr,
> > -					     irq->config ==
> > VGIC_CONFIG_LEVEL);
> > +			if (!visible) {
> > +				irq->enabled = false;
> > +				irq->active = false;
> > +				irq->pending_latch = false;
> > +				irq->line_level = false;
> > +			} else {
> > +				irq->enabled = test_bit(i, cpu_if-
> > >vgic_ppi_enabler);
> > +				irq->active = test_bit(i, cpu_if-
> > >vgic_ppi_activer);
> >  			}
> >  		}
> 
> [Severity: High]
> Is it safe to directly set irq->active here without queueing the
> interrupt?
> 
> When restoring a VM with active software-managed PPIs (like the PMU
> or timers),
> we mutate irq->active inside the lock in
> vgic_v5_finalize_ppi_state(), but we
> don't seem to invoke the corresponding queueing machinery.
> 
> Without enqueuing the IRQ onto the VCPU's ap_list, won't this cause
> the
> interrupt to be invisible to the injection path and get lost on VM
> restore?

No, because we always consider all PPIs (that we intend to expose to
the guest). We don't have an AP list at all for PPIs on GICv5.

> 
> >  
> > -		vgic_put_irq(vcpu0->kvm, irq);
> > +		vgic_put_irq(kvm, irq);
> >  	}
> 

Thanks,
Sascha