[RFC Patch v1 0/4] arm64: Introduce new IPI as IPI_CALL_NMI_FUNC

[RFC Patch v1 0/4] arm64: Introduce new IPI as IPI_CALL_NMI_FUNC

[Sumit Garg]

With pseudo NMIs support available its possible to configure SGIs to be
triggered as pseudo NMIs running in NMI context. And kernel features
such as kgdb relies on NMI support to round up CPUs which are stuck in
hard lockup state with interrupts disabled.

This patch-set adds support for IPI_CALL_NMI_FUNC which can be triggered
as a pseudo NMI which in turn is leveraged via kgdb to round up CPUs.

After this patch-set we should be able to get a backtrace for a CPU
stuck in HARDLOCKUP. Have a look at an example below from a testcase run
on Developerbox:

$ echo HARDLOCKUP > /sys/kernel/debug/provoke-crash/DIRECT

# Enter kdb via Magic SysRq

[11]kdb> btc
btc: cpu status: Currently on cpu 11
Available cpus: 0-10(I), 11, 12(I), 13, 14-23(I)
<snip>
Stack traceback for pid 623
0xffff00086a644600      623      622  1   13   R  0xffff00086a644fc0  bash
CPU: 13 PID: 623 Comm: bash Not tainted 5.7.0-rc2 #27
Hardware name: Socionext SynQuacer E-series DeveloperBox, BIOS build #73 Apr  6 2020
Call trace:
 dump_backtrace+0x0/0x198
 show_stack+0x18/0x28
 dump_stack+0xb8/0x100
 kgdb_cpu_enter+0x5c0/0x5f8
 kgdb_nmicallback+0xa0/0xa8
 handle_IPI+0x190/0x200
 gic_handle_irq+0x2b8/0x2d8
 el1_irq+0xcc/0x180
 lkdtm_HARDLOCKUP+0x8/0x18
 direct_entry+0x124/0x1c0
 full_proxy_write+0x60/0xb0
 __vfs_write+0x1c/0x48
 vfs_write+0xe4/0x1d0
 ksys_write+0x6c/0xf8
 __arm64_sys_write+0x1c/0x28
 el0_svc_common.constprop.0+0x74/0x1f0
 do_el0_svc+0x24/0x90
 el0_sync_handler+0x178/0x2b8
 el0_sync+0x158/0x180
<snip>

Looking forward to your comments/feedback.

Sumit Garg (4):
  arm64: smp: Introduce a new IPI as IPI_CALL_NMI_FUNC
  irqchip/gic-v3: Add support to handle SGI as pseudo NMI
  irqchip/gic-v3: Enable arch specific IPI as pseudo NMI
  arm64: kgdb: Round up cpus using IPI_CALL_NMI_FUNC

 arch/arm64/include/asm/hardirq.h |  2 +-
 arch/arm64/include/asm/smp.h     |  1 +
 arch/arm64/kernel/kgdb.c         | 15 +++++++++++++++
 arch/arm64/kernel/smp.c          | 36 +++++++++++++++++++++++++++++++++++-
 drivers/irqchip/irq-gic-v3.c     | 36 +++++++++++++++++++++++++++++++-----
 5 files changed, 83 insertions(+), 7 deletions(-)

-- 
2.7.4

[RFC Patch v1 1/4] arm64: smp: Introduce a new IPI as IPI_CALL_NMI_FUNC

[Sumit Garg]

Introduce a new inter processor interrupt as IPI_CALL_NMI_FUNC that
can be invoked to run special handlers in NMI context. One such handler
example is kgdb_nmicallback() which is invoked in order to round up CPUs
to enter kgdb context.

As currently pseudo NMIs are supported on specific arm64 platforms which
incorporates GICv3 or later version of interrupt controller. In case a
particular platform doesn't support pseudo NMIs, IPI_CALL_NMI_FUNC will
act as a normal IPI which can still be used to invoke special handlers.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 arch/arm64/include/asm/hardirq.h |  2 +-
 arch/arm64/include/asm/smp.h     |  1 +
 arch/arm64/kernel/smp.c          | 20 +++++++++++++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
index 87ad961..abaa23a 100644
--- a/arch/arm64/include/asm/hardirq.h
+++ b/arch/arm64/include/asm/hardirq.h
@@ -13,7 +13,7 @@
 #include <asm/kvm_arm.h>
 #include <asm/sysreg.h>
 
-#define NR_IPI	7
+#define NR_IPI	8
 
 typedef struct {
 	unsigned int __softirq_pending;
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index 40d5ba0..cc32776 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -108,6 +108,7 @@ extern void secondary_entry(void);
 
 extern void arch_send_call_function_single_ipi(int cpu);
 extern void arch_send_call_function_ipi_mask(const struct cpumask *mask);
+extern void arch_send_call_nmi_func_ipi_mask(const struct cpumask *mask);
 
 #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
 extern void arch_send_wakeup_ipi_mask(const struct cpumask *mask);
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 061f60f..42fe7bb 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -74,7 +74,8 @@ enum ipi_msg_type {
 	IPI_CPU_CRASH_STOP,
 	IPI_TIMER,
 	IPI_IRQ_WORK,
-	IPI_WAKEUP
+	IPI_WAKEUP,
+	IPI_CALL_NMI_FUNC
 };
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -798,6 +799,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
 	S(IPI_TIMER, "Timer broadcast interrupts"),
 	S(IPI_IRQ_WORK, "IRQ work interrupts"),
 	S(IPI_WAKEUP, "CPU wake-up interrupts"),
+	S(IPI_CALL_NMI_FUNC, "NMI function call interrupts"),
 };
 
 static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
@@ -856,6 +858,11 @@ void arch_irq_work_raise(void)
 }
 #endif
 
+void arch_send_call_nmi_func_ipi_mask(const struct cpumask *mask)
+{
+	smp_cross_call(mask, IPI_CALL_NMI_FUNC);
+}
+
 static void local_cpu_stop(void)
 {
 	set_cpu_online(smp_processor_id(), false);
@@ -960,6 +967,17 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 		break;
 #endif
 
+	case IPI_CALL_NMI_FUNC:
+		/* Handle it as a normal interrupt if not in NMI context */
+		if (!in_nmi())
+			irq_enter();
+
+		/* nop, IPI handlers for special features can be added here. */
+
+		if (!in_nmi())
+			irq_exit();
+		break;
+
 	default:
 		pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
 		break;
-- 
2.7.4

[RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

With pseudo NMIs enabled, interrupt controller can be configured to
deliver SGI as a pseudo NMI. So add corresponding handling for SGIs.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/irqchip/irq-gic-v3.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d7006ef..be361bf 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -609,17 +609,29 @@ static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
 	if (irqs_enabled)
 		nmi_enter();
 
-	if (static_branch_likely(&supports_deactivate_key))
-		gic_write_eoir(irqnr);
 	/*
 	 * Leave the PSR.I bit set to prevent other NMIs to be
 	 * received while handling this one.
 	 * PSR.I will be restored when we ERET to the
 	 * interrupted context.
 	 */
-	err = handle_domain_nmi(gic_data.domain, irqnr, regs);
-	if (err)
-		gic_deactivate_unhandled(irqnr);
+	if (likely(irqnr > 15)) {
+		if (static_branch_likely(&supports_deactivate_key))
+			gic_write_eoir(irqnr);
+
+		err = handle_domain_nmi(gic_data.domain, irqnr, regs);
+		if (err)
+			gic_deactivate_unhandled(irqnr);
+	} else {
+		gic_write_eoir(irqnr);
+		if (static_branch_likely(&supports_deactivate_key))
+			gic_write_dir(irqnr);
+#ifdef CONFIG_SMP
+		handle_IPI(irqnr, regs);
+#else
+		WARN_ONCE(true, "Unexpected SGI received!\n");
+#endif
+	}
 
 	if (irqs_enabled)
 		nmi_exit();
-- 
2.7.4

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Marc Zyngier]

On Fri, 24 Apr 2020 16:39:12 +0530
Sumit Garg <sumit.garg@linaro.org> wrote:

Hi Sumit,

> With pseudo NMIs enabled, interrupt controller can be configured to
> deliver SGI as a pseudo NMI. So add corresponding handling for SGIs.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index d7006ef..be361bf 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -609,17 +609,29 @@ static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
>  	if (irqs_enabled)
>  		nmi_enter();
>  
> -	if (static_branch_likely(&supports_deactivate_key))
> -		gic_write_eoir(irqnr);
>  	/*
>  	 * Leave the PSR.I bit set to prevent other NMIs to be
>  	 * received while handling this one.
>  	 * PSR.I will be restored when we ERET to the
>  	 * interrupted context.
>  	 */
> -	err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> -	if (err)
> -		gic_deactivate_unhandled(irqnr);
> +	if (likely(irqnr > 15)) {
> +		if (static_branch_likely(&supports_deactivate_key))
> +			gic_write_eoir(irqnr);
> +
> +		err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> +		if (err)
> +			gic_deactivate_unhandled(irqnr);
> +	} else {
> +		gic_write_eoir(irqnr);
> +		if (static_branch_likely(&supports_deactivate_key))
> +			gic_write_dir(irqnr);
> +#ifdef CONFIG_SMP
> +		handle_IPI(irqnr, regs);
> +#else
> +		WARN_ONCE(true, "Unexpected SGI received!\n");
> +#endif
> +	}
>  
>  	if (irqs_enabled)
>  		nmi_exit();

If there is one thing I would like to avoid, it is to add more ugly
hacks to the way we handle SGIs. There is very little reason why SGIs
should be handled differently from all other interrupts. They have the
same properties, and it is only because of the 32bit legacy that we deal
with them in such a cumbersome way. Nothing that we cannot fix though.

What I would really like to see is first a conversion of the SGIs to
normal, full fat interrupts. These interrupts can then be configured as
NMI using the normal API.

I think Julien had something along these lines (or was that limited to
the PMU?). Otherwise, I'll happily help you with that.

Thanks,

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

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Marc Zyngier]

