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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 8 Jan 2026 09:33:30 +0000
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:

> On Tue, 2026-01-06 at 18:43 +0000, Jonathan Cameron wrote:
> > On Fri, 19 Dec 2025 15:52:38 +0000
> > Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> >   
> > > GICv5 has moved from using interrupt ranges for different interrupt
> > > types to using some of the upper bits of the interrupt ID to denote
> > > the interrupt type. This is not compatible with older GICs (which
> > > rely
> > > on ranges of interrupts to determine the type), and hence a set of
> > > helpers is introduced. These helpers take a struct kvm*, and use
> > > the
> > > vgic model to determine how to interpret the interrupt ID.
> > > 
> > > Helpers are introduced for PPIs, SPIs, and LPIs. Additionally, a
> > > helper is introduced to determine if an interrupt is private - SGIs
> > > and PPIs for older GICs, and PPIs only for GICv5.
> > > 
> > > The helpers are plumbed into the core vgic code, as well as the
> > > Arch
> > > Timer and PMU code.
> > > 
> > > There should be no functional changes as part of this change.
> > > 
> > > Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>  
> > Hi Sascha,
> > 
> > A bit of bikeshedding / 'valuable' naming feedback to end the day.
> > Otherwise LGTM.
> >   
> > > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > > index b261fb3968d03..6778f676eaf08 100644
> > > --- a/include/kvm/arm_vgic.h
> > > +++ b/include/kvm/arm_vgic.h  
> > ...
> >   
> > >  enum vgic_type {
> > >  	VGIC_V2,		/* Good ol' GICv2 */
> > > @@ -418,8 +488,12 @@ u64 vgic_v3_get_misr(struct kvm_vcpu *vcpu);
> > >  
> > >  #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_valid_nv5_spi(k, i)	(((i) >=
> > > VGIC_NR_PRIVATE_IRQS) && \
> > >  			((i) < (k)->arch.vgic.nr_spis +
> > > VGIC_NR_PRIVATE_IRQS))
> > > +#define vgic_valid_v5_spi(k, i)	(irq_is_spi(k, i) && \
> > > +				 (FIELD_GET(GICV5_HWIRQ_ID, i) <
> > > (k)->arch.vgic.nr_spis))
> > > +#define vgic_valid_spi(k, i) (vgic_is_v5(k)
> > > ?				\
> > > +			      vgic_valid_v5_spi(k, i) :
> > > vgic_valid_nv5_spi(k, i))  
> > 
> > nv is a little awkward as a name as immediately makes me thinking
> > nested virtualization instead of not v5 (which I guess is the
> > thinking behind that?)
> > 
> > Probably just me and naming it v23 will break if we get to GIC
> > version 23 :)
> > nv5 breaks when we get GICv6 ;)  
> 
> Absolutely agreed here. The v5 and nv5 macros were not used anywhere,
> so I've re-worked this a bit to be more in the style of those added
> earlier:
> 
> -#define vgic_valid_nv5_spi(k, i)       (((i) >= VGIC_NR_PRIVATE_IRQS) && \
> -                       ((i) < (k)->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS))
> -#define vgic_valid_v5_spi(k, i)        (irq_is_spi(k, i) && \
> -                                (FIELD_GET(GICV5_HWIRQ_ID, i) < (k)->arch.vgic.nr_spis))
> -#define vgic_valid_spi(k, i) (vgic_is_v5(k) ?                          \
> -                             vgic_valid_v5_spi(k, i) : vgic_valid_nv5_spi(k, i))
> +#define vgic_valid_spi(k, i)                                           \
> +       ({                                                              \
> +               bool __ret = irq_is_spi(k, i);                          \
> +                                                                       \
> +               switch ((k)->arch.vgic.vgic_model) {                    \
> +               case KVM_DEV_TYPE_ARM_VGIC_V5:                          \
> +                       __ret &= FIELD_GET(GICV5_HWIRQ_ID, i) < (k)->arch.vgic.nr_spis; \
> +                       break;                                          \
> +               default:                                                \
> +                       __ret &= (i) < ((k)->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS); \
> +               }                                                       \
> +                                                                       \
> +               __ret;                                                  \
> +       })
> 
> More verbose (with annoying line lengths), but certainly more scalable
> and removes the naming issue altogether. Personally, I prefer it
> because it is more aligned with the related macros above.
> 
> Is this preferable/acceptable?

Looks good to me, though it's getting complex enough that it might be better
as a static inline function. That will also reduce line lengths
a little.

Jonathan

> 
> Thanks,
> Sascha
> 
> > 
> >   
> > >  
> > >  bool kvm_vcpu_has_pending_irqs(struct kvm_vcpu *vcpu);
> > >  void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);  
> >   
>