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

[sashiko-bot@kernel.org]

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 <Sascha.Bischoff@arm.com>

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?

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724104819.1296803-1-sascha.bischoff@arm.com?part=30