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

[Marc Zyngier <maz@kernel.org>]

On Fri, 12 Dec 2025 15:22:37 +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>
> ---
>  arch/arm64/kvm/arch_timer.c           |  2 +-
>  arch/arm64/kvm/pmu-emul.c             |  6 +++---
>  arch/arm64/kvm/vgic/vgic-kvm-device.c |  2 +-
>  arch/arm64/kvm/vgic/vgic.c            | 14 +++++++-------
>  include/kvm/arm_vgic.h                | 27 +++++++++++++++++++++++----
>  5 files changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 99a07972068d1..6f033f6644219 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -1598,7 +1598,7 @@ int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	if (get_user(irq, uaddr))
>  		return -EFAULT;
>  
> -	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.

>  		return -EINVAL;
>  
>  	mutex_lock(&vcpu->kvm->arch.config_lock);
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index b03dbda7f1ab9..0baf8e0fe23bd 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -939,7 +939,7 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  		 * number against the dimensions of the vgic and make sure
>  		 * it's valid.
>  		 */
> -		if (!irq_is_ppi(irq) && !vgic_valid_spi(vcpu->kvm, irq))
> +		if (!irq_is_ppi(vcpu->kvm, irq) && !vgic_valid_spi(vcpu->kvm, irq))
>  			return -EINVAL;
>  	} else if (kvm_arm_pmu_irq_initialized(vcpu)) {
>  		   return -EINVAL;
> @@ -991,7 +991,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  		if (!kvm_arm_pmu_irq_initialized(vcpu))
>  			continue;
>  
> -		if (irq_is_ppi(irq)) {
> +		if (irq_is_ppi(kvm, irq)) {
>  			if (vcpu->arch.pmu.irq_num != irq)
>  				return false;
>  		} else {
> @@ -1142,7 +1142,7 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  			return -EFAULT;
>  
>  		/* The PMU overflow interrupt can be a PPI or a valid SPI. */
> -		if (!(irq_is_ppi(irq) || irq_is_spi(irq)))
> +		if (!(irq_is_ppi(vcpu->kvm, irq) || irq_is_spi(vcpu->kvm, irq)))
>  			return -EINVAL;
>  
>  		if (!pmu_irq_is_valid(kvm, irq))
> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> index 3d1a776b716d7..b12ba99a423e5 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -639,7 +639,7 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>  		if (vgic_initialized(dev->kvm))
>  			return -EBUSY;
>  
> -		if (!irq_is_ppi(val))
> +		if (!irq_is_ppi(dev->kvm, val))
>  			return -EINVAL;
>  
>  		dev->kvm->arch.vgic.mi_intid = val;
> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 430aa98888fda..2c0e8803342e2 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -94,7 +94,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, u32 intid)
>  	}
>  
>  	/* LPIs */
> -	if (intid >= VGIC_MIN_LPI)
> +	if (irq_is_lpi(kvm, intid))
>  		return vgic_get_lpi(kvm, intid);
>  
>  	return NULL;
> @@ -123,7 +123,7 @@ static void vgic_release_lpi_locked(struct vgic_dist *dist, struct vgic_irq *irq
>  
>  static __must_check bool __vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  {
> -	if (irq->intid < VGIC_MIN_LPI)
> +	if (!irq_is_lpi(kvm, irq->intid))
>  		return false;
>  
>  	return refcount_dec_and_test(&irq->refcount);
> @@ -148,7 +148,7 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  	 * Acquire/release it early on lockdep kernels to make locking issues
>  	 * in rare release paths a bit more obvious.
>  	 */
> -	if (IS_ENABLED(CONFIG_LOCKDEP) && irq->intid >= VGIC_MIN_LPI) {
> +	if (IS_ENABLED(CONFIG_LOCKDEP) && irq_is_lpi(kvm, irq->intid)) {
>  		guard(spinlock_irqsave)(&dist->lpi_xa.xa_lock);
>  	}
>  
> @@ -186,7 +186,7 @@ void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
>  	raw_spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
>  
>  	list_for_each_entry_safe(irq, tmp, &vgic_cpu->ap_list_head, ap_list) {
> -		if (irq->intid >= VGIC_MIN_LPI) {
> +		if (irq_is_lpi(vcpu->kvm, irq->intid)) {
>  			raw_spin_lock(&irq->irq_lock);
>  			list_del(&irq->ap_list);
>  			irq->vcpu = NULL;
> @@ -521,12 +521,12 @@ int kvm_vgic_inject_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  	if (ret)
>  		return ret;
>  
> -	if (!vcpu && intid < VGIC_NR_PRIVATE_IRQS)
> +	if (!vcpu && irq_is_private(kvm, intid))
>  		return -EINVAL;
>  
>  	trace_vgic_update_irq_pending(vcpu ? vcpu->vcpu_idx : 0, intid, level);
>  
> -	if (intid < VGIC_NR_PRIVATE_IRQS)
> +	if (irq_is_private(kvm, intid))
>  		irq = vgic_get_vcpu_irq(vcpu, intid);
>  	else
>  		irq = vgic_get_irq(kvm, intid);
> @@ -685,7 +685,7 @@ int kvm_vgic_set_owner(struct kvm_vcpu *vcpu, unsigned int intid, void *owner)
>  		return -EAGAIN;
>  
>  	/* SGIs and LPIs cannot be wired up to any device */
> -	if (!irq_is_ppi(intid) && !vgic_valid_spi(vcpu->kvm, intid))
> +	if (!irq_is_ppi(vcpu->kvm, intid) && !vgic_valid_spi(vcpu->kvm, intid))
>  		return -EINVAL;
>  
>  	irq = vgic_get_vcpu_irq(vcpu, intid);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index b261fb3968d03..be1f45a494f78 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -19,6 +19,7 @@
>  #include <linux/jump_label.h>
>  
>  #include <linux/irqchip/arm-gic-v4.h>
> +#include <linux/irqchip/arm-gic-v5.h>
>  
>  #define VGIC_V3_MAX_CPUS	512
>  #define VGIC_V2_MAX_CPUS	8
> @@ -31,9 +32,22 @@
>  #define VGIC_MIN_LPI		8192
>  #define KVM_IRQCHIP_NUM_PINS	(1020 - 32)
>  
> -#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
> -#define irq_is_spi(irq) ((irq) >= VGIC_NR_PRIVATE_IRQS && \
> -			 (irq) <= VGIC_MAX_SPI)
> +#define irq_is_ppi_legacy(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
> +#define irq_is_spi_legacy(irq) ((irq) >= VGIC_NR_PRIVATE_IRQS && \
> +					(irq) <= VGIC_MAX_SPI)
> +#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()?

> +
> +#define irq_is_ppi_v5(irq) (FIELD_GET(GICV5_HWIRQ_TYPE, irq) == GICV5_HWIRQ_TYPE_PPI)
> +#define irq_is_spi_v5(irq) (FIELD_GET(GICV5_HWIRQ_TYPE, irq) == GICV5_HWIRQ_TYPE_SPI)
> +#define irq_is_lpi_v5(irq) (FIELD_GET(GICV5_HWIRQ_TYPE, irq) == GICV5_HWIRQ_TYPE_LPI)
> +
> +#define gic_is_v5(k) ((k)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V5)
> +
> +#define irq_is_ppi(k, i) (gic_is_v5(k) ? irq_is_ppi_v5(i) : irq_is_ppi_legacy(i))
> +#define irq_is_spi(k, i) (gic_is_v5(k) ? irq_is_spi_v5(i) : irq_is_spi_legacy(i))
> +#define irq_is_lpi(k, i) (gic_is_v5(k) ? irq_is_lpi_v5(i) : irq_is_lpi_legacy(i))
> +
> +#define irq_is_private(k, i) (gic_is_v5(k) ? irq_is_ppi_v5(i) : i < VGIC_NR_PRIVATE_IRQS)
>  
>  enum vgic_type {
>  	VGIC_V2,		/* Good ol' GICv2 */
> @@ -418,8 +432,13 @@ 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_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?

> +#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.

Thanks,

	M.

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