Re: [PATCH 5/6] irqchip: GICv3: Don't deactivate interrupts forwarded to a guest

[Marc Zyngier <marc.zyngier@arm.com>]

On 12/08/15 16:09, Eric Auger wrote:
> On 08/12/2015 04:20 PM, Marc Zyngier wrote:
>> On 11/08/15 11:03, Eric Auger wrote:
>>> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>>>> Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
>>>> target an interrupt to a VCPU") added just what we needed at the
>>>> lowest level to allow an interrupt to be deactivated by a guest.
>>>>
>>>> When such a request reaches the GIC, it knows it doesn't need to
>>>> perform the deactivation anymore, and can safely leave the guest
>>>> do its magic. This of course requires additional support in both
>>>> VFIO and KVM.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3.c | 29 +++++++++++++++++++++++++++--
>>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>> index e02592b..a1ca9e6 100644
>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>> @@ -70,6 +70,11 @@ static inline int gic_irq_in_rdist(struct irq_data *d)
>>>>  	return gic_irq(d) < 32;
>>>>  }
>>>>  
>>>> +static inline bool forwarded_irq(struct irq_data *d)
>>>> +{
>>>> +	return d->handler_data != NULL;
>>>> +}
>>>> +
>>>>  static inline void __iomem *gic_dist_base(struct irq_data *d)
>>>>  {
>>>>  	if (gic_irq_in_rdist(d))	/* SGI+PPI -> SGI_base for this CPU */
>>>> @@ -231,6 +236,12 @@ static void gic_poke_irq(struct irq_data *d, u32 offset)
>>>>  static void gic_mask_irq(struct irq_data *d)
>>>>  {
>>>>  	gic_poke_irq(d, GICD_ICENABLER);
>>>> +	/*
>>>> +	 * When masking a forwarded interrupt, make sure it is
>>>> +	 * deactivated as well.
>>> To me it is not straightforward to understand why a forwarded IRQ would
>>> need to be DIR'ed when masked. This is needed because of the disable_irq
>>> optimisation, I would add a related comment.
>>>
>>
>> The lazy disable_irq is just an optimization.
> yes that's true but it causes a real problem here since although you
> disabled the IRQ, a new one can show up, we do not call the actual
> handler (that was supposed to forward it to the guest) but still you
> expect the guest to complete it. Practically that's why the host must
> take in charge the deactivation in that case, and this happens during
> the masking with this implementation.

Yeah, I see what you mean. If we end-up here with an active interrupt,
that's because the lazy interrupt masking has been used, and we need to
fix up things.

>>
>> The real reason is that if we mask an interrupt on the host, it is
>> because we don't want the guest to process it at all. There is three cases:
>>
>> 1) The interrupt was inactive: no problem
>> 2) The interrupt was active, but not presented to the guest yet: no
>> problem either. The interrupt will be taken again on unmask.
>> 3) The interrupt was active and presented to the guest: we might get a
>> double deactivate, which shouldn't be a big deal (but mostly should not
>> occur at all).
>>
>> Would something like this make sense?
> yes makes sense. The only thing that scares me a bit is 3: when
> masking/DIR an edge irq (#n) we can have the same new physical IRQ
> showing up when unmasking (#n+1); when guest deactivates the #nth
> virtual IRQ it is going to DIR #n+1 physical IRQ.

That bit is not worrying me too much for a few reasons reasons:
- You normally don't mask a forwarded interrupt. You only do that on
specific events like guest termination. At that point, it doesn't matter
anymore.
- Edge interrupts can always be coalesced. So getting one event instead
of two is not a problem.
- Deactivation (specially on EOI from a guest) is not "refcounted". It
just clears the active bit.

> Also with VGIC state machine, we must be attention not to inject the
> second forwarded edge irq while there is one programmed in the LR. We
> said "it comes from the HW so it must be true"? Not safe anymore here ...

I don't believe this is a problem. You should never end-up masking the
interrupt if you don't intend to tear it down (this is why we have the
active bit - to avoid masking thing in normal operations).

> 
>>
>> On a related note, I wonder if we need to mark the interrupt pending if
>> it is configured as edge. Otherwise, we could loose an interrupt in case
>> 2 (mask, deactivate, unmask, oh look nothing triggers). Thoughts?
> Yes I think this makes sense indeed. Definitively this one will be lost.

Depends. If we are to restore a working interrupt flow, then we need it.
If we just mask to allow an interrupt to be "unforwarded", then we do
not have to care.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...