On 2020-04-25 11:29, Marc Zyngier wrote:
> On Fri, 24 Apr 2020 16:39:12 +0530
> Sumit Garg <sumit.garg@linaro.org> wrote:
> 
> Hi Sumit,
> 
>> With pseudo NMIs enabled, interrupt controller can be configured to
>> deliver SGI as a pseudo NMI. So add corresponding handling for SGIs.
>> 
>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/irqchip/irq-gic-v3.c 
>> b/drivers/irqchip/irq-gic-v3.c
>> index d7006ef..be361bf 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -609,17 +609,29 @@ static inline void gic_handle_nmi(u32 irqnr, 
>> struct pt_regs *regs)
>>  	if (irqs_enabled)
>>  		nmi_enter();
>> 
>> -	if (static_branch_likely(&supports_deactivate_key))
>> -		gic_write_eoir(irqnr);
>>  	/*
>>  	 * Leave the PSR.I bit set to prevent other NMIs to be
>>  	 * received while handling this one.
>>  	 * PSR.I will be restored when we ERET to the
>>  	 * interrupted context.
>>  	 */
>> -	err = handle_domain_nmi(gic_data.domain, irqnr, regs);
>> -	if (err)
>> -		gic_deactivate_unhandled(irqnr);
>> +	if (likely(irqnr > 15)) {
>> +		if (static_branch_likely(&supports_deactivate_key))
>> +			gic_write_eoir(irqnr);
>> +
>> +		err = handle_domain_nmi(gic_data.domain, irqnr, regs);
>> +		if (err)
>> +			gic_deactivate_unhandled(irqnr);
>> +	} else {
>> +		gic_write_eoir(irqnr);
>> +		if (static_branch_likely(&supports_deactivate_key))
>> +			gic_write_dir(irqnr);
>> +#ifdef CONFIG_SMP
>> +		handle_IPI(irqnr, regs);
>> +#else
>> +		WARN_ONCE(true, "Unexpected SGI received!\n");
>> +#endif
>> +	}
>> 
>>  	if (irqs_enabled)
>>  		nmi_exit();
> 
> If there is one thing I would like to avoid, it is to add more ugly
> hacks to the way we handle SGIs. There is very little reason why SGIs
> should be handled differently from all other interrupts. They have the
> same properties, and it is only because of the 32bit legacy that we 
> deal
> with them in such a cumbersome way. Nothing that we cannot fix though.
> 
> What I would really like to see is first a conversion of the SGIs to
> normal, full fat interrupts. These interrupts can then be configured as
> NMI using the normal API.
> 
> I think Julien had something along these lines (or was that limited to
> the PMU?). Otherwise, I'll happily help you with that.

OK, to give you an idea of what I am after, here's a small series[1] 
that
can be used as a base (it has been booted exactly *once* on a model, and
is thus absolutely perfect ;-).

There is still a bit of work to be able to actually request a SGI (they
are hard-wired as chained interrupts so far, as this otherwise changes
the output of /proc/interrupts, among other things), but you will
hopefully see what I'm aiming for.

Thanks,

         M.

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

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

Hi Marc,

Thanks for your comments and apologies for my delayed response as I
was exploring ideas that you have shared.

On Sat, 25 Apr 2020 at 20:02, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-04-25 11:29, Marc Zyngier wrote:
> > On Fri, 24 Apr 2020 16:39:12 +0530
> > Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Sumit,
> >
> >> With pseudo NMIs enabled, interrupt controller can be configured to
> >> deliver SGI as a pseudo NMI. So add corresponding handling for SGIs.
> >>
> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >> ---
> >>  drivers/irqchip/irq-gic-v3.c | 22 +++++++++++++++++-----
> >>  1 file changed, 17 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3.c
> >> b/drivers/irqchip/irq-gic-v3.c
> >> index d7006ef..be361bf 100644
> >> --- a/drivers/irqchip/irq-gic-v3.c
> >> +++ b/drivers/irqchip/irq-gic-v3.c
> >> @@ -609,17 +609,29 @@ static inline void gic_handle_nmi(u32 irqnr,
> >> struct pt_regs *regs)
> >>      if (irqs_enabled)
> >>              nmi_enter();
> >>
> >> -    if (static_branch_likely(&supports_deactivate_key))
> >> -            gic_write_eoir(irqnr);
> >>      /*
> >>       * Leave the PSR.I bit set to prevent other NMIs to be
> >>       * received while handling this one.
> >>       * PSR.I will be restored when we ERET to the
> >>       * interrupted context.
> >>       */
> >> -    err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> >> -    if (err)
> >> -            gic_deactivate_unhandled(irqnr);
> >> +    if (likely(irqnr > 15)) {
> >> +            if (static_branch_likely(&supports_deactivate_key))
> >> +                    gic_write_eoir(irqnr);
> >> +
> >> +            err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> >> +            if (err)
> >> +                    gic_deactivate_unhandled(irqnr);
> >> +    } else {
> >> +            gic_write_eoir(irqnr);
> >> +            if (static_branch_likely(&supports_deactivate_key))
> >> +                    gic_write_dir(irqnr);
> >> +#ifdef CONFIG_SMP
> >> +            handle_IPI(irqnr, regs);
> >> +#else
> >> +            WARN_ONCE(true, "Unexpected SGI received!\n");
> >> +#endif
> >> +    }
> >>
> >>      if (irqs_enabled)
> >>              nmi_exit();
> >
> > If there is one thing I would like to avoid, it is to add more ugly
> > hacks to the way we handle SGIs. There is very little reason why SGIs
> > should be handled differently from all other interrupts. They have the
> > same properties, and it is only because of the 32bit legacy that we
> > deal
> > with them in such a cumbersome way. Nothing that we cannot fix though.
> >
> > What I would really like to see is first a conversion of the SGIs to
> > normal, full fat interrupts. These interrupts can then be configured as
> > NMI using the normal API.
> >
> > I think Julien had something along these lines (or was that limited to
> > the PMU?). Otherwise, I'll happily help you with that.
>
> OK, to give you an idea of what I am after, here's a small series[1]
> that
> can be used as a base (it has been booted exactly *once* on a model, and
> is thus absolutely perfect ;-).

Thanks for this series. I have re-based my patch-set on top of this
series [1] and just dropped this patch #2. It works fine for me.

>
> There is still a bit of work to be able to actually request a SGI (they
> are hard-wired as chained interrupts so far, as this otherwise changes
> the output of /proc/interrupts, among other things), but you will
> hopefully see what I'm aiming for.

I was exploring this idea: "request a SGI". I guess here you meant to
request a new SGI as a normal NMI/IRQ via common APIs such as
request_percpu_nmi() or request_percpu_irq() rather than statically
adding a new IPI as per this patch [2], correct? If yes, then I have
following follow up queries:

1. Do you envision any drivers to use SGIs in a similar manner as they
use SPIs or PPIs?
2. How do you envision allocation of SGIs as currently they are
hardcoded in an arch specific file (like arch/arm64/kernel/smp.c
+794)?
3. AFAIK, the major difference among SGIs and SPIs or PPIs is the
trigger method where SGIs are software triggered and SPIs or PPIs are
hardware triggered. And I couldn't find a generalized method across
architectures to invoke SGIs. So how do you envision drivers to invoke
SGIs in an architecture agnostic manner?

[1] https://git.linaro.org/people/sumit.garg/linux.git/?h=kgdb-nmi
[2] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=fc89e5f395f89966110554a15ab0fa0f0d471132

-Sumit

>
> Thanks,
>
>          M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-sgi
> --
> Jazz is not dead. It just smells funny...

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Marc Zyngier]

Hi Sumit,

