Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 19/02/15 15:24, Ian Campbell wrote:
> Edge trigger arch timer interrupts really don't make much sense, so if
> we discover we are booting on such a system issue a warning.
> 
> So far this has only been seen on the fast model emulators which have
> both an incorrect DT description of the interrupt and a writeable
> ICFGR allowing us to program the incorrect configuration. Other
> platforms have incorrect DT descriptions (warned about by previous
> patch) but the corresponding ICFGR isn't actually writeable so the
> eventual configuration is level as desired.
> 
> I did consider overriding the incorrect DT on such systems but since
> so far it has only been observed on emulators and we have code in
> place to deal with edge triggering here I think warning is sufficient
> for now.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/time.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 418748d..7304cd8 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -198,6 +198,32 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>      vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq);
>  }
>  
> +/*
> + * Arch timer interrupt really ought to be level triggered, since the
> + * design of the timer/comparator mechanism is based around that
> + * concept.
> + *
> + * However some firmware (incorrectly) describes the interrupts as
> + * edge triggered and, worse, some hardware allows us to program the
> + * interrupt contoller as edge triggered.

controller

> + *
> + * Check each interrupt and warn if we find ourselves in this situation.
> + */

Based on the comment, would it make sense to override the type of
interrupt to level in anycase? Even if the GIC allows us to write on ICFGR.

> +static void check_timer_irq_cfg(unsigned int irq, const char *which)
> +{
> +    struct irq_desc *desc = irq_to_desc(irq);
> +
> +    /*
> +     * The interrupt contoller driver will update desc->arch.type with

controller

> +     * the actual type which ended up configured in the hardware.
> +     */
> +    if ( desc->arch.type & DT_IRQ_TYPE_LEVEL_LOW )
> +        return;
> +
> +    printk(XENLOG_WARNING
> +           "WARNING: %s-timer IRQ%u is not level triggered.\n", which, irq);
> +}
> +
>  /* Set up the timer interrupt on this CPU */
>  void __cpuinit init_timer_interrupt(void)
>  {
> @@ -215,6 +241,10 @@ void __cpuinit init_timer_interrupt(void)
>                     "virtimer", NULL);
>      request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
>                  "phytimer", NULL);
> +
> +    check_timer_irq_cfg(timer_irq[TIMER_HYP_PPI], "hypervisor");
> +    check_timer_irq_cfg(timer_irq[TIMER_VIRT_PPI], "virtual");
> +    check_timer_irq_cfg(timer_irq[TIMER_PHYS_NONSECURE_PPI], "NS-physical");
>  }
>  
>  /* Wait a set number of microseconds */
> 

Regards,


-- 
Julien Grall