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

[Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>]

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