| Message ID | 1548084825-8803-10-git-send-email-julien.thierry@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: provide pseudo NMI with GICv3 | expand |
On Mon, Jan 21, 2019 at 03:33:28PM +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. > > 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@arm.com> > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
On Mon, 21 Jan 2019 at 16:36, Julien Thierry <julien.thierry@arm.com> 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@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/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. > +{ > + 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 22/01/2019 20:18, Ard Biesheuvel wrote: > On Mon, 21 Jan 2019 at 16:36, Julien Thierry <julien.thierry@arm.com> 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@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/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. 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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 23 Jan 2019 at 09:56, Julien Thierry <julien.thierry@arm.com> wrote: > > > > On 22/01/2019 20:18, Ard Biesheuvel wrote: > > On Mon, 21 Jan 2019 at 16:36, Julien Thierry <julien.thierry@arm.com> 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@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/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@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > -- > Julien Thierry
On Mon, 21 Jan 2019 15:33:28 +0000, Julien Thierry <julien.thierry@arm.com> 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@arm.com> > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> M.
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) +{ + 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
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@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/kernel/process.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ arch/arm64/mm/proc.S | 11 ----------- 2 files changed, 45 insertions(+), 11 deletions(-)