On 2020-04-28 15:11, Sumit Garg wrote:
> Hi Marc,
> 
> Thanks for your comments and apologies for my delayed response as I
> was exploring ideas that you have shared.
> 
> On Sat, 25 Apr 2020 at 20:02, Marc Zyngier <maz@kernel.org> wrote:
>> 
>> On 2020-04-25 11:29, Marc Zyngier wrote:
>> > On Fri, 24 Apr 2020 16:39:12 +0530
>> > Sumit Garg <sumit.garg@linaro.org> wrote:
>> >
>> > Hi Sumit,
>> >
>> >> With pseudo NMIs enabled, interrupt controller can be configured to
>> >> deliver SGI as a pseudo NMI. So add corresponding handling for SGIs.
>> >>
>> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>> >> ---
>> >>  drivers/irqchip/irq-gic-v3.c | 22 +++++++++++++++++-----
>> >>  1 file changed, 17 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/irqchip/irq-gic-v3.c
>> >> b/drivers/irqchip/irq-gic-v3.c
>> >> index d7006ef..be361bf 100644
>> >> --- a/drivers/irqchip/irq-gic-v3.c
>> >> +++ b/drivers/irqchip/irq-gic-v3.c
>> >> @@ -609,17 +609,29 @@ static inline void gic_handle_nmi(u32 irqnr,
>> >> struct pt_regs *regs)
>> >>      if (irqs_enabled)
>> >>              nmi_enter();
>> >>
>> >> -    if (static_branch_likely(&supports_deactivate_key))
>> >> -            gic_write_eoir(irqnr);
>> >>      /*
>> >>       * Leave the PSR.I bit set to prevent other NMIs to be
>> >>       * received while handling this one.
>> >>       * PSR.I will be restored when we ERET to the
>> >>       * interrupted context.
>> >>       */
>> >> -    err = handle_domain_nmi(gic_data.domain, irqnr, regs);
>> >> -    if (err)
>> >> -            gic_deactivate_unhandled(irqnr);
>> >> +    if (likely(irqnr > 15)) {
>> >> +            if (static_branch_likely(&supports_deactivate_key))
>> >> +                    gic_write_eoir(irqnr);
>> >> +
>> >> +            err = handle_domain_nmi(gic_data.domain, irqnr, regs);
>> >> +            if (err)
>> >> +                    gic_deactivate_unhandled(irqnr);
>> >> +    } else {
>> >> +            gic_write_eoir(irqnr);
>> >> +            if (static_branch_likely(&supports_deactivate_key))
>> >> +                    gic_write_dir(irqnr);
>> >> +#ifdef CONFIG_SMP
>> >> +            handle_IPI(irqnr, regs);
>> >> +#else
>> >> +            WARN_ONCE(true, "Unexpected SGI received!\n");
>> >> +#endif
>> >> +    }
>> >>
>> >>      if (irqs_enabled)
>> >>              nmi_exit();
>> >
>> > If there is one thing I would like to avoid, it is to add more ugly
>> > hacks to the way we handle SGIs. There is very little reason why SGIs
>> > should be handled differently from all other interrupts. They have the
>> > same properties, and it is only because of the 32bit legacy that we
>> > deal
>> > with them in such a cumbersome way. Nothing that we cannot fix though.
>> >
>> > What I would really like to see is first a conversion of the SGIs to
>> > normal, full fat interrupts. These interrupts can then be configured as
>> > NMI using the normal API.
>> >
>> > I think Julien had something along these lines (or was that limited to
>> > the PMU?). Otherwise, I'll happily help you with that.
>> 
>> OK, to give you an idea of what I am after, here's a small series[1]
>> that
>> can be used as a base (it has been booted exactly *once* on a model, 
>> and
>> is thus absolutely perfect ;-).
> 
> Thanks for this series. I have re-based my patch-set on top of this
> series [1] and just dropped this patch #2. It works fine for me.

I just had a look.

"irqchip/gic-v3: Enable arch specific IPI as pseudo NMI" is still done
the wrong way, I'm afraid. You directly poke into the GIC configuration,
which isn't acceptable, as you leave the rest of the kernel completely
unaware that this is a NMI. You should make the interrupt a NMI as it
is being configured in gic_smp_init(), calling request_nmi() at this 
stage.

>> 
>> There is still a bit of work to be able to actually request a SGI 
>> (they
>> are hard-wired as chained interrupts so far, as this otherwise changes
>> the output of /proc/interrupts, among other things), but you will
>> hopefully see what I'm aiming for.
> 
> I was exploring this idea: "request a SGI". I guess here you meant to
> request a new SGI as a normal NMI/IRQ via common APIs such as
> request_percpu_nmi() or request_percpu_irq() rather than statically
> adding a new IPI as per this patch [2], correct? If yes, then I have
> following follow up queries:
> 
> 1. Do you envision any drivers to use SGIs in a similar manner as they
> use SPIs or PPIs?

No. SGIs are already pretty much all allocated for the kernel's internal
needs and once we allocate an additional one for this KGDB feature,
we're out of non-secure SGIs. We could start a multiplexing scheme to
overcome this, but the kernel already has plenty of other mechanisms
for internal communication. After all, why would you need anything more
than smp_call_function()?

The single use case I can imagine is that you'd want to signal a CPU
that isn't running Linux. This would require a lot more work than
just an interrupt, and is out of scope for the time being.

> 2. How do you envision allocation of SGIs as currently they are
> hardcoded in an arch specific file (like arch/arm64/kernel/smp.c
> +794)?

What I would like is for the arch code to request these interrupts as
normal interrupts, for which there is one problem to solve: how do you
find out about the Linux IRQ number for a given IPI. Or rather, how
do you get rid of the requirement to have IPI numbers at all and just
say "give me a per-cpu interrupt that I can use as an IPI, and by the
way here's the handler you can call".

And I insist: this is only for the arch code. Nothing else.

> 3. AFAIK, the major difference among SGIs and SPIs or PPIs is the
> trigger method where SGIs are software triggered and SPIs or PPIs are
> hardware triggered. And I couldn't find a generalized method across
> architectures to invoke SGIs. So how do you envision drivers to invoke
> SGIs in an architecture agnostic manner?

Well, SGIs are not architecture agnostic. They are fundamentally part of
the GIC architecture, which only exists for the two ARM architectures.

SGIs are not a general purpose mechanism. IPIs are, and we have services
on top of IPIs, such as invoking a function on a remote CPU. What else
do you need?

Thanks,

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

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

Hi Marc,

On Wed, 29 Apr 2020 at 13:53, Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Sumit,
>
> On 2020-04-28 15:11, Sumit Garg wrote:
> > Hi Marc,
> >
> > Thanks for your comments and apologies for my delayed response as I
> > was exploring ideas that you have shared.
> >
> > On Sat, 25 Apr 2020 at 20:02, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On 2020-04-25 11:29, Marc Zyngier wrote:
> >> > On Fri, 24 Apr 2020 16:39:12 +0530
> >> > Sumit Garg <sumit.garg@linaro.org> wrote:
> >> >
> >> > Hi Sumit,
> >> >
> >> >> With pseudo NMIs enabled, interrupt controller can be configured to
> >> >> deliver SGI as a pseudo NMI. So add corresponding handling for SGIs.
> >> >>
> >> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >> >> ---
> >> >>  drivers/irqchip/irq-gic-v3.c | 22 +++++++++++++++++-----
> >> >>  1 file changed, 17 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/drivers/irqchip/irq-gic-v3.c
> >> >> b/drivers/irqchip/irq-gic-v3.c
> >> >> index d7006ef..be361bf 100644
> >> >> --- a/drivers/irqchip/irq-gic-v3.c
> >> >> +++ b/drivers/irqchip/irq-gic-v3.c
> >> >> @@ -609,17 +609,29 @@ static inline void gic_handle_nmi(u32 irqnr,
> >> >> struct pt_regs *regs)
> >> >>      if (irqs_enabled)
> >> >>              nmi_enter();
> >> >>
> >> >> -    if (static_branch_likely(&supports_deactivate_key))
> >> >> -            gic_write_eoir(irqnr);
> >> >>      /*
> >> >>       * Leave the PSR.I bit set to prevent other NMIs to be
> >> >>       * received while handling this one.
> >> >>       * PSR.I will be restored when we ERET to the
> >> >>       * interrupted context.
> >> >>       */
> >> >> -    err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> >> >> -    if (err)
> >> >> -            gic_deactivate_unhandled(irqnr);
> >> >> +    if (likely(irqnr > 15)) {
> >> >> +            if (static_branch_likely(&supports_deactivate_key))
> >> >> +                    gic_write_eoir(irqnr);
> >> >> +
> >> >> +            err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> >> >> +            if (err)
> >> >> +                    gic_deactivate_unhandled(irqnr);
> >> >> +    } else {
> >> >> +            gic_write_eoir(irqnr);
> >> >> +            if (static_branch_likely(&supports_deactivate_key))
> >> >> +                    gic_write_dir(irqnr);
> >> >> +#ifdef CONFIG_SMP
> >> >> +            handle_IPI(irqnr, regs);
> >> >> +#else
> >> >> +            WARN_ONCE(true, "Unexpected SGI received!\n");
> >> >> +#endif
> >> >> +    }
> >> >>
> >> >>      if (irqs_enabled)
> >> >>              nmi_exit();
> >> >
> >> > If there is one thing I would like to avoid, it is to add more ugly
> >> > hacks to the way we handle SGIs. There is very little reason why SGIs
> >> > should be handled differently from all other interrupts. They have the
> >> > same properties, and it is only because of the 32bit legacy that we
> >> > deal
> >> > with them in such a cumbersome way. Nothing that we cannot fix though.
> >> >
> >> > What I would really like to see is first a conversion of the SGIs to
> >> > normal, full fat interrupts. These interrupts can then be configured as
> >> > NMI using the normal API.
> >> >
> >> > I think Julien had something along these lines (or was that limited to
> >> > the PMU?). Otherwise, I'll happily help you with that.
> >>
> >> OK, to give you an idea of what I am after, here's a small series[1]
> >> that
> >> can be used as a base (it has been booted exactly *once* on a model,
> >> and
> >> is thus absolutely perfect ;-).
> >
> > Thanks for this series. I have re-based my patch-set on top of this
> > series [1] and just dropped this patch #2. It works fine for me.
>
> I just had a look.
>
> "irqchip/gic-v3: Enable arch specific IPI as pseudo NMI" is still done
> the wrong way, I'm afraid. You directly poke into the GIC configuration,
> which isn't acceptable, as you leave the rest of the kernel completely
> unaware that this is a NMI. You should make the interrupt a NMI as it
> is being configured in gic_smp_init(), calling request_nmi() at this
> stage.

