Re: [PATCH v6 3/4] MIPS: Ingenic: Add XBurst SMP support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mips: idle: cached need_resched() load after dcache flush in
  xburst_wait_irqoff()
- [Medium] MIPS: Ingenic: missing IRQF_NO_SUSPEND on mailbox IPI in
  xburst_prepare_cpus()
- [Low] mips: idle: xburst_wait_irqoff() calls non-noinstr
  r4k_blast_dcache()
--

commit 2871a54058216bdde1330a67071ead0fedc13363
Author: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>

MIPS: Ingenic: Add XBurst SMP support

This commit adds SMP support for the second XBurst core on the JZ4780 SoC
using CP0 mailboxes for IPIs and adds an XBurst idle handler that flushes
the private data cache before executing the wait instruction.

Link: https://lkml.iu.edu/hypermail/linux/kernel/2005.2/03488.html
Link: https://lists.openwall.net/linux-kernel/2020/05/13/1482
Link: https://lists.openwall.net/linux-kernel/2020/05/19/1099

> diff --git a/arch/mips/ingenic/ingenic-smp.c b/arch/mips/ingenic/ingenic-smp.c
> new file mode 100644
> index 0000000000000..27b9b573a7e07
> --- /dev/null
> +++ b/arch/mips/ingenic/ingenic-smp.c

[ ... ]

> +static void __init xburst_prepare_cpus(unsigned int max_cpus)
> +{
> +	struct device_node *np;
> +	u32 reim;
> +	int ret;
> +
> +	ret = request_irq(XBURST_MBOX_IRQ, xburst_mbox_interrupt,
> +			  IRQF_PERCPU | IRQF_NO_THREAD, "core mailbox", NULL);

[Severity: Medium]
Should this request_irq() call include IRQF_NO_SUSPEND?

Unlike request_percpu_irq(), request_irq() with IRQF_PERCPU does not set
IRQF_NO_SUSPEND automatically, leaving desc->no_suspend_depth at 0.  During
system suspend (such as suspend-to-idle), suspend_device_irqs() disables
XBURST_MBOX_IRQ, which runs mask_mips_irq() and clears STATUSF_IP3 in
CP0_STATUS on the calling CPU.  That blocks IPIs during noirq suspend and
s2idle, and if resume_device_irqs() runs on the other CPU, STATUSF_IP3 stays
masked after resume and hangs subsequent cross-CPU smp_call_function() calls.

> diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
> index 80e8a04a642e0..6c44715d81d5b 100644
> --- a/arch/mips/kernel/idle.c
> +++ b/arch/mips/kernel/idle.c
> @@ -18,6 +18,7 @@
>  #include <asm/cpu-type.h>
>  #include <asm/idle.h>
>  #include <asm/mipsregs.h>
> +#include <asm/r4kcache.h>
>  
>  /*
>   * Not all of the MIPS CPUs have the "wait" instruction available. Moreover,
> @@ -71,6 +72,28 @@ static void __cpuidle rm7k_wait_irqoff(void)
>  		"	.set	pop					\n");
>  }
>  
> +/* XBurst gates its private cache clock in WAIT; write back dirty data first. */
> +static void __cpuidle xburst_wait_irqoff(void)
> +{
> +	unsigned long pending;
> +
> +	if (need_resched())
> +		return;
> +
> +	r4k_blast_dcache();

[Severity: Low]
Does calling r4k_blast_dcache() from __cpuidle xburst_wait_irqoff() violate
noinstr idle rules?

xburst_wait_irqoff() runs between ct_cpuidle_enter() and ct_cpuidle_exit()
while RCU is not watching, but r4k_blast_dcache() calls out-of-line
blast_dcache32() in .text, which only has notrace from inline and lacks
__no_sanitize_coverage, __no_profile and __no_kcsan.  Besides making
cpu_in_idle() return false during the flush, builds with CONFIG_KCOV,
CONFIG_GCOV_PROFILE_ALL or CONFIG_KCSAN run instrumentation while RCU is not
watching and dirty the private L1 D-cache on return from blast_dcache32()
right before wait gates the cache clock.

> +	/* Recheck work that may have arrived during the cache flush. */
> +	pending = read_c0_cause() & read_c0_status() & CAUSEF_IP;
> +	if (!need_resched() && !pending) {

[Severity: High]
Does calling need_resched() after r4k_blast_dcache() pull the idle task's
struct thread_info cache line back into the private L1 D-cache right before
wait stops the cache clock?

need_resched() loads current_thread_info()->flags from KSEG0 cacheable
memory, caching the line with _TIF_NEED_RESCHED clear in the sleeping CPU's
private L1 D-cache.  When the other CPU sets _TIF_NEED_RESCHED on the idle
thread_info->flags and sends SMP_RESCHEDULE_YOURSELF while the cache clock is
gated:

CPU0                              CPU1
xburst_wait_irqoff()
  r4k_blast_dcache()
  need_resched()
    /* caches thread_info with
     * _TIF_NEED_RESCHED == 0
     */
  wait
                                  resched_curr()
                                    set_tsk_need_resched(idle)
                                    smp_send_reschedule()
  <mailbox IPI wakes CPU>
  handle_int / irq_enter()
    /* writes regs / preempt_count
     * in stale thread_info line
     */
  scheduler_ipi() /* no-op */
  /* _TIF_NEED_RESCHED lost */

On MIPS, scheduler_ipi() does not modify thread_info->flags, so when CPU0
takes the interrupt and writes other fields in the same 32-byte struct
thread_info cache line (or if remote snoops stall while the cache clock is
stopped), the reschedule is lost or the waking CPU stalls.  Since local IRQs
are disabled and any remote wakeup during r4k_blast_dcache() raises the
mailbox IPI in CP0_CAUSE, checking pending alone avoids touching the D-cache
after the flush.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113190836.470937-1-f.fainelli@gmail.com?part=3