Re: [RFC PATCH 10/36] arm64: irqflags: save and use both DAIF and PMR

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

On 7/10/26 10:02, Jinjie Ruan wrote:
> 
> On 7/9/2026 8:13 PM, Vladimir Murzin wrote:
>> From: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>>
>> With pseudo-NMIs enabled, both DAIF and the PMR affect interrupt masking.
>> Now that we have a type which can track both of them at the same time,
>> update our irqflags implementation to use it.
>>
>> Save DAIF flags in all cases, as they can be manipulated directly by other
>> code, and the PMR if it is in use.
>>
>> When checking if IRQs are disabled, now that we always save DAIF we can
>> check that the I flag is set and bypass checking the PMR if it is.
>> We can also properly check if PMR masks interrupts (PMR < GIC_PRIO_IRQON),
>> now that we don't need to rely on the GIC_PRIO_PSR_I_SET bit being set in
>> the PMR to know if DAIF is already masking interrupts.
>> Update `irqs_priority_unmasked()` to align with this change.
>>
>> This allows us to remove the `__daif_...` and `__pmr_...` versions
>> of the save and check functions, as they are now unified.
>>
>> We can reasonably merge the two `__{daif,pmr}_irq_restore()` functions
>> in the main one, as the DAIF and PMR values are properly split now.
>>
>> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> ---
>>  arch/arm64/include/asm/irqflags.h | 110 ++++++------------------------
>>  arch/arm64/include/asm/ptrace.h   |   2 +-
>>  2 files changed, 23 insertions(+), 89 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index 7775904ba6a9..62f047702493 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -95,117 +95,48 @@ static __always_inline void arch_local_irq_disable(void)
>>  	}
>>  }
>>  
>> -static __always_inline arm64_exc_hwstate_t __daif_local_save_flags(void)
>> -{
>> -	return (arm64_exc_hwstate_t){ .daif = read_sysreg(daif) };
>> -}
>> -
>> -static __always_inline arm64_exc_hwstate_t __pmr_local_save_flags(void)
>> -{
>> -	return (arm64_exc_hwstate_t){ .pmr = read_sysreg_s(SYS_ICC_PMR_EL1) };
>> -}
>> -
>>  /*
>>   * Save the current interrupt enable state.
>>   */
>>  static __always_inline unsigned long arch_local_save_flags(void)
>>  {
>> -	if (system_uses_irq_prio_masking()) {
>> -		return __pmr_local_save_flags().flags;
>> -	} else {
>> -		return __daif_local_save_flags().flags;
>> -	}
>> -}
>> +	arm64_exc_hwstate_t hwstate = { .daif = read_sysreg(daif) };
>>  
>> -static __always_inline
>> -bool __daif_irqs_disabled_flags(arm64_exc_hwstate_t hwstate)
>> -{
>> -	return hwstate.daif & PSR_I_BIT;
>> -}
>> +	if (system_uses_irq_prio_masking())
>> +		hwstate.pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>>  
>> -static __always_inline
>> -bool __pmr_irqs_disabled_flags(arm64_exc_hwstate_t hwstate)
>> -{
>> -	return hwstate.pmr != GIC_PRIO_IRQON;
>> +	return hwstate.flags;
>>  }
>>  
>>  static __always_inline bool arch_irqs_disabled_flags(unsigned long flags)
>>  {
>>  	arm64_exc_hwstate_t hwstate = { .flags = flags };
>>  
>> -	if (system_uses_irq_prio_masking()) {
>> -		return __pmr_irqs_disabled_flags(hwstate);
>> -	} else {
>> -		return __daif_irqs_disabled_flags(hwstate);
>> -	}
>> -}
>> +	/* If I is set, the PMR doesn't matter: interrupts will not be taken. */
>> +	if (hwstate.daif & PSR_I_BIT)
>> +		return true;
>>  
>> -static __always_inline bool __daif_irqs_disabled(void)
>> -{
>> -	return __daif_irqs_disabled_flags(__daif_local_save_flags());
>> -}
>> +	if (system_uses_irq_prio_masking() && hwstate.pmr < GIC_PRIO_IRQON)
> I think "hwstate.pmr == GIC_PRIO_IRQOFF" is fine, Besides DAIF.I bit set
> and pmr is GIC_PRIO_IRQOFF, are there any other possible cases?
> 

Numerically, PMR can hold values ordered as:

  IRQOFF < IRQON < IRQON | GIC_PRIO_PSR_I_SET

So hwstate.pmr < GIC_PRIO_IRQON is indeed IRQOFF.

The only other possibility would be a buggy case with the DAIF.I bit
clear and PMR set to IRQON | GIC_PRIO_PSR_I_SET.

So I think we can temporarily add a debug check for that, which would
disappear together with GIC_PRIO_PSR_I_SET. Something like:

