Re: [RFC PATCH v2 19/45] arm64: cpuidle: use new helpers to bypass interrupt priority masking

[Vladimir Murzin <vladimir.murzin@arm.com>]

On 8/11/26 09:59, Jinjie Ruan wrote:
> 
> 在 2026/7/28 0:34, Vladimir Murzin 写道:
>> From: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>>
>> When the CPU goes into WFI and pseudo-NMIs are in use, we need to make
>> sure that interrupts are not masked by the PMR otherwise they would not
>> reach the CPU and wake it up.
>>
>> Now that we have a proper API that handles both DAIF and PMR, extend it
>> to provide helpers allowing to temporarily switch to masking interrupts
>> via DAIF only when interrupt priorities are in use.
>> This could allow other parts of the code to make use of it and makes it
>> easier to check for proper use.
>>
>> Replace the custom cpuidle helper with those new helpers.
>>
>> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> ---
>>  arch/arm64/include/asm/cpuidle.h            | 18 ++----
>>  arch/arm64/include/asm/interrupts/masking.h | 61 +++++++++++++++++++++
>>  2 files changed, 65 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
>> index 2047713e097d..b70ef5fd5e57 100644
>> --- a/arch/arm64/include/asm/cpuidle.h
>> +++ b/arch/arm64/include/asm/cpuidle.h
>> @@ -5,32 +5,22 @@
>>  #include <asm/proc-fns.h>
>>  
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>> -#include <asm/arch_gicv3.h>
>> +#include <asm/interrupts/masking.h>
>>  
>>  struct arm_cpuidle_irq_context {
>> -	unsigned long pmr;
>> -	unsigned long daif_bits;
>> +	arm64_exc_hwstates_t arm64_context;
>>  };
>>  
>>  #define arm_cpuidle_save_irq_context(__c)				\
>>  	do {								\
>>  		struct arm_cpuidle_irq_context *c = __c;		\
>> -		if (system_uses_irq_prio_masking()) {			\
>> -			c->daif_bits = read_sysreg(daif);		\
>> -			write_sysreg(c->daif_bits | PSR_I_BIT | PSR_F_BIT, \
>> -				     daif);				\
>> -			c->pmr = gic_read_pmr();			\
>> -			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET); \
>> -		}							\
>> +		c->arm64_context = local_exceptions_force_daif_save(); 	\
>>  	} while (0)
>>  
>>  #define arm_cpuidle_restore_irq_context(__c)				\
>>  	do {								\
>>  		struct arm_cpuidle_irq_context *c = __c;		\
>> -		if (system_uses_irq_prio_masking()) {			\
>> -			gic_write_pmr(c->pmr);				\
>> -			write_sysreg(c->daif_bits, daif);		\
>> -		}							\
>> +		local_exceptions_force_daif_restore(c->arm64_context);	\
>>  	} while (0)
>>  #else
>>  struct arm_cpuidle_irq_context { };
>> diff --git a/arch/arm64/include/asm/interrupts/masking.h b/arch/arm64/include/asm/interrupts/masking.h
>> index 193089fda252..e13852442062 100644
>> --- a/arch/arm64/include/asm/interrupts/masking.h
>> +++ b/arch/arm64/include/asm/interrupts/masking.h
>> @@ -89,6 +89,67 @@ static inline void local_exceptions_final_mask(void)
>>  }
>>  #endif /* CONFIG_DEBUG_IRQFLAGS */
>>  
>> +/*
>> + * In some cases, WFI or guest entry for example, we always want interrupts
>> + * to reach the CPU even if masked. Masking via the PMR prevents them from
>> + * reaching the CPU and waking it up.
>> + * Force IRQ masking using DAIF by raising the priority mask
>> + * and setting the IF flags.
>> + *
>> + * Should only be called when IRQs are already masked.
>> + */
>> +static inline arm64_exc_hwstates_t local_exceptions_force_daif_save(void)
>> +{
>> +	arm64_exc_hwstates_t states = {};
>> +
>> +	if (!system_uses_irq_prio_masking())
>> +		return states;
>> +
>> +	/*
>> +	 * Cannot use lockdep_assert here as idle entry enables hardirqs
>> +	 * while keeping interrupts masked.
>> +	 */
>> +	WARN_ON_ONCE(!irqs_disabled());
>> +
>> +	states.saved.daif = read_sysreg(daif);
>> +	states.saved.pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> I'd suggest using gic_read_pmr() as it aligns better with the original
> implementation.
> 

IIUC, the reason gic_read_pmr() exists is to be used in code shared between
arm64 and arm32. Since we are entirely in arm64, IMO, we better use sysreg
directly.

>> +	/*
>> +	 * We might have IF set or unset. In case IF already set orring with IF
>> +	 * won't change anything, combined with PMR set to IRQON we might become
>> +	 * one of contexts: CRITICAL, ERROR or NONMI. In case IF is unset (which
>> +	 * also implies that DA is unset) orring with IF and combining with PMR
>> +	 * set to IRQON would effectively gives us NONMI context.
>> +	 */
> The comment seems redundant to me, because __arm64_update_exc_hwstate()
> only needs the hwstate parameter and has no dependency on the context type.
> 

Ok. I'll drop it in the next version.

> Otherwise, LGTM
> Reviewed-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 

Cheers
Vladimir