Re: [PATCH v3 07/10] KVM: arm/arm64: vgic: Allow HW interrupts for non-shared devices

[Christoffer Dall]

On Mon, Aug 10, 2015 at 03:21:01PM +0200, Eric Auger wrote:
> From: Marc Zyngier <marc.zyngier@xxxxxxx>
> 
> So far, the only use of the HW interrupt facility was the timer,
> implying that the active state is context-switched for each vcpu,
> as the device is is shared across all vcpus.
> 
> This does not work for a device that has been assigned to a VM,
> as the guest is entierely in control of that device (the HW is
> not shared). In that case, it makes sense to bypass the whole
> active state switching.
> 
> Also the VGIC state machine is adapted to support those assigned
> (non shared) HW IRQs:
> - nly can be sampled when it is pending
> - when queueing the IRQ (programming the LR), the pending state is
>   removed as for edge sensitive IRQs
> - queued state is not modelled. Level state is not modelled
> - its injection always is valid since steming from the HW.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> 
> ---
> 
> - a mix of
>   [PATCH v4 11/11] KVM: arm/arm64: vgic: Allow HW interrupts for
>                    non-shared devices
>   [RFC v2 2/4] KVM: arm: vgic: fix state machine for forwarded IRQ
> ---
>  include/kvm/arm_vgic.h    |  6 +++--
>  virt/kvm/arm/arch_timer.c |  3 ++-
>  virt/kvm/arm/vgic.c       | 58 +++++++++++++++++++++++++++++++++++------------
>  3 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index d901f1a..7ef9ce0 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -163,7 +163,8 @@ struct irq_phys_map {
>  	u32			virt_irq;
>  	u32			phys_irq;
>  	u32			irq;
> -	bool			active;
> +	bool			shared;
> +	bool			active; /* Only valid if shared */
>  };
>  
>  struct irq_phys_map_entry {
> @@ -356,7 +357,8 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -					   int virt_irq, int irq);
> +					   int virt_irq, int irq,
> +					   bool shared);
>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
>  bool kvm_vgic_get_phys_irq_active(struct irq_phys_map *map);
>  void kvm_vgic_set_phys_irq_active(struct irq_phys_map *map, bool active);
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 76e38d2..db21d8f 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -203,7 +203,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  	 * Tell the VGIC that the virtual interrupt is tied to a
>  	 * physical interrupt. We do that once per VCPU.
>  	 */
> -	map = kvm_vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
> +	map = kvm_vgic_map_phys_irq(vcpu, irq->irq,
> +				    host_vtimer_irq, true);
>  	if (WARN_ON(IS_ERR(map)))
>  		return PTR_ERR(map);
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 9eb489a..fbd5ba5 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -400,7 +400,11 @@ void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq)
>  
>  static bool vgic_can_sample_irq(struct kvm_vcpu *vcpu, int irq)
>  {
> -	return !vgic_irq_is_queued(vcpu, irq);
> +	struct irq_phys_map *map = vgic_irq_map_search(vcpu, irq);
> +	bool shared_hw = map && !map->shared;

why is shared true when map->shared is false?

> +
> +	return !vgic_irq_is_queued(vcpu, irq) ||
> +			(shared_hw && vgic_dist_irq_is_pending(vcpu, irq));

so for forwarded, non-shared, level-triggered IRQs, we always sample the
line if it's pending?  Why?

>  }
>  
>  /**
> @@ -1150,19 +1154,26 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
>  		 * active in the physical world. Otherwise the
>  		 * physical interrupt will fire and the guest will
>  		 * exit before processing the virtual interrupt.
> +		 *
> +		 * This is of course only valid for a shared
> +		 * interrupt. A non shared interrupt should already be
> +		 * active.
>  		 */
>  		if (map) {
> -			int ret;
> -
> -			BUG_ON(!map->active);
>  			vlr.hwirq = map->phys_irq;
>  			vlr.state |= LR_HW;
>  			vlr.state &= ~LR_EOI_INT;
>  
> -			ret = irq_set_irqchip_state(map->irq,
> -						    IRQCHIP_STATE_ACTIVE,
> -						    true);
> -			WARN_ON(ret);
> +			if (map->shared) {
> +				int ret;
> +
> +				BUG_ON(!map->active);
> +				ret = irq_set_irqchip_state(
> +						map->irq,
> +						IRQCHIP_STATE_ACTIVE,
> +						true);
> +				WARN_ON(ret);
> +			}

this stuff all needs to be rebased onto my latest timer/active state
rework series.

>  
>  			/*
>  			 * Make sure we're not going to sample this
> @@ -1229,10 +1240,13 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>  
>  static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>  {
> +	struct irq_phys_map *map = vgic_irq_map_search(vcpu, irq);
> +	bool shared_hw = map && !map->shared;

same question as above?

> +
>  	if (!vgic_can_sample_irq(vcpu, irq))
>  		return true; /* level interrupt, already queued */
>  
> -	if (vgic_queue_irq(vcpu, 0, irq)) {
> +	if (vgic_queue_irq(vcpu, 0, irq) || shared_hw) {
>  		if (vgic_irq_is_edge(vcpu, irq)) {
>  			vgic_dist_irq_clear_pending(vcpu, irq);
>  			vgic_cpu_irq_clear(vcpu, irq);
> @@ -1411,7 +1425,12 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
>  		return 0;
>  
>  	map = vgic_irq_map_search(vcpu, vlr.irq);
> -	BUG_ON(!map || !map->active);
> +	BUG_ON(!map);
> +
> +	if (!map->shared)
> +		return 0;
> +
> +	BUG_ON(map->shared && !map->active);
>  
>  	ret = irq_get_irqchip_state(map->irq,
>  				    IRQCHIP_STATE_ACTIVE,
> @@ -1563,6 +1582,7 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>  	int edge_triggered, level_triggered;
>  	int enabled;
>  	bool ret = true, can_inject = true;
> +	bool shared_hw = map && !map->shared;

same

>  
>  	if (irq_num >= min(kvm->arch.vgic.nr_irqs, 1020))
>  		return -EINVAL;
> @@ -1573,7 +1593,8 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>  	edge_triggered = vgic_irq_is_edge(vcpu, irq_num);
>  	level_triggered = !edge_triggered;
>  
> -	if (!vgic_validate_injection(vcpu, irq_num, level)) {
> +	if (!vgic_validate_injection(vcpu, irq_num, level) &&
> +		!shared_hw) {
>  		ret = false;
>  		goto out;
>  	}
> @@ -1742,16 +1763,21 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
>   * @vcpu: The VCPU pointer
>   * @virt_irq: The virtual irq number
>   * @irq: The Linux IRQ number
> + * @shared: Indicates if the interrupt has to be context-switched or
> + *          if it is private to a VM
>   *
>   * Establish a mapping between a guest visible irq (@virt_irq) and a
>   * Linux irq (@irq). On injection, @virt_irq will be associated with
>   * the physical interrupt represented by @irq. This mapping can be
>   * established multiple times as long as the parameters are the same.
> + * If @shared is true, the active state of the interrupt will be
> + * context-switched.
>   *
>   * Returns a valid pointer on success, and an error pointer otherwise
>   */
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -					   int virt_irq, int irq)
> +					   int virt_irq, int irq,
> +					   bool shared)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
> @@ -1785,7 +1811,8 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  	if (map) {
>  		/* Make sure this mapping matches */
>  		if (map->phys_irq != phys_irq	||
> -		    map->irq      != irq)
> +		    map->irq      != irq	||
> +		    map->shared   != shared)
>  			map = ERR_PTR(-EINVAL);
>  
>  		/* Found an existing, valid mapping */
> @@ -1796,6 +1823,7 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  	map->virt_irq = virt_irq;
>  	map->phys_irq = phys_irq;
>  	map->irq      = irq;
> +	map->shared   = shared;
>  
>  	list_add_tail_rcu(&entry->entry, root);
>  
> @@ -1846,7 +1874,7 @@ static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
>   */
>  bool kvm_vgic_get_phys_irq_active(struct irq_phys_map *map)
>  {
> -	BUG_ON(!map);
> +	BUG_ON(!map || !map->shared);
>  	return map->active;
>  }
>  
> @@ -1858,7 +1886,7 @@ bool kvm_vgic_get_phys_irq_active(struct irq_phys_map *map)
>   */
>  void kvm_vgic_set_phys_irq_active(struct irq_phys_map *map, bool active)
>  {
> -	BUG_ON(!map);
> +	BUG_ON(!map || !map->shared);
>  	map->active = active;
>  }
>  
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/