Re: [RFC PATCH v2 12/45] arm64: interrupts: Add common exception state helpers

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

On 7/28/26 09:20, Jinjie Ruan wrote:
> 
[snip]

Sorry, Jinjie, I missed other comments...

>> +
>> +static __always_inline
>> +void __arm64_update_exc_hwstate(arm64_exc_hwstate_t hwstate, bool force)
>> +{
>> +	barrier();
> I don't understand the purpose of the barrier at the beginning and end
> of the function. Is this fix also required for the original
> implementation of local_daif_restore()?
> 

The entry and exit barrier() calls prevent the compiler from moving
surrounding memory accesses across the exception state switch, so
caller memory accesses remain on the intended side of the exception
state switch. This is modeled after local_irq_{en,dis}able() and is
indeed different from local_daif_{restore,mask}().

__arm64_update_exc_hwstate() is a generic helper which can be used to
mask and unmask exceptions, while local_daif_{restore,mask}() seem to
be unidirectional. local_daif_mask() (and local_daif_save()) mask
exceptions and have a compiler barrier in the form of a memory clobber,
while local_daif_restore() and local_daif_inherit(), IIUC, are used
to unmask exceptions, and I could not spot a compiler barrier in any
form there.

I _assume_ that having a relaxed local_daif_{restore,inherit}() is OK,
yet I have not validated all use cases.

>> +
>> +	if (system_uses_irq_prio_masking() &&
>> +	    hwstate.pmr == GIC_PRIO_IRQOFF &&
>> +	    force) {
>> +		/*
>> +		 * There has been concern that the write to daif
>> +		 * might be reordered before this write to PMR.
>> +		 * From the ARM ARM DDI 0487D.a, section D1.7.1
>> +		 * "Accessing PSTATE fields":
>> +		 *   Writes to the PSTATE fields have side-effects on
>> +		 *   various aspects of the PE operation. All of these
>> +		 *   side-effects are guaranteed:
>> +		 *     - Not to be visible to earlier instructions in
>> +		 *       the execution stream.
>> +		 *     - To be visible to later instructions in the
>> +		 *       execution stream
>> +		 *
>> +		 * Also, writes to PMR are self-synchronizing, so no
>> +		 * interrupts with a lower priority than PMR is signaled
>> +		 * to the PE after the write.
>> +		 *
>> +		 * So we don't need additional synchronization here.
>> +		 */
>> +		write_sysreg_s(hwstate.pmr, SYS_ICC_PMR_EL1);
>> +	}
>> +
>> +	barrier();
> This barrier seems redundant and inconsistent with the comment above.
> 

This is a compiler barrier and has nothing to do with the comment
above. However, it is not needed here and bellow, since the sysreg
accessors use asm volatile, so the compiler preserves the relative
order of the PMR and DAIF accesses.

Cheers
Vladimir

>> +	write_sysreg(hwstate.daif, daif);
>> +	barrier();
>> +
>> +	if (system_uses_irq_prio_masking() &&
>> +	    hwstate.pmr == GIC_PRIO_IRQON &&
>> +	    force) {
>> +		write_sysreg_s(hwstate.pmr, SYS_ICC_PMR_EL1);
>> +		pmr_sync();
>> +	}
>> +
>> +	barrier();
>> +}
> If we no use pesudo NMI, the barrier is not nessary,
> 
> static __always_inline
> void __arm64_update_exc_hwstate(arm64_exc_hwstate_t hwstate, bool force)
> {
>     if (system_uses_irq_prio_masking() && force) {
>         if (hwstate.pmr == GIC_PRIO_IRQOFF)
>             write_sysreg_s(hwstate.pmr, SYS_ICC_PMR_EL1);
> 
>         write_sysreg(hwstate.daif, daif);
>         if (hwstate.pmr == GIC_PRIO_IRQON) {
>             barrier();
>             write_sysreg_s(hwstate.pmr, SYS_ICC_PMR_EL1);
>             pmr_sync();
>         }
>     } else {
>         write_sysreg(hwstate.daif, daif);
>     }
> }
> 
> otherwise, LGTM
> Reviewed-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
>> +
>> +static __always_inline
>> +void arm64_update_exc_hwstate(arm64_exc_hwstate_t hwstate)
>> +{
>> +	__arm64_update_exc_hwstate(hwstate, true);
>> +}
>> +
>> +static __always_inline
>> +void arm64_update_exc_context(arm64_exc_context_t context)
>> +{
>> +	arm64_exc_hwstate_t hwstate = arm64_exc_hwstate_of_context(context);
>> +
>> +	arm64_update_exc_hwstate(hwstate);
>> +}
>> +#endif /* __ASM_INTERRUPTS_COMMON_FLAGS_H */
>