Re: [PATCH v4 08/48] KVM: arm64: gic-v5: Add VPE doorbell domain

On Sat, 2026-07-25 at 11:56 +0100, Marc Zyngier wrote:
> On Fri, 24 Jul 2026 11:50:28 +0100,
> Sascha Bischoff <[email protected]> wrote:
> > 
> > VPE doorbells allow the GICv5 hardware to notify KVM when an SPI or
> > LPI
> > can be signalled to a non-resident VPE. This provides the mechanism
> > used
> > to wake blocked vcpus once the hardware determines that the
> > interrupt is
> > eligible to be delivered.
> > 
> > Add support for a per-VM VPE doorbell irq domain. The domain is
> > created
> > under the GICv5 LPI domain, with one doorbell allocated per VPE.
> > Store
> > the allocated doorbell base in the VM's GICv5 state so that later
> > patches can request per-vcpu doorbell IRQs and use them for IRS
> > commands and wakeups.
> > 
> > Add the per-VPE doorbell state to the GICv5 CPU interface state.
> > The
> > doorbell IRQ number is populated when the IRQs are requested, and
> > the
> > db_fired state is used by later patches once doorbell delivery is
> > wired
> > up.
> > 
> > Signed-off-by: Sascha Bischoff <[email protected]>
> > ---
> >  arch/arm64/kvm/vgic/vgic-init.c    |  18 ++--
> >  arch/arm64/kvm/vgic/vgic-v5.c      | 139
> > +++++++++++++++++++++++++++++
> >  arch/arm64/kvm/vgic/vgic.h         |   1 +
> >  include/kvm/arm_vgic.h             |   4 +
> >  include/linux/irqchip/arm-gic-v5.h |   2 +
> >  5 files changed, 158 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-init.c
> > b/arch/arm64/kvm/vgic/vgic-init.c
> > index 907057881b26a..625d352756fcf 100644
> > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > @@ -492,16 +492,22 @@ static void kvm_vgic_dist_destroy(struct kvm
> > *kvm)
> >  	dist->nr_spis = 0;
> >  	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
> >  
> > -	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> > +	switch (dist->vgic_model) {
> > +	case KVM_DEV_TYPE_ARM_VGIC_V2:
> > +		dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
> > +		break;
> > +	case KVM_DEV_TYPE_ARM_VGIC_V3:
> >  		list_for_each_entry_safe(rdreg, next, &dist-
> > >rd_regions, list)
> >  			vgic_v3_free_redist_region(kvm, rdreg);
> >  		INIT_LIST_HEAD(&dist->rd_regions);
> > -	} else {
> > -		dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
> > -	}
> >  
> > -	if (vgic_supports_direct_irqs(kvm))
> > -		vgic_v4_teardown(kvm);
> > +		if (vgic_supports_direct_irqs(kvm))
> > +			vgic_v4_teardown(kvm);
> > +		break;
> > +	case KVM_DEV_TYPE_ARM_VGIC_V5:
> > +		vgic_v5_teardown(kvm);
> > +		break;
> > +	}
> >  
> >  	xa_destroy(&dist->lpi_xa);
> >  }
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 3f7b132110114..ebd7c3ede975f 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> > @@ -7,6 +7,7 @@
> >  
> >  #include <linux/bitops.h>
> >  #include <linux/irqchip/arm-vgic-info.h>
> > +#include <linux/irqdomain.h>
> >  
> >  #include "vgic.h"
> >  
> > @@ -152,6 +153,134 @@ int vgic_v5_probe(const struct gic_kvm_info
> > *info)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * This set of irq_chip functions is specific for doorbells.
> > + */
> > +static const struct irq_chip vgic_v5_db_irq_chip = {
> > +	.name = "GICv5-DB",
> > +	.irq_mask = irq_chip_mask_parent,
> > +	.irq_unmask = irq_chip_unmask_parent,
> > +	.irq_eoi = irq_chip_eoi_parent,
> > +	.irq_set_affinity = irq_chip_set_affinity_parent,
> > +	.irq_get_irqchip_state = irq_chip_get_parent_state,
> > +	.irq_set_irqchip_state = irq_chip_set_parent_state,
> > +	.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_SKIP_SET_WAKE |
> > +		 IRQCHIP_MASK_ON_SUSPEND,
> > +};
> > +
> > +static void vgic_v5_irq_db_domain_free(struct irq_domain *domain,
> > +				       unsigned int virq, unsigned
> > int nr_irqs)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < nr_irqs; i++) {
> > +		struct irq_data *d =
> > irq_domain_get_irq_data(domain, virq + i);
> > +
> > +		irq_set_handler(virq + i, NULL);
> > +		irq_domain_reset_irq_data(d);
> > +	}
> > +
> > +	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> > +}
> > +
> > +static int vgic_v5_irq_db_domain_alloc(struct irq_domain *domain,
> > +				       unsigned int virq, unsigned
> > int nr_irqs,
> > +				       void *arg)
> > +{
> > +	const struct irq_chip *chip = &vgic_v5_db_irq_chip;
> > +	struct vgic_v5_vm *vm = arg;
> > +	struct irq_data *irqd;
> > +	int ret;
> > +
> > +	if (!vm) {
> > +		kvm_err("invalid parameter for doorbell irq
> > allocation\n");
> > +		return -EINVAL;
> > +	}
> 
> How can this happen?

I don't think it realistically can. We can't make it this far without
that so the check is utterly pointless. I've dropped it.

> 
> > +
> > +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> > NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (int i = 0; i < nr_irqs; i++) {
> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, i,
> > chip,
> > +					      domain->host_data);
> > +		irqd = irq_desc_get_irq_data(irq_to_desc(virq +
> > i));
> > +		irqd_set_single_target(irqd);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops vgic_v5_irq_db_domain_ops = {
> > +	.alloc = vgic_v5_irq_db_domain_alloc,
> > +	.free = vgic_v5_irq_db_domain_free,
> > +};
> > +
> > +static int vgic_v5_create_per_vm_domain(struct kvm *kvm)
> > +{
> > +	struct vgic_v5_vm *vm = &kvm->arch.vgic.gicv5_vm;
> > +	int nr_vcpus = atomic_read(&kvm->online_vcpus);
> > +	int id = task_pid_nr(current);
> > +	int ret, db_virq = 0;
> > +
> > +	if (!gicv5_global_data.lpi_domain) {
> > +		kvm_err("LPI domain uninitialized, can't set up
> > KVM Doorbells\n");
> > +		return -ENODEV;
> > +	}
> 
> Surely we should fail KVM's initialisation if we don't have the LPI
> domain, rather than checking this at the point of creating the
> doorbells. I can't imagine how we'd run on GICv5 without LPIs
> anyway...

I've moved this check to probe time. It might even make sense to skip
the registration with KVM in the host driver altogether, however.

> 
> > +
> > +	vm->fwnode = irq_domain_alloc_named_id_fwnode("GICv5-vpe-
> > db", id);
> > +	if (!vm->fwnode)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * KVM per-VM VPE DB domain; child of LPI domain; only
> > ever handles
> > +	 * doorbells. We know how many doorbells we have, and
> > therefore we
> > +	 * create a linear domain.
> 
> The linear domain thing is an irqdomain implementation detail. We
> don't need to capture this here.

OK, removed.

> 
> > +	 */
> > +	vm->domain =
> > irq_domain_create_hierarchy(gicv5_global_data.lpi_domain,
> > +						 0, nr_vcpus, vm-
> > >fwnode,
> > +						
> > &vgic_v5_irq_db_domain_ops, vm);
> > +	if (!vm->domain) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	db_virq = irq_domain_alloc_irqs(vm->domain, nr_vcpus,
> > NUMA_NO_NODE, vm);
> > +	if (db_virq <= 0) {
> > +		ret = db_virq;
> > +		goto err;
> > +	}
> > +
> > +	kvm->arch.vgic.gicv5_vm.vpe_db_base = db_virq;
> > +
> > +	return 0;
> > +
> > +err:
> > +	if (db_virq > 0)
> > +		irq_domain_free_irqs(db_virq, nr_vcpus);
> > +	if (vm->domain)
> > +		irq_domain_remove(vm->domain);
> > +	if (vm->fwnode)
> > +		irq_domain_free_fwnode(vm->fwnode);
> > +
> > +	kvm->arch.vgic.gicv5_vm.vpe_db_base = 0;
> > +	vm->domain = NULL;
> > +	vm->fwnode = NULL;
> > +
> > +	return ret;
> > +}
> > +
> > +static void vgic_v5_teardown_per_vm_domain(struct vgic_v5_vm *vm)
> > +{
> > +	if (!vm->domain)
> > +		return;
> > +
> > +	irq_domain_remove(vm->domain);
> > +	irq_domain_free_fwnode(vm->fwnode);
> > +	vm->domain = NULL;
> > +	vm->fwnode = NULL;
> > +}
> > +
> >  void vgic_v5_reset(struct kvm_vcpu *vcpu)
> >  {
> >  	/*
> > @@ -167,10 +296,16 @@ void vgic_v5_reset(struct kvm_vcpu *vcpu)
> >  	vcpu->arch.vgic_cpu.num_pri_bits = 5;
> >  }
> >  
> > +void vgic_v5_teardown(struct kvm *kvm)
> > +{
> > +	vgic_v5_teardown_per_vm_domain(&kvm->arch.vgic.gicv5_vm);
> > +}
> 
> Since this is a global symbol, I'd like to make sure this is never
> called concurrently with anything related. Could you please add
> 
> 	lockdep_assert_held(&kvm->arch.config_lock);
> 
> here?

Done.

> 
> > +
> >  int vgic_v5_init(struct kvm *kvm)
> >  {
> >  	struct kvm_vcpu *vcpu;
> >  	unsigned long idx;
> > +	int ret;
> 
> and here?

And done.

> 
> >  
> >  	if (vgic_initialized(kvm))
> >  		return 0;
> > @@ -182,6 +317,10 @@ int vgic_v5_init(struct kvm *kvm)
> >  		}
> >  	}
> >  
> > +	ret = vgic_v5_create_per_vm_domain(kvm);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* We only allow userspace to drive the SW_PPI, if it is
> > implemented. */
> >  	bitmap_zero(kvm->arch.vgic.gicv5_vm.userspace_ppis,
> >  		    VGIC_V5_NR_PRIVATE_IRQS);
> > diff --git a/arch/arm64/kvm/vgic/vgic.h
> > b/arch/arm64/kvm/vgic/vgic.h
> > index f45f7e3ec4d6e..f2f5fdc3211d7 100644
> > --- a/arch/arm64/kvm/vgic/vgic.h
> > +++ b/arch/arm64/kvm/vgic/vgic.h
> > @@ -366,6 +366,7 @@ void vgic_debug_destroy(struct kvm *kvm);
> >  int vgic_v5_probe(const struct gic_kvm_info *info);
> >  void vgic_v5_reset(struct kvm_vcpu *vcpu);
> >  int vgic_v5_init(struct kvm *kvm);
> > +void vgic_v5_teardown(struct kvm *kvm);
> >  int vgic_v5_map_resources(struct kvm *kvm);
> >  void vgic_v5_set_ppi_ops(struct kvm_vcpu *vcpu, u32 vintid);
> >  bool vgic_v5_has_pending_ppi(struct kvm_vcpu *vcpu);
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 8d65a18fefb80..bff2b7c896d55 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -392,6 +392,10 @@ struct vgic_v5_vm {
> >  	 * convenient way to do that).
> >  	 */
> >  	DECLARE_BITMAP(vgic_ppi_hmr, VGIC_V5_NR_PRIVATE_IRQS);
> > +
> > +	struct fwnode_handle	*fwnode;
> > +	struct irq_domain	*domain;
> > +	int			vpe_db_base;
> >  };
> >  
> >  struct vgic_dist {
> > diff --git a/include/linux/irqchip/arm-gic-v5.h
> > b/include/linux/irqchip/arm-gic-v5.h
> > index 83cfcd056b250..21c8a69f99bb6 100644
> > --- a/include/linux/irqchip/arm-gic-v5.h
> > +++ b/include/linux/irqchip/arm-gic-v5.h
> > @@ -613,6 +613,8 @@ void gicv5_irs_syncr(void);
> >  
> >  /* Embedded in kvm.arch */
> >  struct gicv5_vpe {
> > +	int			db;
> > +	bool			db_fired;
> >  	bool			resident;
> >  };
> 
> Thanks,
> 
> 	M.
>