[PATCH 0/7] KVM: arm/arm64: Rework arch timer IRQ interface

[PATCH 0/7] KVM: arm/arm64: Rework arch timer IRQ interface

[Andre Przywara]

Hi,

currently the VGIC uses a linked list of irq_phys_map entries to store
mappings between virtual interrupts and mapped physical interrupts.
The virtual arch timer (as so far the only user of this feature) uses
a pointer to an allocated list entry to talk to the VGIC on several
occassions when handling mapped IRQs.
This is not only unneeded, but also exposes the (private) implementation
details of storing this mapping to other parts of the code, which will
cause issues with the new VGIC implementation.
So this series makes the irq_phys_map private to the VGIC
implementation, which improves abstraction and allows the new VGIC to
come without nasty hacks.

Based on 4.6-rc3.

Cheers,
Andre.

Andre Przywara (6):
  KVM: arm/arm64: remove unneeded map parameter for vgic_update_irq_pending()
  KVM: arm/arm64: directly pass virtual IRQ number on injecting mapped IRQ
  KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_map_is_active()
  KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_unmap_phys_irq()
  KVM: arm/arm64: remove irq_phys_map from the arch timer
  KVM: arm/arm64: remove irq_phys_map pointer from kvm_vgic_map_phys_irq()

Christoffer Dall (1):
  KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

 include/kvm/arm_arch_timer.h |  4 +--
 include/kvm/arm_vgic.h       | 10 +++----
 virt/kvm/arm/arch_timer.c    | 40 ++++++++++++++++++--------
 virt/kvm/arm/vgic.c          | 67 +++++++++++++++-----------------------------
 4 files changed, 58 insertions(+), 63 deletions(-)

-- 
2.7.3

[PATCH 1/7] KVM: arm/arm64: remove unneeded map parameter for vgic_update_irq_pending()

[Andre Przywara]

We actually don't use the irq_phys_map parameter in
vgic_update_irq_pending(), so let's just remove it.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 00429b3..7282881 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1522,7 +1522,6 @@ static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
 }
 
 static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
-				   struct irq_phys_map *map,
 				   unsigned int irq_num, bool level)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
@@ -1661,7 +1660,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 	if (map)
 		return -EINVAL;
 
-	return vgic_update_irq_pending(kvm, cpuid, NULL, irq_num, level);
+	return vgic_update_irq_pending(kvm, cpuid, irq_num, level);
 }
 
 /**
@@ -1687,7 +1686,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
 	if (ret)
 		return ret;
 
-	return vgic_update_irq_pending(kvm, cpuid, map, map->virt_irq, level);
+	return vgic_update_irq_pending(kvm, cpuid, map->virt_irq, level);
 }
 
 static irqreturn_t vgic_maintenance_handler(int irq, void *data)
-- 
2.7.3

[PATCH 1/7] KVM: arm/arm64: remove unneeded map parameter for vgic_update_irq_pending()

[Eric Auger]

Reviewed-by: Eric Auger <eric.auger@linaro.org>

Eric
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> We actually don't use the irq_phys_map parameter in
> vgic_update_irq_pending(), so let's just remove it.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  virt/kvm/arm/vgic.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 00429b3..7282881 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1522,7 +1522,6 @@ static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>  }
>  
>  static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> -				   struct irq_phys_map *map,
>  				   unsigned int irq_num, bool level)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
> @@ -1661,7 +1660,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  	if (map)
>  		return -EINVAL;
>  
> -	return vgic_update_irq_pending(kvm, cpuid, NULL, irq_num, level);
> +	return vgic_update_irq_pending(kvm, cpuid, irq_num, level);
>  }
>  
>  /**
> @@ -1687,7 +1686,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>  	if (ret)
>  		return ret;
>  
> -	return vgic_update_irq_pending(kvm, cpuid, map, map->virt_irq, level);
> +	return vgic_update_irq_pending(kvm, cpuid, map->virt_irq, level);
>  }
>  
>  static irqreturn_t vgic_maintenance_handler(int irq, void *data)
> 

[PATCH 2/7] KVM: arm/arm64: directly pass virtual IRQ number on injecting mapped IRQ

[Andre Przywara]

When we want to inject a hardware mapped IRQ into a guest, we actually
only need the virtual IRQ number from the irq_phys_map.
So let's pass this number directly from the arch timer to the VGIC
to avoid using the map as a parameter.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/arm_vgic.h    | 2 +-
 virt/kvm/arm/arch_timer.c | 2 +-
 virt/kvm/arm/vgic.c       | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 281caf8..c4574da 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -341,7 +341,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level);
 int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
-			       struct irq_phys_map *map, bool level);
+			       unsigned int virt_irq, bool level);
 void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index a9ad4fe..eb56f1e 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -140,7 +140,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
 				   timer->irq.level);
 	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
-					 timer->map,
+					 timer->map->virt_irq,
 					 timer->irq.level);
 	WARN_ON(ret);
 }
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 7282881..9937d41 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1667,7 +1667,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
  * kvm_vgic_inject_mapped_irq - Inject a physically mapped IRQ to the vgic
  * @kvm:     The VM structure pointer
  * @cpuid:   The CPU for PPIs
- * @map:     Pointer to a irq_phys_map structure describing the mapping
+ * @virt_irq: The virtual IRQ to be injected
  * @level:   Edge-triggered:  true:  to trigger the interrupt
  *			      false: to ignore the call
  *	     Level-sensitive  true:  raise the input signal
@@ -1678,7 +1678,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
  * being HIGH and 0 being LOW and all devices being active-HIGH.
  */
 int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
-			       struct irq_phys_map *map, bool level)
+			       unsigned int virt_irq, bool level)
 {
 	int ret;
 
@@ -1686,7 +1686,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
 	if (ret)
 		return ret;
 
-	return vgic_update_irq_pending(kvm, cpuid, map->virt_irq, level);
+	return vgic_update_irq_pending(kvm, cpuid, virt_irq, level);
 }
 
 static irqreturn_t vgic_maintenance_handler(int irq, void *data)
-- 
2.7.3

[PATCH 2/7] KVM: arm/arm64: directly pass virtual IRQ number on injecting mapped IRQ

[Eric Auger]

