Re: [Xen-devel] Xen 4.5 random freeze question

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

It is not the same: I would like to set GICH_V2_LR_MAINTENANCE_IRQ only
for non-hardware irqs (desc == NULL) and keep avoiding
GICH_V2_LR_MAINTENANCE_IRQ and setting GICH_LR_HW for hardware irqs.

Also testing on 394b7e587b05d0f4a5fd6f067b38339ab5a77121 would avoid
other potential bugs introduced later.

On Tue, 18 Nov 2014, Andrii Tseglytskyi wrote:
> What if I try on top of current master branch the following code:
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 31fb81a..6764ab7 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -36,6 +36,8 @@
>  #include <asm/io.h>
>  #include <asm/gic.h>
> 
> +#define GIC_DEBUG 1
> +
>  /*
>   * LR register definitions are GIC v2 specific.
>   * Moved these definitions from header file to here
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bcaded9..c03d6a6 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -41,7 +41,7 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
> 
>  #define lr_all_full() (this_cpu(lr_mask) == ((1 <<
> gic_hw_ops->info->nr_lrs) - 1))
> 
> -#undef GIC_DEBUG
> +#define GIC_DEBUG 1
> 
>  static void gic_update_one_lr(struct vcpu *v, int i);
> 
> It is equivalent to what you proposing - my code contains
> PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI, as result the following lone will
> be executed:
>  lr_reg |= GICH_V2_LR_MAINTENANCE_IRQ; inside gicv2_update_lr() function
> 
> regards,
> Andrii
> 
> On Tue, Nov 18, 2014 at 5:39 PM, Stefano Stabellini
> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > On Tue, 18 Nov 2014, Andrii Tseglytskyi wrote:
> >> OK, I see that GICH_V2_LR_MAINTENANCE_IRQ must always be set and
> >> everything works fine
> >> The following 2 patches fixes xen/master for my platform.
> >>
> >> Stefano, could you please take a look to these changes?
> >>
> >> commit 3628a0aa35706a8f532af865ed784536ce514eca
> >> Author: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
> >> Date:   Tue Nov 18 14:20:42 2014 +0200
> >>
> >>     xen/arm: dra7: always set GICH_V2_LR_MAINTENANCE_IRQ flag
> >>
> >>     Change-Id: Ia380b3507a182b11592588f65fd23693d4f87434
> >>     Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
> >>
> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> >> index 31fb81a..093ecdb 100644
> >> --- a/xen/arch/arm/gic-v2.c
> >> +++ b/xen/arch/arm/gic-v2.c
> >> @@ -396,13 +396,14 @@ static void gicv2_update_lr(int lr, const struct
> >> pending_irq *p,
> >>                                               << 
> >> GICH_V2_LR_PRIORITY_SHIFT) |
> >>                ((p->irq & GICH_V2_LR_VIRTUAL_MASK) <<
> >> GICH_V2_LR_VIRTUAL_SHIFT));
> >>
> >> -    if ( p->desc != NULL )
> >> +    if ( platform_has_quirk(PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI) )
> >>      {
> >> -        if ( platform_has_quirk(PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI) )
> >> -            lr_reg |= GICH_V2_LR_MAINTENANCE_IRQ;
> >> -        else
> >> -            lr_reg |= GICH_V2_LR_HW | ((p->desc->irq &
> >> GICH_V2_LR_PHYSICAL_MASK )
> >> -                            << GICH_V2_LR_PHYSICAL_SHIFT);
> >> +        lr_reg |= GICH_V2_LR_MAINTENANCE_IRQ;
> >> +    }
> >> +    else if ( p->desc != NULL )
> >> +    {
> >> +        lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & 
> >> GICH_V2_LR_PHYSICAL_MASK )
> >> +                       << GICH_V2_LR_PHYSICAL_SHIFT);
> >>      }
> >>
> >>      writel_gich(lr_reg, GICH_LR + lr * 4);
> >
> > Actually in case p->desc == NULL (the irq is not an hardware irq, it
> > could be the virtual timer irq or the evtchn irq), you shouldn't need
> > the maintenance interrupt, if the bug was really due to GICH_LR_HW not
> > working correctly on OMAP5. This changes might only be better at
> > "hiding" the real issue.
> >
> > Maybe the problem is exactly the opposite: the new scheme for avoiding
> > maintenance interrupts doesn't work for software interrupts.
> > The commit that should make them work correctly after the
> > no-maintenance-irq commit is 394b7e587b05d0f4a5fd6f067b38339ab5a77121
> > If you look at the changes to gic_update_one_lr in that commit, you'll
> > see that is going to set a software irq as PENDING if it is already ACTIVE.
> > Maybe that doesn't work correctly on OMAP5.
> >
> > Could you try this patch on top of
> > 394b7e587b05d0f4a5fd6f067b38339ab5a77121?  It should help us understand
> > if the problem is specifically with software irqs.
> >
> >
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index b7516c0..d8a17c9 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -66,7 +66,7 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
> >  /* Maximum cpu interface per GIC */
> >  #define NR_GIC_CPU_IF 8
> >
> > -#undef GIC_DEBUG
> > +#define GIC_DEBUG 1
> >
> >  static void gic_update_one_lr(struct vcpu *v, int i);
> >
> > @@ -563,6 +563,8 @@ static inline void gic_set_lr(int lr, struct 
> > pending_irq *p,
> >          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> >      if ( p->desc != NULL )
> >          lr_val |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
> > +    else
> > +        lr_val |= GICH_LR_MAINTENANCE_IRQ;
> >
> >      GICH[GICH_LR + lr] = lr_val;
> >
> 
> 
> 
> -- 
> 
> Andrii Tseglytskyi | Embedded Dev
> GlobalLogic
> www.globallogic.com
> 

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