Re: [PATCH for-4.5] xen/arm: Fix virtual timer on ARMv8 Model

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2014-11-25 at 17:44 +0000, Julien Grall wrote:
> ARMv8 model may not disable correctly the timer interrupt when Xen

"correct disable"

> context switch to an idle vCPU. Therefore Xen may receive a spurious

"context switches" and s/spurious/unexpected/ (since spurious has a
specific meaning in the h/w which does not match what is happening here)

> timer interrupt. As the idle domain doesn't have vGIC, Xen will crash
> when trying to inject the interrupt with the following stack trace.
> 
> (XEN)    [<0000000000228388>] _spin_lock_irqsave+0x28/0x94 (PC)
> (XEN)    [<0000000000228380>] _spin_lock_irqsave+0x20/0x94 (LR)
> (XEN)    [<0000000000250510>] vgic_vcpu_inject_irq+0x40/0x1b0
> (XEN)    [<000000000024bcd0>] vtimer_interrupt+0x4c/0x54
> (XEN)    [<0000000000247010>] do_IRQ+0x1a4/0x220
> (XEN)    [<0000000000244864>] gic_interrupt+0x50/0xec
> (XEN)    [<000000000024fbac>] do_trap_irq+0x20/0x2c
> (XEN)    [<0000000000255240>] hyp_irq+0x5c/0x60
> (XEN)    [<0000000000241084>] context_switch+0xb8/0xc4
> (XEN)    [<000000000022482c>] schedule+0x684/0x6d0
> (XEN)    [<000000000022785c>] __do_softirq+0xcc/0xe8
> (XEN)    [<00000000002278d4>] do_softirq+0x14/0x1c
> (XEN)    [<0000000000240fac>] idle_loop+0x134/0x154
> (XEN)    [<000000000024c160>] start_secondary+0x14c/0x15c
> (XEN)    [<0000000000000001>] 0000000000000001
> 
> While we receive spurious virtual timer interrupt, this could be safely
> ignore for the time being. A proper fix need to be found for Xen 4.6.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although I wonder if we should log, perhaps rate limited or only once.

Also, I've some grammar nits (above and below) which I can fix on commit
if there is no resend...

> 
> ---
> 
> This patch is a bug fix candidate for Xen 4.5. Any ARMv8 model may
> randomly crash when running Xen.

CCing Konrad.

> This patch don't inject the virtual timer interrupt if the current VCPU
> is the idle one. Entering in this function with the idle VCPU is already
> a bug itself. For now, I think this patch is the safest way to resolve
> the problem.
> 
> Meanwhile, I'm investigating with ARM to see wheter the bug comes from
> Xen or the model.
> ---
>  xen/arch/arm/time.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index a6436f1..83c74cb 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -169,6 +169,14 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>  
>  static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>  {
> +    /*
> +     * ARMv8 model may not disable correctly the timer interrupt when

"correctly disable"

> +     * Xen context switch to an idle vCPU. Therefore Xen may receive

"context switches" and "may receive an unexpected timer interrupt"

> +     * timer interrupt.
> +     */
> +    if ( is_idle_vcpu(current) )
> +        return;
> +
>      current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
>      WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
>      vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq);