Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

Hi Marc,

On 14/09/2020 14:06, Marek Szyprowski wrote:
> Hi Marc,
> 
> On 01.09.2020 16:43, Marc Zyngier wrote:
>> Change the way we deal with GIC SGIs by turning them into proper
>> IRQs, and calling into the arch code to register the interrupt range
>> instead of a callback.
>>
>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> This patch landed in linux next-20200914 as commit ac063232d4b0 
> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it breaks 
> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM 
> Cortex A9 based). Here are the last lines from the bootlog:

I am observing the same thing on several Tegra boards (both arm and
arm64). Bisect is pointing to this commit. Reverting this alone does not
appear to be enough to fix the issue.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi Jon,

+Linus, who is facing a similar issue.

On 2020-09-16 15:16, Jon Hunter wrote:
> Hi Marc,
> 
> On 14/09/2020 14:06, Marek Szyprowski wrote:
>> Hi Marc,
>> 
>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>> Change the way we deal with GIC SGIs by turning them into proper
>>> IRQs, and calling into the arch code to register the interrupt range
>>> instead of a callback.
>>> 
>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> This patch landed in linux next-20200914 as commit ac063232d4b0
>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it 
>> breaks
>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>> Cortex A9 based). Here are the last lines from the bootlog:
> 
> I am observing the same thing on several Tegra boards (both arm and
> arm64). Bisect is pointing to this commit. Reverting this alone does 
> not
> appear to be enough to fix the issue.

Right, I am just massively by the GICv3 spec, and failed to remember
that ye olde GIC exposes the source CPU in AIR *and* wants it back, 
while
newer GICs deal with that transparently.

Can you try the patch below and let me know?

         M.

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 98743afdaea6..56492bf8b6f9 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -121,9 +121,10 @@ static struct gic_chip_data 
gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;

  static struct gic_kvm_info gic_v2_kvm_info;

+static DEFINE_PER_CPU(u32, sgi_intid);
+
  #ifdef CONFIG_GIC_NON_BANKED
  static DEFINE_STATIC_KEY_FALSE(frankengic_key);
-static DEFINE_PER_CPU(u32, sgi_intid);

  static void enable_frankengic(void)
  {
@@ -135,16 +136,6 @@ static inline bool is_frankengic(void)
  	return static_branch_unlikely(&frankengic_key);
  }

-static inline void set_sgi_intid(u32 intid)
-{
-	this_cpu_write(sgi_intid, intid);
-}
-
-static inline u32 get_sgi_intid(void)
-{
-	return this_cpu_read(sgi_intid);
-}
-
  static inline void __iomem *__get_base(union gic_base *base)
  {
  	if (is_frankengic())
@@ -160,8 +151,6 @@ static inline void __iomem *__get_base(union 
gic_base *base)
  #define gic_data_cpu_base(d)	((d)->cpu_base.common_base)
  #define enable_frankengic()	do { } while(0)
  #define is_frankengic()		false
-#define set_sgi_intid(i)	do { } while(0)
-#define get_sgi_intid()		0
  #endif

  static inline void __iomem *gic_dist_base(struct irq_data *d)
@@ -236,8 +225,8 @@ static void gic_eoi_irq(struct irq_data *d)
  {
  	u32 hwirq = gic_irq(d);

-	if (is_frankengic() && hwirq < 16)
-		hwirq = get_sgi_intid();
+	if (hwirq < 16)
+		hwirq = this_cpu_read(sgi_intid);

  	writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_EOI);
  }
@@ -365,14 +354,13 @@ static void __exception_irq_entry 
gic_handle_irq(struct pt_regs *regs)
  			smp_rmb();

  			/*
-			 * Samsung's funky GIC encodes the source CPU in
-			 * GICC_IAR, leading to the deactivation to fail if
-			 * not written back as is to GICC_EOI.  Stash the
-			 * INTID away for gic_eoi_irq() to write back.
-			 * This only works because we don't nest SGIs...
+			 * The GIC encodes the source CPU in GICC_IAR,
+			 * leading to the deactivation to fail if not
+			 * written back as is to GICC_EOI.  Stash the INTID
+			 * away for gic_eoi_irq() to write back.  This only
+			 * works because we don't nest SGIs...
  			 */
-			if (is_frankengic())
-				set_sgi_intid(irqstat);
+			this_cpu_write(sgi_intid, intid);
  		}

  		handle_domain_irq(gic->domain, irqnr, regs);

-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 16:10, Marc Zyngier wrote:
> Hi Jon,
> 
> +Linus, who is facing a similar issue.
> 
> On 2020-09-16 15:16, Jon Hunter wrote:
>> Hi Marc,
>>
>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>> Hi Marc,
>>>
>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>> IRQs, and calling into the arch code to register the interrupt range
>>>> instead of a callback.
>>>>
>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it breaks
>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>> Cortex A9 based). Here are the last lines from the bootlog:
>>
>> I am observing the same thing on several Tegra boards (both arm and
>> arm64). Bisect is pointing to this commit. Reverting this alone does not
>> appear to be enough to fix the issue.
> 
> Right, I am just massively by the GICv3 spec, and failed to remember
> that ye olde GIC exposes the source CPU in AIR *and* wants it back, while
> newer GICs deal with that transparently.
> 
> Can you try the patch below and let me know?

Yes will do.

> @@ -365,14 +354,13 @@ static void __exception_irq_entry
> gic_handle_irq(struct pt_regs *regs)
>              smp_rmb();
> 
>              /*
> -             * Samsung's funky GIC encodes the source CPU in
> -             * GICC_IAR, leading to the deactivation to fail if
> -             * not written back as is to GICC_EOI.  Stash the
> -             * INTID away for gic_eoi_irq() to write back.
> -             * This only works because we don't nest SGIs...
> +             * The GIC encodes the source CPU in GICC_IAR,
> +             * leading to the deactivation to fail if not
> +             * written back as is to GICC_EOI.  Stash the INTID
> +             * away for gic_eoi_irq() to write back.  This only
> +             * works because we don't nest SGIs...
>               */
> -            if (is_frankengic())
> -                set_sgi_intid(irqstat);
> +            this_cpu_write(sgi_intid, intid);

I assume that it should be irqstat here and not intid?

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-16 16:46, Jon Hunter wrote:
> On 16/09/2020 16:10, Marc Zyngier wrote:
>> Hi Jon,
>> 
>> +Linus, who is facing a similar issue.
>> 
>> On 2020-09-16 15:16, Jon Hunter wrote:
>>> Hi Marc,
>>> 
>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>> Hi Marc,
>>>> 
>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>> IRQs, and calling into the arch code to register the interrupt 
>>>>> range
>>>>> instead of a callback.
>>>>> 
>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it 
>>>> breaks
>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>> 
>>> I am observing the same thing on several Tegra boards (both arm and
>>> arm64). Bisect is pointing to this commit. Reverting this alone does 
>>> not
>>> appear to be enough to fix the issue.
>> 
>> Right, I am just massively by the GICv3 spec, and failed to remember
>> that ye olde GIC exposes the source CPU in AIR *and* wants it back, 
>> while
>> newer GICs deal with that transparently.
>> 
>> Can you try the patch below and let me know?
> 
> Yes will do.
> 
>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>> gic_handle_irq(struct pt_regs *regs)
>>              smp_rmb();
>> 
>>              /*
>> -             * Samsung's funky GIC encodes the source CPU in
>> -             * GICC_IAR, leading to the deactivation to fail if
>> -             * not written back as is to GICC_EOI.  Stash the
>> -             * INTID away for gic_eoi_irq() to write back.
>> -             * This only works because we don't nest SGIs...
>> +             * The GIC encodes the source CPU in GICC_IAR,
>> +             * leading to the deactivation to fail if not
>> +             * written back as is to GICC_EOI.  Stash the INTID
>> +             * away for gic_eoi_irq() to write back.  This only
>> +             * works because we don't nest SGIs...
>>               */
>> -            if (is_frankengic())
>> -                set_sgi_intid(irqstat);
>> +            this_cpu_write(sgi_intid, intid);
> 
> I assume that it should be irqstat here and not intid?

Indeed. As you can tell, I haven't even tried to compile it, sorry about 
that.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 16:55, Marc Zyngier wrote:
> On 2020-09-16 16:46, Jon Hunter wrote:
>> On 16/09/2020 16:10, Marc Zyngier wrote:
>>> Hi Jon,
>>>
>>> +Linus, who is facing a similar issue.
>>>
>>> On 2020-09-16 15:16, Jon Hunter wrote:
>>>> Hi Marc,
>>>>
>>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>>> Hi Marc,
>>>>>
>>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>>> IRQs, and calling into the arch code to register the interrupt range
>>>>>> instead of a callback.
>>>>>>
>>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it
>>>>> breaks
>>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>>>
>>>> I am observing the same thing on several Tegra boards (both arm and
>>>> arm64). Bisect is pointing to this commit. Reverting this alone does
>>>> not
>>>> appear to be enough to fix the issue.
>>>
>>> Right, I am just massively by the GICv3 spec, and failed to remember
>>> that ye olde GIC exposes the source CPU in AIR *and* wants it back,
>>> while
>>> newer GICs deal with that transparently.
>>>
>>> Can you try the patch below and let me know?
>>
>> Yes will do.
>>
>>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>>> gic_handle_irq(struct pt_regs *regs)
>>>              smp_rmb();
>>>
>>>              /*
>>> -             * Samsung's funky GIC encodes the source CPU in
>>> -             * GICC_IAR, leading to the deactivation to fail if
>>> -             * not written back as is to GICC_EOI.  Stash the
>>> -             * INTID away for gic_eoi_irq() to write back.
>>> -             * This only works because we don't nest SGIs...
>>> +             * The GIC encodes the source CPU in GICC_IAR,
>>> +             * leading to the deactivation to fail if not
>>> +             * written back as is to GICC_EOI.  Stash the INTID
>>> +             * away for gic_eoi_irq() to write back.  This only
>>> +             * works because we don't nest SGIs...
>>>               */
>>> -            if (is_frankengic())
>>> -                set_sgi_intid(irqstat);
>>> +            this_cpu_write(sgi_intid, intid);
>>
>> I assume that it should be irqstat here and not intid?
> 
> Indeed. As you can tell, I haven't even tried to compile it, sorry about
> that.

No worries, I got the gist. However, even with this change, it still
does not boot :-(

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-16 16:58, Jon Hunter wrote:
> On 16/09/2020 16:55, Marc Zyngier wrote:
>> On 2020-09-16 16:46, Jon Hunter wrote:
>>> On 16/09/2020 16:10, Marc Zyngier wrote:
>>>> Hi Jon,
>>>> 
>>>> +Linus, who is facing a similar issue.
>>>> 
>>>> On 2020-09-16 15:16, Jon Hunter wrote:
>>>>> Hi Marc,
>>>>> 
>>>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>>>> Hi Marc,
>>>>>> 
>>>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>>>> IRQs, and calling into the arch code to register the interrupt 
>>>>>>> range
>>>>>>> instead of a callback.
>>>>>>> 
>>>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it
>>>>>> breaks
>>>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad 
>>>>>> ARM
>>>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>>>> 
>>>>> I am observing the same thing on several Tegra boards (both arm and
>>>>> arm64). Bisect is pointing to this commit. Reverting this alone 
>>>>> does
>>>>> not
>>>>> appear to be enough to fix the issue.
>>>> 
>>>> Right, I am just massively by the GICv3 spec, and failed to remember
>>>> that ye olde GIC exposes the source CPU in AIR *and* wants it back,
>>>> while
>>>> newer GICs deal with that transparently.
>>>> 
>>>> Can you try the patch below and let me know?
>>> 
>>> Yes will do.
>>> 
>>>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>>>> gic_handle_irq(struct pt_regs *regs)
>>>>              smp_rmb();
>>>> 
>>>>              /*
>>>> -             * Samsung's funky GIC encodes the source CPU in
>>>> -             * GICC_IAR, leading to the deactivation to fail if
>>>> -             * not written back as is to GICC_EOI.  Stash the
>>>> -             * INTID away for gic_eoi_irq() to write back.
>>>> -             * This only works because we don't nest SGIs...
>>>> +             * The GIC encodes the source CPU in GICC_IAR,
>>>> +             * leading to the deactivation to fail if not
>>>> +             * written back as is to GICC_EOI.  Stash the INTID
>>>> +             * away for gic_eoi_irq() to write back.  This only
>>>> +             * works because we don't nest SGIs...
>>>>               */
>>>> -            if (is_frankengic())
>>>> -                set_sgi_intid(irqstat);
>>>> +            this_cpu_write(sgi_intid, intid);
>>> 
>>> I assume that it should be irqstat here and not intid?
>> 
>> Indeed. As you can tell, I haven't even tried to compile it, sorry 
>> about
>> that.
> 
> No worries, I got the gist. However, even with this change, it still
> does not boot :-(

Do you boot form EL2? If so, you'd also need this:

  static void gic_eoimode1_eoi_irq(struct irq_data *d)
  {
+	u32 hwirq = gic_irq(d);
+
  	/* Do not deactivate an IRQ forwarded to a vcpu. */
  	if (irqd_is_forwarded_to_vcpu(d))
  		return;

+	if (hwirq < 16)
+		hwirq = this_cpu_read(sgi_intid);
+
  	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
  }

If none of that works, we'll need some additional traces. On the other
hand, I just booted this on a GICv2-based system, and it worked fine...

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-16 17:22, Marc Zyngier wrote:
> On 2020-09-16 16:58, Jon Hunter wrote:
>> On 16/09/2020 16:55, Marc Zyngier wrote:
>>> On 2020-09-16 16:46, Jon Hunter wrote:
>>>> On 16/09/2020 16:10, Marc Zyngier wrote:
>>>>> Hi Jon,
>>>>> 
>>>>> +Linus, who is facing a similar issue.
>>>>> 
>>>>> On 2020-09-16 15:16, Jon Hunter wrote:
>>>>>> Hi Marc,
>>>>>> 
>>>>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>>>>> Hi Marc,
>>>>>>> 
>>>>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>>>>> IRQs, and calling into the arch code to register the interrupt 
>>>>>>>> range
>>>>>>>> instead of a callback.
>>>>>>>> 
>>>>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it
>>>>>>> breaks
>>>>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad 
>>>>>>> ARM
>>>>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>>>>> 
>>>>>> I am observing the same thing on several Tegra boards (both arm 
>>>>>> and
>>>>>> arm64). Bisect is pointing to this commit. Reverting this alone 
>>>>>> does
>>>>>> not
>>>>>> appear to be enough to fix the issue.
>>>>> 
>>>>> Right, I am just massively by the GICv3 spec, and failed to 
>>>>> remember
>>>>> that ye olde GIC exposes the source CPU in AIR *and* wants it back,
>>>>> while
>>>>> newer GICs deal with that transparently.
>>>>> 
>>>>> Can you try the patch below and let me know?
>>>> 
>>>> Yes will do.
>>>> 
>>>>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>>>>> gic_handle_irq(struct pt_regs *regs)
>>>>>              smp_rmb();
>>>>> 
>>>>>              /*
>>>>> -             * Samsung's funky GIC encodes the source CPU in
>>>>> -             * GICC_IAR, leading to the deactivation to fail if
>>>>> -             * not written back as is to GICC_EOI.  Stash the
>>>>> -             * INTID away for gic_eoi_irq() to write back.
>>>>> -             * This only works because we don't nest SGIs...
>>>>> +             * The GIC encodes the source CPU in GICC_IAR,
>>>>> +             * leading to the deactivation to fail if not
>>>>> +             * written back as is to GICC_EOI.  Stash the INTID
>>>>> +             * away for gic_eoi_irq() to write back.  This only
>>>>> +             * works because we don't nest SGIs...
>>>>>               */
>>>>> -            if (is_frankengic())
>>>>> -                set_sgi_intid(irqstat);
>>>>> +            this_cpu_write(sgi_intid, intid);
>>>> 
>>>> I assume that it should be irqstat here and not intid?
>>> 
>>> Indeed. As you can tell, I haven't even tried to compile it, sorry 
>>> about
>>> that.
>> 
>> No worries, I got the gist. However, even with this change, it still
>> does not boot :-(
> 
> Do you boot form EL2? If so, you'd also need this:
> 
>  static void gic_eoimode1_eoi_irq(struct irq_data *d)
>  {
> +	u32 hwirq = gic_irq(d);
> +
>  	/* Do not deactivate an IRQ forwarded to a vcpu. */
>  	if (irqd_is_forwarded_to_vcpu(d))
>  		return;
> 
> +	if (hwirq < 16)
> +		hwirq = this_cpu_read(sgi_intid);
> +
>  	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
>  }

Make it that instead:

  static void gic_eoimode1_eoi_irq(struct irq_data *d)
  {
+	u32 hwirq = gic_irq(d);
+
  	/* Do not deactivate an IRQ forwarded to a vcpu. */
  	if (irqd_is_forwarded_to_vcpu(d))
  		return;

-	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
+	if (hwirq < 16)
+		hwirq = this_cpu_read(sgi_intid);
+
+	writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
  }


         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 17:28, Marc Zyngier wrote:

...

> Make it that instead:
> 
>  static void gic_eoimode1_eoi_irq(struct irq_data *d)
>  {
> +    u32 hwirq = gic_irq(d);
> +
>      /* Do not deactivate an IRQ forwarded to a vcpu. */
>      if (irqd_is_forwarded_to_vcpu(d))
>          return;
> 
> -    writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
> +    if (hwirq < 16)
> +        hwirq = this_cpu_read(sgi_intid);
> +
> +    writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
>  }


Unfortunately, still does not boot :-(

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 17:22, Marc Zyngier wrote:
> Do you boot form EL2? 

Not that I am aware of. There is no hypervisor that we are using.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Mikko Perttunen]

Not sure which boards this issue is happening on, but looking at my 
hobby kernel's git history (from a couple of years ago, memory is a bit 
hazy), the commit labeled "Add support for TX2" adds code to drop from 
EL2 to EL1 at boot.

Mikko

On 9/16/20 10:06 PM, Jon Hunter wrote:
> On 16/09/2020 17:22, Marc Zyngier wrote:
>> Do you boot form EL2?
> 
> Not that I am aware of. There is no hypervisor that we are using.
> 
> Jon
> 

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 20:26, Mikko Perttunen wrote:
> Not sure which boards this issue is happening on, but looking at my
> hobby kernel's git history (from a couple of years ago, memory is a bit
> hazy), the commit labeled "Add support for TX2" adds code to drop from
> EL2 to EL1 at boot.

I am seeing boot issues on Tegra20, Tegra30, Tegra186 and Tegra194.
Interestingly, Tegra124 and Tegra210 are booting OK.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Linus Walleij]

On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:

> Can you try the patch below and let me know?

I tried this patch and now Ux500 WORKS. So this patch is definitely
something you should apply.

> -                       if (is_frankengic())
> -                               set_sgi_intid(irqstat);
> +                       this_cpu_write(sgi_intid, intid);

This needs changing to irqstat to compile as pointed out by Jon.

With that:
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi Linus,

On 2020-09-17 08:40, Linus Walleij wrote:
> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
> 
>> Can you try the patch below and let me know?
> 
> I tried this patch and now Ux500 WORKS. So this patch is definitely
> something you should apply.
> 
>> -                       if (is_frankengic())
>> -                               set_sgi_intid(irqstat);
>> +                       this_cpu_write(sgi_intid, intid);
> 
> This needs changing to irqstat to compile as pointed out by Jon.
> 
> With that:
> Tested-by: Linus Walleij <linus.walleij@linaro.org>

Thanks a lot for that.

Still need to understand why some of Jon's systems are left unbootable,
despite having similar GIC implementations (Tegra194 and Tegra210 use
the same GIC-400, and yet only one of the two boots correctly...).

Thanks again,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 08:50, Marc Zyngier wrote:
> Hi Linus,
> 
> On 2020-09-17 08:40, Linus Walleij wrote:
>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>
>>> Can you try the patch below and let me know?
>>
>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>> something you should apply.
>>
>>> -                       if (is_frankengic())
>>> -                               set_sgi_intid(irqstat);
>>> +                       this_cpu_write(sgi_intid, intid);
>>
>> This needs changing to irqstat to compile as pointed out by Jon.
>>
>> With that:
>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thanks a lot for that.
> 
> Still need to understand why some of Jon's systems are left unbootable,
> despite having similar GIC implementations (Tegra194 and Tegra210 use
> the same GIC-400, and yet only one of the two boots correctly...).

So far, I have only tested this patch on Tegra20. Let me try the other
failing boards this morning and see if those still fail.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 08:54, Jon Hunter wrote:
> On 17/09/2020 08:50, Marc Zyngier wrote:
>> Hi Linus,
>> 
>> On 2020-09-17 08:40, Linus Walleij wrote:
>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>> 
>>>> Can you try the patch below and let me know?
>>> 
>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>> something you should apply.
>>> 
>>>> -                       if (is_frankengic())
>>>> -                               set_sgi_intid(irqstat);
>>>> +                       this_cpu_write(sgi_intid, intid);
>>> 
>>> This needs changing to irqstat to compile as pointed out by Jon.
>>> 
>>> With that:
>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>> 
>> Thanks a lot for that.
>> 
>> Still need to understand why some of Jon's systems are left 
>> unbootable,
>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>> the same GIC-400, and yet only one of the two boots correctly...).
> 
> So far, I have only tested this patch on Tegra20. Let me try the other
> failing boards this morning and see if those still fail.

Tegra20 (if I remember well) is a dual A9 with the same GIC 
implementation
as Ux500, hence requiring the source CPU bits to be written back. So 
this
patch should have cured it, but didn't...

/me puzzled.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 09:45, Marc Zyngier wrote:
> On 2020-09-17 08:54, Jon Hunter wrote:
>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>> Hi Linus,
>>>
>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>
>>>>> Can you try the patch below and let me know?
>>>>
>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>> something you should apply.
>>>>
>>>>> -                       if (is_frankengic())
>>>>> -                               set_sgi_intid(irqstat);
>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>
>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>
>>>> With that:
>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> Thanks a lot for that.
>>>
>>> Still need to understand why some of Jon's systems are left unbootable,
>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>> the same GIC-400, and yet only one of the two boots correctly...).
>>
>> So far, I have only tested this patch on Tegra20. Let me try the other
>> failing boards this morning and see if those still fail.
> 
> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
> as Ux500, hence requiring the source CPU bits to be written back. So this
> patch should have cured it, but didn't...
> 
> /me puzzled.

Me too. Maybe there just happens to be something else also going wrong
in next. I am doing a bit more testing to see if applying the fix
directly on top of this change fixes it to try and eliminate anything
else in -next.

Linus, what -next are you testing on? I am using next-20200916.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Jon,

On 17.09.2020 10:49, Jon Hunter wrote:
> On 17/09/2020 09:45, Marc Zyngier wrote:
>> On 2020-09-17 08:54, Jon Hunter wrote:
>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>
>>>>>> Can you try the patch below and let me know?
>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>> something you should apply.
>>>>>
>>>>>> -                       if (is_frankengic())
>>>>>> -                               set_sgi_intid(irqstat);
>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>
>>>>> With that:
>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Thanks a lot for that.
>>>>
>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>> failing boards this morning and see if those still fail.
>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>> as Ux500, hence requiring the source CPU bits to be written back. So this
>> patch should have cured it, but didn't...
>>
>> /me puzzled.
> Me too. Maybe there just happens to be something else also going wrong
> in next. I am doing a bit more testing to see if applying the fix
> directly on top of this change fixes it to try and eliminate anything
> else in -next.
>
> Linus, what -next are you testing on? I am using next-20200916.

next-20200916 completely broken on ARM and ARM64. Please check 
next-20200915 + the mentioned fix or just check 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 09:54, Marek Szyprowski wrote:
> Hi Jon,
> 
> On 17.09.2020 10:49, Jon Hunter wrote:
>> On 17/09/2020 09:45, Marc Zyngier wrote:
>>> On 2020-09-17 08:54, Jon Hunter wrote:
>>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>>>> Can you try the patch below and let me know?
>>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>>> something you should apply.
>>>>>>
>>>>>>> -                       if (is_frankengic())
>>>>>>> -                               set_sgi_intid(irqstat);
>>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>>
>>>>>> With that:
>>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>>> Thanks a lot for that.
>>>>>
>>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>>> failing boards this morning and see if those still fail.
>>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>>> as Ux500, hence requiring the source CPU bits to be written back. So this
>>> patch should have cured it, but didn't...
>>>
>>> /me puzzled.
>> Me too. Maybe there just happens to be something else also going wrong
>> in next. I am doing a bit more testing to see if applying the fix
>> directly on top of this change fixes it to try and eliminate anything
>> else in -next.
>>
>> Linus, what -next are you testing on? I am using next-20200916.
> 
> next-20200916 completely broken on ARM and ARM64. Please check 
> next-20200915 + the mentioned fix or just check 
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq

Ah thanks! Any idea what is causing the other failure on next-20200916?

Yes we have noticed that now everything fails next-20200916 so not just
this issue.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Jon,

On 17.09.2020 11:09, Jon Hunter wrote:
> On 17/09/2020 09:54, Marek Szyprowski wrote:
>> On 17.09.2020 10:49, Jon Hunter wrote:
>>> On 17/09/2020 09:45, Marc Zyngier wrote:
>>>> On 2020-09-17 08:54, Jon Hunter wrote:
>>>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>>
>>>>>>>> Can you try the patch below and let me know?
>>>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>>>> something you should apply.
>>>>>>>
>>>>>>>> -                       if (is_frankengic())
>>>>>>>> -                               set_sgi_intid(irqstat);
>>>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>>>
>>>>>>> With that:
>>>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>> Thanks a lot for that.
>>>>>>
>>>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>>>> failing boards this morning and see if those still fail.
>>>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>>>> as Ux500, hence requiring the source CPU bits to be written back. So this
>>>> patch should have cured it, but didn't...
>>>>
>>>> /me puzzled.
>>> Me too. Maybe there just happens to be something else also going wrong
>>> in next. I am doing a bit more testing to see if applying the fix
>>> directly on top of this change fixes it to try and eliminate anything
>>> else in -next.
>>>
>>> Linus, what -next are you testing on? I am using next-20200916.
>> next-20200916 completely broken on ARM and ARM64. Please check
>> next-20200915 + the mentioned fix or just check
>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
> Ah thanks! Any idea what is causing the other failure on next-20200916?
>
> Yes we have noticed that now everything fails next-20200916 so not just
> this issue.