Sure, I will try to follow your suggestion. But I think it's better to
first generalize the base IPI allocation scheme.

>
> >>
> >> There is still a bit of work to be able to actually request a SGI
> >> (they
> >> are hard-wired as chained interrupts so far, as this otherwise changes
> >> the output of /proc/interrupts, among other things), but you will
> >> hopefully see what I'm aiming for.
> >
> > I was exploring this idea: "request a SGI". I guess here you meant to
> > request a new SGI as a normal NMI/IRQ via common APIs such as
> > request_percpu_nmi() or request_percpu_irq() rather than statically
> > adding a new IPI as per this patch [2], correct? If yes, then I have
> > following follow up queries:
> >
> > 1. Do you envision any drivers to use SGIs in a similar manner as they
> > use SPIs or PPIs?
>
> No. SGIs are already pretty much all allocated for the kernel's internal
> needs and once we allocate an additional one for this KGDB feature,
> we're out of non-secure SGIs. We could start a multiplexing scheme to
> overcome this, but the kernel already has plenty of other mechanisms
> for internal communication. After all, why would you need anything more
> than smp_call_function()?
>
> The single use case I can imagine is that you'd want to signal a CPU
> that isn't running Linux. This would require a lot more work than
> just an interrupt, and is out of scope for the time being.

Thanks for the clarification.

>
> > 2. How do you envision allocation of SGIs as currently they are
> > hardcoded in an arch specific file (like arch/arm64/kernel/smp.c
> > +794)?
>
> What I would like is for the arch code to request these interrupts as
> normal interrupts, for which there is one problem to solve: how do you
> find out about the Linux IRQ number for a given IPI. Or rather, how
> do you get rid of the requirement to have IPI numbers at all and just
> say "give me a per-cpu interrupt that I can use as an IPI, and by the
> way here's the handler you can call".

I think what you are looking for here is something that could be
sufficed via enabling "CONFIG_GENERIC_IRQ_IPI" framework for arm64/arm
architectures. It's currently used for mips architecture. Looking at
its implementation, I think it should be possible to hook up SGIs
under new IPI irq_domain for GICv2/v3.

So with this framework we should be able to dynamically allocate IPIs
and use common APIs such as request_irq()/request_nmi() to tell IPI
specific handlers.

If this approach looks sane to you then I can start working on a PoC
implementation for arm64.

>
> And I insist: this is only for the arch code. Nothing else.
>

Makes sense.

> > 3. AFAIK, the major difference among SGIs and SPIs or PPIs is the
> > trigger method where SGIs are software triggered and SPIs or PPIs are
> > hardware triggered. And I couldn't find a generalized method across
> > architectures to invoke SGIs. So how do you envision drivers to invoke
> > SGIs in an architecture agnostic manner?
>
> Well, SGIs are not architecture agnostic. They are fundamentally part of
> the GIC architecture, which only exists for the two ARM architectures.
>
> SGIs are not a general purpose mechanism. IPIs are, and we have services
> on top of IPIs, such as invoking a function on a remote CPU. What else
> do you need?

Yeah that was mine understanding as well. But I was just clarifying if
you have any further use-cases in mind for IPIs.

-Sunit

>
> Thanks,
>
>           M.
> --
> Jazz is not dead. It just smells funny...

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Marc Zyngier]

On Thu, 30 Apr 2020 12:50:28 +0530
Sumit Garg <sumit.garg@linaro.org> wrote:

> Hi Marc,
> 
> On Wed, 29 Apr 2020 at 13:53, Marc Zyngier <maz@kernel.org> wrote:

[...]

> > What I would like is for the arch code to request these interrupts as
> > normal interrupts, for which there is one problem to solve: how do you
> > find out about the Linux IRQ number for a given IPI. Or rather, how
> > do you get rid of the requirement to have IPI numbers at all and just
> > say "give me a per-cpu interrupt that I can use as an IPI, and by the
> > way here's the handler you can call".  
> 
> I think what you are looking for here is something that could be
> sufficed via enabling "CONFIG_GENERIC_IRQ_IPI" framework for arm64/arm
> architectures. It's currently used for mips architecture. Looking at
> its implementation, I think it should be possible to hook up SGIs
> under new IPI irq_domain for GICv2/v3.
> 
> So with this framework we should be able to dynamically allocate IPIs
> and use common APIs such as request_irq()/request_nmi() to tell IPI
> specific handlers.
> 
> If this approach looks sane to you then I can start working on a PoC
> implementation for arm64.

I can't say I'm keen. This IPI framework doesn't really work for the
GIC:

- it requires a separate irqdomain to be able to guarantee that you
  allocate the hwirq in the SGI range. What is the point?
- it creates yet another level of indirection on IPI injection

This framework was created to deal with two cases:
- systems that can't represent their IPI with a single hwirq spanning
  all the CPUs
- "accelerator cores" that don't run Linux

The GIC architecture avoids the first point, and I don't even want to
think of the second one.

Also, it doesn't solve the initial problem, which is to bootstrap the
whole thing. The IPI framework relies on an irqdomain to be created the
first place. This would mean teaching the arch code about the
intricacies of irqdomains, FW nodes and other terrible things. All
things which are better hidden in the GIC drivers (not to mention the
other horror stories that are the RPi-2/3 irqchip and the Huawei GIC
knock-off).

What I have in mind is to replace the set_smp_cross_call() with
something that passes the required set of information (interrupt range,
at the very least). The only thing that I plan to reuse from the IPI
framework is the chip->ipi_send_mask() callback.

Thanks,

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

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

On Thu, 30 Apr 2020 at 14:43, Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 30 Apr 2020 12:50:28 +0530
> Sumit Garg <sumit.garg@linaro.org> wrote:
>
> > Hi Marc,
> >
> > On Wed, 29 Apr 2020 at 13:53, Marc Zyngier <maz@kernel.org> wrote:
>
> [...]
>
> > > What I would like is for the arch code to request these interrupts as
> > > normal interrupts, for which there is one problem to solve: how do you
> > > find out about the Linux IRQ number for a given IPI. Or rather, how
> > > do you get rid of the requirement to have IPI numbers at all and just
> > > say "give me a per-cpu interrupt that I can use as an IPI, and by the
> > > way here's the handler you can call".
> >
> > I think what you are looking for here is something that could be
> > sufficed via enabling "CONFIG_GENERIC_IRQ_IPI" framework for arm64/arm
> > architectures. It's currently used for mips architecture. Looking at
> > its implementation, I think it should be possible to hook up SGIs
> > under new IPI irq_domain for GICv2/v3.
> >
> > So with this framework we should be able to dynamically allocate IPIs
> > and use common APIs such as request_irq()/request_nmi() to tell IPI
> > specific handlers.
> >
> > If this approach looks sane to you then I can start working on a PoC
> > implementation for arm64.
>
> I can't say I'm keen. This IPI framework doesn't really work for the
> GIC:
>
> - it requires a separate irqdomain to be able to guarantee that you
>   allocate the hwirq in the SGI range. What is the point?
> - it creates yet another level of indirection on IPI injection
>
> This framework was created to deal with two cases:
> - systems that can't represent their IPI with a single hwirq spanning
>   all the CPUs
> - "accelerator cores" that don't run Linux
>
> The GIC architecture avoids the first point, and I don't even want to
> think of the second one.
>
> Also, it doesn't solve the initial problem, which is to bootstrap the
> whole thing. The IPI framework relies on an irqdomain to be created the
> first place. This would mean teaching the arch code about the
> intricacies of irqdomains, FW nodes and other terrible things. All
> things which are better hidden in the GIC drivers (not to mention the
> other horror stories that are the RPi-2/3 irqchip and the Huawei GIC
> knock-off).
>
> What I have in mind is to replace the set_smp_cross_call() with
> something that passes the required set of information (interrupt range,
> at the very least). The only thing that I plan to reuse from the IPI
> framework is the chip->ipi_send_mask() callback.
>

Fair enough, I will just pass the allocated interrupt range base
instead of set_smp_cross_call() and use __ipi_send_mask() to invoke a
particular IPI.

And to request an arch specific IPI as NMI, will use
arch_get_ipinr_nmi() and in turn use request_percpu_nmi() to turn that
particular IPI as NMI.

Is there anything that I missed here?

-Sumit

> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

Hi Marc,

