[RFC v2 0/4] chip/vgic adaptations for forwarded irq

[RFC v2 0/4] chip/vgic adaptations for forwarded irq

[Eric Auger]

This series proposes some fixes that appeared to be necessary
to integrate IRQ forwarding in KVM/VFIO.

- deactivation of the forwarded IRQ in irq_disabled case
- a specific handling of forwarded IRQ into the VGIC state machine.
- deactivation of physical IRQ and unforwarding on vgic destruction
- rb_tree lock in vgic.c

Integrated pieces can be found at
ssh://git.linaro.org/people/eric.auger/linux.git
on branch irqfd_integ_v9

v1 -> v2:
- change title of the series (formerly "vgic additions for forwarded irq")
- "[RFC 4/4] KVM: arm: vgic: handle irqfd forwarded IRQ injection
  before vgic readiness" now handled in ARM irqfd series
- add chip.c patch file

Eric Auger (4):
  chip.c: complete the forwarded IRQ in case the handler is not reached
  KVM: arm: vgic: fix state machine for forwarded IRQ
  KVM: arm: vgic: add forwarded irq rbtree lock
  KVM: arm: vgic: cleanup forwarded IRQs on destroy

 include/kvm/arm_vgic.h |   1 +
 kernel/irq/chip.c      |   8 +++-
 virt/kvm/arm/vgic.c    | 106 ++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 94 insertions(+), 21 deletions(-)

-- 
1.9.1

[RFC v2 1/4] chip.c: complete the forwarded IRQ in case the handler is not reached

[Eric Auger]

With current handle_fasteoi_irq implementation, in case irqd_irq_disabled
is true (disable_irq was called) or !irq_may_run, the IRQ is not completed.
Only the running priority is dropped. IN those cases, the IRQ will
never be forwarded and hence will never be deactivated by anyone else.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 kernel/irq/chip.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 2f9571b..f12cce6 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -561,8 +561,12 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 	raw_spin_unlock(&desc->lock);
 	return;
 out:
-	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
-		eoi_irq(desc, chip);
+	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED)) {
+		if (chip->irq_priority_drop)
+			chip->irq_priority_drop(&desc->irq_data);
+		if (chip->irq_eoi)
+			chip->irq_eoi(&desc->irq_data);
+	}
 	raw_spin_unlock(&desc->lock);
 }
 EXPORT_SYMBOL_GPL(handle_fasteoi_irq);
-- 
1.9.1

[RFC v2 2/4] KVM: arm: vgic: fix state machine for forwarded IRQ

[Eric Auger]

Fix multiple injection of level sensitive forwarded IRQs.
With current code, the second injection fails since the state bitmaps
are not reset (process_maintenance is not called anymore).

New implementation follows those principles:
- A forwarded IRQ only can be sampled when it is pending
- when queueing the IRQ (programming the LR), the pending state is removed
  as for edge sensitive IRQs
- an injection of a forwarded IRQ is considered always valid since
  coming from the HW and level always is 1.

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

---

v1 -> v2:
- integration in new vgic_can_sample_irq
- remove the pending state when programming the LR
---
 virt/kvm/arm/vgic.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index cd00cf2..433ecba 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -361,7 +361,10 @@ static 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_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
+	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
+
+	return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq) ||
+		(is_forwarded && vgic_dist_irq_is_pending(vcpu, irq));
 }
 
 static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
@@ -1296,6 +1299,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct vgic_lr vlr;
 	int lr;
+	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
 
 	/* Sanitize the input... */
 	BUG_ON(sgi_source_id & ~7);
@@ -1331,7 +1335,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 	vlr.irq = irq;
 	vlr.source = sgi_source_id;
 	vlr.state = LR_STATE_PENDING;
-	if (!vgic_irq_is_edge(vcpu, irq))
+	if (!vgic_irq_is_edge(vcpu, irq) && !is_forwarded)
 		vlr.state |= LR_EOI_INT;
 
 	vgic_set_lr(vcpu, lr, vlr);
