Re: [RFC PATCH 3/9] irqchip: GIC: Convert to EOImode == 1

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

On 01/07/14 17:34, Stefano Stabellini wrote:
> On Tue, 1 Jul 2014, Marc Zyngier wrote:
>> Hi Stefano,
>>
>> On 30/06/14 20:09, Stefano Stabellini wrote:
>>> On Wed, 25 Jun 2014, Anup Patel wrote:
>>>> Hi Marc,
>>>>
>>>> On Wed, Jun 25, 2014 at 2:58 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> So far, GICv2 has been used in with EOImode == 0. The effect of this
>>>>> mode is to perform the priority drop and the deactivation of the
>>>>> interrupt at the same time.
>>>>>
>>>>> While this works perfectly for Linux (we only have a single priority),
>>>>> it causes issues when an interrupt is forwarded to a guest, and when
>>>>> we want the guest to perform the EOI itself.
>>>>>
>>>>> For this case, the GIC architecture provides EOImode == 1, where:
>>>>> - A write to the EOI register drops the priority of the interrupt and leaves
>>>>> it active. Other interrupts at the same priority level can now be taken,
>>>>> but the active interrupt cannot be taken again
>>>>> - A write to the DIR marks the interrupt as inactive, meaning it can
>>>>> now be taken again.
>>>>>
>>>>> We only enable this feature when booted in HYP mode. Also, as most device
>>>>> trees are broken (they report the CPU interface size to be 4kB, while
>>>>> the GICv2 CPU interface size is 8kB), output a warning if we're booted
>>>>> in HYP mode, and disable the feature.
>>>>>
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> ---
>>>>>  drivers/irqchip/irq-gic.c       | 61 +++++++++++++++++++++++++++++++++++++----
>>>>>  include/linux/irqchip/arm-gic.h |  4 +++
>>>>>  2 files changed, 59 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>>>> index 508b815..9295bf2 100644
>>>>> --- a/drivers/irqchip/irq-gic.c
>>>>> +++ b/drivers/irqchip/irq-gic.c
>>>>> @@ -45,6 +45,7 @@
>>>>>  #include <asm/irq.h>
>>>>>  #include <asm/exception.h>
>>>>>  #include <asm/smp_plat.h>
>>>>> +#include <asm/virt.h>
>>>>>
>>>>>  #include "irq-gic-common.h"
>>>>>  #include "irqchip.h"
>>>>> @@ -94,6 +95,10 @@ struct irq_chip gic_arch_extn = {
>>>>>         .irq_set_wake   = NULL,
>>>>>  };
>>>>>
>>>>> +static struct irq_chip *gic_chip;
>>>>> +
>>>>> +static bool supports_deactivate = false;
>>>>> +
>>>>>  #ifndef MAX_GIC_NR
>>>>>  #define MAX_GIC_NR     1
>>>>>  #endif
>>>>> @@ -185,6 +190,12 @@ static void gic_eoi_irq(struct irq_data *d)
>>>>>         writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>>>>>  }
>>>>>
>>>>> +static void gic_eoi_dir_irq(struct irq_data *d)
>>>>> +{
>>>>> +       gic_eoi_irq(d);
>>>>> +       writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
>>>>> +}
>>>
>>> Would it be better if you moved the gic_eoi_irq call earlier? Maybe
>>> somewhere in gic_handle_irq?
>>
>> I'm not sure I see what we'd gain by doing so. Can you elaborate?
> 
> You would be dropping the priority earlier. It would be beneficial if
> Linux was running the interrupt handlers with interrupts enabled, but I
> realize now that it is not the case.

Ah, I see what you mean. No, as you noticed, we run the handlers with
interrupts disabled, so keeping the EOI and DIR close together is
probably what makes the most sense.

Thanks,

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