Hi Marc,
On 08/06/2015 06:44 PM, Marc Zyngier wrote:
> On 05/08/15 14:47, Christoffer Dall wrote:
>> On Wed, Aug 05, 2015 at 01:47:27PM +0200, Eric Auger wrote:
>>> On 08/05/2015 12:53 PM, Christoffer Dall wrote:
>>>> On Wed, Aug 05, 2015 at 10:44:09AM +0100, Marc Zyngier wrote:
>>>>> On 05/08/15 08:32, Eric Auger wrote:
>>>>>> Hi Marc,
>>>>>> On 08/04/2015 06:44 PM, Marc Zyngier wrote:
>>>>>>> On 04/08/15 17:21, Eric Auger wrote:
>>>>>>>> Hi Marc,
>>>>>>>> On 07/24/2015 05:55 PM, Marc Zyngier wrote:
>>>>>>>>> Virtual interrupts mapped to a HW interrupt should only be triggered
>>>>>>>>> from inside the kernel. Otherwise, you could end up confusing the
>>>>>>>>> kernel (and the GIC's) state machine.
>>>>>>>>>
>>>>>>>>> Rearrange the injection path so that kvm_vgic_inject_irq is
>>>>>>>>> used for non-mapped interrupts, and kvm_vgic_inject_mapped_irq is
>>>>>>>>> used for mapped interrupts. The latter should only be called from
>>>>>>>>> inside the kernel (timer, VFIO).
>>>>>>>> nit: I would replace VFIO by irqfd.
>>>>>>>> VFIO just triggers the eventfd/irqfd. This is KVM/irqfd that injects the
>>>>>>>> virtual irq upon the irqfd signaling and he irqfd adaptation/ARM
>>>>>>>> currently is implemented in vgic.c
>>>>>>>
>>>>>>> Ah, thanks for reminding me of the right terminology, I tend to think of
>>>>>>> it as one big bag of nasty tricks... ;-)
>>>>>>>
>>>>>>> I'll update the commit message.
>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>>>>>>> ---
>>>>>>>>> include/kvm/arm_vgic.h | 2 +
>>>>>>>>> virt/kvm/arm/vgic.c | 99 ++++++++++++++++++++++++++++++++++----------------
>>>>>>>>> 2 files changed, 70 insertions(+), 31 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>>>>>>> index 7306b4b..f6bfd79 100644
>>>>>>>>> --- a/include/kvm/arm_vgic.h
>>>>>>>>> +++ b/include/kvm/arm_vgic.h
>>>>>>>>> @@ -351,6 +351,8 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>>>>>>>>> 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);
>>>>>>>>> void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>>>>>>> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>>>>>>>> int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
>>>>>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>>>>>> index 3f7b690..e40ef70 100644
>>>>>>>>> --- a/virt/kvm/arm/vgic.c
>>>>>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>>>>>> @@ -1533,7 +1533,8 @@ static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>>>>>>>>> - unsigned int irq_num, bool level)
>>>>>>>>> + struct irq_phys_map *map,
>>>>>>>>> + unsigned int irq_num, bool level)
>>>>>>>>> {
>>>>>>>> In vgic_update_irq_pending, I needed to modify the following line and
>>>>>>>> add the "&& !map".
>>>>>>>>
>>>>>>>> if (!vgic_validate_injection(vcpu, irq_num, level) && !map) {
>>>>>>>>
>>>>>>>> Without that, the level being not properly modeled for level sensitive
>>>>>>>> forwarded IRQs, the 2d injection fails.
>>>>>>>
>>>>>>> Ah! Is that because we never see the line being reset to zero, and the
>>>>>>> VGIC still sees the line as pending at the distributor level?
>>>>>> yes indeed
>>>>>
>>>>> Then it is a bigger problem we need to solve, and your solution just
>>>>> papers over the issue.
>>>>>
>>>>> The main problem is that irqfd is essentially an edge-triggered
>>>>> signalling. Fire and forget. Given that we're dealing with a level
>>>>> triggered interrupt, we end up with the interrupt still marked as
>>>>> pending (nobody took the signal down).
>>> this does not really relate to irqfd: irqfd also comes with the concept
>>> of resamplefd. in case the IRQ is not forwarded, this is the
>>> irqfd_resampler_ack function that toggles the IRQ down (eventfd.c). Its
>>> execution is triggered in vgic_process_maintenance. With forwarding the
>>> EOI is not trappable anymore so this disappears.
>>>
>>>
>>>>> The usual way to get out of that mess in is to evaluate the state of the
>>>>> level on EOI. But we can't trap on EOI for a HW interrupt.
>>>>>
>>>>> So it raises the question: should we instead consider the HW pending
>>>>> state instead of the software one for mapped interrupts? It is
>>>>> expensive, but it feels more correct.
>>>>>
>>>> I thought we already covered this at LCA. For mapped interrupts
>>>> (forwarded) we should never consider the software pending state, because
>>>> that state is managed by the hardware. Or am I confusing concepts here?
>>>
>>> Yes we discussed we should bypass most of the SW states.
>>>
>>> in my previous integration I proposed a patch "KVM: arm: vgic: fix state
>>> machine for forwarded IRQ". What's wrong with the below approach?
>>>
>>>
>>> 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.
>>>
>> I don't see anything wrong, and I thought this was what we discussed.
>
> I think I just lost track of what we've discussed at LCA. Ignore me.
>
> I'll repost the updated first 10 patches tomorrow. Eric, are you willing
> to take custody of patch 11 as part of one of your series?
Yes no problem I can take this over if you prefer. I can easily test it.
May I put it in the irq forwarding series?
Best Regards
Eric
>
> Thanks,
>
> M.
>
--
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
