Re: [PATCH 16/32] KVM: arm64: gic: Introduce irq_queue and set_pending_state to irq_ops

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

On Wed, 2025-12-17 at 09:34 +0000, Marc Zyngier wrote:
> On Fri, 12 Dec 2025 15:22:40 +0000,
> Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> > 
> > There are times when the default behaviour of vgic_queue_irq_unlock
> > is
> > undesirable. This is because some GICs, such a GICv5 which is the
> > main
> > driver for this change, handle the majority of the interrupt
> > lifecycle
> > in hardware. In this case, there is no need for a per-VCPU AP list
> > as
> > the interrupt can be made pending directly. This is done either via
> > the ICH_PPI_x_EL2 registers for PPIs, or with the VDPEND system
> > instruction for SPIs and LPIs.
> > 
> > The queue_irq_unlock function is made overridable using a new
> > function
> > pointer in struct irq_ops. In kvm_vgic_inject_irq,
> > vgic_queue_irq_unlock is overridden if the function pointer is
> > non-null.
> > 
> > Additionally, a new function is added via a function pointer -
> > set_pending_state. The intent is for this to be used to directly
> > set
> > the pending state in hardware.
> > 
> > Both of these new irq_ops are unused in this change - it is purely
> > providing the infrastructure itself. The subsequent PPI injection
> > changes provide a demonstration of their usage.
> > 
> > Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic.c |  9 ++++++++-
> >  include/kvm/arm_vgic.h     | 15 +++++++++++++++
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic.c
> > b/arch/arm64/kvm/vgic/vgic.c
> > index 1fe3dcc997860..fc01c6d07fe62 100644
> > --- a/arch/arm64/kvm/vgic/vgic.c
> > +++ b/arch/arm64/kvm/vgic/vgic.c
> > @@ -547,7 +547,14 @@ int kvm_vgic_inject_irq(struct kvm *kvm,
> > struct kvm_vcpu *vcpu,
> >  	else
> >  		irq->pending_latch = true;
> >  
> > -	vgic_queue_irq_unlock(kvm, irq, flags);
> > +	if (irq->ops && irq->ops->set_pending_state)
> > +		WARN_ON_ONCE(!irq->ops->set_pending_state(vcpu,
> > irq));
> > +
> > +	if (irq->ops && irq->ops->queue_irq_unlock)
> > +		WARN_ON_ONCE(!irq->ops->queue_irq_unlock(kvm, irq,
> > flags));
> > +	else
> > +		vgic_queue_irq_unlock(kvm, irq, flags);
> 
> I find it slightly dubious to WARN() in one case but not the other.

Yeah, noted. That goes away as I have...

> 
> More importantly, why isn't the per-irq queue_unlock operation tucked
> into the vgic_queue_irq_unlock() primitive? We have 16 call sites for
> this function, and it is odd that only the injection primitive would
> benefit from this.

...now tucked this into vgic_queue_irq_unlock() as you suggest. You're
quite right in that it should be a more general thing, and it is broken
otherwise.

Thanks,
Sascha

> 
> Thanks,
> 
> 	M.
>