Re: [PATCH v9 09/26] arm64: Unmask PMR before going idle

[Ard Biesheuvel]

On Wed, 23 Jan 2019 at 09:56, Julien Thierry <julien.thierry@xxxxxxx> wrote:
>
>
>
> On 22/01/2019 20:18, Ard Biesheuvel wrote:
> > On Mon, 21 Jan 2019 at 16:36, Julien Thierry <julien.thierry@xxxxxxx> wrote:
> >>
> >> CPU does not received signals for interrupts with a priority masked by
> >> ICC_PMR_EL1. This means the CPU might not come back from a WFI
> >> instruction.
> >>
> >> Make sure ICC_PMR_EL1 does not mask interrupts when doing a WFI.
> >>
> >> Since the logic of cpu_do_idle is becoming a bit more complex than just
> >> two instructions, lets turn it from ASM to C.
> >>
> >> Signed-off-by: Julien Thierry <julien.thierry@xxxxxxx>
> >> Suggested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> >> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> >> Cc: Will Deacon <will.deacon@xxxxxxx>
> >> ---
> >>  arch/arm64/kernel/process.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>  arch/arm64/mm/proc.S        | 11 -----------
> >>  2 files changed, 45 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> >> index 6d410fc..f05b63f 100644
> >> --- a/arch/arm64/kernel/process.c
> >> +++ b/arch/arm64/kernel/process.c
> >> @@ -51,6 +51,7 @@
> >>  #include <linux/thread_info.h>
> >>
> >>  #include <asm/alternative.h>
> >> +#include <asm/arch_gicv3.h>
> >>  #include <asm/compat.h>
> >>  #include <asm/cacheflush.h>
> >>  #include <asm/exec.h>
> >> @@ -74,6 +75,50 @@
> >>
> >>  void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
> >>
> >> +static inline void __cpu_do_idle(void)
> >> +{
> >> +       dsb(sy);
> >> +       wfi();
> >> +}
> >> +
> >> +static inline void __cpu_do_idle_irqprio(void)
> >
> > Can we drop the 'inline's from all the function definitions in .c
> > files? (not just in this patch)
> > We tend to leave that to the compiler.
> >
>
> Sure, I guess cpu_do_idle() isn't a performance critical path anyway.
>

And even if they were, it would not make a difference, since the
compiler will inline this anyway.

For arm64, inline expands to

#define inline inline __attribute__((__always_inline__)) __gnu_inline \
        __maybe_unused notrace

so please just avoid it where you can.


> Unless anyone argues against it, I'll drop these inlines for the next
> version.
>
> Thanks,
>
> Julien
>
> >> +{
> >> +       unsigned long pmr;
> >> +       unsigned long daif_bits;
> >> +
> >> +       daif_bits = read_sysreg(daif);
> >> +       write_sysreg(daif_bits | PSR_I_BIT, daif);
> >> +
> >> +       /*
> >> +        * Unmask PMR before going idle to make sure interrupts can
> >> +        * be raised.
> >> +        */
> >> +       pmr = gic_read_pmr();
> >> +       gic_write_pmr(GIC_PRIO_IRQON);
> >> +
> >> +       __cpu_do_idle();
> >> +
> >> +       gic_write_pmr(pmr);
> >> +       write_sysreg(daif_bits, daif);
> >> +}
> >> +
> >> +/*
> >> + *     cpu_do_idle()
> >> + *
> >> + *     Idle the processor (wait for interrupt).
> >> + *
> >> + *     If the CPU supports priority masking we must do additional work to
> >> + *     ensure that interrupts are not masked at the PMR (because the core will
> >> + *     not wake up if we block the wake up signal in the interrupt controller).
> >> + */
> >> +void cpu_do_idle(void)
> >> +{
> >> +       if (system_uses_irq_prio_masking())
> >> +               __cpu_do_idle_irqprio();
> >> +       else
> >> +               __cpu_do_idle();
> >> +}
> >> +
> >>  /*
> >>   * This is our default idle handler.
> >>   */
> >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> >> index 73886a5..3ea4f3b 100644
> >> --- a/arch/arm64/mm/proc.S
> >> +++ b/arch/arm64/mm/proc.S
> >> @@ -55,17 +55,6 @@
> >>
> >>  #define MAIR(attr, mt) ((attr) << ((mt) * 8))
> >>
> >> -/*
> >> - *     cpu_do_idle()
> >> - *
> >> - *     Idle the processor (wait for interrupt).
> >> - */
> >> -ENTRY(cpu_do_idle)
> >> -       dsb     sy                              // WFI may enter a low-power mode
> >> -       wfi
> >> -       ret
> >> -ENDPROC(cpu_do_idle)
> >> -
> >>  #ifdef CONFIG_CPU_PM
> >>  /**
> >>   * cpu_do_suspend - save CPU registers context
> >> --
> >> 1.9.1
> >>
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> Julien Thierry