Re: [PATCH 15/32] KVM: arm64: gic-v5: Implement direct injection of PPIs

[Marc Zyngier <maz@kernel.org>]

On Fri, 12 Dec 2025 15:22:40 +0000,
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> 
> GICv5 is able to directly inject PPI pending state into a guest using
> a mechanism called DVI whereby the pending bit for a paticular PPI is
> driven directly by the physically-connected hardware. This mechanism
> itself doesn't allow for any ID translation, so the host interrupt is
> directly mapped into a guest with the same interrupt ID.
> 
> When mapping a virtual interrupt to a physical interrupt via
> kvm_vgic_map_irq for a GICv5 guest, check if the interrupt itself is a
> PPI or not. If it is, and the host's interrupt ID matches that used
> for the guest DVI is enabled, and the interrupt itself is marked as
> directly_injected.
> 
> When the interrupt is unmapped again, this process is reversed, and
> DVI is disabled for the interrupt again.
> 
> Note: the expectation is that a directly injected PPI is disabled on
> the host while the guest state is loaded. The reason is that although
> DVI is enabled to drive the guest's pending state directly, the host
> pending state also remains driven. In order to avoid the same PPI
> firing on both the host and the guest, the host's interrupt must be
> disabled (masked). This is left up to the code that owns the device
> generating the PPI as this needs to be handled on a per-VM basis. One
> VM might use DVI, while another might not, in which case the physical
> PPI should be enabled for the latter.
> 
> Co-authored-by: Timothy Hayes <timothy.hayes@arm.com>
> Signed-off-by: Timothy Hayes <timothy.hayes@arm.com>
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> ---
>  arch/arm64/kvm/vgic/vgic-v5.c | 22 ++++++++++++++++++++++
>  arch/arm64/kvm/vgic/vgic.c    | 16 ++++++++++++++++
>  arch/arm64/kvm/vgic/vgic.h    |  1 +
>  include/kvm/arm_vgic.h        |  1 +
>  4 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> index 2fb2db23ed39a..22558080711eb 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> @@ -54,6 +54,28 @@ int vgic_v5_probe(const struct gic_kvm_info *info)
>  	return 0;
>  }
>  
> +/*
> + * Sets/clears the corresponding bit in the ICH_PPI_DVIR register.
> + */
> +int vgic_v5_set_ppi_dvi(struct kvm_vcpu *vcpu, u32 irq, bool dvi)
> +{
> +	struct vgic_v5_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v5;
> +	u32 ppi = FIELD_GET(GICV5_HWIRQ_ID, irq);
> +
> +	if (ppi >= 128)
> +		return -EINVAL;

Surely this is bad. *very* bad. How can we get here the first place?

> +
> +	if (dvi) {
> +		/* Set the bit */
> +		cpu_if->vgic_ppi_dvir[ppi / 64] |= 1UL << (ppi % 64);
> +	} else {
> +		/* Clear the bit */
> +		cpu_if->vgic_ppi_dvir[ppi / 64] &= ~(1UL << (ppi % 64));
> +	}

This should be simplified:

diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
index d74cc3543b9a4..f434ee85f7e1a 100644
--- a/arch/arm64/kvm/vgic/vgic-v5.c
+++ b/arch/arm64/kvm/vgic/vgic-v5.c
@@ -191,8 +191,8 @@ bool vgic_v5_ppi_set_pending_state(struct kvm_vcpu *vcpu,
 				   struct vgic_irq *irq)
 {
 	struct vgic_v5_cpu_if *cpu_if;
-	const u32 id_bit = BIT_ULL(irq->intid % 64);
 	const u32 reg = FIELD_GET(GICV5_HWIRQ_ID, irq->intid) / 64;
+	unsigned long *p;
 
 	if (!vcpu || !irq)
 		return false;
@@ -203,10 +203,8 @@ bool vgic_v5_ppi_set_pending_state(struct kvm_vcpu *vcpu,
 
 	cpu_if = &vcpu->arch.vgic_cpu.vgic_v5;
 
-	if (irq_is_pending(irq))
-		cpu_if->vgic_ppi_pendr[reg] |= id_bit;
-	else
-		cpu_if->vgic_ppi_pendr[reg] &= ~id_bit;
+	p = (unsigned long *)&cpu_if->vgic_ppi_pendr[reg];
+	__assign_bit(irq->intid % 64, p, irq_is_pending(irq));
 
 	return true;
 }
@@ -449,17 +447,13 @@ int vgic_v5_set_ppi_dvi(struct kvm_vcpu *vcpu, u32 irq, bool dvi)
 {
 	struct vgic_v5_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v5;
 	u32 ppi = FIELD_GET(GICV5_HWIRQ_ID, irq);
+	unsigned long *p;
 
 	if (ppi >= 128)
 		return -EINVAL;
 
-	if (dvi) {
-		/* Set the bit */
-		cpu_if->vgic_ppi_dvir[ppi / 64] |= 1UL << (ppi % 64);
-	} else {
-		/* Clear the bit */
-		cpu_if->vgic_ppi_dvir[ppi / 64] &= ~(1UL << (ppi % 64));
-	}
+	p = (unsigned long *)&cpu_if->vgic_ppi_dvir[ppi / 64];
+	__assign_bit(ppi % 64, p, dvi);
 
 	return 0;
 }

(yes, unsigned long and u64 are the same thing on any sane
architecture).

> +
> +	return 0;
> +}
> +
>  void vgic_v5_load(struct kvm_vcpu *vcpu)
>  {
>  	struct vgic_v5_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v5;
> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 1005ff5f36235..1fe3dcc997860 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -577,12 +577,28 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
>  	irq->host_irq = host_irq;
>  	irq->hwintid = data->hwirq;
>  	irq->ops = ops;
> +
> +	if (vgic_is_v5(vcpu->kvm)) {
> +		/* Nothing for us to do */
> +		if (!irq_is_ppi_v5(irq->intid))
> +			return 0;
> +
> +		if (FIELD_GET(GICV5_HWIRQ_ID, irq->intid) == irq->hwintid) {
> +			if (!vgic_v5_set_ppi_dvi(vcpu, irq->hwintid, true))
> +				irq->directly_injected = true;

The error handling gives me the creeps. If we can end-up at this stage
with the wrong INTID, we're screwed.

> +		}
> +	}
> +
>  	return 0;
>  }
>  
>  /* @irq->irq_lock must be held */
>  static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
>  {
> +	if (irq->directly_injected && vgic_is_v5(irq->target_vcpu->kvm))
> +		WARN_ON(vgic_v5_set_ppi_dvi(irq->target_vcpu, irq->hwintid, false));
> +
> +	irq->directly_injected = false;
>  	irq->hw = false;
>  	irq->hwintid = 0;
>  	irq->ops = NULL;
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index 6e1f386dffade..b6e3f5e3aba18 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -363,6 +363,7 @@ void vgic_debug_init(struct kvm *kvm);
>  void vgic_debug_destroy(struct kvm *kvm);
>  
>  int vgic_v5_probe(const struct gic_kvm_info *info);
> +int vgic_v5_set_ppi_dvi(struct kvm_vcpu *vcpu, u32 irq, bool dvi);
>  void vgic_v5_load(struct kvm_vcpu *vcpu);
>  void vgic_v5_put(struct kvm_vcpu *vcpu);
>  void vgic_v5_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 45d83f45b065d..ce9e149b85a58 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -163,6 +163,7 @@ struct vgic_irq {
>  	bool enabled:1;
>  	bool active:1;
>  	bool hw:1;			/* Tied to HW IRQ */
> +	bool directly_injected:1;	/* A directly injected HW IRQ */
>  	bool on_lr:1;			/* Present in a CPU LR */
>  	refcount_t refcount;		/* Used for LPIs */
>  	u32 hwintid;			/* HW INTID number */

Thanks,

	M.

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