On Thu, 30 Apr 2020 at 17:43, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Thu, 30 Apr 2020 at 14:43, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 30 Apr 2020 12:50:28 +0530
> > Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > > Hi Marc,
> > >
> > > On Wed, 29 Apr 2020 at 13:53, Marc Zyngier <maz@kernel.org> wrote:
> >
> > [...]
> >
> > > > What I would like is for the arch code to request these interrupts as
> > > > normal interrupts, for which there is one problem to solve: how do you
> > > > find out about the Linux IRQ number for a given IPI. Or rather, how
> > > > do you get rid of the requirement to have IPI numbers at all and just
> > > > say "give me a per-cpu interrupt that I can use as an IPI, and by the
> > > > way here's the handler you can call".
> > >
> > > I think what you are looking for here is something that could be
> > > sufficed via enabling "CONFIG_GENERIC_IRQ_IPI" framework for arm64/arm
> > > architectures. It's currently used for mips architecture. Looking at
> > > its implementation, I think it should be possible to hook up SGIs
> > > under new IPI irq_domain for GICv2/v3.
> > >
> > > So with this framework we should be able to dynamically allocate IPIs
> > > and use common APIs such as request_irq()/request_nmi() to tell IPI
> > > specific handlers.
> > >
> > > If this approach looks sane to you then I can start working on a PoC
> > > implementation for arm64.
> >
> > I can't say I'm keen. This IPI framework doesn't really work for the
> > GIC:
> >
> > - it requires a separate irqdomain to be able to guarantee that you
> >   allocate the hwirq in the SGI range. What is the point?
> > - it creates yet another level of indirection on IPI injection
> >
> > This framework was created to deal with two cases:
> > - systems that can't represent their IPI with a single hwirq spanning
> >   all the CPUs
> > - "accelerator cores" that don't run Linux
> >
> > The GIC architecture avoids the first point, and I don't even want to
> > think of the second one.
> >
> > Also, it doesn't solve the initial problem, which is to bootstrap the
> > whole thing. The IPI framework relies on an irqdomain to be created the
> > first place. This would mean teaching the arch code about the
> > intricacies of irqdomains, FW nodes and other terrible things. All
> > things which are better hidden in the GIC drivers (not to mention the
> > other horror stories that are the RPi-2/3 irqchip and the Huawei GIC
> > knock-off).
> >
> > What I have in mind is to replace the set_smp_cross_call() with
> > something that passes the required set of information (interrupt range,
> > at the very least). The only thing that I plan to reuse from the IPI
> > framework is the chip->ipi_send_mask() callback.
> >
>
> Fair enough, I will just pass the allocated interrupt range base
> instead of set_smp_cross_call() and use __ipi_send_mask() to invoke a
> particular IPI.

Thinking more about this, there seems to be multiple irqchip drivers
registering softirq API via set_smp_cross_call(). So we need to
introduce a new API instead of replacing set_smp_cross_call() under
"CONFIG_GENERIC_IRQ_IPI" macro until all drivers switch to IPIs as
full interrupts.

BTW, could we take up this generalization as follow-up work as it
seems to be independent of current IPI NMI work?

>
> And to request an arch specific IPI as NMI, will use
> arch_get_ipinr_nmi() and in turn use request_percpu_nmi() to turn that
> particular IPI as NMI.

I have updated the second patch [1] in my tree to incorporate these
changes. The updated commit log is as follows:

commit 25c96663126264ec758c49a4a01a9c285f4ccc61
Author: Sumit Garg <sumit.garg@linaro.org>
Date:   Wed Apr 22 16:29:59 2020 +0530

    irqchip/gic-v3: Setup arch specific IPI as pseudo NMI

    Add support to mark arch specific IPI as pseudo NMI. Currently its used
    to allow arm64 specific IPI_CALL_NMI_FUNC to be marked as pseudo NMI.

    Brief description of changes:
    - Update NMI setup/teardown routines for SGIs.
    - Enable NMI support prior to gic_smp_init().
    - Setup custom flow handler for SGI setup as NMI.
    - Request, prepare and enable arch specific IPI as per CPU NMI using
      common APIs.

    Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

Please have a look and let me know if this is something you were looking for.

[1] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=25c96663126264ec758c49a4a01a9c285f4ccc61

-Sumit

> > Thanks,
> >
> >         M.
> > --
> > Jazz is not dead. It just smells funny...

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

On Fri, 1 May 2020 at 18:33, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Marc,
>
> On Thu, 30 Apr 2020 at 17:43, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Thu, 30 Apr 2020 at 14:43, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 30 Apr 2020 12:50:28 +0530
> > > Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > > Hi Marc,
> > > >
> > > > On Wed, 29 Apr 2020 at 13:53, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > [...]
> > >
> > > > > What I would like is for the arch code to request these interrupts as
> > > > > normal interrupts, for which there is one problem to solve: how do you
> > > > > find out about the Linux IRQ number for a given IPI. Or rather, how
> > > > > do you get rid of the requirement to have IPI numbers at all and just
> > > > > say "give me a per-cpu interrupt that I can use as an IPI, and by the
> > > > > way here's the handler you can call".
> > > >
> > > > I think what you are looking for here is something that could be
> > > > sufficed via enabling "CONFIG_GENERIC_IRQ_IPI" framework for arm64/arm
> > > > architectures. It's currently used for mips architecture. Looking at
> > > > its implementation, I think it should be possible to hook up SGIs
> > > > under new IPI irq_domain for GICv2/v3.
> > > >
> > > > So with this framework we should be able to dynamically allocate IPIs
> > > > and use common APIs such as request_irq()/request_nmi() to tell IPI
> > > > specific handlers.
> > > >
> > > > If this approach looks sane to you then I can start working on a PoC
> > > > implementation for arm64.
> > >
> > > I can't say I'm keen. This IPI framework doesn't really work for the
> > > GIC:
> > >
> > > - it requires a separate irqdomain to be able to guarantee that you
> > >   allocate the hwirq in the SGI range. What is the point?
> > > - it creates yet another level of indirection on IPI injection
> > >
> > > This framework was created to deal with two cases:
> > > - systems that can't represent their IPI with a single hwirq spanning
> > >   all the CPUs
> > > - "accelerator cores" that don't run Linux
> > >
> > > The GIC architecture avoids the first point, and I don't even want to
> > > think of the second one.
> > >
> > > Also, it doesn't solve the initial problem, which is to bootstrap the
> > > whole thing. The IPI framework relies on an irqdomain to be created the
> > > first place. This would mean teaching the arch code about the
> > > intricacies of irqdomains, FW nodes and other terrible things. All
> > > things which are better hidden in the GIC drivers (not to mention the
> > > other horror stories that are the RPi-2/3 irqchip and the Huawei GIC
> > > knock-off).
> > >
> > > What I have in mind is to replace the set_smp_cross_call() with
> > > something that passes the required set of information (interrupt range,
> > > at the very least). The only thing that I plan to reuse from the IPI
> > > framework is the chip->ipi_send_mask() callback.
> > >
> >
> > Fair enough, I will just pass the allocated interrupt range base
> > instead of set_smp_cross_call() and use __ipi_send_mask() to invoke a
> > particular IPI.
>
> Thinking more about this, there seems to be multiple irqchip drivers
> registering softirq API via set_smp_cross_call(). So we need to
> introduce a new API instead of replacing set_smp_cross_call() under
> "CONFIG_GENERIC_IRQ_IPI" macro until all drivers switch to IPIs as
> full interrupts.
>
> BTW, could we take up this generalization as follow-up work as it
> seems to be independent of current IPI NMI work?
>
> >
> > And to request an arch specific IPI as NMI, will use
> > arch_get_ipinr_nmi() and in turn use request_percpu_nmi() to turn that
> > particular IPI as NMI.
>
> I have updated the second patch [1] in my tree to incorporate these
> changes. The updated commit log is as follows:
>
> commit 25c96663126264ec758c49a4a01a9c285f4ccc61
> Author: Sumit Garg <sumit.garg@linaro.org>
> Date:   Wed Apr 22 16:29:59 2020 +0530
>
>     irqchip/gic-v3: Setup arch specific IPI as pseudo NMI
>
>     Add support to mark arch specific IPI as pseudo NMI. Currently its used
>     to allow arm64 specific IPI_CALL_NMI_FUNC to be marked as pseudo NMI.
>
>     Brief description of changes:
>     - Update NMI setup/teardown routines for SGIs.
>     - Enable NMI support prior to gic_smp_init().
>     - Setup custom flow handler for SGI setup as NMI.
>     - Request, prepare and enable arch specific IPI as per CPU NMI using
>       common APIs.
>
>     Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> Please have a look and let me know if this is something you were looking for.
>

In case there are no major objections to this approach, I will post
complete v2 patch-set (alongwith Marc's patches) for detailed review.

-Sumit

> [1] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=25c96663126264ec758c49a4a01a9c285f4ccc61
>
> -Sumit
>
> > > Thanks,
> > >
> > >         M.
> > > --
> > > Jazz is not dead. It just smells funny...

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Marc Zyngier]

On 2020-05-05 05:09, Sumit Garg wrote:
> On Fri, 1 May 2020 at 18:33, Sumit Garg <sumit.garg@linaro.org> wrote:

[...]

