Messages in this thread Patch in this message | | | Date | Thu, 29 Nov 2018 17:44:37 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH v6 08/24] arm64: Unmask PMR before going idle |
| |
On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry 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. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/mm/proc.S | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > index 2c75b0b..3c7064c 100644 > --- a/arch/arm64/mm/proc.S > +++ b/arch/arm64/mm/proc.S > @@ -20,6 +20,7 @@ > > #include <linux/init.h> > #include <linux/linkage.h> > +#include <linux/irqchip/arm-gic-v3.h> > #include <asm/assembler.h> > #include <asm/asm-offsets.h> > #include <asm/hwcap.h> > @@ -53,10 +54,27 @@ > * 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). > */ > ENTRY(cpu_do_idle) > +alternative_if_not ARM64_HAS_IRQ_PRIO_MASKING > + dsb sy // WFI may enter a low-power mode > + wfi > + ret > +alternative_else > + mrs x0, daif // save I bit > + msr daifset, #2 // set I bit > + mrs_s x1, SYS_ICC_PMR_EL1 // save PMR > +alternative_endif > + mov x2, #GIC_PRIO_IRQON > + msr_s SYS_ICC_PMR_EL1, x2 // unmask PMR > dsb sy // WFI may enter a low-power mode
Is the DSB SY sufficient and necessary to synchronise the update of SYS_ICC_PMR_EL1? We don't need an ISB too?
> wfi > + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR
Likewise, we don't need any barriers here before we poke DAIF?
> + msr daif, x0 // restore I bit > ret > ENDPROC(cpu_do_idle)
If we build without CONFIG_ARM64_PSEUDO_NMI surely we don't want to emit the alternative?
How about we move this to C, and have something like the below?
For the !CONFIG_ARM64_PSEUDO_NMI case it generates identical assembly to the existing cpu_do_idle(). Note that I've assumed we don't need barriers, which (as above) I'm not certain of.
Thanks, Mark.
---->8---- diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 7f1628effe6d..ccd2ad8c5e2f 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -73,6 +73,40 @@ EXPORT_SYMBOL_GPL(pm_power_off); void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); +static inline void __cpu_do_idle(void) +{ + /* WFI may enter a low-power mode */ + dsb(sy); + wfi(); +} + +/* + * When using priority masking we need to take extra care, etc. + */ +static inline void __cpu_do_idle_irqprio(void) +{ + unsigned long flags = arch_local_irq_save(); + unsigned long pmr = gic_read_pmr(); + + gic_write_pmr(GIC_PRIO_IRQON); + + __cpu_do_idle(); + + gic_write_pmr(pmr); + arch_local_irq_enable(); +} + +/* + * Idle the processor (wait for interrupt). + */ +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 03646e6a2ef4..38c0171e52e2 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -49,17 +49,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
| |