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@xxxxxxxxxx>
>>>>
>>>> ---
>>>>
>>>> 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
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
