| Message ID | 1542023835-21446-9-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, 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
On 29/11/18 17:44, Mark Rutland wrote: > 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? > DSB SY is necessary when we unmask interrupts to make sure that the redistributor sees the update to PMR before we do WFI. My understanding is that the resdistributor is free to stop forwarding interrupts to the CPU interface if from its point of view those interrupts don't have a high enough priority. As for the ISB, I don't think we need one because writes to PMR are self-synchronizing, so the write to PMR should be seen before DSB SY and wfi. >> wfi >> + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR > > Likewise, we don't need any barriers here before we poke DAIF? Here we don't need DSB SY because the value being restored is either: - GIC_PRIO_IRQON which is the same as the current value, the redistributor is already aware of it. - GIC_PRIO_IRQOFF and the self-synchronization of PMR ensures that no interrupts with priorities lower than the value of PMR can be taken (this does not require to be seen by the redistributor). For the ISB, I have this small doubt about whether it is needed between WFI and MSR PMR. But there is this bit in the ARM ARM section D12.1.3 "General behavior of accesses to the AArch64 System registers", subsection "Synchronization requirements for AArch64 System registers": "Direct writes using the instructions in Table D11-2 on page D11-2660 require synchronization before software can rely on the effects of changes to the System registers to affect instructions appearing in program order after the direct write to the System register. Direct writes to these registers are not allowed to affect any instructions appearing in program order before the direct write." ICC_PMR_EL1 is part of the mentioned table. And reordering the direct write to PMR before the WFI would definitely affect the WFI instruction, so my interpretation is that this would not be allowed by the architecture. So I don't think we need the ISB either, but my understanding could be wrong. > >> + 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(); The issue with this is that in patch 10, arch_local_irq_* functions toggle PMR rather than PSR.I. I could use local_daif_mask but I don't think disabling debug and async is good. Otherwise and can do a small bit of inline assembly and have something like: static inline void __cpu_do_idle_irqprio(void) { unsigned long flags = arch_local_irq_save(); // set PSR.I gic_write_pmr(GIC_PRIO_IRQON); __cpu_do_idle(); arch_local_irq_restore(flags); // restores PMR and PSR.I } Otherwise I agree, moving it to C makes it more readable and avoid generating code that will never get called. I'll do it for the next version. Thanks, > + 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 > >
On Fri, Nov 30, 2018 at 10:55:47AM +0000, Julien Thierry wrote: > On 29/11/18 17:44, Mark Rutland wrote: > > On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote: > >> + 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? > > DSB SY is necessary when we unmask interrupts to make sure that the > redistributor sees the update to PMR before we do WFI. My understanding > is that the resdistributor is free to stop forwarding interrupts to the > CPU interface if from its point of view those interrupts don't have a > high enough priority. > > As for the ISB, I don't think we need one because writes to PMR are > self-synchronizing, so the write to PMR should be seen before DSB SY and > wfi. Having looked at ARM IHI 0069D, 8.1.6 "Observability of the effects of accesses to the GIC registers", I think I agree. My specific concern was that a CPU might complete the DSB before the MSR, but I think it's clear per the GIC spec it's clear that an ISB is not expected between the MSR and DSB, even if that's unusual. > >> wfi > >> + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR > > > > Likewise, we don't need any barriers here before we poke DAIF? > > Here we don't need DSB SY because the value being restored is either: > - GIC_PRIO_IRQON which is the same as the current value, the > redistributor is already aware of it. > - GIC_PRIO_IRQOFF and the self-synchronization of PMR ensures that no > interrupts with priorities lower than the value of PMR can be taken > (this does not require to be seen by the redistributor). > > For the ISB, I have this small doubt about whether it is needed between > WFI and MSR PMR. But there is this bit in the ARM ARM section D12.1.3 > "General behavior of accesses to the AArch64 System registers", > subsection "Synchronization requirements for AArch64 System registers": > > "Direct writes using the instructions in Table D11-2 on page D11-2660 > require synchronization before software can rely on the effects of > changes to the System registers to affect instructions appearing in > program order after the direct write to the System register. Direct > writes to these registers are not allowed to affect any instructions > appearing in program order before the direct write." > > ICC_PMR_EL1 is part of the mentioned table. I think that's a defect in the ARM ARM, given it disagrees with the GIC spec. > And reordering the direct write to PMR before the WFI would definitely > affect the WFI instruction, so my interpretation is that this would > not be allowed by the architecture. So I don't think we need the ISB > either, but my understanding could be wrong. We already assume that a DSB can't be re-ordered w.r.t. the WFI, so as long as the DSB can't complete before the MSR, I think we're good. > > > >> + 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(); > > The issue with this is that in patch 10, arch_local_irq_* functions > toggle PMR rather than PSR.I. > > I could use local_daif_mask but I don't think disabling debug and async > is good. Otherwise and can do a small bit of inline assembly and have > something like: Can we factor out the existing arch_local_irq_save() somehow? Thanks, Mark.
On 30/11/18 13:37, Mark Rutland wrote: > On Fri, Nov 30, 2018 at 10:55:47AM +0000, Julien Thierry wrote: >> On 29/11/18 17:44, Mark Rutland wrote: >>> On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote: > >>>> + 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? >> >> DSB SY is necessary when we unmask interrupts to make sure that the >> redistributor sees the update to PMR before we do WFI. My understanding >> is that the resdistributor is free to stop forwarding interrupts to the >> CPU interface if from its point of view those interrupts don't have a >> high enough priority. >> >> As for the ISB, I don't think we need one because writes to PMR are >> self-synchronizing, so the write to PMR should be seen before DSB SY and >> wfi. > > Having looked at ARM IHI 0069D, 8.1.6 "Observability of the effects of > accesses to the GIC registers", I think I agree. > > My specific concern was that a CPU might complete the DSB before the > MSR, but I think it's clear per the GIC spec it's clear that an ISB is > not expected between the MSR and DSB, even if that's unusual. > >>>> wfi >>>> + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR >>> >>> Likewise, we don't need any barriers here before we poke DAIF? >> >> Here we don't need DSB SY because the value being restored is either: >> - GIC_PRIO_IRQON which is the same as the current value, the >> redistributor is already aware of it. >> - GIC_PRIO_IRQOFF and the self-synchronization of PMR ensures that no >> interrupts with priorities lower than the value of PMR can be taken >> (this does not require to be seen by the redistributor). >> >> For the ISB, I have this small doubt about whether it is needed between >> WFI and MSR PMR. But there is this bit in the ARM ARM section D12.1.3 >> "General behavior of accesses to the AArch64 System registers", >> subsection "Synchronization requirements for AArch64 System registers": >> >> "Direct writes using the instructions in Table D11-2 on page D11-2660 >> require synchronization before software can rely on the effects of >> changes to the System registers to affect instructions appearing in >> program order after the direct write to the System register. Direct >> writes to these registers are not allowed to affect any instructions >> appearing in program order before the direct write." >> >> ICC_PMR_EL1 is part of the mentioned table. > > I think that's a defect in the ARM ARM, given it disagrees with the GIC > spec. > >> And reordering the direct write to PMR before the WFI would definitely >> affect the WFI instruction, so my interpretation is that this would >> not be allowed by the architecture. So I don't think we need the ISB >> either, but my understanding could be wrong. > > We already assume that a DSB can't be re-ordered w.r.t. the WFI, so as > long as the DSB can't complete before the MSR, I think we're good. > >>> >>>> + 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(); >> >> The issue with this is that in patch 10, arch_local_irq_* functions >> toggle PMR rather than PSR.I. >> >> I could use local_daif_mask but I don't think disabling debug and async >> is good. Otherwise and can do a small bit of inline assembly and have >> something like: > > Can we factor out the existing arch_local_irq_save() somehow? > I'm not sure I understand what you're suggesting. Using individual accessors for PMR and PSR.I instead of arch_local_irq_save? I am just a bit concerned about having too many functions to play with either (especially since most of the time those functions end up being single use). Thanks,
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 wfi + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR + msr daif, x0 // restore I bit ret ENDPROC(cpu_do_idle)
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(+)