> In case there are no major objections to this approach, I will post
> complete v2 patch-set (alongwith Marc's patches) for detailed review.

As this is still a work in progress (I'm currently wrestling with
the stupid RPi driver), whatever you have is unlikely to apply on
top of the final series.

I'm not going to stop you from posting the patches, it is just that
they will be obsolete by the end of the week...

Thanks,

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

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

On Tue, 5 May 2020 at 15:38, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-05-05 05:09, Sumit Garg wrote:
> > On Fri, 1 May 2020 at 18:33, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> [...]
>
> > In case there are no major objections to this approach, I will post
> > complete v2 patch-set (alongwith Marc's patches) for detailed review.
>
> As this is still a work in progress (I'm currently wrestling with
> the stupid RPi driver), whatever you have is unlikely to apply on
> top of the final series.
>
> I'm not going to stop you from posting the patches, it is just that
> they will be obsolete by the end of the week...

Thanks for the heads up. Will wait for your final series.

But while working on an NMI request, I noticed a hack in common gic
code [1] which basically enables all SGIs for every CPU by default.
This hack is quite similar to mine initial hack to set priority for a
particular SGI by default to act as pseudo NMI.

Due to this hack I got following error message while configuring SGI as NMI:

[    0.000000] GICv3: Cannot set NMI property of enabled IRQ 8
[    0.000000] genirq: Failed to setup NMI delivery: irq 8

I think chained IRQs worked for you due to this hack only as it
doesn't seem to enable SGIs per CPU.

IMO, as we shift to SGIs being standard interrupts, we should also
rely on standard interrupt framework to enable SGIs. So it seems the
correct way would be to use "request_percpu_irq()" and
"enable_percpu_irq()" for configuring SGIs as demonstrated in updated
commit here [2].

Also, we should get rid of this hack as demonstrated via commit [3].

Apart from above changes, there was a minor update needed for commit
"irqchip/gic-v3: Describe the SGI range" [4].

I hope these updates are useful for you while preparing the final series.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-common.c#n155
[2] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=e208979b5165d753d144db57e0cb8646fdedc495
[3] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=cd6d0d7cea14ac16156f0dbd297940df382f8cea
[4] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=1180e9c54547ec05d96cc6b36c26005059c90d9a

-Sumit

>
> Thanks,
>
>          M.
> --
> Jazz is not dead. It just smells funny...

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg]

Hi Marc,

On Tue, 5 May 2020 at 17:03, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Tue, 5 May 2020 at 15:38, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On 2020-05-05 05:09, Sumit Garg wrote:
> > > On Fri, 1 May 2020 at 18:33, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > [...]
> >
> > > In case there are no major objections to this approach, I will post
> > > complete v2 patch-set (alongwith Marc's patches) for detailed review.
> >
> > As this is still a work in progress (I'm currently wrestling with
> > the stupid RPi driver), whatever you have is unlikely to apply on
> > top of the final series.
> >
> > I'm not going to stop you from posting the patches, it is just that
> > they will be obsolete by the end of the week...
>
> Thanks for the heads up. Will wait for your final series.
>

Were you able to give a final shape to your SGIs related patch-set?

-Sumit

> But while working on an NMI request, I noticed a hack in common gic
> code [1] which basically enables all SGIs for every CPU by default.
> This hack is quite similar to mine initial hack to set priority for a
> particular SGI by default to act as pseudo NMI.
>
> Due to this hack I got following error message while configuring SGI as NMI:
>
> [    0.000000] GICv3: Cannot set NMI property of enabled IRQ 8
> [    0.000000] genirq: Failed to setup NMI delivery: irq 8
>
> I think chained IRQs worked for you due to this hack only as it
> doesn't seem to enable SGIs per CPU.
>
> IMO, as we shift to SGIs being standard interrupts, we should also
> rely on standard interrupt framework to enable SGIs. So it seems the
> correct way would be to use "request_percpu_irq()" and
> "enable_percpu_irq()" for configuring SGIs as demonstrated in updated
> commit here [2].
>
> Also, we should get rid of this hack as demonstrated via commit [3].
>
> Apart from above changes, there was a minor update needed for commit
> "irqchip/gic-v3: Describe the SGI range" [4].
>
> I hope these updates are useful for you while preparing the final series.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-common.c#n155
> [2] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=e208979b5165d753d144db57e0cb8646fdedc495
> [3] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=cd6d0d7cea14ac16156f0dbd297940df382f8cea
> [4] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=1180e9c54547ec05d96cc6b36c26005059c90d9a
>
> -Sumit
>
> >
> > Thanks,
> >
> >          M.
> > --
> > Jazz is not dead. It just smells funny...

Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Marc Zyngier]

On 2020-05-13 11:02, Sumit Garg wrote:
> Hi Marc,
> 
> On Tue, 5 May 2020 at 17:03, Sumit Garg <sumit.garg@linaro.org> wrote:
>> 
>> On Tue, 5 May 2020 at 15:38, Marc Zyngier <maz@kernel.org> wrote:
>> >
>> > On 2020-05-05 05:09, Sumit Garg wrote:
>> > > On Fri, 1 May 2020 at 18:33, Sumit Garg <sumit.garg@linaro.org> wrote:
>> >
>> > [...]
>> >
>> > > In case there are no major objections to this approach, I will post
>> > > complete v2 patch-set (alongwith Marc's patches) for detailed review.
>> >
>> > As this is still a work in progress (I'm currently wrestling with
>> > the stupid RPi driver), whatever you have is unlikely to apply on
>> > top of the final series.
>> >
>> > I'm not going to stop you from posting the patches, it is just that
>> > they will be obsolete by the end of the week...
>> 
>> Thanks for the heads up. Will wait for your final series.
>> 
> 
> Were you able to give a final shape to your SGIs related patch-set?

See 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq

Thanks,

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

[RFC Patch v1 3/4] irqchip/gic-v3: Enable arch specific IPI as pseudo NMI

[Sumit Garg]

Add support to mark arch specific IPI as pseudo NMI. Currently its used
to allows arm64 specific IPI_CALL_NMI_FUNC to be marked as pseudo NMI.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 arch/arm64/kernel/smp.c      |  5 +++++
 drivers/irqchip/irq-gic-v3.c | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 42fe7bb..27c8ee1 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -802,6 +802,11 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
 	S(IPI_CALL_NMI_FUNC, "NMI function call interrupts"),
 };
 
+int arch_get_ipinr_nmi(void)
+{
+	return IPI_CALL_NMI_FUNC;
+}
+
 static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
 {
 	trace_ipi_raise(target, ipi_types[ipinr]);
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index be361bf..a3d2cf3 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1048,6 +1048,11 @@ static int gic_dist_supports_lpis(void)
 		!gicv3_nolpi);
 }
 
+int __weak arch_get_ipinr_nmi(void)
+{
+	return -1;
+}
+
 static void gic_cpu_init(void)
 {
 	void __iomem *rbase;
@@ -1072,6 +1077,15 @@ static void gic_cpu_init(void)
 
 	gic_cpu_config(rbase, gic_data.ppi_nr + 16, gic_redist_wait_for_rwp);
 
+	if (gic_supports_nmi()) {
+		int ipinr;
+
+		ipinr = arch_get_ipinr_nmi();
+		if (ipinr >= 0 && ipinr < 16)
+			writeb_relaxed(GICD_INT_NMI_PRI,
+				       rbase + GICD_IPRIORITYR + ipinr);
+	}
+
 	/* initialise system registers */
 	gic_cpu_sys_reg_init();
 }
-- 
2.7.4

[RFC Patch v1 4/4] arm64: kgdb: Round up cpus using IPI_CALL_NMI_FUNC

[Sumit Garg]

arm64 platforms with GICv3 or later supports pseudo NMIs which can be
leveraged to round up CPUs which are stuck in hard lockup state with
interrupts disabled that wouldn't be possible with a normal IPI.

So instead switch to round up CPUs using IPI_CALL_NMI_FUNC. And in
case a particular arm64 platform doesn't supports pseudo NMIs,
IPI_CALL_NMI_FUNC will act as a normal IPI which maintains existing
kgdb functionality.

Also, one thing to note here is that with CPUs running in NMI context,
kernel has special handling for printk() which involves CPU specific
buffers and defering printk() until exit from NMI context. But with kgdb
we don't want to defer printk() especially backtrace on corresponding
CPUs. So switch to normal printk() context instead prior to entering
kgdb context.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 arch/arm64/kernel/kgdb.c | 15 +++++++++++++++
 arch/arm64/kernel/smp.c  | 17 ++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 4311992..0851ead 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -14,6 +14,7 @@
 #include <linux/kgdb.h>
 #include <linux/kprobes.h>
 #include <linux/sched/task_stack.h>
+#include <linux/smp.h>
 
 #include <asm/debug-monitors.h>
 #include <asm/insn.h>
@@ -353,3 +354,17 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 	return aarch64_insn_write((void *)bpt->bpt_addr,
 			*(u32 *)bpt->saved_instr);
 }
+
+#ifdef CONFIG_SMP
+void kgdb_roundup_cpus(void)
+{
+	struct cpumask mask;
+
+	cpumask_copy(&mask, cpu_online_mask);
+	cpumask_clear_cpu(raw_smp_processor_id(), &mask);
+	if (cpumask_empty(&mask))
+		return;
+
+	arch_send_call_nmi_func_ipi_mask(&mask);
+}
+#endif
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 27c8ee1..c7158f6e8 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -31,6 +31,7 @@
 #include <linux/of.h>
 #include <linux/irq_work.h>
 #include <linux/kexec.h>
+#include <linux/kgdb.h>
 #include <linux/kvm_host.h>
 
 #include <asm/alternative.h>
@@ -976,9 +977,19 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 		/* Handle it as a normal interrupt if not in NMI context */
 		if (!in_nmi())
 			irq_enter();
-
-		/* nop, IPI handlers for special features can be added here. */
-
+#ifdef CONFIG_KGDB
+		if (atomic_read(&kgdb_active) != -1) {
+			/*
+			 * For kgdb to work properly, we need printk to operate
+			 * in normal context.
+			 */
+			if (in_nmi())
+				printk_nmi_exit();
+			kgdb_nmicallback(raw_smp_processor_id(), regs);
+			if (in_nmi())
+				printk_nmi_enter();
+		}
+#endif
 		if (!in_nmi())
 			irq_exit();
 		break;
-- 
2.7.4

Re: [RFC Patch v1 4/4] arm64: kgdb: Round up cpus using IPI_CALL_NMI_FUNC

[Doug Anderson]

Hi,

On Fri, Apr 24, 2020 at 4:11 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> arm64 platforms with GICv3 or later supports pseudo NMIs which can be
> leveraged to round up CPUs which are stuck in hard lockup state with
> interrupts disabled that wouldn't be possible with a normal IPI.
>
> So instead switch to round up CPUs using IPI_CALL_NMI_FUNC. And in
> case a particular arm64 platform doesn't supports pseudo NMIs,
> IPI_CALL_NMI_FUNC will act as a normal IPI which maintains existing
> kgdb functionality.
>
> Also, one thing to note here is that with CPUs running in NMI context,
> kernel has special handling for printk() which involves CPU specific
> buffers and defering printk() until exit from NMI context. But with kgdb
> we don't want to defer printk() especially backtrace on corresponding
> CPUs. So switch to normal printk() context instead prior to entering
> kgdb context.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  arch/arm64/kernel/kgdb.c | 15 +++++++++++++++
>  arch/arm64/kernel/smp.c  | 17 ++++++++++++++---
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 4311992..0851ead 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -14,6 +14,7 @@
>  #include <linux/kgdb.h>
>  #include <linux/kprobes.h>
>  #include <linux/sched/task_stack.h>
> +#include <linux/smp.h>
>
>  #include <asm/debug-monitors.h>
>  #include <asm/insn.h>
> @@ -353,3 +354,17 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
>         return aarch64_insn_write((void *)bpt->bpt_addr,
>                         *(u32 *)bpt->saved_instr);
>  }
> +
> +#ifdef CONFIG_SMP
> +void kgdb_roundup_cpus(void)
> +{
> +       struct cpumask mask;
> +
> +       cpumask_copy(&mask, cpu_online_mask);
> +       cpumask_clear_cpu(raw_smp_processor_id(), &mask);
> +       if (cpumask_empty(&mask))
> +               return;
> +
> +       arch_send_call_nmi_func_ipi_mask(&mask);
> +}
> +#endif
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 27c8ee1..c7158f6e8 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -31,6 +31,7 @@
>  #include <linux/of.h>
>  #include <linux/irq_work.h>
>  #include <linux/kexec.h>
> +#include <linux/kgdb.h>
>  #include <linux/kvm_host.h>
>
>  #include <asm/alternative.h>
> @@ -976,9 +977,19 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>                 /* Handle it as a normal interrupt if not in NMI context */
>                 if (!in_nmi())
>                         irq_enter();
> -
> -               /* nop, IPI handlers for special features can be added here. */
> -
> +#ifdef CONFIG_KGDB

My vote would be to keep "ifdef"s out of the middle of functions.  Can
you put your code in "arch/arm64/kernel/kgdb.c" and then have a dummpy
no-op function if "CONFIG_KGDB" isn't defined?


> +               if (atomic_read(&kgdb_active) != -1) {
> +                       /*
> +                        * For kgdb to work properly, we need printk to operate
> +                        * in normal context.
> +                        */
> +                       if (in_nmi())
> +                               printk_nmi_exit();

It feels like all the printk management belongs in kgdb_nmicallback().
...or is there some reason that this isn't a problem for other
platforms using NMI?  Maybe it's just that nobody has noticed it yet?


> +                       kgdb_nmicallback(raw_smp_processor_id(), regs);

Why do you need to call raw_smp_processor_id()?  Are you expecting a
different value than the local variable "cpu"?


> +                       if (in_nmi())
> +                               printk_nmi_enter();
> +               }
> +#endif
>                 if (!in_nmi())
>                         irq_exit();
>                 break;

Not that I really know what I'm talking about since I really don't
know arm64 at this level very well, but I'll ask anyway and probably
look like a fool...  I had a note that said:

* Will Deacon says:
*
* the whole roundup code is sketchy and it's the only place in the kernel
* which tries to perform I-cache maintenance with irqs disabled, leading
* to this nasty hack in the arch code:
*
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/cacheflush.h#n74

I presume that, if nothing else, the comment needs to be updated.
...but is the situation any better (or worse?) with your new solution?

-Doug

Re: [RFC Patch v1 4/4] arm64: kgdb: Round up cpus using IPI_CALL_NMI_FUNC

[Sumit Garg]

Hi Doug,

Thanks for your comments.

On Sat, 25 Apr 2020 at 02:17, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Apr 24, 2020 at 4:11 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > arm64 platforms with GICv3 or later supports pseudo NMIs which can be
> > leveraged to round up CPUs which are stuck in hard lockup state with
> > interrupts disabled that wouldn't be possible with a normal IPI.
> >
> > So instead switch to round up CPUs using IPI_CALL_NMI_FUNC. And in
> > case a particular arm64 platform doesn't supports pseudo NMIs,
> > IPI_CALL_NMI_FUNC will act as a normal IPI which maintains existing
> > kgdb functionality.
> >
> > Also, one thing to note here is that with CPUs running in NMI context,
> > kernel has special handling for printk() which involves CPU specific
> > buffers and defering printk() until exit from NMI context. But with kgdb
> > we don't want to defer printk() especially backtrace on corresponding
> > CPUs. So switch to normal printk() context instead prior to entering
> > kgdb context.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  arch/arm64/kernel/kgdb.c | 15 +++++++++++++++
> >  arch/arm64/kernel/smp.c  | 17 ++++++++++++++---
> >  2 files changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> > index 4311992..0851ead 100644
> > --- a/arch/arm64/kernel/kgdb.c
> > +++ b/arch/arm64/kernel/kgdb.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/kgdb.h>
> >  #include <linux/kprobes.h>
> >  #include <linux/sched/task_stack.h>
> > +#include <linux/smp.h>
> >
> >  #include <asm/debug-monitors.h>
> >  #include <asm/insn.h>
> > @@ -353,3 +354,17 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> >         return aarch64_insn_write((void *)bpt->bpt_addr,
> >                         *(u32 *)bpt->saved_instr);
> >  }
> > +
> > +#ifdef CONFIG_SMP
> > +void kgdb_roundup_cpus(void)
> > +{
> > +       struct cpumask mask;
> > +
> > +       cpumask_copy(&mask, cpu_online_mask);
> > +       cpumask_clear_cpu(raw_smp_processor_id(), &mask);
> > +       if (cpumask_empty(&mask))
> > +               return;
> > +
> > +       arch_send_call_nmi_func_ipi_mask(&mask);
> > +}
> > +#endif
> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > index 27c8ee1..c7158f6e8 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/of.h>
> >  #include <linux/irq_work.h>
> >  #include <linux/kexec.h>
> > +#include <linux/kgdb.h>
> >  #include <linux/kvm_host.h>
> >
> >  #include <asm/alternative.h>
> > @@ -976,9 +977,19 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
> >                 /* Handle it as a normal interrupt if not in NMI context */
> >                 if (!in_nmi())
> >                         irq_enter();
> > -
> > -               /* nop, IPI handlers for special features can be added here. */
> > -
> > +#ifdef CONFIG_KGDB
>
> My vote would be to keep "ifdef"s out of the middle of functions.  Can
> you put your code in "arch/arm64/kernel/kgdb.c" and then have a dummpy
> no-op function if "CONFIG_KGDB" isn't defined?
>

Sure.

>
> > +               if (atomic_read(&kgdb_active) != -1) {
> > +                       /*
> > +                        * For kgdb to work properly, we need printk to operate
> > +                        * in normal context.
> > +                        */
> > +                       if (in_nmi())
> > +                               printk_nmi_exit();
>
> It feels like all the printk management belongs in kgdb_nmicallback().
> ...or is there some reason that this isn't a problem for other
> platforms using NMI?  Maybe it's just that nobody has noticed it yet?
>

Initially I was skeptical of moving this printk handling in the common
kgdb framework but after exploring other platforms like x86 (probably
unnoticed bug), I agree with you that it belongs to
kgdb_nmicallback(). So I will move it there.

>
> > +                       kgdb_nmicallback(raw_smp_processor_id(), regs);
>
> Why do you need to call raw_smp_processor_id()?  Are you expecting a
> different value than the local variable "cpu"?

Ah, no. Will use the local variable "cpu" instead.

>
>
> > +                       if (in_nmi())
> > +                               printk_nmi_enter();
> > +               }
> > +#endif
> >                 if (!in_nmi())
> >                         irq_exit();
> >                 break;
>
> Not that I really know what I'm talking about since I really don't
> know arm64 at this level very well, but I'll ask anyway and probably
> look like a fool...  I had a note that said:
>
> * Will Deacon says:
> *
> * the whole roundup code is sketchy and it's the only place in the kernel
> * which tries to perform I-cache maintenance with irqs disabled, leading
> * to this nasty hack in the arch code:
> *
> * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/cacheflush.h#n74
>
> I presume that, if nothing else, the comment needs to be updated.
> ...but is the situation any better (or worse?) with your new solution?

