From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered Date: Thu, 19 Feb 2015 15:41:16 +0000 Message-ID: <54E6041C.6020300@linaro.org> References: <1424359395.30924.89.camel@citrix.com> <1424359443-21584-2-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1424359443-21584-2-git-send-email-ian.campbell@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , xen-devel@lists.xen.org Cc: tim@xen.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.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 > --- > 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