@@ -1372,11 +1376,12 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
 
 static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
 {
+	bool is_forwarded = (vgic_get_phys_irq(vcpu, irq) >= 0);
 	if (!vgic_can_sample_irq(vcpu, irq))
 		return true; /* level interrupt, already queued */
 
 	if (vgic_queue_irq(vcpu, 0, irq)) {
-		if (vgic_irq_is_edge(vcpu, irq)) {
+		if (vgic_irq_is_edge(vcpu, irq) || is_forwarded) {
 			vgic_dist_irq_clear_pending(vcpu, irq);
 			vgic_cpu_irq_clear(vcpu, irq);
 		} else {
@@ -1626,14 +1631,17 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
 	int edge_triggered, level_triggered;
 	int enabled;
 	bool ret = true;
+	bool is_forwarded;
 
 	spin_lock(&dist->lock);
 
 	vcpu = kvm_get_vcpu(kvm, cpuid);
+	is_forwarded = (vgic_get_phys_irq(vcpu, irq_num) >= 0);
+
 	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) && !is_forwarded) {
 		ret = false;
 		goto out;
 	}
-- 
1.9.1

[RFC v2 2/4] KVM: arm: vgic: fix state machine for forwarded IRQ

[Christoffer Dall]

On Wed, Feb 11, 2015 at 09:20:55AM +0100, Eric Auger wrote:
> Fix multiple injection of level sensitive forwarded IRQs.
> With current code, the second injection fails since the state bitmaps
> are not reset (process_maintenance is not called anymore).
> 
> New implementation follows those principles:
> - A forwarded IRQ only can be sampled when it is pending

why?

> - when queueing the IRQ (programming the LR), the pending state is removed
>   as for edge sensitive IRQs
> - an injection of a forwarded IRQ is considered always valid since
>   coming from the HW and level always is 1.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v1 -> v2:
> - integration in new vgic_can_sample_irq
> - remove the pending state when programming the LR
> ---
>  virt/kvm/arm/vgic.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index cd00cf2..433ecba 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -361,7 +361,10 @@ static 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_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);

can you create a wrapper function for is_forwarded?

> +
> +	return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq) ||
> +		(is_forwarded && vgic_dist_irq_is_pending(vcpu, irq));
>  }
>  
>  static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
> @@ -1296,6 +1299,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct vgic_lr vlr;
>  	int lr;
> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
>  
>  	/* Sanitize the input... */
>  	BUG_ON(sgi_source_id & ~7);
> @@ -1331,7 +1335,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>  	vlr.irq = irq;
>  	vlr.source = sgi_source_id;
>  	vlr.state = LR_STATE_PENDING;
> -	if (!vgic_irq_is_edge(vcpu, irq))
> +	if (!vgic_irq_is_edge(vcpu, irq) && !is_forwarded)
>  		vlr.state |= LR_EOI_INT;
>  
>  	vgic_set_lr(vcpu, lr, vlr);
> @@ -1372,11 +1376,12 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>  
>  static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>  {
> +	bool is_forwarded = (vgic_get_phys_irq(vcpu, irq) >= 0);
>  	if (!vgic_can_sample_irq(vcpu, irq))
>  		return true; /* level interrupt, already queued */
>  
>  	if (vgic_queue_irq(vcpu, 0, irq)) {
> -		if (vgic_irq_is_edge(vcpu, irq)) {
> +		if (vgic_irq_is_edge(vcpu, irq) || is_forwarded) {
>  			vgic_dist_irq_clear_pending(vcpu, irq);
>  			vgic_cpu_irq_clear(vcpu, irq);
>  		} else {
> @@ -1626,14 +1631,17 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>  	int edge_triggered, level_triggered;
>  	int enabled;
>  	bool ret = true;
> +	bool is_forwarded;
>  
>  	spin_lock(&dist->lock);
>  
>  	vcpu = kvm_get_vcpu(kvm, cpuid);
> +	is_forwarded = (vgic_get_phys_irq(vcpu, irq_num) >= 0);
> +
>  	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) && !is_forwarded) {

why is it again that we don't trust validate for forwarded irqs?  Should
it not be checked inside validate?  Otherwise, this seems to deserve a
comment.

>  		ret = false;
>  		goto out;
>  	}

Thanks,
-Christoffer

[RFC v2 2/4] KVM: arm: vgic: fix state machine for forwarded IRQ

[Eric Auger]

Hi Christoffer,

On 05/06/2015 04:26 PM, Christoffer Dall wrote:
> On Wed, Feb 11, 2015 at 09:20:55AM +0100, Eric Auger wrote:
>> Fix multiple injection of level sensitive forwarded IRQs.
>> With current code, the second injection fails since the state bitmaps
>> are not reset (process_maintenance is not called anymore).
>>
>> New implementation follows those principles:
>> - A forwarded IRQ only can be sampled when it is pending
> 
> why?
For forwarded IRQ there is no modeled queued state (same as edge). The
pending state is reset as soon as the vIRQ gets queued, in
vgic_queue_hwirq (also same as edge). This modeling makes sure the vIRQ
is injected once. I did not model the pending state since the above
modeling looks simple and modeling the queued state did not work
properly: I observed new forwarded IRQ could hit before the LR was seen
cleaned. So overall, to me, current model looks closer to edge sensitive
IRQs and looks simple & reliable compared to attempting to model any
queued state.

> 
>> - when queueing the IRQ (programming the LR), the pending state is removed
>>   as for edge sensitive IRQs
>> - an injection of a forwarded IRQ is considered always valid since
>>   coming from the HW and level always is 1.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - integration in new vgic_can_sample_irq
>> - remove the pending state when programming the LR
>> ---
>>  virt/kvm/arm/vgic.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index cd00cf2..433ecba 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -361,7 +361,10 @@ static 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_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
>> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
> 
> can you create a wrapper function for is_forwarded?
yes sure
> 
>> +
>> +	return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq) ||
>> +		(is_forwarded && vgic_dist_irq_is_pending(vcpu, irq));
>>  }
>>  
>>  static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
>> @@ -1296,6 +1299,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>  	struct vgic_lr vlr;
>>  	int lr;
>> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
>>  
>>  	/* Sanitize the input... */
>>  	BUG_ON(sgi_source_id & ~7);
>> @@ -1331,7 +1335,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>  	vlr.irq = irq;
>>  	vlr.source = sgi_source_id;
>>  	vlr.state = LR_STATE_PENDING;
>> -	if (!vgic_irq_is_edge(vcpu, irq))
>> +	if (!vgic_irq_is_edge(vcpu, irq) && !is_forwarded)
>>  		vlr.state |= LR_EOI_INT;
>>  
>>  	vgic_set_lr(vcpu, lr, vlr);
>> @@ -1372,11 +1376,12 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>  
>>  static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>>  {
>> +	bool is_forwarded = (vgic_get_phys_irq(vcpu, irq) >= 0);
>>  	if (!vgic_can_sample_irq(vcpu, irq))
>>  		return true; /* level interrupt, already queued */
>>  
>>  	if (vgic_queue_irq(vcpu, 0, irq)) {
>> -		if (vgic_irq_is_edge(vcpu, irq)) {
>> +		if (vgic_irq_is_edge(vcpu, irq) || is_forwarded) {
>>  			vgic_dist_irq_clear_pending(vcpu, irq);
>>  			vgic_cpu_irq_clear(vcpu, irq);
>>  		} else {
>> @@ -1626,14 +1631,17 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>>  	int edge_triggered, level_triggered;
>>  	int enabled;
>>  	bool ret = true;
>> +	bool is_forwarded;
>>  
>>  	spin_lock(&dist->lock);
>>  
>>  	vcpu = kvm_get_vcpu(kvm, cpuid);
>> +	is_forwarded = (vgic_get_phys_irq(vcpu, irq_num) >= 0);
>> +
>>  	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) && !is_forwarded) {
> 
> why is it again that we don't trust validate for forwarded irqs?
forwarded IRQs are directly linked to HW IRQs. If the forwarded IRQ is
received this means the guest completed last HW IRQ occurence and it is
valid to inject the new one so following the discussions we had with
Marc I skipped that check.

  Should
> it not be checked inside validate?  Otherwise, this seems to deserve a
> comment.
It is equal to me. Let me know what is your preference according to
above comment.

- Eric
> 
>>  		ret = false;
>>  		goto out;
>>  	}
> 
> Thanks,
> -Christoffer
> 

[RFC v2 2/4] KVM: arm: vgic: fix state machine for forwarded IRQ

[Christoffer Dall]

On Thu, May 07, 2015 at 09:48:25AM +0200, Eric Auger wrote:
> Hi Christoffer,
> 
> On 05/06/2015 04:26 PM, Christoffer Dall wrote:
> > On Wed, Feb 11, 2015 at 09:20:55AM +0100, Eric Auger wrote:
> >> Fix multiple injection of level sensitive forwarded IRQs.
> >> With current code, the second injection fails since the state bitmaps
> >> are not reset (process_maintenance is not called anymore).
> >>
> >> New implementation follows those principles:
> >> - A forwarded IRQ only can be sampled when it is pending
> > 
> > why?
> For forwarded IRQ there is no modeled queued state (same as edge). The
> pending state is reset as soon as the vIRQ gets queued, in
> vgic_queue_hwirq (also same as edge). This modeling makes sure the vIRQ
> is injected once. I did not model the pending state since the above
> modeling looks simple and modeling the queued state did not work
> properly: I observed new forwarded IRQ could hit before the LR was seen
> cleaned. So overall, to me, current model looks closer to edge sensitive
> IRQs and looks simple & reliable compared to attempting to model any
> queued state.
> 

hmm, reading this, I'm remembering that the rationale was that the
pending state is maintained in the hardware so we never need to resample
any software state.  If the interrupt hits again (injected from VFIO for
example) it must have not been pending, otherwise we have a bug.

Is this the right way to look at it?

I think this needs to be documented somewhere in the code.


> > 
> >> - when queueing the IRQ (programming the LR), the pending state is removed
> >>   as for edge sensitive IRQs
> >> - an injection of a forwarded IRQ is considered always valid since
> >>   coming from the HW and level always is 1.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - integration in new vgic_can_sample_irq
> >> - remove the pending state when programming the LR
> >> ---
> >>  virt/kvm/arm/vgic.c | 16 ++++++++++++----
> >>  1 file changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> >> index cd00cf2..433ecba 100644
> >> --- a/virt/kvm/arm/vgic.c
> >> +++ b/virt/kvm/arm/vgic.c
> >> @@ -361,7 +361,10 @@ static 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_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
> >> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
> > 
> > can you create a wrapper function for is_forwarded?
> yes sure
> > 
> >> +
> >> +	return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq) ||
> >> +		(is_forwarded && vgic_dist_irq_is_pending(vcpu, irq));
> >>  }
> >>  
> >>  static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
> >> @@ -1296,6 +1299,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
> >>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> >>  	struct vgic_lr vlr;
> >>  	int lr;
> >> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
> >>  
> >>  	/* Sanitize the input... */
> >>  	BUG_ON(sgi_source_id & ~7);
> >> @@ -1331,7 +1335,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
> >>  	vlr.irq = irq;
> >>  	vlr.source = sgi_source_id;
> >>  	vlr.state = LR_STATE_PENDING;
> >> -	if (!vgic_irq_is_edge(vcpu, irq))
> >> +	if (!vgic_irq_is_edge(vcpu, irq) && !is_forwarded)
> >>  		vlr.state |= LR_EOI_INT;
> >>  
> >>  	vgic_set_lr(vcpu, lr, vlr);
> >> @@ -1372,11 +1376,12 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
> >>  
> >>  static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
> >>  {
> >> +	bool is_forwarded = (vgic_get_phys_irq(vcpu, irq) >= 0);
> >>  	if (!vgic_can_sample_irq(vcpu, irq))
> >>  		return true; /* level interrupt, already queued */
> >>  
> >>  	if (vgic_queue_irq(vcpu, 0, irq)) {
> >> -		if (vgic_irq_is_edge(vcpu, irq)) {
> >> +		if (vgic_irq_is_edge(vcpu, irq) || is_forwarded) {
> >>  			vgic_dist_irq_clear_pending(vcpu, irq);
> >>  			vgic_cpu_irq_clear(vcpu, irq);
> >>  		} else {
> >> @@ -1626,14 +1631,17 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> >>  	int edge_triggered, level_triggered;
> >>  	int enabled;
> >>  	bool ret = true;
> >> +	bool is_forwarded;
> >>  
> >>  	spin_lock(&dist->lock);
> >>  
> >>  	vcpu = kvm_get_vcpu(kvm, cpuid);
> >> +	is_forwarded = (vgic_get_phys_irq(vcpu, irq_num) >= 0);
> >> +
> >>  	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) && !is_forwarded) {
> > 
> > why is it again that we don't trust validate for forwarded irqs?
> forwarded IRQs are directly linked to HW IRQs. If the forwarded IRQ is
> received this means the guest completed last HW IRQ occurence and it is
> valid to inject the new one so following the discussions we had with
> Marc I skipped that check.

right, like I said above.  We need to document this; it's not trivial.

> 
>   Should
> > it not be checked inside validate?  Otherwise, this seems to deserve a
> > comment.
> It is equal to me. Let me know what is your preference according to
> above comment.
> 
I would fold it into validate and clearly comment that function so that
it's clear what it does.

Thanks,
-Christoffer

[RFC v2 2/4] KVM: arm: vgic: fix state machine for forwarded IRQ

[Eric Auger]

On 05/07/2015 11:20 AM, Christoffer Dall wrote:
> On Thu, May 07, 2015 at 09:48:25AM +0200, Eric Auger wrote:
>> Hi Christoffer,
>>
>> On 05/06/2015 04:26 PM, Christoffer Dall wrote:
>>> On Wed, Feb 11, 2015 at 09:20:55AM +0100, Eric Auger wrote:
>>>> Fix multiple injection of level sensitive forwarded IRQs.
>>>> With current code, the second injection fails since the state bitmaps
>>>> are not reset (process_maintenance is not called anymore).
>>>>
>>>> New implementation follows those principles:
>>>> - A forwarded IRQ only can be sampled when it is pending
>>>
>>> why?
>> For forwarded IRQ there is no modeled queued state (same as edge). The
>> pending state is reset as soon as the vIRQ gets queued, in
>> vgic_queue_hwirq (also same as edge). This modeling makes sure the vIRQ
>> is injected once. I did not model the pending state since the above
>> modeling looks simple and modeling the queued state did not work
>> properly: I observed new forwarded IRQ could hit before the LR was seen
>> cleaned. So overall, to me, current model looks closer to edge sensitive
>> IRQs and looks simple & reliable compared to attempting to model any
>> queued state.
>>
> 
> hmm, reading this, I'm remembering that the rationale was that the
> pending state is maintained in the hardware so we never need to resample
> any software state.  If the interrupt hits again (injected from VFIO for
> example) it must have not been pending, otherwise we have a bug.
> 
> Is this the right way to look at it?
I think so
> 
> I think this needs to be documented somewhere in the code.
> 
> 
>>>
>>>> - when queueing the IRQ (programming the LR), the pending state is removed
>>>>   as for edge sensitive IRQs
>>>> - an injection of a forwarded IRQ is considered always valid since
>>>>   coming from the HW and level always is 1.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - integration in new vgic_can_sample_irq
>>>> - remove the pending state when programming the LR
>>>> ---
>>>>  virt/kvm/arm/vgic.c | 16 ++++++++++++----
>>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>> index cd00cf2..433ecba 100644
>>>> --- a/virt/kvm/arm/vgic.c
>>>> +++ b/virt/kvm/arm/vgic.c
>>>> @@ -361,7 +361,10 @@ static 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_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
>>>> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
>>>
>>> can you create a wrapper function for is_forwarded?
>> yes sure
>>>
>>>> +
>>>> +	return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq) ||
>>>> +		(is_forwarded && vgic_dist_irq_is_pending(vcpu, irq));
>>>>  }
>>>>  
>>>>  static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
>>>> @@ -1296,6 +1299,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>>>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>>  	struct vgic_lr vlr;
>>>>  	int lr;
>>>> +	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
>>>>  
>>>>  	/* Sanitize the input... */
>>>>  	BUG_ON(sgi_source_id & ~7);
>>>> @@ -1331,7 +1335,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>>>  	vlr.irq = irq;
>>>>  	vlr.source = sgi_source_id;
>>>>  	vlr.state = LR_STATE_PENDING;
>>>> -	if (!vgic_irq_is_edge(vcpu, irq))
>>>> +	if (!vgic_irq_is_edge(vcpu, irq) && !is_forwarded)
>>>>  		vlr.state |= LR_EOI_INT;
>>>>  
>>>>  	vgic_set_lr(vcpu, lr, vlr);
>>>> @@ -1372,11 +1376,12 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>>>  
>>>>  static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>>>>  {
>>>> +	bool is_forwarded = (vgic_get_phys_irq(vcpu, irq) >= 0);
>>>>  	if (!vgic_can_sample_irq(vcpu, irq))
>>>>  		return true; /* level interrupt, already queued */
>>>>  
>>>>  	if (vgic_queue_irq(vcpu, 0, irq)) {
>>>> -		if (vgic_irq_is_edge(vcpu, irq)) {
>>>> +		if (vgic_irq_is_edge(vcpu, irq) || is_forwarded) {
>>>>  			vgic_dist_irq_clear_pending(vcpu, irq);
>>>>  			vgic_cpu_irq_clear(vcpu, irq);
>>>>  		} else {
>>>> @@ -1626,14 +1631,17 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>>>>  	int edge_triggered, level_triggered;
>>>>  	int enabled;
>>>>  	bool ret = true;
>>>> +	bool is_forwarded;
>>>>  
>>>>  	spin_lock(&dist->lock);
>>>>  
>>>>  	vcpu = kvm_get_vcpu(kvm, cpuid);
>>>> +	is_forwarded = (vgic_get_phys_irq(vcpu, irq_num) >= 0);
>>>> +
>>>>  	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) && !is_forwarded) {
>>>
>>> why is it again that we don't trust validate for forwarded irqs?
>> forwarded IRQs are directly linked to HW IRQs. If the forwarded IRQ is
>> received this means the guest completed last HW IRQ occurence and it is
>> valid to inject the new one so following the discussions we had with
>> Marc I skipped that check.
> 
> right, like I said above.  We need to document this; it's not trivial.
> 
>>
>>   Should
>>> it not be checked inside validate?  Otherwise, this seems to deserve a
>>> comment.
>> It is equal to me. Let me know what is your preference according to
>> above comment.
>>
> I would fold it into validate and clearly comment that function so that
> it's clear what it does.
ok

- Eric
> 
> Thanks,
> -Christoffer
> 

[RFC v2 3/4] KVM: arm: vgic: add forwarded irq rbtree lock

[Eric Auger]

Add a lock related to the rb tree manipulation. The rb tree can be
searched in one thread (irqfd handler for instance) and map/unmap
may happen in another.

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

---

v2 -> v3:
re-arrange lock sequence in vgic_map_phys_irq
---
 include/kvm/arm_vgic.h |  1 +
 virt/kvm/arm/vgic.c    | 56 ++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 1a49108..ad7229b 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -220,6 +220,7 @@ struct vgic_dist {
 	unsigned long		*irq_pending_on_cpu;
 
 	struct rb_root		irq_phys_map;
+	spinlock_t			rb_tree_lock;
 #endif
 };
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 433ecba..dd72ca2 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1756,9 +1756,22 @@ static struct rb_root *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
 
 int vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
 {
-	struct rb_root *root = vgic_get_irq_phys_map(vcpu, virt_irq);
-	struct rb_node **new = &root->rb_node, *parent = NULL;
+	struct rb_root *root;
+	struct rb_node **new, *parent = NULL;
 	struct irq_phys_map *new_map;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	root = vgic_get_irq_phys_map(vcpu, virt_irq);
+	new = &root->rb_node;
+
+	new_map = kzalloc(sizeof(*new_map), GFP_KERNEL);
+	if (!new_map)
+		return -ENOMEM;
+
+	new_map->virt_irq = virt_irq;
+	new_map->phys_irq = phys_irq;
+
+	spin_lock(&dist->rb_tree_lock);
 
 	/* Boilerplate rb_tree code */
 	while (*new) {
@@ -1770,19 +1783,16 @@ int vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
 			new = &(*new)->rb_left;
 		else if (this->virt_irq > virt_irq)
 			new = &(*new)->rb_right;
-		else
+		else {
+			kfree(new_map);
+			spin_unlock(&dist->rb_tree_lock);
 			return -EEXIST;
+		}
 	}
 
-	new_map = kzalloc(sizeof(*new_map), GFP_KERNEL);
-	if (!new_map)
-		return -ENOMEM;
-
-	new_map->virt_irq = virt_irq;
-	new_map->phys_irq = phys_irq;
-
 	rb_link_node(&new_map->node, parent, new);
 	rb_insert_color(&new_map->node, root);
+	spin_unlock(&dist->rb_tree_lock);
 
 	return 0;
 }
@@ -1811,24 +1821,39 @@ static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
 
 int vgic_get_phys_irq(struct kvm_vcpu *vcpu, int virt_irq)
 {
-	struct irq_phys_map *map = vgic_irq_map_search(vcpu, virt_irq);
+	struct irq_phys_map *map;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+	int ret;
+
+	spin_lock(&dist->rb_tree_lock);
+	map = vgic_irq_map_search(vcpu, virt_irq);
 
 	if (map)
-		return map->phys_irq;
+		ret = map->phys_irq;
+	else
+		ret =  -ENOENT;
+
+	spin_unlock(&dist->rb_tree_lock);
+	return ret;
 
-	return -ENOENT;
 }
 
 int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
 {
-	struct irq_phys_map *map = vgic_irq_map_search(vcpu, virt_irq);
+	struct irq_phys_map *map;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	spin_lock(&dist->rb_tree_lock);
+
+	map = vgic_irq_map_search(vcpu, virt_irq);
 
 	if (map && map->phys_irq == phys_irq) {
 		rb_erase(&map->node, vgic_get_irq_phys_map(vcpu, virt_irq));
 		kfree(map);
+		spin_unlock(&dist->rb_tree_lock);
 		return 0;
 	}
-
+	spin_unlock(&dist->rb_tree_lock);
 	return -ENOENT;
 }
 
@@ -2071,6 +2096,7 @@ int kvm_vgic_create(struct kvm *kvm)
 	ret = 0;
 
 	spin_lock_init(&kvm->arch.vgic.lock);
+	spin_lock_init(&kvm->arch.vgic.rb_tree_lock);
 	kvm->arch.vgic.in_kernel = true;
 	kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
 	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
-- 
1.9.1

[RFC v2 4/4] KVM: arm: vgic: cleanup forwarded IRQs on destroy

[Eric Auger]

When the VGIC is destroyed it must take care of
- restoring the forwarded IRQs in non forwarded state,
- deactivating the IRQ in case the guest left without doing it
- cleaning nodes of the phys_map rbtree

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

---

v1 -> v2:
- remove vgic_clean_irq_phys_map call in kvm_vgic_destroy
  (useless since already called in kvm_vgic_vcpu_destroy)
---
 virt/kvm/arm/vgic.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index dd72ca2..ace8e46 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -32,6 +32,7 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
+#include <linux/spinlock.h>
 
 /*
  * How the whole thing works (courtesy of Christoffer Dall):
@@ -103,6 +104,8 @@ static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
 static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr, struct vgic_lr lr_desc);
 static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
+static void vgic_clean_irq_phys_map(struct kvm_vcpu *vcpu,
+				    struct rb_root *root);
 
 static const struct vgic_ops *vgic_ops;
 static const struct vgic_params *vgic;
@@ -1819,6 +1822,36 @@ static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
 	return NULL;
 }
 
+static void vgic_clean_irq_phys_map(struct kvm_vcpu *vcpu,
+				    struct rb_root *root)
+{
+	unsigned long flags;
+
+	while (1) {
+		struct rb_node *node = rb_first(root);
+		struct irq_phys_map *map;
+		struct irq_desc *desc;
+		struct irq_data *d;
+		struct irq_chip *chip;
+
+		if (!node)
+			break;
+
+		map = container_of(node, struct irq_phys_map, node);
+		desc = irq_to_desc(map->phys_irq);
+
+		raw_spin_lock_irqsave(&desc->lock, flags);
+		d = &desc->irq_data;
+		chip = desc->irq_data.chip;
+		irqd_clr_irq_forwarded(d);
+		chip->irq_eoi(d);
+		raw_spin_unlock_irqrestore(&desc->lock, flags);
+
+		rb_erase(node, root);
+		kfree(map);
+	}
+}
+
 int vgic_get_phys_irq(struct kvm_vcpu *vcpu, int virt_irq)
 {
 	struct irq_phys_map *map;
@@ -1861,6 +1894,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 
+	vgic_clean_irq_phys_map(vcpu, &vgic_cpu->irq_phys_map);
 	kfree(vgic_cpu->pending_shared);
 	kfree(vgic_cpu->vgic_irq_lr_map);
 	vgic_cpu->pending_shared = NULL;
-- 
1.9.1

[RFC v2 0/4] chip/vgic adaptations for forwarded irq

[Christoffer Dall]

Hi Eric,

On Wed, Feb 11, 2015 at 09:20:53AM +0100, Eric Auger wrote:
> This series proposes some fixes that appeared to be necessary
> to integrate IRQ forwarding in KVM/VFIO.
> 
> - deactivation of the forwarded IRQ in irq_disabled case
> - a specific handling of forwarded IRQ into the VGIC state machine.
> - deactivation of physical IRQ and unforwarding on vgic destruction
> - rb_tree lock in vgic.c
> 
> Integrated pieces can be found at
> ssh://git.linaro.org/people/eric.auger/linux.git
> on branch irqfd_integ_v9
> 
What are the dependencies for this at this point?

I assume it relies at least on the split EOI/priority drop changes?

Are you going to respin this when there are newer versions of the
dependencies out, or what are the plans?

Thanks,
-Christoffer

[RFC v2 0/4] chip/vgic adaptations for forwarded irq

[Eric Auger]

On 05/06/2015 04:27 PM, Christoffer Dall wrote:
> Hi Eric,
> 
> On Wed, Feb 11, 2015 at 09:20:53AM +0100, Eric Auger wrote:
>> This series proposes some fixes that appeared to be necessary
>> to integrate IRQ forwarding in KVM/VFIO.
>>
>> - deactivation of the forwarded IRQ in irq_disabled case
>> - a specific handling of forwarded IRQ into the VGIC state machine.
>> - deactivation of physical IRQ and unforwarding on vgic destruction
>> - rb_tree lock in vgic.c
>>
>> Integrated pieces can be found at
>> ssh://git.linaro.org/people/eric.auger/linux.git
>> on branch irqfd_integ_v9
>>
> What are the dependencies for this at this point?
> 
> I assume it relies at least on the split EOI/priority drop changes?

Yes it now only depends on split EOI/priority drop changes since
"genirq: Saving/restoring the irqchip state of an irq line" now is
upstreamed.

> 
> Are you going to respin this when there are newer versions of the
> dependencies out, or what are the plans?

Yes I will respin according to new versions. I am currently using a
rebased version of Marc's original RFC "ARM: Forwarding physical
interrupts to a guest VM" (http://lwn.net/Articles/603514/) which is a
superset of [PATCH] genirq: Add support for priority-drop/deactivate
interrupt controllers.

- Eric
> 
> Thanks,
> -Christoffer
> 

[RFC v2 0/4] chip/vgic adaptations for forwarded irq

[Christoffer Dall]

On Wed, May 06, 2015 at 05:32:53PM +0200, Eric Auger wrote:
> On 05/06/2015 04:27 PM, Christoffer Dall wrote:
> > Hi Eric,
> > 
> > On Wed, Feb 11, 2015 at 09:20:53AM +0100, Eric Auger wrote:
> >> This series proposes some fixes that appeared to be necessary
> >> to integrate IRQ forwarding in KVM/VFIO.
> >>
> >> - deactivation of the forwarded IRQ in irq_disabled case
> >> - a specific handling of forwarded IRQ into the VGIC state machine.
> >> - deactivation of physical IRQ and unforwarding on vgic destruction
> >> - rb_tree lock in vgic.c
> >>
> >> Integrated pieces can be found at
> >> ssh://git.linaro.org/people/eric.auger/linux.git
> >> on branch irqfd_integ_v9
> >>
> > What are the dependencies for this at this point?
> > 
> > I assume it relies at least on the split EOI/priority drop changes?
> 
> Yes it now only depends on split EOI/priority drop changes since
> "genirq: Saving/restoring the irqchip state of an irq line" now is
> upstreamed.
> 
> > 
> > Are you going to respin this when there are newer versions of the
> > dependencies out, or what are the plans?
> 
> Yes I will respin according to new versions. I am currently using a
> rebased version of Marc's original RFC "ARM: Forwarding physical
> interrupts to a guest VM" (http://lwn.net/Articles/603514/) which is a
> superset of [PATCH] genirq: Add support for priority-drop/deactivate
> interrupt controllers.
> 

ok, once there's movement on the dependency and you respin, I'll review
the rest of this in detail.

-Christoffer

[RFC v2 0/4] chip/vgic adaptations for forwarded irq

[Eric Auger]

On 05/07/2015 11:17 AM, Christoffer Dall wrote:
> On Wed, May 06, 2015 at 05:32:53PM +0200, Eric Auger wrote:
>> On 05/06/2015 04:27 PM, Christoffer Dall wrote:
>>> Hi Eric,
>>>
>>> On Wed, Feb 11, 2015 at 09:20:53AM +0100, Eric Auger wrote:
>>>> This series proposes some fixes that appeared to be necessary
>>>> to integrate IRQ forwarding in KVM/VFIO.
>>>>
>>>> - deactivation of the forwarded IRQ in irq_disabled case
>>>> - a specific handling of forwarded IRQ into the VGIC state machine.
>>>> - deactivation of physical IRQ and unforwarding on vgic destruction
>>>> - rb_tree lock in vgic.c
>>>>
>>>> Integrated pieces can be found at
>>>> ssh://git.linaro.org/people/eric.auger/linux.git
>>>> on branch irqfd_integ_v9
>>>>
>>> What are the dependencies for this at this point?
>>>
>>> I assume it relies at least on the split EOI/priority drop changes?
>>
>> Yes it now only depends on split EOI/priority drop changes since
>> "genirq: Saving/restoring the irqchip state of an irq line" now is
>> upstreamed.
>>
>>>
>>> Are you going to respin this when there are newer versions of the
>>> dependencies out, or what are the plans?
>>
>> Yes I will respin according to new versions. I am currently using a
>> rebased version of Marc's original RFC "ARM: Forwarding physical
>> interrupts to a guest VM" (http://lwn.net/Articles/603514/) which is a
>> superset of [PATCH] genirq: Add support for priority-drop/deactivate
>> interrupt controllers.
>>
> 
> ok, once there's movement on the dependency and you respin, I'll review
> the rest of this in detail.

OK thanks

Eric
> 
> -Christoffer
>