static __always_inline bool arch_irqs_disabled_flags(unsigned long flags)
{
	arm64_exc_hwstate_t hwstate = { .flags = flags };

	if (hwstate.daif & PSR_I_BIT)
		return true;

	if (system_uses_irq_prio_masking()) {
		WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_IRQFLAGS) &&
			     hwstate.pmr == GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);

		return hwstate.pmr != GIC_PRIO_IRQON;
	}

	return false;
}

>> +		return true;
>>  
>> -static __always_inline bool __pmr_irqs_disabled(void)
>> -{
>> -	return __pmr_irqs_disabled_flags(__pmr_local_save_flags());
>> +	return false;
>>  }
>>  
>>  static __always_inline bool arch_irqs_disabled(void)
>>  {
>> -	if (system_uses_irq_prio_masking()) {
>> -		return __pmr_irqs_disabled();
>> -	} else {
>> -		return __daif_irqs_disabled();
>> -	}
>> -}
>> -
>> -static __always_inline arm64_exc_hwstate_t __daif_local_irq_save(void)
>> -{
>> -	arm64_exc_hwstate_t hwstate = __daif_local_save_flags();
>> -
>> -	__daif_local_irq_disable();
>> -
>> -	return hwstate;
>> -}
>> -
>> -static __always_inline arm64_exc_hwstate_t __pmr_local_irq_save(void)
>> -{
>> -	arm64_exc_hwstate_t hwstate = __pmr_local_save_flags();
>> -
>> -	/*
>> -	 * There are too many states with IRQs disabled, just keep the current
>> -	 * state if interrupts are already disabled/masked.
>> -	 */
>> -	if (!__pmr_irqs_disabled_flags(hwstate))
>> -		__pmr_local_irq_disable();
>> -
>> -	return hwstate;
>> +	return arch_irqs_disabled_flags(arch_local_save_flags());
>>  }
>>  
>>  static __always_inline unsigned long arch_local_irq_save(void)
>>  {
>> -	if (system_uses_irq_prio_masking()) {
>> -		return __pmr_local_irq_save().flags;
>> -	} else {
>> -		return __daif_local_irq_save().flags;
>> -	}
>> -}
>> +	unsigned long flags = arch_local_save_flags();
>>  
>> -static __always_inline
>> -void __daif_local_irq_restore(arm64_exc_hwstate_t hwstate)
>> -{
>> -	barrier();
>> -	write_sysreg(hwstate.daif, daif);
>> -	barrier();
>> -}
>> +	if (system_uses_irq_prio_masking())
>> +		__pmr_local_irq_disable();
>> +	else
>> +		__daif_local_irq_disable();
>>  
>> -static __always_inline
>> -void __pmr_local_irq_restore(arm64_exc_hwstate_t hwstate)
>> -{
>> -	barrier();
>> -	write_sysreg_s(hwstate.pmr, SYS_ICC_PMR_EL1);
>> -	pmr_sync();
>> -	barrier();
>> +	return flags;
>>  }
>>  
>>  /*
>> @@ -215,11 +146,14 @@ static __always_inline void arch_local_irq_restore(unsigned long flags)
>>  {
>>  	arm64_exc_hwstate_t hwstate = { .flags = flags };
>>  
>> +	barrier();
>>  	if (system_uses_irq_prio_masking()) {
>> -		__pmr_local_irq_restore(hwstate);
>> +		write_sysreg_s(hwstate.pmr, SYS_ICC_PMR_EL1);
>> +		pmr_sync();
>>  	} else {
>> -		__daif_local_irq_restore(hwstate);
>> +		write_sysreg(hwstate.daif, daif);
>>  	}
>> +	barrier();
>>  }
>>  
>>  #endif /* __ASM_IRQFLAGS_H */
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index f7dc5fb9427d..192eb97cd50b 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -205,7 +205,7 @@ static inline void forget_syscall(struct pt_regs *regs)
>>  
>>  #define irqs_priority_unmasked(regs)					\
>>  	(system_uses_irq_prio_masking() ?				\
>> -		(regs)->pmr == GIC_PRIO_IRQON :				\
>> +		(regs)->pmr >= GIC_PRIO_IRQON :				\
> I think "=== GIC_PRIO_IRQON" is fine.
> 

Agree. This can be aligned to arch_irqs_disabled_flags() (maybe as a separate
patch):

static __always_inline bool regs_irqs_disabled(const struct pt_regs *regs)
{
	if (regs->pstate & PSR_I_BIT)
		return true;

	if (system_uses_irq_prio_masking()) {
		WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_IRQFLAGS) &&
			     regs->pmr == GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);

		return regs->pmr != GIC_PRIO_IRQON;
	}

	return false;
}

What do you reckon?

> otherwise, LGTM
> Reviewed-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
>>  		true)
>>  
>>  static __always_inline bool regs_irqs_disabled(const struct pt_regs *regs)
> 

Cheers
Vladimir