Re: [PATCH] KVM: arm/arm64: Keep mapped, disabled interrupts quiet

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Thu, Oct 08, 2015 at 01:36:09PM +0100, Marc Zyngier wrote:
> If a mapped interrupt is disabled, we must make sure the
> corresponding physical interrupt cannot fire, as we are not
> injecting the interrupt, and not setting the active bit.
> 
> For example, a guest disabling its timer interrupt at the GIC level
> but leaving the timer firing would stop making progress as noone
> would be able to prevent this timer from firing and interrupting
> the host. Not quite what is expected. And if we're rebooting
> or turning a vcpu off while the interrupt is about to fire,
> we're exactly going to face this.
> 
> In order to cope with this, parse the list of mapped interrupts,
> and mark it as active if we're about to run the guest (or inactive
> if we've exited). Hopefully, nobody is going to run with zillions
> of disabled, mapped interrupts, right?
> 
> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> My gut feeling is that this vgic_dist_irq_set_pending should always
> be done, but I'd like some other eyes to have a look at it.
> 
> Tested on Seattle (running 4.3-rc4) with a script hammering CPU hotplug.
> 
>  virt/kvm/arm/vgic.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)

Tested on Juno so:

Tested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 6bd1c9b..0ad3f7e 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1092,6 +1092,11 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
>  
> +	if (vlr.state & LR_HW) {
> +		vgic_dist_irq_set_pending(vcpu, irq);
> +		vlr.hwirq = 0;
> +	}
> +
>  	vlr.state = 0;
>  	vgic_set_lr(vcpu, lr_nr, vlr);
>  	clear_bit(lr_nr, vgic_cpu->lr_used);
> @@ -1232,6 +1237,57 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>  }
>  
>  /*
> + * If a mapped interrupt is disabled, we must make sure the
> + * corresponding physical interrupt cannot fire, as we are not
> + * injecting the interrupt, and not setting the active bit.
> + *
> + * Parse the list of mapped interrupts, and mark it as active if we're
> + * about to run the guest (or inactive if we've exited). Hopefully,
> + * nobody is going to run with zillions of disabled, mapped
> + * interrupts...
> + */
> +static void vgic_handle_disabled_mapped_irq(struct kvm_vcpu *vcpu, bool enter)
> +{
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	struct vgic_bitmap *spi_bitmap;
> +	struct list_head *root;
> +	struct irq_phys_map_entry *entry;
> +	struct irq_phys_map *map;
> +	int ret;
> +
> +	rcu_read_lock();
> +
> +	/* Check for PPIs */
> +	root = &vgic_cpu->irq_phys_map_list;
> +	list_for_each_entry_rcu(entry, root, entry) {
> +		map = &entry->map;
> +		if (!vgic_irq_is_enabled(vcpu, map->virt_irq)) {
> +			ret = irq_set_irqchip_state(map->irq,
> +						    IRQCHIP_STATE_ACTIVE,
> +						    enter);
> +			WARN_ON(ret);
> +		}
> +	}
> +
> +	/* Check for SPIs routed to this vcpu */
> +	root = &dist->irq_phys_map_list;
> +	spi_bitmap = &dist->irq_spi_target[vcpu->vcpu_id];
> +	list_for_each_entry_rcu(entry, root, entry) {
> +		map = &entry->map;
> +		if (!vgic_irq_is_enabled(vcpu, map->virt_irq) &&
> +		    vgic_bitmap_get_irq_val(spi_bitmap, 0, map->virt_irq)) {
> +			ret = irq_set_irqchip_state(map->irq,
> +						    IRQCHIP_STATE_ACTIVE,
> +						    enter);
> +			WARN_ON(ret);
> +		}
> +	}
> +
> +	rcu_read_unlock();
> +}
> +
> +/*
>   * Fill the list registers with pending interrupts before running the
>   * guest.
>   */
> @@ -1320,6 +1376,8 @@ epilog:
>  			WARN_ON(ret);
>  		}
>  	}
> +
> +	vgic_handle_disabled_mapped_irq(vcpu, true);
>  }
>  
>  static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> @@ -1484,6 +1542,8 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>  		vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
>  	}
>  
> +	vgic_handle_disabled_mapped_irq(vcpu, false);
> +
>  	/* Check if we still have something up our sleeve... */
>  	pending = find_first_zero_bit(elrsr_ptr, vgic->nr_lr);
>  	if (level_pending || pending < vgic->nr_lr)
> -- 
> 2.1.4
>