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

Re: [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

Re: [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

Re: [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
> 

Re: [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
> 

Re: [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

Re: [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

Re: [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
> 

Re: [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
>