Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650760 > unrolled thread

Re: [PATCH 08/10] KVM: arm/arm64: vgic: Handle unshared mapped interrupts

Started byMarc Zyngier <marc.zyngier@arm.com>
First post2017-05-25 21:20 +0200
Last post2017-05-30 15:00 +0200
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 08/10] KVM: arm/arm64: vgic: Handle unshared mapped interrupts Marc Zyngier <marc.zyngier@arm.com> - 2017-05-25 21:20 +0200
    Re: [PATCH 08/10] KVM: arm/arm64: vgic: Handle unshared mapped  interrupts Auger Eric <eric.auger@redhat.com> - 2017-05-30 15:00 +0200

#1650760 — Re: [PATCH 08/10] KVM: arm/arm64: vgic: Handle unshared mapped interrupts

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-05-25 21:20 +0200
SubjectRe: [PATCH 08/10] KVM: arm/arm64: vgic: Handle unshared mapped interrupts
Message-ID<tL6sF-3NA-11@gated-at.bofh.it>
On Wed, May 24 2017 at 10:13:21 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> Virtual interrupts directly mapped to physical interrupts require
> some special care. Their pending and active state must be observed
> at distributor level and not in the list register.
>
> Also a level sensitive interrupt's level is not toggled down by any
> maintenance IRQ handler as the EOI is not trapped.
>
> This patch adds an host_irq field in vgic_irq struct to easily
> get the irqchip state of the host irq. We also handle the
> physical IRQ case in vgic_validate_injection and add helpers to
> get the line level and active state.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/kvm/arm_vgic.h    |  4 +++-
>  virt/kvm/arm/arch_timer.c |  3 ++-
>  virt/kvm/arm/vgic/vgic.c  | 44 ++++++++++++++++++++++++++++++++++++++------
>  virt/kvm/arm/vgic/vgic.h  |  9 ++++++++-
>  4 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index ef71858..695ebc7 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -112,6 +112,7 @@ struct vgic_irq {
>  	bool hw;			/* Tied to HW IRQ */
>  	struct kref refcount;		/* Used for LPIs */
>  	u32 hwintid;			/* HW INTID number */
> +	unsigned int host_irq;		/* linux irq corresponding to hwintid */
>  	union {
>  		u8 targets;			/* GICv2 target VCPUs mask */
>  		u32 mpidr;			/* GICv3 target VCPU */
> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>  			bool level);
>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>  			       bool level);
> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> +			  u32 virt_irq, u32 phys_irq);
>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 5976609..45f4779 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  	 * Tell the VGIC that the virtual interrupt is tied to a
>  	 * physical interrupt. We do that once per VCPU.
>  	 */
> -	ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
> +	ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
> +				    vtimer->irq.irq, phys_irq);
>  	if (ret)
>  		return ret;
>  
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 83b24d2..aa0618c 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -137,6 +137,28 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  	kfree(irq);
>  }
>  
> +bool irq_line_level(struct vgic_irq *irq)
> +{
> +	bool line_level = irq->line_level;
> +
> +	if (unlikely(is_unshared_mapped(irq)))

The "unshared" bit doesn't mean much to me. Do you want to say "an
interrupt that belongs to a device only accessed by a single VM"?

Given that this can only be an SPI, can we use something like
"is_mapped_spi()" instead? I find it a lot more readable, but I'm open
to alternative suggestions.

> +		WARN_ON(irq_get_irqchip_state(irq->host_irq,
> +					      IRQCHIP_STATE_PENDING,
> +					      &line_level));
> +	return line_level;
> +}
> +
> +bool irq_is_active(struct vgic_irq *irq)
> +{
> +	bool is_active = irq->active;
> +
> +	if (unlikely(is_unshared_mapped(irq)))
> +		WARN_ON(irq_get_irqchip_state(irq->host_irq,
> +					      IRQCHIP_STATE_ACTIVE,
> +					      &is_active));
> +	return is_active;
> +}
> +
>  /**
>   * kvm_vgic_target_oracle - compute the target vcpu for an irq
>   *
> @@ -153,7 +175,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
>  	DEBUG_SPINLOCK_BUG_ON(!spin_is_locked(&irq->irq_lock));
>  
>  	/* If the interrupt is active, it must stay on the current vcpu */
> -	if (irq->active)
> +	if (irq_is_active(irq))
>  		return irq->vcpu ? : irq->target_vcpu;
>  
>  	/*
> @@ -195,14 +217,18 @@ static int vgic_irq_cmp(void *priv, struct list_head *a, struct list_head *b)
>  {
>  	struct vgic_irq *irqa = container_of(a, struct vgic_irq, ap_list);
>  	struct vgic_irq *irqb = container_of(b, struct vgic_irq, ap_list);
> +	bool activea, activeb;
>  	bool penda, pendb;
>  	int ret;
>  
>  	spin_lock(&irqa->irq_lock);
>  	spin_lock_nested(&irqb->irq_lock, SINGLE_DEPTH_NESTING);
>  
> -	if (irqa->active || irqb->active) {
> -		ret = (int)irqb->active - (int)irqa->active;
> +	activea = irq_is_active(irqa);
> +	activeb = irq_is_active(irqb);
> +
> +	if (activea || activeb) {
> +		ret = (int)activeb - (int)activea;
>  		goto out;
>  	}
>  
> @@ -234,13 +260,17 @@ static void vgic_sort_ap_list(struct kvm_vcpu *vcpu)
>  
>  /*
>   * Only valid injection if changing level for level-triggered IRQs or for a
> - * rising edge.
> + * rising edge. Injection of virtual interrupts associated to physical
> + * interrupts always is valid.
>   */
>  static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
>  {
>  	switch (irq->config) {
>  	case VGIC_CONFIG_LEVEL:
> -		return irq->line_level != level;
> +		if (unlikely(is_unshared_mapped(irq)))
> +			return true;
> +		else
> +			return irq->line_level != level;

This would be more readable as:

		return (irq->line_level != level ||
                	unlikely(is_unshared_mapped(irq)));

>  	case VGIC_CONFIG_EDGE:
>  		return level;
>  	}
> @@ -392,7 +422,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>  	return 0;
>  }
>  
> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> +			  u32 virt_irq, u32 phys_irq)
>  {
>  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
>  
> @@ -402,6 +433,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>  
>  	irq->hw = true;
>  	irq->hwintid = phys_irq;
> +	irq->host_irq = host_irq;

If you're now passing the Linux IRQ to the mapping function, you might
as well move the code that extracts the host hwirq here as well.

>  
>  	spin_unlock(&irq->irq_lock);
>  	vgic_put_irq(vcpu->kvm, irq);
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index da83e4c..dc4972b 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -17,6 +17,7 @@
>  #define __KVM_ARM_VGIC_NEW_H__
>  
>  #include <linux/irqchip/arm-gic-common.h>
> +#include <linux/interrupt.h>
>  
>  #define PRODUCT_ID_KVM		0x4b	/* ASCII code K */
>  #define IMPLEMENTER_ARM		0x43b
> @@ -96,14 +97,20 @@
>  /* we only support 64 kB translation table page size */
>  #define KVM_ITS_L1E_ADDR_MASK		GENMASK_ULL(51, 16)
>  
> +bool irq_line_level(struct vgic_irq *irq);
> +bool irq_is_active(struct vgic_irq *irq);
> +
>  static inline bool irq_is_pending(struct vgic_irq *irq)
>  {
>  	if (irq->config == VGIC_CONFIG_EDGE)
>  		return irq->pending_latch;
>  	else
> -		return irq->pending_latch || irq->line_level;
> +		return irq->pending_latch || irq_line_level(irq);
>  }
>  
> +#define is_unshared_mapped(i) \
> +((i)->hw && (i)->intid >= VGIC_NR_PRIVATE_IRQS && (i)->intid < 1020)
> +
>  /*
>   * This struct provides an intermediate representation of the fields contained
>   * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

[toc] | [next] | [standalone]

#1653211 — Re: [PATCH 08/10] KVM: arm/arm64: vgic: Handle unshared mapped interrupts

FromAuger Eric <eric.auger@redhat.com>
Date2017-05-30 15:00 +0200
SubjectRe: [PATCH 08/10] KVM: arm/arm64: vgic: Handle unshared mapped interrupts
Message-ID<tMOUF-7tv-3@gated-at.bofh.it>
In reply to#1650760
Hi Marc,

On 25/05/2017 21:14, Marc Zyngier wrote:
> On Wed, May 24 2017 at 10:13:21 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
>> Virtual interrupts directly mapped to physical interrupts require
>> some special care. Their pending and active state must be observed
>> at distributor level and not in the list register.
>>
>> Also a level sensitive interrupt's level is not toggled down by any
>> maintenance IRQ handler as the EOI is not trapped.
>>
>> This patch adds an host_irq field in vgic_irq struct to easily
>> get the irqchip state of the host irq. We also handle the
>> physical IRQ case in vgic_validate_injection and add helpers to
>> get the line level and active state.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  include/kvm/arm_vgic.h    |  4 +++-
>>  virt/kvm/arm/arch_timer.c |  3 ++-
>>  virt/kvm/arm/vgic/vgic.c  | 44 ++++++++++++++++++++++++++++++++++++++------
>>  virt/kvm/arm/vgic/vgic.h  |  9 ++++++++-
>>  4 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index ef71858..695ebc7 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -112,6 +112,7 @@ struct vgic_irq {
>>  	bool hw;			/* Tied to HW IRQ */
>>  	struct kref refcount;		/* Used for LPIs */
>>  	u32 hwintid;			/* HW INTID number */
>> +	unsigned int host_irq;		/* linux irq corresponding to hwintid */
>>  	union {
>>  		u8 targets;			/* GICv2 target VCPUs mask */
>>  		u32 mpidr;			/* GICv3 target VCPU */
>> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>>  			bool level);
>>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>>  			       bool level);
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> +			  u32 virt_irq, u32 phys_irq);
>>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>>  bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>>  
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 5976609..45f4779 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>  	 * Tell the VGIC that the virtual interrupt is tied to a
>>  	 * physical interrupt. We do that once per VCPU.
>>  	 */
>> -	ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
>> +	ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
>> +				    vtimer->irq.irq, phys_irq);
>>  	if (ret)
>>  		return ret;
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 83b24d2..aa0618c 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -137,6 +137,28 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>>  	kfree(irq);
>>  }
>>  
>> +bool irq_line_level(struct vgic_irq *irq)
>> +{
>> +	bool line_level = irq->line_level;
>> +
>> +	if (unlikely(is_unshared_mapped(irq)))
> 
> The "unshared" bit doesn't mean much to me. Do you want to say "an
> interrupt that belongs to a device only accessed by a single VM"?

Yes. This was the former naming. timer used shared HW irq and others
were dubberd unshared (https://lkml.org/lkml/2015/11/19/362).
> 
> Given that this can only be an SPI, can we use something like
> "is_mapped_spi()" instead? I find it a lot more readable, but I'm open
> to alternative suggestions.
Yep!

Thanks

Eric
> 
>> +		WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> +					      IRQCHIP_STATE_PENDING,
>> +					      &line_level));
>> +	return line_level;
>> +}
>> +
>> +bool irq_is_active(struct vgic_irq *irq)
>> +{
>> +	bool is_active = irq->active;
>> +
>> +	if (unlikely(is_unshared_mapped(irq)))
>> +		WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> +					      IRQCHIP_STATE_ACTIVE,
>> +					      &is_active));
>> +	return is_active;
>> +}
>> +
>>  /**
>>   * kvm_vgic_target_oracle - compute the target vcpu for an irq
>>   *
>> @@ -153,7 +175,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
>>  	DEBUG_SPINLOCK_BUG_ON(!spin_is_locked(&irq->irq_lock));
>>  
>>  	/* If the interrupt is active, it must stay on the current vcpu */
>> -	if (irq->active)
>> +	if (irq_is_active(irq))
>>  		return irq->vcpu ? : irq->target_vcpu;
>>  
>>  	/*
>> @@ -195,14 +217,18 @@ static int vgic_irq_cmp(void *priv, struct list_head *a, struct list_head *b)
>>  {
>>  	struct vgic_irq *irqa = container_of(a, struct vgic_irq, ap_list);
>>  	struct vgic_irq *irqb = container_of(b, struct vgic_irq, ap_list);
>> +	bool activea, activeb;
>>  	bool penda, pendb;
>>  	int ret;
>>  
>>  	spin_lock(&irqa->irq_lock);
>>  	spin_lock_nested(&irqb->irq_lock, SINGLE_DEPTH_NESTING);
>>  
>> -	if (irqa->active || irqb->active) {
>> -		ret = (int)irqb->active - (int)irqa->active;
>> +	activea = irq_is_active(irqa);
>> +	activeb = irq_is_active(irqb);
>> +
>> +	if (activea || activeb) {
>> +		ret = (int)activeb - (int)activea;
>>  		goto out;
>>  	}
>>  
>> @@ -234,13 +260,17 @@ static void vgic_sort_ap_list(struct kvm_vcpu *vcpu)
>>  
>>  /*
>>   * Only valid injection if changing level for level-triggered IRQs or for a
>> - * rising edge.
>> + * rising edge. Injection of virtual interrupts associated to physical
>> + * interrupts always is valid.
>>   */
>>  static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
>>  {
>>  	switch (irq->config) {
>>  	case VGIC_CONFIG_LEVEL:
>> -		return irq->line_level != level;
>> +		if (unlikely(is_unshared_mapped(irq)))
>> +			return true;
>> +		else
>> +			return irq->line_level != level;
> 
> This would be more readable as:
> 
> 		return (irq->line_level != level ||
>                 	unlikely(is_unshared_mapped(irq)));

OK

Thanks

Eric
> 
>>  	case VGIC_CONFIG_EDGE:
>>  		return level;
>>  	}
>> @@ -392,7 +422,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>>  	return 0;
>>  }
>>  
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> +			  u32 virt_irq, u32 phys_irq)
>>  {
>>  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
>>  
>> @@ -402,6 +433,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>>  
>>  	irq->hw = true;
>>  	irq->hwintid = phys_irq;
>> +	irq->host_irq = host_irq;
> 
> If you're now passing the Linux IRQ to the mapping function, you might
> as well move the code that extracts the host hwirq here as well.
> 
>>  
>>  	spin_unlock(&irq->irq_lock);
>>  	vgic_put_irq(vcpu->kvm, irq);
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index da83e4c..dc4972b 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -17,6 +17,7 @@
>>  #define __KVM_ARM_VGIC_NEW_H__
>>  
>>  #include <linux/irqchip/arm-gic-common.h>
>> +#include <linux/interrupt.h>
>>  
>>  #define PRODUCT_ID_KVM		0x4b	/* ASCII code K */
>>  #define IMPLEMENTER_ARM		0x43b
>> @@ -96,14 +97,20 @@
>>  /* we only support 64 kB translation table page size */
>>  #define KVM_ITS_L1E_ADDR_MASK		GENMASK_ULL(51, 16)
>>  
>> +bool irq_line_level(struct vgic_irq *irq);
>> +bool irq_is_active(struct vgic_irq *irq);
>> +
>>  static inline bool irq_is_pending(struct vgic_irq *irq)
>>  {
>>  	if (irq->config == VGIC_CONFIG_EDGE)
>>  		return irq->pending_latch;
>>  	else
>> -		return irq->pending_latch || irq->line_level;
>> +		return irq->pending_latch || irq_line_level(irq);
>>  }
>>  
>> +#define is_unshared_mapped(i) \
>> +((i)->hw && (i)->intid >= VGIC_NR_PRIVATE_IRQS && (i)->intid < 1020)
>> +
>>  /*
>>   * This struct provides an intermediate representation of the fields contained
>>   * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
> 
> Thanks,
> 
> 	M.
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web