Re: [PATCH 09/15] KVM: arm64: vgic-v5: align priority comparison with other GICs

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Thu, 2026-03-26 at 15:35 +0000, Marc Zyngier wrote:
> The way the effective priority mask is computed, and then compared
> to the priority of an interrupt to decide whether to wake-up or not,
> is slightly odd, and breaks at the limits.
> 
> This could result in spurious wake-ups that are undesirable.
> 
> Adopt the GICv[23] logic instead, which checks that the priority
> value
> is strictly lower than the mask.
> 
> Fixes: 933e5288fa971 ("KVM: arm64: gic-v5: Check for pending PPIs")
> Link:
> https://sashiko.dev/#/patchset/20260319154937.3619520-1-sascha.bischoff%40arm.com
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-v5.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> b/arch/arm64/kvm/vgic/vgic-v5.c
> index 0f269321ece4b..75372bbfb6a6a 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> @@ -238,7 +238,7 @@ static u32
> vgic_v5_get_effective_priority_mask(struct kvm_vcpu *vcpu)
>  	 */
>  	priority_mask = FIELD_GET(FEAT_GCIE_ICH_VMCR_EL2_VPMR,
> cpu_if->vgic_vmcr);
>  
> -	return min(highest_ap, priority_mask + 1);
> +	return min(highest_ap, priority_mask);

Hi Marc,

This part of your change (dropping the `- 1`) is not correct for GICv5.
The GICv[23] PMR works differently to the GICv5 PCR.

For GICv[23] the mask is exclusive, i.e., only higher priority (lower
numerical value) interrupts are of sufficient priority to be signalled.

For GICv5, the priority of an interrupt can be equal to or higher than
(numerically lower than) the mask. See DMSQKF in the GICv5 spec:

A physical interrupt has Sufficient priority to be signaled when all of
the following are true:
   * The priority of the interrupt is higher than the physical running
   priority for the Physical Interrupt Domain.
   * The priority of the interrupt is equal to or higher than the
   Physical Priority Mask for the Physical Interrupt Domain.
   
Therefore, we require this `+ 1` for the priority_mask in order to allow
us to combine the active priority and priority mask. Else, they operate on
different scales.

I'd tried to explain this in a comment that lies just outside the diff,
but hadn't explicitly called out that GICv5 operates differently to
GICv[23] in this regard. Apologies.
   
>  }
>  
>  /*
> @@ -367,7 +367,7 @@ bool vgic_v5_has_pending_ppi(struct kvm_vcpu
> *vcpu)
>  
>  		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
>  			has_pending = (irq->enabled &&
> irq_is_pending(irq) &&
> -				       irq->priority <=
> priority_mask);
> +				       irq->priority <
> priority_mask);

I agree that this was wrong and should never have included the
equality. This was definitely a bug!

Thanks,
Sascha

>  
>  		vgic_put_irq(vcpu->kvm, irq);
>