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

[Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>]

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);

commit 110ad1914f04a5e52ec9d49a9aeb7df488f524b1
Author: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
Date:   Tue Nov 18 12:14:42 2014 +0200

    xen/arm: dra7: add PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI

    Change-Id: Ic6285d5aea803fb0bfef50ffcc35e20b5bfb7a77
    Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>

diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index 9d6e504..fb6686f 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -166,6 +166,11 @@ static const struct dt_device_match
dra7_blacklist_dev[] __initconst =
     { /* sentinel */ },
 };

+static uint32_t dra7_quirks(void)
+{
+    return PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
+}
+
 PLATFORM_START(omap5, "TI OMAP5")
     .compatible = omap5_dt_compat,
     .init_time = omap5_init_time,
@@ -186,6 +191,7 @@ PLATFORM_START(dra7, "TI DRA7")
     .dom0_gnttab_start = 0x4b000000,
     .dom0_gnttab_size = 0x20000,
     .blacklist_dev = dra7_blacklist_dev,
+    .quirks = dra7_quirks,
 PLATFORM_END

 /*

On Tue, Nov 18, 2014 at 1:31 PM, Andrii Tseglytskyi
<andrii.tseglytskyi@xxxxxxxxxxxxxxx> wrote:
> Strange - looks like baseline code already does the same, that you
> sent me yesterday. The only what is needed - is to set
> PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI.
> But baseline contains an issue. And in the same time changes on top of
> 5495a512b63bad868c147198f7f049c2617d468c work fine.
>
> Regards,
> Andrii
>
> On Tue, Nov 18, 2014 at 12:41 PM, Andrii Tseglytskyi
> <andrii.tseglytskyi@xxxxxxxxxxxxxxx> wrote:
>> Hi Stefano,
>>
>> On Mon, Nov 17, 2014 at 8:02 PM, Stefano Stabellini
>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>>> On Mon, 17 Nov 2014, Andrii Tseglytskyi wrote:
>>>> Hi Stefano,
>>>>
>>>> Thank you for your answer.
>>>>
>>>> On Mon, Nov 17, 2014 at 6:39 PM, Stefano Stabellini
>>>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>>>> > Although it is possible that that patch is the cause of your problem,
>>>> > unfortunately it is part of a significant rework of the GIC driver in
>>>> > Xen and I am afraid that testing with only a portion of that patch
>>>> > series might introduce other subtle bugs.  For your reference the series
>>>> > starts at commit 6f91502be64a05d0635454d629118b96ae38b50f and ends at
>>>> > commit 72eaf29e8d70784aaf066ead79df1295a25ecfd0.
>>>> >
>>>>
>>>> Yes, I tested with and without the whole series.
>>>
>>> And the result is that the series causes the problem?
>>>
>>
>> Yes.
>>
>>>
>>>> > If 5495a512b63bad868c147198f7f049c2617d468c is really the cause of your
>>>> > problem, one idea that comes to mind is that GICH_LR_MAINTENANCE_IRQ
>>>> > might not work correctly on your platform. It wouldn't be the first time
>>>> > that we see hardware behaving that way, especially if you are using the
>>>> > GIC secure registers instead of the non-secure register as GICH_LRn.HW
>>>> > can only deactivate non-secure interrupts. This is usually due to a
>>>> > configuration error in u-boot.
>>>> >
>>>> > Could you please try to set PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI for your
>>>> > platform?
>>>> >
>>>>
>>>> I tried this. Unfortunately it doesn't help.
>>>
>>> Could you try the following patch on top of
>>> 5495a512b63bad868c147198f7f049c2617d468c ?
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 302c031..a286376 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -557,10 +557,8 @@ static inline void gic_set_lr(int lr, struct 
>>> pending_irq *p,
>>>      BUG_ON(lr < 0);
>>>      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
>>>
>>> -    lr_val = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>>> +    lr_val = state | GICH_LR_MAINTENANCE_IRQ | ((p->priority >> 3) << 
>>> GICH_LR_PRIORITY_SHIFT) |
>>>          ((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);
>>>
>>>      GICH[GICH_LR + lr] = lr_val;
>>>
>>> @@ -622,6 +620,12 @@ out:
>>>      return;
>>>  }
>>>
>>> +static void gic_irq_eoi(void *info)
>>> +{
>>> +    int virq = (uintptr_t) info;
>>> +    GICC[GICC_DIR] = virq;
>>> +}
>>> +
>>>  static void gic_update_one_lr(struct vcpu *v, int i)
>>>  {
>>>      struct pending_irq *p;
>>> @@ -639,7 +643,10 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>>          irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
>>>          p = irq_to_pending(v, irq);
>>>          if ( p->desc != NULL )
>>> +        {
>>> +            gic_irq_eoi((void*)(uintptr_t)irq);
>>>              p->desc->status &= ~IRQ_INPROGRESS;
>>> +        }
>>>          clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
>>>          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
>>>                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
>>
>>
>> It helps! Thank you a lot!
>> I did about ~30 reboots and got no hangs. The only what is needed - is
>> to rebase these changes on top of xen/master branch.
>> Changes in patch can be applied only on top of
>> 5495a512b63bad868c147198f7f049c2617d468c
>> Will you do this change? Is it acceptable for baseline?
>>
>> Regards,
>> Andrii
>>
>> --
>>
>> Andrii Tseglytskyi | Embedded Dev
>> GlobalLogic
>> www.globallogic.com
>
>
>
> --
>
> 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