The issue is caused by commit c999bd436fe9 ("mm/cma: make number of CMA 
areas dynamic, remove CONFIG_CMA_AREAS")

https://lore.kernel.org/linux-arm-kernel/20200915205703.34572-1-mike.kravetz@oracle.com/

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 10:13, Marek Szyprowski wrote:

[...]

>>>> Linus, what -next are you testing on? I am using next-20200916.
>>> next-20200916 completely broken on ARM and ARM64. Please check
>>> next-20200915 + the mentioned fix or just check
>>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
>> Ah thanks! Any idea what is causing the other failure on 
>> next-20200916?
>> 
>> Yes we have noticed that now everything fails next-20200916 so not 
>> just
>> this issue.
> 
> The issue is caused by commit c999bd436fe9 ("mm/cma: make number of CMA
> areas dynamic, remove CONFIG_CMA_AREAS")
> 
> https://lore.kernel.org/linux-arm-kernel/20200915205703.34572-1-mike.kravetz@oracle.com/

There is a workaround here[1] for arm64, but I doubt that's the end of
it (32bit is still dead).

         M.

[1] 
https://lore.kernel.org/linux-arm-kernel/20200916085933.25220-1-song.bao.hua@hisilicon.com/
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 09:54, Marek Szyprowski wrote:
> Hi Jon,
> 
> On 17.09.2020 10:49, Jon Hunter wrote:
>> On 17/09/2020 09:45, Marc Zyngier wrote:
>>> On 2020-09-17 08:54, Jon Hunter wrote:
>>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>>>> Can you try the patch below and let me know?
>>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>>> something you should apply.
>>>>>>
>>>>>>> -                       if (is_frankengic())
>>>>>>> -                               set_sgi_intid(irqstat);
>>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>>
>>>>>> With that:
>>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>>> Thanks a lot for that.
>>>>>
>>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>>> failing boards this morning and see if those still fail.
>>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>>> as Ux500, hence requiring the source CPU bits to be written back. So this
>>> patch should have cured it, but didn't...
>>>
>>> /me puzzled.
>> Me too. Maybe there just happens to be something else also going wrong
>> in next. I am doing a bit more testing to see if applying the fix
>> directly on top of this change fixes it to try and eliminate anything
>> else in -next.
>>
>> Linus, what -next are you testing on? I am using next-20200916.
> 
> next-20200916 completely broken on ARM and ARM64. Please check 
> next-20200915 + the mentioned fix or just check 
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq

OK, I have confirmed that on Tegra20 and Tegra30, that next-20200915 +
Marc's fix boots fine.

Tegra186 and Tegra194 are not booting, but I am wondering if this is yet
another issue that is not related. I have not actually bisected on these
boards, but I am now bisecting on Tegra186 to confirm.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 15:53, Jon Hunter wrote:

...

>> next-20200916 completely broken on ARM and ARM64. Please check 
>> next-20200915 + the mentioned fix or just check 
>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
> 
> OK, I have confirmed that on Tegra20 and Tegra30, that next-20200915 +
> Marc's fix boots fine.
> 
> Tegra186 and Tegra194 are not booting, but I am wondering if this is yet
> another issue that is not related. I have not actually bisected on these
> boards, but I am now bisecting on Tegra186 to confirm.

Hmm ... well bisect on Tegra186 is also pointing to this commit, but the
fix is not working there. Wonder what's going on with this gic-400?

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 19:24, Jon Hunter wrote:
> On 17/09/2020 15:53, Jon Hunter wrote:
> 
> ...
> 
>>> next-20200916 completely broken on ARM and ARM64. Please check
>>> next-20200915 + the mentioned fix or just check
>>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
>> 
>> OK, I have confirmed that on Tegra20 and Tegra30, that next-20200915 +
>> Marc's fix boots fine.
>> 
>> Tegra186 and Tegra194 are not booting, but I am wondering if this is 
>> yet
>> another issue that is not related. I have not actually bisected on 
>> these
>> boards, but I am now bisecting on Tegra186 to confirm.
> 
> Hmm ... well bisect on Tegra186 is also pointing to this commit, but 
> the
> fix is not working there. Wonder what's going on with this gic-400?

It's not GIC400. I have tons of machines with GIC400 around me, and they
don't even need the fix. What happens if you only boot the non-Denver 
CPUs?

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 09:49, Jon Hunter wrote:
> On 17/09/2020 09:45, Marc Zyngier wrote:
>> On 2020-09-17 08:54, Jon Hunter wrote:

>>> So far, I have only tested this patch on Tegra20. Let me try the 
>>> other
>>> failing boards this morning and see if those still fail.
>> 
>> Tegra20 (if I remember well) is a dual A9 with the same GIC 
>> implementation
>> as Ux500, hence requiring the source CPU bits to be written back. So 
>> this
>> patch should have cured it, but didn't...
>> 
>> /me puzzled.
> 
> Me too. Maybe there just happens to be something else also going wrong
> in next. I am doing a bit more testing to see if applying the fix
> directly on top of this change fixes it to try and eliminate anything
> else in -next.
> 
> Linus, what -next are you testing on? I am using next-20200916.

You can directly try [1], which has all the queued fixes (and only 
that).

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Linus Walleij]

On Thu, Sep 17, 2020 at 10:49 AM Jon Hunter <jonathanh@nvidia.com> wrote:

> Linus, what -next are you testing on? I am using next-20200916.

That's what I use. But the Ux500 graphics are simple and does not
use CMA and that is why I don't see this crash (I assume).

Yours,
Linus Walleij