Hi Andre,
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> When we want to inject a hardware mapped IRQ into a guest, we actually
> only need the virtual IRQ number from the irq_phys_map.
> So let's pass this number directly from the arch timer to the VGIC
> to avoid using the map as a parameter.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/arm_vgic.h    | 2 +-
>  virt/kvm/arm/arch_timer.c | 2 +-
>  virt/kvm/arm/vgic.c       | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 281caf8..c4574da 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -341,7 +341,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level);
>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
> -			       struct irq_phys_map *map, bool level);
> +			       unsigned int virt_irq, bool level);
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index a9ad4fe..eb56f1e 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -140,7 +140,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
>  	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
>  				   timer->irq.level);
>  	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
> -					 timer->map,
> +					 timer->map->virt_irq,
>  					 timer->irq.level);
>  	WARN_ON(ret);
>  }
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 7282881..9937d41 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1667,7 +1667,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>   * kvm_vgic_inject_mapped_irq - Inject a physically mapped IRQ to the vgic
>   * @kvm:     The VM structure pointer
>   * @cpuid:   The CPU for PPIs
> - * @map:     Pointer to a irq_phys_map structure describing the mapping
> + * @virt_irq: The virtual IRQ to be injected
>   * @level:   Edge-triggered:  true:  to trigger the interrupt
>   *			      false: to ignore the call
>   *	     Level-sensitive  true:  raise the input signal
> @@ -1678,7 +1678,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>   * being HIGH and 0 being LOW and all devices being active-HIGH.
>   */
>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
> -			       struct irq_phys_map *map, bool level)
> +			       unsigned int virt_irq, bool level)
>  {
Could make sense to merge kvm_vgic_inject_mapped_irq and
kvm_vgic_inject_irq and just add a bool argument telling whether the
request comes from the userspace (if I remember well we wanted to
prevent the userspace from injecting a mapping irq). This would avoid
duplication. We can make it later though.

Reviewed-by: Eric Auger <eric.auger@linaro.org>

Cheers

Eric


>  	int ret;
>  
> @@ -1686,7 +1686,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>  	if (ret)
>  		return ret;
>  
> -	return vgic_update_irq_pending(kvm, cpuid, map->virt_irq, level);
> +	return vgic_update_irq_pending(kvm, cpuid, virt_irq, level);
>  }
>  
>  static irqreturn_t vgic_maintenance_handler(int irq, void *data)
> 

[PATCH 2/7] KVM: arm/arm64: directly pass virtual IRQ number on injecting mapped IRQ

[Andre Przywara]

Hi Eric,

thanks for looking at the patches!

On 21/04/16 18:09, Eric Auger wrote:
> Hi Andre,
> On 04/15/2016 04:04 PM, Andre Przywara wrote:
>> When we want to inject a hardware mapped IRQ into a guest, we actually
>> only need the virtual IRQ number from the irq_phys_map.
>> So let's pass this number directly from the arch timer to the VGIC
>> to avoid using the map as a parameter.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/arm_vgic.h    | 2 +-
>>  virt/kvm/arm/arch_timer.c | 2 +-
>>  virt/kvm/arm/vgic.c       | 6 +++---
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 281caf8..c4574da 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -341,7 +341,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>  			bool level);
>>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>> -			       struct irq_phys_map *map, bool level);
>> +			       unsigned int virt_irq, bool level);
>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index a9ad4fe..eb56f1e 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -140,7 +140,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
>>  	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
>>  				   timer->irq.level);
>>  	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
>> -					 timer->map,
>> +					 timer->map->virt_irq,
>>  					 timer->irq.level);
>>  	WARN_ON(ret);
>>  }
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 7282881..9937d41 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1667,7 +1667,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>   * kvm_vgic_inject_mapped_irq - Inject a physically mapped IRQ to the vgic
>>   * @kvm:     The VM structure pointer
>>   * @cpuid:   The CPU for PPIs
>> - * @map:     Pointer to a irq_phys_map structure describing the mapping
>> + * @virt_irq: The virtual IRQ to be injected
>>   * @level:   Edge-triggered:  true:  to trigger the interrupt
>>   *			      false: to ignore the call
>>   *	     Level-sensitive  true:  raise the input signal
>> @@ -1678,7 +1678,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>   * being HIGH and 0 being LOW and all devices being active-HIGH.
>>   */
>>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>> -			       struct irq_phys_map *map, bool level)
>> +			       unsigned int virt_irq, bool level)
>>  {
> Could make sense to merge kvm_vgic_inject_mapped_irq and
> kvm_vgic_inject_irq and just add a bool argument telling whether the
> request comes from the userspace (if I remember well we wanted to
> prevent the userspace from injecting a mapping irq). This would avoid
> duplication. We can make it later though.

Yes, actually I merge both of them in the new VGIC series (44/45: [1]).
But this mini-series aims to stay as close as possible to the original
code, so I think we will properly unify this once the old VGIC gets removed.

But thanks for the heads up with the userspace injection and mapped
IRQs, I was wondering the other day what the differences between the two
actually were. This may mean I have to rework the 44/45 patch.

> Reviewed-by: Eric Auger <eric.auger@linaro.org>

Merci!

Cheers,
Andre.

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-April/019671.html

>>  	int ret;
>>  
>> @@ -1686,7 +1686,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>>  	if (ret)
>>  		return ret;
>>  
>> -	return vgic_update_irq_pending(kvm, cpuid, map->virt_irq, level);
>> +	return vgic_update_irq_pending(kvm, cpuid, virt_irq, level);
>>  }
>>  
>>  static irqreturn_t vgic_maintenance_handler(int irq, void *data)
>>
> 

[PATCH 3/7] KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_map_is_active()

[Andre Przywara]

For getting the active state of a mapped IRQ, we actually only need
the virtual IRQ number, not the pointer to the mapping entry.
Pass the virtual IRQ number from the arch timer to the VGIC directly.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/arm_vgic.h    | 2 +-
 virt/kvm/arm/arch_timer.c | 2 +-
 virt/kvm/arm/vgic.c       | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index c4574da..5a34adc 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -347,7 +347,7 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
 					   int virt_irq, int irq);
 int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
-bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
+bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
 
 #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
 #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index eb56f1e..793465b 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -246,7 +246,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	* to ensure that hardware interrupts from the timer triggers a guest
 	* exit.
 	*/
-	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map))
+	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map->virt_irq))
 		phys_active = true;
 	else
 		phys_active = false;
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 9937d41..6911327 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1103,18 +1103,18 @@ static bool dist_active_irq(struct kvm_vcpu *vcpu)
 	return test_bit(vcpu->vcpu_id, dist->irq_active_on_cpu);
 }
 
-bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
+bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq)
 {
 	int i;
 
 	for (i = 0; i < vcpu->arch.vgic_cpu.nr_lr; i++) {
 		struct vgic_lr vlr = vgic_get_lr(vcpu, i);
 
-		if (vlr.irq == map->virt_irq && vlr.state & LR_STATE_ACTIVE)
+		if (vlr.irq == virt_irq && vlr.state & LR_STATE_ACTIVE)
 			return true;
 	}
 
-	return vgic_irq_is_active(vcpu, map->virt_irq);
+	return vgic_irq_is_active(vcpu, virt_irq);
 }
 
 /*
-- 
2.7.3

[PATCH 3/7] KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_map_is_active()

[Eric Auger]

Hi Andre,
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> For getting the active state of a mapped IRQ, we actually only need
> the virtual IRQ number, not the pointer to the mapping entry.
> Pass the virtual IRQ number from the arch timer to the VGIC directly.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/arm_vgic.h    | 2 +-
>  virt/kvm/arm/arch_timer.c | 2 +-
>  virt/kvm/arm/vgic.c       | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index c4574da..5a34adc 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -347,7 +347,7 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  					   int virt_irq, int irq);
>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> -bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> +bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
>  #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index eb56f1e..793465b 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -246,7 +246,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>  	* to ensure that hardware interrupts from the timer triggers a guest
>  	* exit.
>  	*/
> -	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map))
> +	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map->virt_irq))
>  		phys_active = true;
>  	else
>  		phys_active = false;
nit:
phys_active =
	(timer->irq.level ||
		kvm_vgic_map_is_active(vcpu, timer->map->virt_irq)); ?

checkpatch warning:

#40: FILE: virt/kvm/arm/arch_timer.c:278:
+	if (timer->irq.level || kvm_vgic_map_is_active(vcpu,
timer->map->virt_irq))

Besides
Reviewed-by: Eric Auger <eric.auger@linaro.org>

Cheers

Eric
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 9937d41..6911327 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1103,18 +1103,18 @@ static bool dist_active_irq(struct kvm_vcpu *vcpu)
>  	return test_bit(vcpu->vcpu_id, dist->irq_active_on_cpu);
>  }
>  
> -bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
> +bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq)
>  {
>  	int i;
>  
>  	for (i = 0; i < vcpu->arch.vgic_cpu.nr_lr; i++) {
>  		struct vgic_lr vlr = vgic_get_lr(vcpu, i);
>  
> -		if (vlr.irq == map->virt_irq && vlr.state & LR_STATE_ACTIVE)
> +		if (vlr.irq == virt_irq && vlr.state & LR_STATE_ACTIVE)
>  			return true;
>  	}
>  
> -	return vgic_irq_is_active(vcpu, map->virt_irq);
> +	return vgic_irq_is_active(vcpu, virt_irq);
>  }
>  
>  /*
> 

[PATCH 4/7] KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_unmap_phys_irq()

[Andre Przywara]

kvm_vgic_unmap_phys_irq() only needs the virtual IRQ number, so let's
just pass that between the arch timer and the VGIC to get rid of
the irq_phys_map pointer.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/arm_vgic.h    |  2 +-
 virt/kvm/arm/arch_timer.c |  2 +-
 virt/kvm/arm/vgic.c       | 11 ++++-------
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 5a34adc..43eeb18 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -346,7 +346,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
 					   int virt_irq, int irq);
-int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
+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);
 
 #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 793465b..b4d96b1 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -477,7 +477,7 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
 
 	timer_disarm(timer);
 	if (timer->map)
-		kvm_vgic_unmap_phys_irq(vcpu, timer->map);
+		kvm_vgic_unmap_phys_irq(vcpu, timer->map->virt_irq);
 }
 
 void kvm_timer_enable(struct kvm *kvm)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 6911327..2d7ae35 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1813,25 +1813,22 @@ static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
 /**
  * kvm_vgic_unmap_phys_irq - Remove a virtual to physical IRQ mapping
  * @vcpu: The VCPU pointer
- * @map: The pointer to a mapping obtained through kvm_vgic_map_phys_irq
+ * @virt_irq: The virtual IRQ number to be unmapped
  *
  * Remove an existing mapping between virtual and physical interrupts.
  */
-int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
+int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct irq_phys_map_entry *entry;
 	struct list_head *root;
 