I think the situation remains the same with new solution as well. As
either we use IPI being a pseudo NMI or a normal IRQ to roundup CPUs,
kgdb still does I-cache maintenance with irqs disabled which could
lead to a deadlock trying to IPI the secondary CPUs without this nasty
hack in the arch code.

-Sumit

>
> -Doug

Re: [RFC Patch v1 0/4] arm64: Introduce new IPI as IPI_CALL_NMI_FUNC

[Doug Anderson]

Hi,

On Fri, Apr 24, 2020 at 4:11 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> With pseudo NMIs support available its possible to configure SGIs to be
> triggered as pseudo NMIs running in NMI context. And kernel features
> such as kgdb relies on NMI support to round up CPUs which are stuck in
> hard lockup state with interrupts disabled.
>
> This patch-set adds support for IPI_CALL_NMI_FUNC which can be triggered
> as a pseudo NMI which in turn is leveraged via kgdb to round up CPUs.
>
> After this patch-set we should be able to get a backtrace for a CPU
> stuck in HARDLOCKUP. Have a look at an example below from a testcase run
> on Developerbox:
>
> $ echo HARDLOCKUP > /sys/kernel/debug/provoke-crash/DIRECT
>
> # Enter kdb via Magic SysRq
>
> [11]kdb> btc
> btc: cpu status: Currently on cpu 11
> Available cpus: 0-10(I), 11, 12(I), 13, 14-23(I)
> <snip>
> Stack traceback for pid 623
> 0xffff00086a644600      623      622  1   13   R  0xffff00086a644fc0  bash
> CPU: 13 PID: 623 Comm: bash Not tainted 5.7.0-rc2 #27
> Hardware name: Socionext SynQuacer E-series DeveloperBox, BIOS build #73 Apr  6 2020
> Call trace:
>  dump_backtrace+0x0/0x198
>  show_stack+0x18/0x28
>  dump_stack+0xb8/0x100
>  kgdb_cpu_enter+0x5c0/0x5f8
>  kgdb_nmicallback+0xa0/0xa8
>  handle_IPI+0x190/0x200
>  gic_handle_irq+0x2b8/0x2d8
>  el1_irq+0xcc/0x180
>  lkdtm_HARDLOCKUP+0x8/0x18
>  direct_entry+0x124/0x1c0
>  full_proxy_write+0x60/0xb0
>  __vfs_write+0x1c/0x48
>  vfs_write+0xe4/0x1d0
>  ksys_write+0x6c/0xf8
>  __arm64_sys_write+0x1c/0x28
>  el0_svc_common.constprop.0+0x74/0x1f0
>  do_el0_svc+0x24/0x90
>  el0_sync_handler+0x178/0x2b8
>  el0_sync+0x158/0x180
> <snip>
>
> Looking forward to your comments/feedback.
>
> Sumit Garg (4):
>   arm64: smp: Introduce a new IPI as IPI_CALL_NMI_FUNC
>   irqchip/gic-v3: Add support to handle SGI as pseudo NMI
>   irqchip/gic-v3: Enable arch specific IPI as pseudo NMI
>   arm64: kgdb: Round up cpus using IPI_CALL_NMI_FUNC
>
>  arch/arm64/include/asm/hardirq.h |  2 +-
>  arch/arm64/include/asm/smp.h     |  1 +
>  arch/arm64/kernel/kgdb.c         | 15 +++++++++++++++
>  arch/arm64/kernel/smp.c          | 36 +++++++++++++++++++++++++++++++++++-
>  drivers/irqchip/irq-gic-v3.c     | 36 +++++++++++++++++++++++++++++++-----
>  5 files changed, 83 insertions(+), 7 deletions(-)

This is amazing!

* picked your patches back to my current 5.4 tree
* turned on "CONFIG_ARM64_PSEUDO_NMI"
* set the "irqchip.gicv3_pseudo_nmi=1" command line

...and bam I can trace on the locked up CPU instead of being left in the dark.

I'm not sure I'm going to be too much use in actually doing the review
of the code since I'm not really an expert at how SGIs work (it took
me a while to realize that it must stand for software generated
interrupts) nor the bowels of the GIC.  I tried to do what little
review I could.

In any case, I'll keep this in my local patch stack for now and keep
testing it to make sure I don't notice any weird problems.

-Doug

Re: [RFC Patch v1 0/4] arm64: Introduce new IPI as IPI_CALL_NMI_FUNC

[Sumit Garg]

On Sat, 25 Apr 2020 at 02:20, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Apr 24, 2020 at 4:11 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > With pseudo NMIs support available its possible to configure SGIs to be
> > triggered as pseudo NMIs running in NMI context. And kernel features
> > such as kgdb relies on NMI support to round up CPUs which are stuck in
> > hard lockup state with interrupts disabled.
> >
> > This patch-set adds support for IPI_CALL_NMI_FUNC which can be triggered
> > as a pseudo NMI which in turn is leveraged via kgdb to round up CPUs.
> >
> > After this patch-set we should be able to get a backtrace for a CPU
> > stuck in HARDLOCKUP. Have a look at an example below from a testcase run
> > on Developerbox:
> >
> > $ echo HARDLOCKUP > /sys/kernel/debug/provoke-crash/DIRECT
> >
> > # Enter kdb via Magic SysRq
> >
> > [11]kdb> btc
> > btc: cpu status: Currently on cpu 11
> > Available cpus: 0-10(I), 11, 12(I), 13, 14-23(I)
> > <snip>
> > Stack traceback for pid 623
> > 0xffff00086a644600      623      622  1   13   R  0xffff00086a644fc0  bash
> > CPU: 13 PID: 623 Comm: bash Not tainted 5.7.0-rc2 #27
> > Hardware name: Socionext SynQuacer E-series DeveloperBox, BIOS build #73 Apr  6 2020
> > Call trace:
> >  dump_backtrace+0x0/0x198
> >  show_stack+0x18/0x28
> >  dump_stack+0xb8/0x100
> >  kgdb_cpu_enter+0x5c0/0x5f8
> >  kgdb_nmicallback+0xa0/0xa8
> >  handle_IPI+0x190/0x200
> >  gic_handle_irq+0x2b8/0x2d8
> >  el1_irq+0xcc/0x180
> >  lkdtm_HARDLOCKUP+0x8/0x18
> >  direct_entry+0x124/0x1c0
> >  full_proxy_write+0x60/0xb0
> >  __vfs_write+0x1c/0x48
> >  vfs_write+0xe4/0x1d0
> >  ksys_write+0x6c/0xf8
> >  __arm64_sys_write+0x1c/0x28
> >  el0_svc_common.constprop.0+0x74/0x1f0
> >  do_el0_svc+0x24/0x90
> >  el0_sync_handler+0x178/0x2b8
> >  el0_sync+0x158/0x180
> > <snip>
> >
> > Looking forward to your comments/feedback.
> >
> > Sumit Garg (4):
> >   arm64: smp: Introduce a new IPI as IPI_CALL_NMI_FUNC
> >   irqchip/gic-v3: Add support to handle SGI as pseudo NMI
> >   irqchip/gic-v3: Enable arch specific IPI as pseudo NMI
> >   arm64: kgdb: Round up cpus using IPI_CALL_NMI_FUNC
> >
> >  arch/arm64/include/asm/hardirq.h |  2 +-
> >  arch/arm64/include/asm/smp.h     |  1 +
> >  arch/arm64/kernel/kgdb.c         | 15 +++++++++++++++
> >  arch/arm64/kernel/smp.c          | 36 +++++++++++++++++++++++++++++++++++-
> >  drivers/irqchip/irq-gic-v3.c     | 36 +++++++++++++++++++++++++++++++-----
> >  5 files changed, 83 insertions(+), 7 deletions(-)
>
> This is amazing!
>
> * picked your patches back to my current 5.4 tree
> * turned on "CONFIG_ARM64_PSEUDO_NMI"
> * set the "irqchip.gicv3_pseudo_nmi=1" command line
>
> ...and bam I can trace on the locked up CPU instead of being left in the dark.
>
> I'm not sure I'm going to be too much use in actually doing the review
> of the code since I'm not really an expert at how SGIs work (it took
> me a while to realize that it must stand for software generated
> interrupts) nor the bowels of the GIC.  I tried to do what little
> review I could.
>
> In any case, I'll keep this in my local patch stack for now and keep
> testing it to make sure I don't notice any weird problems.

Thanks for your review and testing.

-Sumit

>
> -Doug