[PATCH v3 05/20] KVM: arm/arm64: Support calling vgic_update_irq_pending from irq context

[cdall@linaro.org (Christoffer Dall)]

On Mon, Oct 09, 2017 at 05:37:43PM +0100, Marc Zyngier wrote:
> On 23/09/17 01:41, Christoffer Dall wrote:
> > We are about to optimize our timer handling logic which involves
> > injecting irqs to the vgic directly from the irq handler.
> > 
> > Unfortunately, the injection path can take any AP list lock and irq lock
> > and we must therefore make sure to use spin_lock_irqsave where ever
> > interrupts are enabled and we are taking any of those locks, to avoid
> > deadlocking between process context and the ISR.
> > 
> > This changes a lot of the VGIC code, but The good news are that the
> > changes are mostly mechanical.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic/vgic-its.c     | 17 +++++++-----
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 +++++++++------
> >  virt/kvm/arm/vgic/vgic-mmio-v3.c | 17 +++++++-----
> >  virt/kvm/arm/vgic/vgic-mmio.c    | 44 +++++++++++++++++------------
> >  virt/kvm/arm/vgic/vgic-v2.c      |  5 ++--
> >  virt/kvm/arm/vgic/vgic-v3.c      | 12 ++++----
> >  virt/kvm/arm/vgic/vgic.c         | 60 +++++++++++++++++++++++++---------------
> >  virt/kvm/arm/vgic/vgic.h         |  3 +-
> >  8 files changed, 108 insertions(+), 72 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> > index f51c1e1..9f5e347 100644
> > --- a/virt/kvm/arm/vgic/vgic-its.c
> > +++ b/virt/kvm/arm/vgic/vgic-its.c
> > @@ -278,6 +278,7 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
> >  	u64 propbase = GICR_PROPBASER_ADDRESS(kvm->arch.vgic.propbaser);
> >  	u8 prop;
> >  	int ret;
> > +	unsigned long flags;
> >  
> >  	ret = kvm_read_guest(kvm, propbase + irq->intid - GIC_LPI_OFFSET,
> >  			     &prop, 1);
> > @@ -285,15 +286,15 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
> >  	if (ret)
> >  		return ret;
> >  
> > -	spin_lock(&irq->irq_lock);
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  	if (!filter_vcpu || filter_vcpu == irq->target_vcpu) {
> >  		irq->priority = LPI_PROP_PRIORITY(prop);
> >  		irq->enabled = LPI_PROP_ENABLE_BIT(prop);
> >  
> > -		vgic_queue_irq_unlock(kvm, irq);
> > +		vgic_queue_irq_unlock(kvm, irq, flags);
> >  	} else {
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  	}
> >  
> >  	return 0;
> > @@ -393,6 +394,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
> >  	int ret = 0;
> >  	u32 *intids;
> >  	int nr_irqs, i;
> > +	unsigned long flags;
> >  
> >  	nr_irqs = vgic_copy_lpi_list(vcpu, &intids);
> >  	if (nr_irqs < 0)
> > @@ -420,9 +422,9 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
> >  		}
> >  
> >  		irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]);
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		irq->pending_latch = pendmask & (1U << bit_nr);
> > -		vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  
> > @@ -515,6 +517,7 @@ static int vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its,
> >  {
> >  	struct kvm_vcpu *vcpu;
> >  	struct its_ite *ite;
> > +	unsigned long flags;
> >  
> >  	if (!its->enabled)
> >  		return -EBUSY;
> > @@ -530,9 +533,9 @@ static int vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its,
> >  	if (!vcpu->arch.vgic_cpu.lpis_enabled)
> >  		return -EBUSY;
> >  
> > -	spin_lock(&ite->irq->irq_lock);
> > +	spin_lock_irqsave(&ite->irq->irq_lock, flags);
> >  	ite->irq->pending_latch = true;
> > -	vgic_queue_irq_unlock(kvm, ite->irq);
> > +	vgic_queue_irq_unlock(kvm, ite->irq, flags);
> >  
> >  	return 0;
> >  }
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index b3d4a10..e21e2f4 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -74,6 +74,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
> >  	int mode = (val >> 24) & 0x03;
> >  	int c;
> >  	struct kvm_vcpu *vcpu;
> > +	unsigned long flags;
> >  
> >  	switch (mode) {
> >  	case 0x0:		/* as specified by targets */
> > @@ -97,11 +98,11 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
> >  
> >  		irq = vgic_get_irq(source_vcpu->kvm, vcpu, intid);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		irq->pending_latch = true;
> >  		irq->source |= 1U << source_vcpu->vcpu_id;
> >  
> > -		vgic_queue_irq_unlock(source_vcpu->kvm, irq);
> > +		vgic_queue_irq_unlock(source_vcpu->kvm, irq, flags);
> >  		vgic_put_irq(source_vcpu->kvm, irq);
> >  	}
> >  }
> > @@ -131,6 +132,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
> >  	u8 cpu_mask = GENMASK(atomic_read(&vcpu->kvm->online_vcpus) - 1, 0);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	/* GICD_ITARGETSR[0-7] are read-only */
> >  	if (intid < VGIC_NR_PRIVATE_IRQS)
> > @@ -140,13 +142,13 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, NULL, intid + i);
> >  		int target;
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		irq->targets = (val >> (i * 8)) & cpu_mask;
> >  		target = irq->targets ? __ffs(irq->targets) : 0;
> >  		irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
> >  
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  }
> > @@ -174,17 +176,18 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = addr & 0x0f;
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for (i = 0; i < len; i++) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		irq->source &= ~((val >> (i * 8)) & 0xff);
> >  		if (!irq->source)
> >  			irq->pending_latch = false;
> >  
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  }
> > @@ -195,19 +198,20 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = addr & 0x0f;
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for (i = 0; i < len; i++) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		irq->source |= (val >> (i * 8)) & 0xff;
> >  
> >  		if (irq->source) {
> >  			irq->pending_latch = true;
> > -			vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  		} else {
> > -			spin_unlock(&irq->irq_lock);
> > +			spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		}
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 408ef06..8378610 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -129,6 +129,7 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,
> >  {
> >  	int intid = VGIC_ADDR_TO_INTID(addr, 64);
> >  	struct vgic_irq *irq;
> > +	unsigned long flags;
> >  
> >  	/* The upper word is WI for us since we don't implement Aff3. */
> >  	if (addr & 4)
> > @@ -139,13 +140,13 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,
> >  	if (!irq)
> >  		return;
> >  
> > -	spin_lock(&irq->irq_lock);
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  	/* We only care about and preserve Aff0, Aff1 and Aff2. */
> >  	irq->mpidr = val & GENMASK(23, 0);
> >  	irq->target_vcpu = kvm_mpidr_to_vcpu(vcpu->kvm, irq->mpidr);
> >  
> > -	spin_unlock(&irq->irq_lock);
> > +	spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  	vgic_put_irq(vcpu->kvm, irq);
> >  }
> >  
> > @@ -241,11 +242,12 @@ static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for (i = 0; i < len * 8; i++) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		if (test_bit(i, &val)) {
> >  			/*
> >  			 * pending_latch is set irrespective of irq type
> > @@ -253,10 +255,10 @@ static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
> >  			 * restore irq config before pending info.
> >  			 */
> >  			irq->pending_latch = true;
> > -			vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  		} else {
> >  			irq->pending_latch = false;
> > -			spin_unlock(&irq->irq_lock);
> > +			spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		}
> >  
> >  		vgic_put_irq(vcpu->kvm, irq);
> > @@ -799,6 +801,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> >  	int sgi, c;
> >  	int vcpu_id = vcpu->vcpu_id;
> >  	bool broadcast;
> > +	unsigned long flags;
> >  
> >  	sgi = (reg & ICC_SGI1R_SGI_ID_MASK) >> ICC_SGI1R_SGI_ID_SHIFT;
> >  	broadcast = reg & BIT_ULL(ICC_SGI1R_IRQ_ROUTING_MODE_BIT);
> > @@ -837,10 +840,10 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> >  
> >  		irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		irq->pending_latch = true;
> >  
> > -		vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  }
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index c1e4bdd..deb51ee 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -69,13 +69,14 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for_each_set_bit(i, &val, len * 8) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		irq->enabled = true;
> > -		vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> > @@ -87,15 +88,16 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for_each_set_bit(i, &val, len * 8) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		irq->enabled = false;
> >  
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  }
> > @@ -126,14 +128,15 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for_each_set_bit(i, &val, len * 8) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		irq->pending_latch = true;
> >  
> > -		vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  }
> > @@ -144,15 +147,16 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for_each_set_bit(i, &val, len * 8) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		irq->pending_latch = false;
> >  
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  }
> > @@ -181,7 +185,8 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> >  				    bool new_active_state)
> >  {
> >  	struct kvm_vcpu *requester_vcpu;
> > -	spin_lock(&irq->irq_lock);
> > +	unsigned long flags;
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  	/*
> >  	 * The vcpu parameter here can mean multiple things depending on how
> > @@ -216,9 +221,9 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> >  
> >  	irq->active = new_active_state;
> >  	if (new_active_state)
> > -		vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  	else
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  }
> >  
> >  /*
> > @@ -352,14 +357,15 @@ void vgic_mmio_write_priority(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for (i = 0; i < len; i++) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		/* Narrow the priority range to what we actually support */
> >  		irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - VGIC_PRI_BITS);
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> > @@ -390,6 +396,7 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> >  {
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 2);
> >  	int i;
> > +	unsigned long flags;
> >  
> >  	for (i = 0; i < len * 4; i++) {
> >  		struct vgic_irq *irq;
> > @@ -404,14 +411,14 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> >  			continue;
> >  
> >  		irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		if (test_bit(i * 2 + 1, &val))
> >  			irq->config = VGIC_CONFIG_EDGE;
> >  		else
> >  			irq->config = VGIC_CONFIG_LEVEL;
> >  
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  }
> > @@ -443,6 +450,7 @@ void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> >  {
> >  	int i;
> >  	int nr_irqs = vcpu->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> > +	unsigned long flags;
> >  
> >  	for (i = 0; i < 32; i++) {
> >  		struct vgic_irq *irq;
> > @@ -459,12 +467,12 @@ void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> >  		 * restore irq config before line level.
> >  		 */
> >  		new_level = !!(val & (1U << i));
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		irq->line_level = new_level;
> >  		if (new_level)
> > -			vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  		else
> > -			spin_unlock(&irq->irq_lock);
> > +			spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> > index e4187e5..8089710 100644
> > --- a/virt/kvm/arm/vgic/vgic-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-v2.c
> > @@ -62,6 +62,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
> >  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> >  	struct vgic_v2_cpu_if *cpuif = &vgic_cpu->vgic_v2;
> >  	int lr;
> > +	unsigned long flags;
> >  
> >  	cpuif->vgic_hcr &= ~GICH_HCR_UIE;
> >  
> > @@ -77,7 +78,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
> >  
> >  		irq = vgic_get_irq(vcpu->kvm, vcpu, intid);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		/* Always preserve the active bit */
> >  		irq->active = !!(val & GICH_LR_ACTIVE_BIT);
> > @@ -104,7 +105,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
> >  				irq->pending_latch = false;
> >  		}
> >  
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  
> > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> > index 96ea597..863351c 100644
> > --- a/virt/kvm/arm/vgic/vgic-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-v3.c
> > @@ -44,6 +44,7 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
> >  	struct vgic_v3_cpu_if *cpuif = &vgic_cpu->vgic_v3;
> >  	u32 model = vcpu->kvm->arch.vgic.vgic_model;
> >  	int lr;
> > +	unsigned long flags;
> >  
> >  	cpuif->vgic_hcr &= ~ICH_HCR_UIE;
> >  
> > @@ -66,7 +67,7 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
> >  		if (!irq)	/* An LPI could have been unmapped. */
> >  			continue;
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  		/* Always preserve the active bit */
> >  		irq->active = !!(val & ICH_LR_ACTIVE_BIT);
> > @@ -94,7 +95,7 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
> >  				irq->pending_latch = false;
> >  		}
> >  
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(vcpu->kvm, irq);
> >  	}
> >  
> > @@ -278,6 +279,7 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
> >  	bool status;
> >  	u8 val;
> >  	int ret;
> > +	unsigned long flags;
> >  
> >  retry:
> >  	vcpu = irq->target_vcpu;
> > @@ -296,13 +298,13 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
> >  
> >  	status = val & (1 << bit_nr);
> >  
> > -	spin_lock(&irq->irq_lock);
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> >  	if (irq->target_vcpu != vcpu) {
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		goto retry;
> >  	}
> >  	irq->pending_latch = status;
> > -	vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +	vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> >  
> >  	if (status) {
> >  		/* clear consumed data */
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index e1f7dbc..b1bd238 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -53,6 +53,10 @@ struct vgic_global kvm_vgic_global_state __ro_after_init = {
> >   *   vcpuX->vcpu_id < vcpuY->vcpu_id:
> >   *     spin_lock(vcpuX->arch.vgic_cpu.ap_list_lock);
> >   *     spin_lock(vcpuY->arch.vgic_cpu.ap_list_lock);
> > + *
> > + * Since the VGIC must support injecting virtual interrupts from ISRs, we have
> > + * to use the spin_lock_irqsave/spin_unlock_irqrestore versions of outer
> > + * spinlocks for any lock that may be taken while injecting an interrupt.
> >   */
> >  
> >  /*
> > @@ -261,7 +265,8 @@ static bool vgic_validate_injection(struct vgic_irq *irq, bool level, void *owne
> >   * Needs to be entered with the IRQ lock already held, but will return
> >   * with all locks dropped.
> >   */
> > -bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
> > +bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> > +			   unsigned long flags)
> >  {
> >  	struct kvm_vcpu *vcpu;
> >  
> > @@ -279,7 +284,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
> >  		 * not need to be inserted into an ap_list and there is also
> >  		 * no more work for us to do.
> >  		 */
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  
> >  		/*
> >  		 * We have to kick the VCPU here, because we could be
> > @@ -301,11 +306,11 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
> >  	 * We must unlock the irq lock to take the ap_list_lock where
> >  	 * we are going to insert this new pending interrupt.
> >  	 */
> > -	spin_unlock(&irq->irq_lock);
> > +	spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  
> >  	/* someone can do stuff here, which we re-check below */
> >  
> > -	spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
> > +	spin_lock_irqsave(&vcpu->arch.vgic_cpu.ap_list_lock, flags);
> >  	spin_lock(&irq->irq_lock);
> >  
> >  	/*
> > @@ -322,9 +327,9 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
> >  
> >  	if (unlikely(irq->vcpu || vcpu != vgic_target_oracle(irq))) {
> >  		spin_unlock(&irq->irq_lock);
> > -		spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> > +		spin_unlock_irqrestore(&vcpu->arch.vgic_cpu.ap_list_lock, flags);
> >  
> > -		spin_lock(&irq->irq_lock);
> > +		spin_lock_irqsave(&irq->irq_lock, flags);
> >  		goto retry;
> >  	}
> >  
> > @@ -337,7 +342,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
> >  	irq->vcpu = vcpu;
> >  
> >  	spin_unlock(&irq->irq_lock);
> > -	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> > +	spin_unlock_irqrestore(&vcpu->arch.vgic_cpu.ap_list_lock, flags);
> >  
> >  	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> >  	kvm_vcpu_kick(vcpu);
> > @@ -367,6 +372,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >  {
> >  	struct kvm_vcpu *vcpu;
> >  	struct vgic_irq *irq;
> > +	unsigned long flags;
> >  	int ret;
> >  
> >  	trace_vgic_update_irq_pending(cpuid, intid, level);
> > @@ -383,11 +389,11 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >  	if (!irq)
> >  		return -EINVAL;
> >  
> > -	spin_lock(&irq->irq_lock);
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  	if (!vgic_validate_injection(irq, level, owner)) {
> >  		/* Nothing to see here, move along... */
> > -		spin_unlock(&irq->irq_lock);
> > +		spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  		vgic_put_irq(kvm, irq);
> >  		return 0;
> >  	}
> > @@ -397,7 +403,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >  	else
> >  		irq->pending_latch = true;
> >  
> > -	vgic_queue_irq_unlock(kvm, irq);
> > +	vgic_queue_irq_unlock(kvm, irq, flags);
> >  	vgic_put_irq(kvm, irq);
> >  
> >  	return 0;
> > @@ -406,15 +412,16 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >  int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
> >  {
> >  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
> > +	unsigned long flags;
> >  
> >  	BUG_ON(!irq);
> >  
> > -	spin_lock(&irq->irq_lock);
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  	irq->hw = true;
> >  	irq->hwintid = phys_irq;
> >  
> > -	spin_unlock(&irq->irq_lock);
> > +	spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  	vgic_put_irq(vcpu->kvm, irq);
> >  
> >  	return 0;
> > @@ -423,6 +430,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
> >  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
> >  {
> >  	struct vgic_irq *irq;
> > +	unsigned long flags;
> >  
> >  	if (!vgic_initialized(vcpu->kvm))
> >  		return -EAGAIN;
> > @@ -430,12 +438,12 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
> >  	irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
> >  	BUG_ON(!irq);
> >  
> > -	spin_lock(&irq->irq_lock);
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> >  
> >  	irq->hw = false;
> >  	irq->hwintid = 0;
> >  
> > -	spin_unlock(&irq->irq_lock);
> > +	spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  	vgic_put_irq(vcpu->kvm, irq);
> >  
> >  	return 0;
> > @@ -486,9 +494,10 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
> >  {
> >  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> >  	struct vgic_irq *irq, *tmp;
> > +	unsigned long flags;
> >  
> >  retry:
> > -	spin_lock(&vgic_cpu->ap_list_lock);
> > +	spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
> >  
> >  	list_for_each_entry_safe(irq, tmp, &vgic_cpu->ap_list_head, ap_list) {
> >  		struct kvm_vcpu *target_vcpu, *vcpuA, *vcpuB;
> > @@ -528,7 +537,7 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
> >  		/* This interrupt looks like it has to be migrated. */
> >  
> >  		spin_unlock(&irq->irq_lock);
> > -		spin_unlock(&vgic_cpu->ap_list_lock);
> > +		spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
> >  
> >  		/*
> >  		 * Ensure locking order by always locking the smallest
> > @@ -542,7 +551,7 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
> >  			vcpuB = vcpu;
> >  		}
> >  
> > -		spin_lock(&vcpuA->arch.vgic_cpu.ap_list_lock);
> > +		spin_lock_irqsave(&vcpuA->arch.vgic_cpu.ap_list_lock, flags);
> >  		spin_lock_nested(&vcpuB->arch.vgic_cpu.ap_list_lock,
> >  				 SINGLE_DEPTH_NESTING);
> >  		spin_lock(&irq->irq_lock);
> > @@ -566,11 +575,11 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
> >  
> >  		spin_unlock(&irq->irq_lock);
> >  		spin_unlock(&vcpuB->arch.vgic_cpu.ap_list_lock);
> > -		spin_unlock(&vcpuA->arch.vgic_cpu.ap_list_lock);
> > +		spin_unlock_irqrestore(&vcpuA->arch.vgic_cpu.ap_list_lock, flags);
> >  		goto retry;
> >  	}
> >  
> > -	spin_unlock(&vgic_cpu->ap_list_lock);
> > +	spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
> >  }
> >  
> >  static inline void vgic_fold_lr_state(struct kvm_vcpu *vcpu)
> > @@ -703,6 +712,8 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
> >  	if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
> >  		return;
> >  
> > +	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
> > +
> >  	spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
> >  	vgic_flush_lr_state(vcpu);
> >  	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> > @@ -735,11 +746,12 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
> >  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> >  	struct vgic_irq *irq;
> >  	bool pending = false;
> > +	unsigned long flags;
> >  
> >  	if (!vcpu->kvm->arch.vgic.enabled)
> >  		return false;
> >  
> > -	spin_lock(&vgic_cpu->ap_list_lock);
> > +	spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
> >  
> >  	list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
> >  		spin_lock(&irq->irq_lock);
> > @@ -750,7 +762,7 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
> >  			break;
> >  	}
> >  
> > -	spin_unlock(&vgic_cpu->ap_list_lock);
> > +	spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
> >  
> >  	return pending;
> >  }
> > @@ -776,13 +788,15 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq)
> >  {
> >  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
> >  	bool map_is_active;
> > +	unsigned long flags;
> >  
> >  	if (!vgic_initialized(vcpu->kvm))
> >  		return false;
> > +	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
> >  
> > -	spin_lock(&irq->irq_lock);
> > +	spin_lock_irqsave(&irq->irq_lock, flags);
> 
> I'm a bit puzzled by this sequence: Either interrupts are disabled and
> we don't need the irqsave version, or they aren't and the BUG_ON will
> fire. kvm_vgic_map_is_active is called (indirectly) from
> kvm_timer_flush_hwstate. And at this stage of the patches, we definitely
> call this function with interrupts enabled.
> 
> Is it just a patch splitting snafu? Or something more serious? Same goes
> for the DEBUG_SPINLOCK_BUG_ON in kvm_vgic_flush_hwstate.

It's a leftover thing from before I realized that this also needs to be
called from kvm_timer_vcpu_load_vgic, which has interrupts enabled, and
so I changed the simple spin_lock/spin_unlock to the irqsave/irqrestore
versions, but apparently forgot to take out the assert.  (And apparently
didn't run this with spinlock debugging enabled).

Thanks for spotting it.
-Christoffer

> >  	map_is_active = irq->hw && irq->active;
> > -	spin_unlock(&irq->irq_lock);
> > +	spin_unlock_irqrestore(&irq->irq_lock, flags);
> >  	vgic_put_irq(vcpu->kvm, irq);
> >  
> >  	return map_is_active;
> > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> > index bf9ceab..4f8aecb 100644
> > --- a/virt/kvm/arm/vgic/vgic.h
> > +++ b/virt/kvm/arm/vgic/vgic.h
> > @@ -140,7 +140,8 @@ vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev,
> >  struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
> >  			      u32 intid);
> >  void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
> > -bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
> > +bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> > +			   unsigned long flags);
> >  void vgic_kick_vcpus(struct kvm *kvm);
> >  
> >  int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> > 
> 
> Otherwise looks good to me.
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...