-	if (!map)
-		return -EINVAL;
-
-	root = vgic_get_irq_phys_map_list(vcpu, map->virt_irq);
+	root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
 
 	spin_lock(&dist->irq_phys_map_lock);
 
 	list_for_each_entry(entry, root, entry) {
-		if (&entry->map == map) {
+		if (entry->map.virt_irq == virt_irq) {
 			list_del_rcu(&entry->entry);
 			call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
 			break;
-- 
2.7.3

[PATCH 4/7] KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_unmap_phys_irq()

[Eric Auger]

Reviewed-by: Eric Auger <eric.auger@linaro.org>

Eric

On 04/15/2016 04:04 PM, Andre Przywara wrote:
> kvm_vgic_unmap_phys_irq() only needs the virtual IRQ number, so let's
> just pass that between the arch timer and the VGIC to get rid of
> the irq_phys_map pointer.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/arm_vgic.h    |  2 +-
>  virt/kvm/arm/arch_timer.c |  2 +-
>  virt/kvm/arm/vgic.c       | 11 ++++-------
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 5a34adc..43eeb18 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -346,7 +346,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  					   int virt_irq, int irq);
> -int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> +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);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 793465b..b4d96b1 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -477,7 +477,7 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
>  
>  	timer_disarm(timer);
>  	if (timer->map)
> -		kvm_vgic_unmap_phys_irq(vcpu, timer->map);
> +		kvm_vgic_unmap_phys_irq(vcpu, timer->map->virt_irq);
>  }
>  
>  void kvm_timer_enable(struct kvm *kvm)
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 6911327..2d7ae35 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1813,25 +1813,22 @@ static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
>  /**
>   * kvm_vgic_unmap_phys_irq - Remove a virtual to physical IRQ mapping
>   * @vcpu: The VCPU pointer
> - * @map: The pointer to a mapping obtained through kvm_vgic_map_phys_irq
> + * @virt_irq: The virtual IRQ number to be unmapped
>   *
>   * Remove an existing mapping between virtual and physical interrupts.
>   */
> -int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
> +int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct irq_phys_map_entry *entry;
>  	struct list_head *root;
>  
> -	if (!map)
> -		return -EINVAL;
> -
> -	root = vgic_get_irq_phys_map_list(vcpu, map->virt_irq);
> +	root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
>  
>  	spin_lock(&dist->irq_phys_map_lock);
>  
>  	list_for_each_entry(entry, root, entry) {
> -		if (&entry->map == map) {
> +		if (entry->map.virt_irq == virt_irq) {
>  			list_del_rcu(&entry->entry);
>  			call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
>  			break;
> 

[PATCH 5/7] KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

[Andre Przywara]

From: Christoffer Dall <christoffer.dall@linaro.org>

The communication of a Linux IRQ number from outside the VGIC to the
vgic was a leftover from the day when the vgic code cared about how a
particular device injects virtual interrupts mapped to a physical
interrupt.

We can safely remove this notion, leaving all physical IRQ handling to
be done in the device driver (the arch timer in this case), which makes
room for a saner API for the new VGIC.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/arm_vgic.h    |  3 +--
 virt/kvm/arm/arch_timer.c | 22 ++++++++++++++++++++--
 virt/kvm/arm/vgic.c       | 20 ++------------------
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 43eeb18..49c559e 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -157,7 +157,6 @@ struct vgic_io_device {
 struct irq_phys_map {
 	u32			virt_irq;
 	u32			phys_irq;
-	u32			irq;
 };
 
 struct irq_phys_map_entry {
@@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
 void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
 int kvm_vgic_vcpu_pending_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 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 b4d96b1..cfdf88f 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	if (timer->active_cleared_last && !phys_active)
 		return;
 
-	ret = irq_set_irqchip_state(timer->map->irq,
+	ret = irq_set_irqchip_state(host_vtimer_irq,
 				    IRQCHIP_STATE_ACTIVE,
 				    phys_active);
 	WARN_ON(ret);
@@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct irq_phys_map *map;
+	struct irq_desc *desc;
+	struct irq_data *data;
+	int phys_irq;
 
 	/*
 	 * The vcpu timer irq number cannot be determined in
@@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 	kvm_timer_update_state(vcpu);
 
 	/*
+	 * Find the physical IRQ number corresponding to the host_vtimer_irq
+	 */
+	desc = irq_to_desc(host_vtimer_irq);
+	if (!desc) {
+		kvm_err("%s: no interrupt descriptor\n", __func__);
+		return -EINVAL;
+	}
+
+	data = irq_desc_get_irq_data(desc);
+	while (data->parent_data)
+		data = data->parent_data;
+
+	phys_irq = data->hwirq;
+
+	/*
 	 * 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, phys_irq);
 	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 2d7ae35..ac5838b 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
  * 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 phys_irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
 	struct irq_phys_map *map;
 	struct irq_phys_map_entry *entry;
-	struct irq_desc *desc;
-	struct irq_data *data;
-	int phys_irq;
 
-	desc = irq_to_desc(irq);
-	if (!desc) {
-		kvm_err("%s: no interrupt descriptor\n", __func__);
-		return ERR_PTR(-EINVAL);
-	}
-
-	data = irq_desc_get_irq_data(desc);
-	while (data->parent_data)
-		data = data->parent_data;
-
-	phys_irq = data->hwirq;
 
 	/* Create a new mapping */
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -1756,8 +1742,7 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
 	map = vgic_irq_map_search(vcpu, virt_irq);
 	if (map) {
 		/* Make sure this mapping matches */
-		if (map->phys_irq != phys_irq	||
-		    map->irq      != irq)
+		if (map->phys_irq != phys_irq)
 			map = ERR_PTR(-EINVAL);
 
 		/* Found an existing, valid mapping */
@@ -1767,7 +1752,6 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
 	map           = &entry->map;
 	map->virt_irq = virt_irq;
 	map->phys_irq = phys_irq;
-	map->irq      = irq;
 
 	list_add_tail_rcu(&entry->entry, root);
 
-- 
2.7.3

[PATCH 5/7] KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

[Eric Auger]

Hi Andre,
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
> 
> The communication of a Linux IRQ number from outside the VGIC to the
> vgic was a leftover from the day when the vgic code cared about how a
> particular device injects virtual interrupts mapped to a physical
> interrupt.
> 
> We can safely remove this notion, leaving all physical IRQ handling to
> be done in the device driver (the arch timer in this case), which makes
> room for a saner API for the new VGIC.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/arm_vgic.h    |  3 +--
>  virt/kvm/arm/arch_timer.c | 22 ++++++++++++++++++++--
>  virt/kvm/arm/vgic.c       | 20 ++------------------
>  3 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 43eeb18..49c559e 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -157,7 +157,6 @@ struct vgic_io_device {
>  struct irq_phys_map {
>  	u32			virt_irq;
>  	u32			phys_irq;
> -	u32			irq;
>  };
>  
>  struct irq_phys_map_entry {
> @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_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 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 b4d96b1..cfdf88f 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>  	if (timer->active_cleared_last && !phys_active)
>  		return;
>  
> -	ret = irq_set_irqchip_state(timer->map->irq,
> +	ret = irq_set_irqchip_state(host_vtimer_irq,
>  				    IRQCHIP_STATE_ACTIVE,
>  				    phys_active);
>  	WARN_ON(ret);
> @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  {
>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>  	struct irq_phys_map *map;
> +	struct irq_desc *desc;
> +	struct irq_data *data;
> +	int phys_irq;
>  
>  	/*
>  	 * The vcpu timer irq number cannot be determined in
> @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  	kvm_timer_update_state(vcpu);
>  
>  	/*
> +	 * Find the physical IRQ number corresponding to the host_vtimer_irq
> +	 */
> +	desc = irq_to_desc(host_vtimer_irq);
> +	if (!desc) {
can this really happen?
> +		kvm_err("%s: no interrupt descriptor\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	data = irq_desc_get_irq_data(desc);
> +	while (data->parent_data)
> +		data = data->parent_data;
> +
> +	phys_irq = data->hwirq;
> +
> +	/*
>  	 * 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, phys_irq);
>  	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 2d7ae35..ac5838b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
>   * Returns a valid pointer on success, and an error pointer otherwise
>   */
the doc comment must be updated
 * @irq: The Linux IRQ number

Besides

Reviewed-by: Eric Auger <eric.auger@linaro.org>

Cheers

Eric

>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -					   int virt_irq, int irq)
> +					   int virt_irq, int phys_irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
>  	struct irq_phys_map *map;
>  	struct irq_phys_map_entry *entry;
> -	struct irq_desc *desc;
> -	struct irq_data *data;
> -	int phys_irq;
>  
> -	desc = irq_to_desc(irq);
> -	if (!desc) {
> -		kvm_err("%s: no interrupt descriptor\n", __func__);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	data = irq_desc_get_irq_data(desc);
> -	while (data->parent_data)
> -		data = data->parent_data;
> -
> -	phys_irq = data->hwirq;
>  
>  	/* Create a new mapping */
>  	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> @@ -1756,8 +1742,7 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  	map = vgic_irq_map_search(vcpu, virt_irq);
>  	if (map) {
>  		/* Make sure this mapping matches */
> -		if (map->phys_irq != phys_irq	||
> -		    map->irq      != irq)
> +		if (map->phys_irq != phys_irq)
>  			map = ERR_PTR(-EINVAL);
>  
>  		/* Found an existing, valid mapping */
> @@ -1767,7 +1752,6 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  	map           = &entry->map;
>  	map->virt_irq = virt_irq;
>  	map->phys_irq = phys_irq;
> -	map->irq      = irq;
>  
>  	list_add_tail_rcu(&entry->entry, root);
>  
> 

[PATCH 5/7] KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

[Christoffer Dall]

On Thu, Apr 21, 2016 at 07:41:01PM +0200, Eric Auger wrote:
> Hi Andre,
> On 04/15/2016 04:04 PM, Andre Przywara wrote:
> > From: Christoffer Dall <christoffer.dall@linaro.org>
> > 
> > The communication of a Linux IRQ number from outside the VGIC to the
> > vgic was a leftover from the day when the vgic code cared about how a
> > particular device injects virtual interrupts mapped to a physical
> > interrupt.
> > 
> > We can safely remove this notion, leaving all physical IRQ handling to
> > be done in the device driver (the arch timer in this case), which makes
> > room for a saner API for the new VGIC.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  include/kvm/arm_vgic.h    |  3 +--
> >  virt/kvm/arm/arch_timer.c | 22 ++++++++++++++++++++--
> >  virt/kvm/arm/vgic.c       | 20 ++------------------
> >  3 files changed, 23 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 43eeb18..49c559e 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -157,7 +157,6 @@ struct vgic_io_device {
> >  struct irq_phys_map {
> >  	u32			virt_irq;
> >  	u32			phys_irq;
> > -	u32			irq;
> >  };
> >  
> >  struct irq_phys_map_entry {
> > @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
> >  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> >  int kvm_vgic_vcpu_pending_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 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 b4d96b1..cfdf88f 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >  	if (timer->active_cleared_last && !phys_active)
> >  		return;
> >  
> > -	ret = irq_set_irqchip_state(timer->map->irq,
> > +	ret = irq_set_irqchip_state(host_vtimer_irq,
> >  				    IRQCHIP_STATE_ACTIVE,
> >  				    phys_active);
> >  	WARN_ON(ret);
> > @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >  {
> >  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >  	struct irq_phys_map *map;
> > +	struct irq_desc *desc;
> > +	struct irq_data *data;
> > +	int phys_irq;
> >  
> >  	/*
> >  	 * The vcpu timer irq number cannot be determined in
> > @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >  	kvm_timer_update_state(vcpu);
> >  
> >  	/*
> > +	 * Find the physical IRQ number corresponding to the host_vtimer_irq
> > +	 */
> > +	desc = irq_to_desc(host_vtimer_irq);
> > +	if (!desc) {
> can this really happen?

this is just moving the logic.  We had this check before, so I assume
so...

> > +		kvm_err("%s: no interrupt descriptor\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	data = irq_desc_get_irq_data(desc);
> > +	while (data->parent_data)
> > +		data = data->parent_data;
> > +
> > +	phys_irq = data->hwirq;
> > +
> > +	/*
> >  	 * 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, phys_irq);
> >  	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 2d7ae35..ac5838b 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
> >   * Returns a valid pointer on success, and an error pointer otherwise
> >   */
> the doc comment must be updated
>  * @irq: The Linux IRQ number
> 
> Besides
> 
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> 
Thanks!

Andre, let me know if you need me to provide an updated patch or if you
can just tweak that comment.

-Christoffer

[PATCH 5/7] KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

[Andre Przywara]

Hej Christoffer,

On 21/04/16 19:32, Christoffer Dall wrote:
> On Thu, Apr 21, 2016 at 07:41:01PM +0200, Eric Auger wrote:
>> Hi Andre,
>> On 04/15/2016 04:04 PM, Andre Przywara wrote:
>>> From: Christoffer Dall <christoffer.dall@linaro.org>
>>>
>>> The communication of a Linux IRQ number from outside the VGIC to the
>>> vgic was a leftover from the day when the vgic code cared about how a
>>> particular device injects virtual interrupts mapped to a physical
>>> interrupt.
>>>
>>> We can safely remove this notion, leaving all physical IRQ handling to
>>> be done in the device driver (the arch timer in this case), which makes
>>> room for a saner API for the new VGIC.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  include/kvm/arm_vgic.h    |  3 +--
>>>  virt/kvm/arm/arch_timer.c | 22 ++++++++++++++++++++--
>>>  virt/kvm/arm/vgic.c       | 20 ++------------------
>>>  3 files changed, 23 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index 43eeb18..49c559e 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -157,7 +157,6 @@ struct vgic_io_device {
>>>  struct irq_phys_map {
>>>  	u32			virt_irq;
>>>  	u32			phys_irq;
>>> -	u32			irq;
>>>  };
>>>  
>>>  struct irq_phys_map_entry {
>>> @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>  int kvm_vgic_vcpu_pending_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 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 b4d96b1..cfdf88f 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>>>  	if (timer->active_cleared_last && !phys_active)
>>>  		return;
>>>  
>>> -	ret = irq_set_irqchip_state(timer->map->irq,
>>> +	ret = irq_set_irqchip_state(host_vtimer_irq,
>>>  				    IRQCHIP_STATE_ACTIVE,
>>>  				    phys_active);
>>>  	WARN_ON(ret);
>>> @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>>  {
>>>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>>  	struct irq_phys_map *map;
>>> +	struct irq_desc *desc;
>>> +	struct irq_data *data;
>>> +	int phys_irq;
>>>  
>>>  	/*
>>>  	 * The vcpu timer irq number cannot be determined in
>>> @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>>  	kvm_timer_update_state(vcpu);
>>>  
>>>  	/*
>>> +	 * Find the physical IRQ number corresponding to the host_vtimer_irq
>>> +	 */
>>> +	desc = irq_to_desc(host_vtimer_irq);
>>> +	if (!desc) {
>> can this really happen?
> 
> this is just moving the logic.  We had this check before, so I assume
> so...
> 
>>> +		kvm_err("%s: no interrupt descriptor\n", __func__);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	data = irq_desc_get_irq_data(desc);
>>> +	while (data->parent_data)
>>> +		data = data->parent_data;
>>> +
>>> +	phys_irq = data->hwirq;
>>> +
>>> +	/*
>>>  	 * 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, phys_irq);
>>>  	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 2d7ae35..ac5838b 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
>>>   * Returns a valid pointer on success, and an error pointer otherwise
>>>   */
>> the doc comment must be updated
>>  * @irq: The Linux IRQ number
>>
>> Besides
>>
>> Reviewed-by: Eric Auger <eric.auger@linaro.org>
>>
> Thanks!
> 
> Andre, let me know if you need me to provide an updated patch or if you
> can just tweak that comment.

I am fine with fixing this up there, I need to rebase and repost it
anyway as part of the integration into the new VGIC series.

Cheers,
Andre.

[PATCH 5/7] KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

[Andre Przywara]

Hi,

On 21/04/16 18:41, Eric Auger wrote:
> Hi Andre,
> On 04/15/2016 04:04 PM, Andre Przywara wrote:
>> From: Christoffer Dall <christoffer.dall@linaro.org>
>>
>> The communication of a Linux IRQ number from outside the VGIC to the
>> vgic was a leftover from the day when the vgic code cared about how a
>> particular device injects virtual interrupts mapped to a physical
>> interrupt.
>>
>> We can safely remove this notion, leaving all physical IRQ handling to
>> be done in the device driver (the arch timer in this case), which makes
>> room for a saner API for the new VGIC.
>>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/arm_vgic.h    |  3 +--
>>  virt/kvm/arm/arch_timer.c | 22 ++++++++++++++++++++--
>>  virt/kvm/arm/vgic.c       | 20 ++------------------
>>  3 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 43eeb18..49c559e 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -157,7 +157,6 @@ struct vgic_io_device {
>>  struct irq_phys_map {
>>  	u32			virt_irq;
>>  	u32			phys_irq;
>> -	u32			irq;
>>  };
>>  
>>  struct irq_phys_map_entry {
>> @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>  int kvm_vgic_vcpu_pending_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 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 b4d96b1..cfdf88f 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>>  	if (timer->active_cleared_last && !phys_active)
>>  		return;
>>  
>> -	ret = irq_set_irqchip_state(timer->map->irq,
>> +	ret = irq_set_irqchip_state(host_vtimer_irq,
>>  				    IRQCHIP_STATE_ACTIVE,
>>  				    phys_active);
>>  	WARN_ON(ret);
>> @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>  {
>>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>  	struct irq_phys_map *map;
>> +	struct irq_desc *desc;
>> +	struct irq_data *data;
>> +	int phys_irq;
>>  
>>  	/*
>>  	 * The vcpu timer irq number cannot be determined in
>> @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>  	kvm_timer_update_state(vcpu);
>>  
>>  	/*
>> +	 * Find the physical IRQ number corresponding to the host_vtimer_irq
>> +	 */
>> +	desc = irq_to_desc(host_vtimer_irq);
>> +	if (!desc) {
> can this really happen?

I guess not, as vhost_timer_irq is set by request_percpu_irq() and we
check that return value.
But frankly I'd like to go with the check here, as having a kernel NULL
pointer dereference is a really bad alternative.
And if I am not mistaken, this is only called on the reset path, so
quite rarely.

>> +		kvm_err("%s: no interrupt descriptor\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	data = irq_desc_get_irq_data(desc);
>> +	while (data->parent_data)
>> +		data = data->parent_data;
>> +
>> +	phys_irq = data->hwirq;
>> +
>> +	/*
>>  	 * 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, phys_irq);
>>  	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 2d7ae35..ac5838b 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
>>   * Returns a valid pointer on success, and an error pointer otherwise
>>   */
> the doc comment must be updated
>  * @irq: The Linux IRQ number

Indeed, I think I managed to mess this part up during some rebase.

Cheers,
Andre.

> Besides
> 
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> 
> Cheers
> 
> Eric
> 
>>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> -					   int virt_irq, int irq)
>> +					   int virt_irq, int phys_irq)
>>  {
>>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>  	struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
>>  	struct irq_phys_map *map;
>>  	struct irq_phys_map_entry *entry;
>> -	struct irq_desc *desc;
>> -	struct irq_data *data;
>> -	int phys_irq;
>>  
>> -	desc = irq_to_desc(irq);
>> -	if (!desc) {
>> -		kvm_err("%s: no interrupt descriptor\n", __func__);
>> -		return ERR_PTR(-EINVAL);
>> -	}
>> -
>> -	data = irq_desc_get_irq_data(desc);
>> -	while (data->parent_data)
>> -		data = data->parent_data;
>> -
>> -	phys_irq = data->hwirq;
>>  
>>  	/* Create a new mapping */
>>  	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>> @@ -1756,8 +1742,7 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>>  	map = vgic_irq_map_search(vcpu, virt_irq);
>>  	if (map) {
>>  		/* Make sure this mapping matches */
>> -		if (map->phys_irq != phys_irq	||
>> -		    map->irq      != irq)
>> +		if (map->phys_irq != phys_irq)
>>  			map = ERR_PTR(-EINVAL);
>>  
>>  		/* Found an existing, valid mapping */
>> @@ -1767,7 +1752,6 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>>  	map           = &entry->map;
>>  	map->virt_irq = virt_irq;
>>  	map->phys_irq = phys_irq;
>> -	map->irq      = irq;
>>  
>>  	list_add_tail_rcu(&entry->entry, root);
>>  
>>
> 

[PATCH 6/7] KVM: arm/arm64: remove irq_phys_map from the arch timer

[Andre Przywara]

Now that the interface between the arch timer and the VGIC does not
require passing the irq_phys_map entry pointer anymore, let's remove
it from the virtual arch timer and use the virtual IRQ number instead
directly.
The remaining pointer returned by kvm_vgic_map_phys_irq() will be
removed in the following patch.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/arm_arch_timer.h |  4 ++--
 virt/kvm/arm/arch_timer.c    | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index b651aed..1ed70f6 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -53,8 +53,8 @@ struct arch_timer_cpu {
 	/* Timer IRQ */
 	struct kvm_irq_level		irq;
 
-	/* VGIC mapping */
-	struct irq_phys_map		*map;
+	/* The virtual IRQ number */
+	unsigned int			virt_irq;
 
 	/* Active IRQ state caching */
 	bool				active_cleared_last;
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index cfdf88f..1598e23 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -137,10 +137,10 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 
 	timer->active_cleared_last = false;
 	timer->irq.level = new_level;
-	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
+	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->virt_irq,
 				   timer->irq.level);
 	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
-					 timer->map->virt_irq,
+					 timer->virt_irq,
 					 timer->irq.level);
 	WARN_ON(ret);
 }
@@ -246,7 +246,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	* to ensure that hardware interrupts from the timer triggers a guest
 	* exit.
 	*/
-	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map->virt_irq))
+	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->virt_irq))
 		phys_active = true;
 	else
 		phys_active = false;
@@ -351,7 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 	if (WARN_ON(IS_ERR(map)))
 		return PTR_ERR(map);
 
-	timer->map = map;
+	timer->virt_irq = irq->irq;
 	return 0;
 }
 
@@ -494,8 +494,8 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 
 	timer_disarm(timer);
-	if (timer->map)
-		kvm_vgic_unmap_phys_irq(vcpu, timer->map->virt_irq);
+	if (timer->virt_irq)
+		kvm_vgic_unmap_phys_irq(vcpu, timer->virt_irq);
 }
 
 void kvm_timer_enable(struct kvm *kvm)
-- 
2.7.3

[PATCH 6/7] KVM: arm/arm64: remove irq_phys_map from the arch timer

[Eric Auger]

On 04/15/2016 04:04 PM, Andre Przywara wrote:
> Now that the interface between the arch timer and the VGIC does not
> require passing the irq_phys_map entry pointer anymore, let's remove
> it from the virtual arch timer and use the virtual IRQ number instead
> directly.
> The remaining pointer returned by kvm_vgic_map_phys_irq() will be
> removed in the following patch.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/arm_arch_timer.h |  4 ++--
>  virt/kvm/arm/arch_timer.c    | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index b651aed..1ed70f6 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -53,8 +53,8 @@ struct arch_timer_cpu {
>  	/* Timer IRQ */
>  	struct kvm_irq_level		irq;
>  
> -	/* VGIC mapping */
> -	struct irq_phys_map		*map;
> +	/* The virtual IRQ number */
> +	unsigned int			virt_irq;
what's the difference between irq.irq and virt_irq? aren't they duplicates?
>  
>  	/* Active IRQ state caching */
>  	bool				active_cleared_last;
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index cfdf88f..1598e23 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -137,10 +137,10 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
>  
>  	timer->active_cleared_last = false;
>  	timer->irq.level = new_level;
> -	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
> +	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->virt_irq,
>  				   timer->irq.level);
>  	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
> -					 timer->map->virt_irq,
> +					 timer->virt_irq,
>  					 timer->irq.level);
>  	WARN_ON(ret);
>  }
> @@ -246,7 +246,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>  	* to ensure that hardware interrupts from the timer triggers a guest
>  	* exit.
>  	*/
> -	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map->virt_irq))
> +	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->virt_irq))
>  		phys_active = true;
>  	else
>  		phys_active = false;
> @@ -351,7 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  	if (WARN_ON(IS_ERR(map)))
>  		return PTR_ERR(map);
>  
> -	timer->map = map;
> +	timer->virt_irq = irq->irq;
you also have a duplicate above:
        timer->irq.irq = irq->irq;
>  	return 0;
>  }
>  
> @@ -494,8 +494,8 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>  
>  	timer_disarm(timer);
> -	if (timer->map)
> -		kvm_vgic_unmap_phys_irq(vcpu, timer->map->virt_irq);
> +	if (timer->virt_irq)
> +		kvm_vgic_unmap_phys_irq(vcpu, timer->virt_irq);
always unmap?

Cheers

Eric
>  }
>  
>  void kvm_timer_enable(struct kvm *kvm)
> 

[PATCH 6/7] KVM: arm/arm64: remove irq_phys_map from the arch timer

[Andre Przywara]

Salut Eric,

On 21/04/16 18:56, Eric Auger wrote:
> On 04/15/2016 04:04 PM, Andre Przywara wrote:
>> Now that the interface between the arch timer and the VGIC does not
>> require passing the irq_phys_map entry pointer anymore, let's remove
>> it from the virtual arch timer and use the virtual IRQ number instead
>> directly.
>> The remaining pointer returned by kvm_vgic_map_phys_irq() will be
>> removed in the following patch.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/arm_arch_timer.h |  4 ++--
>>  virt/kvm/arm/arch_timer.c    | 12 ++++++------
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
>> index b651aed..1ed70f6 100644
>> --- a/include/kvm/arm_arch_timer.h
>> +++ b/include/kvm/arm_arch_timer.h
>> @@ -53,8 +53,8 @@ struct arch_timer_cpu {
>>  	/* Timer IRQ */
>>  	struct kvm_irq_level		irq;
>>  
>> -	/* VGIC mapping */
>> -	struct irq_phys_map		*map;
>> +	/* The virtual IRQ number */
>> +	unsigned int			virt_irq;
> what's the difference between irq.irq and virt_irq? aren't they duplicates?

As I found out already for myself: nothing.
I think I will remove virt_irq and use irq.irq instead everywhere.

>>  
>>  	/* Active IRQ state caching */
>>  	bool				active_cleared_last;
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index cfdf88f..1598e23 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -137,10 +137,10 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
>>  
>>  	timer->active_cleared_last = false;
>>  	timer->irq.level = new_level;
>> -	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
>> +	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->virt_irq,
>>  				   timer->irq.level);
>>  	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
>> -					 timer->map->virt_irq,
>> +					 timer->virt_irq,
>>  					 timer->irq.level);
>>  	WARN_ON(ret);
>>  }
>> @@ -246,7 +246,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>>  	* to ensure that hardware interrupts from the timer triggers a guest
>>  	* exit.
>>  	*/
>> -	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map->virt_irq))
>> +	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->virt_irq))
>>  		phys_active = true;
>>  	else
>>  		phys_active = false;
>> @@ -351,7 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>  	if (WARN_ON(IS_ERR(map)))
>>  		return PTR_ERR(map);
>>  
>> -	timer->map = map;
>> +	timer->virt_irq = irq->irq;
> you also have a duplicate above:
>         timer->irq.irq = irq->irq;
>>  	return 0;
>>  }
>>  
>> @@ -494,8 +494,8 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
>>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>  
>>  	timer_disarm(timer);
>> -	if (timer->map)
>> -		kvm_vgic_unmap_phys_irq(vcpu, timer->map->virt_irq);
>> +	if (timer->virt_irq)
>> +		kvm_vgic_unmap_phys_irq(vcpu, timer->virt_irq);
> always unmap?

Yeah, makes sense.

Thanks!
Andre

>>  }
>>  
>>  void kvm_timer_enable(struct kvm *kvm)
>>
> 

[PATCH 7/7] KVM: arm/arm64: remove irq_phys_map pointer from kvm_vgic_map_phys_irq()

[Andre Przywara]

Now that the virtual arch timer does not care about the irq_phys_map
anymore, let's rework kvm_vgic_map_phys_irq() to return an error
value instead. Any reference to thap mapping can later be done by
passing the correct combination of VCPU and virtual IRQ number.
This makes the irq_phys_map handling completely private to the
VGIC code.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/arm_vgic.h    |  3 +--
 virt/kvm/arm/arch_timer.c |  8 ++++----
 virt/kvm/arm/vgic.c       | 23 +++++++++++------------
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 49c559e..f842d7d 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -343,8 +343,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
 			       unsigned int virt_irq, bool level);
 void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
-struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
-					   int virt_irq, int phys_irq);
+int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int 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 1598e23..270f971 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -306,10 +306,10 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 			 const struct kvm_irq_level *irq)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	struct irq_phys_map *map;
 	struct irq_desc *desc;
 	struct irq_data *data;
 	int phys_irq;
+	int ret;
 
 	/*
 	 * The vcpu timer irq number cannot be determined in
@@ -347,9 +347,9 @@ 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, phys_irq);
-	if (WARN_ON(IS_ERR(map)))
-		return PTR_ERR(map);
+	ret = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq);
+	if (ret)
+		return ret;
 
 	timer->virt_irq = irq->irq;
 	return 0;
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index ac5838b..00386df 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1712,29 +1712,28 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
 /**
  * kvm_vgic_map_phys_irq - map a virtual IRQ to a physical IRQ
  * @vcpu: The VCPU pointer
- * @virt_irq: The virtual irq number
- * @irq: The Linux IRQ number
+ * @virt_irq: The virtual IRQ number for the guest
+ * @phys_irq: The hardware IRQ number of the host
  *
  * 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
+ * hardware irq (@phys_irq). On injection, @virt_irq will be associated with
+ * the physical interrupt represented by @phys_irq. This mapping can be
  * established multiple times as long as the parameters are the same.
  *
- * Returns a valid pointer on success, and an error pointer otherwise
+ * Returns 0 on success or an error value otherwise.
  */
-struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
-					   int virt_irq, int phys_irq)
+int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
 	struct irq_phys_map *map;
 	struct irq_phys_map_entry *entry;
-
+	int ret = 0;
 
 	/* Create a new mapping */
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	spin_lock(&dist->irq_phys_map_lock);
 
@@ -1743,7 +1742,7 @@ 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 = ERR_PTR(-EINVAL);
+			ret = -EINVAL;
 
 		/* Found an existing, valid mapping */
 		goto out;
@@ -1759,9 +1758,9 @@ out:
 	spin_unlock(&dist->irq_phys_map_lock);
 	/* If we've found a hit in the existing list, free the useless
 	 * entry */
-	if (IS_ERR(map) || map != &entry->map)
+	if (ret || map != &entry->map)
 		kfree(entry);
-	return map;
+	return ret;
 }
 
 static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
-- 
2.7.3

[PATCH 7/7] KVM: arm/arm64: remove irq_phys_map pointer from kvm_vgic_map_phys_irq()

[Eric Auger]

On 04/15/2016 04:04 PM, Andre Przywara wrote:
> Now that the virtual arch timer does not care about the irq_phys_map
> anymore, let's rework kvm_vgic_map_phys_irq() to return an error
> value instead. Any reference to thap mapping can later be done by
> passing the correct combination of VCPU and virtual IRQ number.
> This makes the irq_phys_map handling completely private to the
> VGIC code.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/arm_vgic.h    |  3 +--
>  virt/kvm/arm/arch_timer.c |  8 ++++----
>  virt/kvm/arm/vgic.c       | 23 +++++++++++------------
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 49c559e..f842d7d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -343,8 +343,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>  			       unsigned int virt_irq, bool level);
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> -struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -					   int virt_irq, int phys_irq);
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int 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 1598e23..270f971 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -306,10 +306,10 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  			 const struct kvm_irq_level *irq)
>  {
>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> -	struct irq_phys_map *map;
>  	struct irq_desc *desc;
>  	struct irq_data *data;
>  	int phys_irq;
> +	int ret;
>  
>  	/*
>  	 * The vcpu timer irq number cannot be determined in
> @@ -347,9 +347,9 @@ 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, phys_irq);
> -	if (WARN_ON(IS_ERR(map)))
> -		return PTR_ERR(map);
> +	ret = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq);
> +	if (ret)
> +		return ret;
>  
>  	timer->virt_irq = irq->irq;
>  	return 0;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index ac5838b..00386df 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1712,29 +1712,28 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
>  /**
>   * kvm_vgic_map_phys_irq - map a virtual IRQ to a physical IRQ
>   * @vcpu: The VCPU pointer
> - * @virt_irq: The virtual irq number
> - * @irq: The Linux IRQ number
> + * @virt_irq: The virtual IRQ number for the guest
> + * @phys_irq: The hardware IRQ number of the host
nit: not belonging to this patch ;-)
>   *
>   * 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
> + * hardware irq (@phys_irq). On injection, @virt_irq will be associated with
> + * the physical interrupt represented by @phys_irq. This mapping can be
same

Besides Reviewed-by: Eric Auger <eric.auger@linaro.org>

Have a nice evening

Cheers

Eric


>   * established multiple times as long as the parameters are the same.
>   *
> - * Returns a valid pointer on success, and an error pointer otherwise
> + * Returns 0 on success or an error value otherwise.
>   */
> -struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -					   int virt_irq, int phys_irq)
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
>  	struct irq_phys_map *map;
>  	struct irq_phys_map_entry *entry;
> -
> +	int ret = 0;
>  
>  	/* Create a new mapping */
>  	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>  	if (!entry)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>  
>  	spin_lock(&dist->irq_phys_map_lock);
>  
> @@ -1743,7 +1742,7 @@ 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 = ERR_PTR(-EINVAL);
> +			ret = -EINVAL;
>  
>  		/* Found an existing, valid mapping */
>  		goto out;
> @@ -1759,9 +1758,9 @@ out:
>  	spin_unlock(&dist->irq_phys_map_lock);
>  	/* If we've found a hit in the existing list, free the useless
>  	 * entry */
> -	if (IS_ERR(map) || map != &entry->map)
> +	if (ret || map != &entry->map)
>  		kfree(entry);
> -	return map;
> +	return ret;
>  }
>  
>  static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
>