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

[Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>]

OK got it. Give me a few mins

On Tue, Nov 18, 2014 at 6:14 PM, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> 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
>>



-- 

Andrii Tseglytskyi | Embedded Dev
GlobalLogic
www.globallogic.com

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