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

[Jinjie Ruan <ruanjinjie@huawei.com>]

在 2026/8/21 18:27, Vladimir Murzin 写道:
> 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.

Make sense.

> 
>>> +	/*
>>> +	 * 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
> 
>