Re: [Xen-devel] [PATCH v2 for-4.5] xen/arm: clear UIE on hypervisor entry

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-11-20 at 10:53 +0000, Stefano Stabellini wrote:
> UIE being set can cause maintenance interrupts to occur when Xen writes
> to one or more LR registers. The effect is a busy loop around the
> interrupt handler in Xen
> (http://marc.info/?l=xen-devel&m=141597517132682): everything gets stuck.

I think it would be useful to explain somewhere why/how a spurious
interrupt can occur, if not here then in the comment you've already
added or in another one in gic_clear_lrs where you clear UIE.

The bit about clearing the LRs on entry causing UIE to become deasserted
before we get as far as reading the IAR is a bit subtle.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Reported-and-Tested-by: Andrii Tseglytskyi 
> <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
> CC: konrad.wilk@xxxxxxxxxx

With the expanded commentary:
        Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> ---
> 
> Konrad, this fixes an actual bug, at least on OMAP5. It should have no
> bad side effects on any other platforms as far as I can tell. It should
> go in 4.5.
> 
> Changes in v2:
> 
> - add an in-code comment about maintenance_interrupt not being called.
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 70d10d6..c6c11d3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -403,6 +403,8 @@ void gic_clear_lrs(struct vcpu *v)
>      if ( is_idle_vcpu(v) )
>          return;
>  
> +    gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 0);
> +
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>  
>      while ((i = find_next_bit((const unsigned long *) &this_cpu(lr_mask),
> @@ -527,8 +529,6 @@ void gic_inject(void)
>  
>      if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
> -    else
> -        gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 0);
>  }
>  
>  static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
> @@ -598,6 +598,10 @@ static void maintenance_interrupt(int irq, void *dev_id, 
> struct cpu_user_regs *r
>       * Receiving the interrupt is going to cause gic_inject to be called
>       * on return to guest that is going to clear the old LRs and inject
>       * new interrupts.
> +     *
> +     * Do not add code here: maintenance interrupts caused by setting
> +     * GICH_HCR_UIE, might read as spurious interrupts (1023). As a
> +     * consequence sometimes this handler might not be called.
>       */
>  }
>  



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel