Re: [PATCH v6 08/24] arm64: Unmask PMR before going idle

[Julien Thierry <julien.thierry@arm.com>]

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
>  
> 

-- 
Julien Thierry