[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 at 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
>