Re: [PATCH 06/19] LoongArch: Add exception/interrupt handling

[Arnd Bergmann <arnd@arndb.de>]

On Tue, Jul 27, 2021 at 4:10 PM Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Jul 07, 2021 at 11:56:37PM +1000, Nicholas Piggin wrote:
> > >> +/*
> > >> + * Common Vectored Interrupt
> > >> + * Complete the register saves and invoke the do_vi() handler
> > >> + */
> > >> +SYM_FUNC_START(except_vec_vi_handler)
> > >> +  la      t1, __arch_cpu_idle
> > >> +  LONG_L  t0, sp, PT_EPC
> > >> +  /* 32 byte rollback region */
> > >> +  ori     t0, t0, 0x1f
> > >> +  xori    t0, t0, 0x1f
> > >> +  bne     t0, t1, 1f
> > >> +  LONG_S  t0, sp, PT_EPC
> > >
> > > Seriously, you're having your interrupt handler recover from the idle
> > > race? On a *new* architecture?
> >
> > It's heavily derived from MIPS (does that make the wholesale replacement
> > of arch/mips copyright headers a bit questionable?).
> >
> > I don't think it's such a bad trick though -- restartable sequences
> > before they were cool. It can let you save an irq disable in some
> > cases (depending on the arch of course).
>
> In this case you're making *every* interrupt slower. Simply adding a new
> idle instruction, one that can be called with interrupts disabled and
> will terminate on a pending interrupt, would've solved the issues much
> nicer and reclaimed the cycles spend on this restart trick.

Are we actually sure that loongarch /needs/ this version?

The code was clearly copied from the mips default r4k_wait()
function, but mips also has this one:

/*
 * This variant is preferable as it allows testing need_resched and going to
 * sleep depending on the outcome atomically.  Unfortunately the "It is
 * implementation-dependent whether the pipeline restarts when a non-enabled
 * interrupt is requested" restriction in the MIPS32/MIPS64 architecture makes
 * using this version a gamble.
 */
void __cpuidle r4k_wait_irqoff(void)
{
        if (!need_resched())
                __asm__(
                "       .set    push            \n"
                "       .set    arch=r4000      \n"
                "       wait                    \n"
                "       .set    pop             \n");
        raw_local_irq_enable();
}

        case CPU_LOONGSON64:
                if ((c->processor_id & (PRID_IMP_MASK | PRID_REV_MASK)) >=
                                (PRID_IMP_LOONGSON_64C |
PRID_REV_LOONGSON3A_R2_0) ||
                                (c->processor_id & PRID_IMP_MASK) ==
PRID_IMP_LOONGSON_64R)
                        cpu_wait = r4k_wait;
...
                cpu_wait = r4k_wait;
                if (read_c0_config7() & MIPS_CONF7_WII)
                        cpu_wait = r4k_wait_irqoff;
                if (cpu_has_mips_r6) {
                        cpu_wait = r4k_wait_irqoff;

So a lot of the newer mips variants already get the fixed
version. Maybe the hardware developers fixed it without telling
the kernel people about it?

         Arnd