Re: [RFC PATCH 14/36] arm64: interrupts: introduce generic interrupt masking helpers

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

On 7/14/26 14:13, Liao, Chang wrote:
> 在 2026/7/9 20:13, Vladimir Murzin 写道:
>> From: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>>
>> As for the entry code, we want to replace `local_daif_...` helpers
>> so that they can properly handle both DAIF and PMR, as well controlling
>> their use more strongly.
>>
>> Introduce new `local_all_irqs_...` helpers to replace them, which should
>> only be called in save/restore pairs.
>>
>> Save the requested interrupt state as well, so we can check for
>> inconsistent interrupt masking in between save and restore.
>>
>> There are two exceptions where it does not make sense to force
>> save/restore pairs for modifying the interrupt masks:
>>  - when initializing a CPU or
>>  - preparing to turn it off.
>>
>> As we otherwise want to force save/restore pairs, those cases are
>> handled with specific helpers, making clear that they should not be
>> used outside of those cases, enforced with `CONFIG_DEBUG_IRQFLAGS`
>> enabled.
>>
>> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> ---
>>  arch/arm64/include/asm/interrupts/masking.h | 101 ++++++++++++++++++++
>>  1 file changed, 101 insertions(+)
>>  create mode 100644 arch/arm64/include/asm/interrupts/masking.h
>>
>> diff --git a/arch/arm64/include/asm/interrupts/masking.h b/arch/arm64/include/asm/interrupts/masking.h
>> new file mode 100644
>> index 000000000000..66ee03f7ab68
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/interrupts/masking.h
>> @@ -0,0 +1,101 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2025 Arm Ltd.
>> + */
>> +#ifndef __ASM_INTERRUPTS_MASKING_H
>> +#define __ASM_INTERRUPTS_MASKING_H
>> +
>> +#include <asm/arch_gicv3.h>
>> +#include <asm/bug.h>
>> +#include <asm/cpufeature.h>
>> +#include <asm/interrupts/common_flags.h>
>> +#include <asm/ptrace.h>
>> +
>> +typedef struct arm64_exc_hwstates {
>> +	arm64_exc_hwstate_t saved;
>> +	arm64_exc_hwstate_t expected;
>> +} arm64_exc_hwstates_t;
>> +
>> +#ifdef CONFIG_DEBUG_IRQFLAGS
>> +/* Make sure the CPU init/tear down masking functions are only used once. */
>> +static DEFINE_PER_CPU(bool, irqs_masks_cpu_init_done);
>> +static DEFINE_PER_CPU(bool, irqs_masks_cpu_final_done);
> It is generally not a good idea to define static DEFINE_PER_CPU variables directly in
> a header file, as this creates a separate copy of the variable in every translation unit
> that includes it. Unless this header is strictly limited to a single source file. Please
> correct me if this was designed for some special purpose.

Yup. As I already said in another reply I either need to find an
alternative implementation or move it into a C file.

> 
>> +#endif
>> +
>> +static inline
>> +arm64_exc_hwstates_t local_all_irqs_save_mask(arm64_exc_context_t new)
>> +{
>> +	arm64_exc_hwstate_t state = arm64_exc_hwstate_of_context(new);
>> +	arm64_exc_hwstate_t actual = {.flags = arch_local_save_flags()};
>> +
>> +	if (IS_ENABLED(CONFIG_DEBUG_IRQFLAGS)) {
>> +		bool pnmi = system_uses_irq_prio_masking();
>> +
>> +		WARN_ON_ONCE(new < CRITICAL_CONTEXT &&
>> +			     actual.daif == DAIF_MASK);
>> +
>> +		WARN_ON_ONCE(new < ERROR_CONTEXT &&
>> +			     actual.daif == DAIF_ERRCTX);
>> +
>> +		WARN_ON_ONCE(new < NONMI_CONTEXT &&
>> +			     pnmi && actual.daif == DAIF_PROCCTX_NOIRQ);
>> +
>> +		WARN_ON_ONCE(new < NOIRQ_CONTEXT &&
>> +			     ((pnmi && actual.pmr == GIC_PRIO_IRQOFF) ||
>> +			      (!pnmi && actual.daif == DAIF_PROCCTX_NOIRQ)));
>> +	}
> This verification logic is almost same with the counterpart in arm64_lift_exc_context().
> Would it make sense to factor this out into a shared helper function to avoid duplication?
> 

Yup, it is almost the same. I intentionally keep these checks local to
these functions (for now) rather than spreading them throughout the
codebase.

>> +
>> +	arm64_update_exc_hwstate(state, actual.pmr != state.pmr);
>> +
>> +	if (!arch_irqs_disabled_flags(actual.flags))
>> +		trace_hardirqs_off();
>> +
>> +	return (arm64_exc_hwstates_t){.saved = actual, .expected = state};
>> +}
>> +
>> +static inline void local_all_irqs_restore(arm64_exc_hwstates_t states)
>> +{
>> +	arm64_debug_exc_hwstate(states.expected);
>> +
>> +	if (!arch_irqs_disabled_flags(states.saved.flags))
>> +		trace_hardirqs_on();
>> +
>> +	arm64_update_exc_hwstate(states.saved, true);
>> +}
>> +
>> +#ifdef CONFIG_DEBUG_IRQFLAGS
>> +static inline
>> +void local_all_irqs_cpu_init_mask(arm64_exc_context_t context)
>> +{
>> +	WARN_ON(__this_cpu_read(irqs_masks_cpu_init_done));
>> +	if (context == PROCESS_CONTEXT)
>> +		trace_hardirqs_on();
>> +	arm64_update_exc_context(context, true);
>> +	__this_cpu_write(irqs_masks_cpu_init_done, true);
>> +	__this_cpu_write(irqs_masks_cpu_final_done, false);
>> +}
>> +
>> +static inline void local_all_irqs_final_mask(void)
>> +{
>> +	WARN_ON(__this_cpu_read(irqs_masks_cpu_final_done));
>> +	arm64_update_exc_context(CRITICAL_CONTEXT, true);
>> +	trace_hardirqs_off();
>> +	__this_cpu_write(irqs_masks_cpu_final_done, true);
>> +	__this_cpu_write(irqs_masks_cpu_init_done, false);
>> +}
>> +#else /* CONFIG_DEBUG_IRQFLAGS */
>> +static inline
>> +void local_all_irqs_cpu_init_mask(arm64_exc_context_t context)
>> +{
>> +	if (context == PROCESS_CONTEXT)
>> +		trace_hardirqs_on();
>> +	arm64_update_exc_context(context, true);
>> +}
>> +
>> +static inline void local_all_irqs_final_mask(void)
>> +{
>> +	arm64_update_exc_context(CRITICAL_CONTEXT, true);
>> +	trace_hardirqs_off();
>> +}
>> +#endif /* CONFIG_DEBUG_IRQFLAGS */
>> +#endif /* __ASM_INTERRUPTS_MASKING_H */
> 
> -- BR Liao, Chang
> 

Cheers
Vladimir