Re: [PATCH 07/32] KVM: arm64: gic: Introduce interrupt type helpers

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

On Mon, 2025-12-15 at 13:32 +0000, Marc Zyngier wrote:
> On Fri, 12 Dec 2025 15:22:37 +0000,
> Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> >  
> > -	if (!(irq_is_ppi(irq)))
> > +	if (!(irq_is_ppi(vcpu->kvm, irq)))
> 
> nit: From a high-level perspective, I'd find it mentally more
> satisfying to pass a vcpu to the macro rather than a vm pointer when
> dealing with PPIs. It would also keep the dereference hidden away.
> But
> maybe i just need to go with the flow here.

No, I think you're right. It better aligns with the semantics
underlying private IRQs, and does make the code a bit neater. If we're
ever generically checking (e.g., not using the _v5 variant) and don't
have a vcpu pointer in that context then something is likely awry.

It does make irq_is_private(k, i) a bit strange though as that is used
for things like

	if (!vcpu && irq_is_private(kvm, intid))

in later commits. That needs to take struct kvm if we want to do the
same sort of check. Else, we might as well not have it.

> > +#define irq_is_lpi_legacy(irq) ((irq) > VGIC_MAX_SPI)
> 
> This last line is wrong. v3 LPIs start at 8192, while VGIC_MAX_SPI is
> 1019. Also, "legacy" is remarkably ambiguous. v2 is legacy for v3, v3
> for v4...  You see where this is going.
> 
> I'd rather you have something that denotes the non-GICv5-ness of the
> implementation. irq_is_nv5_ppi()?

You are absolutely correct; that last line is wrong. Fixed, thanks!

And yeah, naming is hard. From my (unique) point of view, everything
that isn't GICv5 is legacy, but that's not exactly helpful for anyone
else.

IMO, nv5 is a bit too close to NV & NV2. I'd prefer something more like
irq_is_non_v5_ppi for the naming, but definitely don't heel strongly.
Happy for suggestions here, but for the time being I've locally changed
to using nv5 in the place of legacy.

> >  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
> >  #define vgic_initialized(k)	((k)->arch.vgic.initialized)
> > -#define vgic_valid_spi(k, i)	(((i) >= VGIC_NR_PRIVATE_IRQS) &&
> > \
> > +#define vgic_ready(k)		((k)->arch.vgic.ready)
> 
> What is this for? Nothing seem to be using it yet. How different is
> it
> from the 'initialized' field?

This is for nothing, and wasn't meant to be here at all. Seems it crept
in from the prototyping, and slipped through the cracks when preparing
this series. Have dropped it. Apologies!

> > +#define vgic_valid_spi_legacy(k, i)	(((i) >=
> > VGIC_NR_PRIVATE_IRQS) && \
> >  			((i) < (k)->arch.vgic.nr_spis +
> > VGIC_NR_PRIVATE_IRQS))
> > +#define vgic_valid_spi_v5(k, i)	(irq_is_spi(k, i) && \
> > +				 (FIELD_GET(GICV5_HWIRQ_ID, i) <
> > (k)->arch.vgic.nr_spis))
> > +#define vgic_valid_spi(k, i) (((k)->arch.vgic.vgic_model !=
> > KVM_DEV_TYPE_ARM_VGIC_V5) ? \
> > +				vgic_valid_spi_legacy(k, i) :
> > vgic_valid_spi_v5(k, i))
> > 
> 
> This macro has its v5/nv5 statements in the opposite order of all the
> others. Some consistency would be welcome.

Have re-ordered to match the order above.

> 
> Thanks,
> 
> 	M.
> 

Thanks,
Sascha