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

[Marc Zyngier <maz@kernel.org>]

On Tue, 31 Mar 2026 16:09:10 +0100,
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> 
> 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.

Nothing to apologise about, this is me not being able to read.

>    
> >  }
> >  
> >  /*
> > @@ -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!

Cool. I'll revert the revert of the first hunk and keep the second
one.

Thanks!

	M.

-- 
Without deviation from the norm, progress is not possible.