[PATCH v4 07/11] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest

[christoffer.dall@linaro.org (Christoffer Dall)]

On Wed, Sep 23, 2015 at 06:55:04PM +0100, Andre Przywara wrote:
> Salut Marc,
> 
> I know that this patch is already merged, but ....
> 
> On 07/08/15 16:45, Marc Zyngier wrote:
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 51c9900..9d009d2 100644
> ...
> > @@ -1364,6 +1397,39 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> >  	return level_pending;
> >  }
> >  
> > +/*
> > + * Save the physical active state, and reset it to inactive.
> > + *
> > + * Return 1 if HW interrupt went from active to inactive, and 0 otherwise.
> > + */
> > +static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
> > +{
> > +	struct irq_phys_map *map;
> > +	int ret;
> > +
> > +	if (!(vlr.state & LR_HW))
> > +		return 0;
> > +
> > +	map = vgic_irq_map_search(vcpu, vlr.irq);
> > +	BUG_ON(!map || !map->active);
> > +
> > +	ret = irq_get_irqchip_state(map->irq,
> > +				    IRQCHIP_STATE_ACTIVE,
> > +				    &map->active);
> > +
> > +	WARN_ON(ret);
> > +
> > +	if (map->active) {
> > +		ret = irq_set_irqchip_state(map->irq,
> > +					    IRQCHIP_STATE_ACTIVE,
> > +					    false);
> > +		WARN_ON(ret);
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> > +}
> > +
> >  /* Sync back the VGIC state after a guest run */
> >  static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> >  {
> > @@ -1378,14 +1444,31 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> >  	elrsr = vgic_get_elrsr(vcpu);
> >  	elrsr_ptr = u64_to_bitmask(&elrsr);
> >  
> > -	/* Clear mappings for empty LRs */
> > -	for_each_set_bit(lr, elrsr_ptr, vgic->nr_lr) {
> > +	/* Deal with HW interrupts, and clear mappings for empty LRs */
> > +	for (lr = 0; lr < vgic->nr_lr; lr++) {
> >  		struct vgic_lr vlr;
> >  
> > -		if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
> > +		if (!test_bit(lr, vgic_cpu->lr_used))
> >  			continue;
> >  
> >  		vlr = vgic_get_lr(vcpu, lr);
> > +		if (vgic_sync_hwirq(vcpu, vlr)) {
> > +			/*
> > +			 * So this is a HW interrupt that the guest
> > +			 * EOI-ed. Clean the LR state and allow the
> > +			 * interrupt to be sampled again.
> > +			 */
> > +			vlr.state = 0;
> > +			vlr.hwirq = 0;
> > +			vgic_set_lr(vcpu, lr, vlr);
> > +			vgic_irq_clear_queued(vcpu, vlr.irq);
> 
> Isn't this line altering common VGIC state without holding the lock?
> Eric removed the coarse dist->lock around the whole
> __kvm_vgic_sync_hwstate() function, we take it now in
> vgic_process_maintenance(), but don't hold it here AFAICT.
> As long as we are only dealing with private timer IRQs this is probably
> not a problem, but the IRQ number could be a SPI as well, right?
> 
I don't see a problematic race with this though, as all we're doing is
to clear a bit in a bitmap, which is always checked atomically, so
adding a lock around this really doesn't change anything as far as I can
tell.

Nevertheless, my rework series also addresses this.

-Christoffer