Re: [PATCH 2/2] arm64: insn: make `aarch64_insn_gen_d{m,s}b()` alternative-safe

[Ada Couprie Diaz <ada.coupriediaz@arm.com>]

Hi Vladimir,

On 19/10/2026 11:49, Vladimir Murzin wrote:
> Hi Ada,
>
> On 10/16/26 17:51, Ada Couprie Diaz wrote:
>> Move `aarch64_insn_gen_d{m,s}b()` and `__get_barrier_crm_val()`
> __get_barrier_crm_val()/__is_valid_barrier_type() has gone, so
> reference to it confuses.
Ah, my bad indeed, I forgot to take it out when reworking this part.
>> to `insn.h` and mark them `__always_inline`.
>> Replace the error print by an inline build-time check
>> now that the compiler can verify all call sites.
> My understanding is that error printing is not alternative-safe,
> so we replace it with a build-time check. The latter requires the
> barrier type to be visible to the compiler, hence moving the
> helper to a header and marking it __always_inline (with the side
> effect that the code may be inlined into noinstr functions).
That is correct !
> However, is it safe to assume that the barrier type will always
> be visible to the compiler? This is true today, but I can imagine
> a scenario where the barrier type is passed through a helper
> defined in a separate translation units, making it impossible for
> the compiler to observe its value at build time. I'm not sure
> whether this is something we should worry about right now.
You are right that an indirect call with variable barrier types would
lead to a build failure, even for "valid" code.
The current helpers in BPF are defines and have fixed barrier types,
which works great with this.

I think that being able to statically verify the validity of instruction
generation is a strongly desired characteristic, so I would be happy
prohibiting other uses that would prevent this.
However, I understand that there is a tradeoff here and that
not everyone might find the benefit worth it.

As it stands, it doesn't impact users of the `aarch64_insn_...`
functions, so now might be the time to think about what we
want here, rather than later when there would be other constraints.
> Another thing I'm wondering about is how we plan to make other
> helpers alternative-safe. Do we eventually intend to move all of
> them into header?
>
> I also noticed that we're not entirely consistent with error
> reporting across helpers. In several cases, we simply return
> AARCH64_BREAK_FAULT without printing anything to the log. Perhaps
> a better approach to making these helpers alternative-safe would
> be to remove error printing altogether, mark them noinstr, and
> return AARCH64_BREAK_FAULT whenever we cannot construct a valid
> opcode?
There's indeed a lot of inconsistency and other helpers not
covered here. Originally[1], I did try to make all the helpers used
in alternative patching safe and this already required to touch
many different helpers which were not really consistent.

As the goal was to make alternative patching safe, I did indeed
move as many helper functions to be `__always_inline` and thus
to the header, as I wanted to benefit from the static validation
that provided.

Trying to phrase my thinking process : to me, it feels like
generating instructions is something that, as much as possible,
should never be able to fail at runtime. Because then, what do
we do if we tried to generate an instruction, and fail ?
For alternative patching, that might be an immediate `BUG()`,
but there might be little to work around the issue
until a fix is made upstream, which could have otherwise been
detected during building...
All of that to say that we /could/ standardize on returning
`AARCH64_BREAK_FAULT` when failing to generate an instruction,
but to me it feels more like a liability that we should try to remove.

Very interested by other's thoughts on this question !

>> As BPF is the only caller, with static arguments, only accept
>> non-deprecated types for each barrier, as per Arm ARM DDI 0487
>> version M.c[0], and update `A64_DMB_ISH` accordingly.
>>
>> As all functions are now inlined and do not call any instrumentable
>> function, they are now safe to call in alternative callbacks.
>>
>> [0]: https://support.arm.com/documentation/ddi0487/mc/
>>           (See C6.2.150, C6.2.152)
>>
>> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> ---
>> [...]
>> diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h
>> index b2fe6e6dcf44..08d514b40a5b 100644
>> --- a/arch/arm64/net/bpf_jit.h
>> +++ b/arch/arm64/net/bpf_jit.h
>> @@ -324,7 +324,7 @@
>>   #define A64_NOP    A64_HINT(AARCH64_INSN_HINT_NOP)
>>   
>>   /* DMB */
>> -#define A64_DMB_ISH aarch64_insn_gen_dmb(AARCH64_INSN_MB_ISH)
>> +#define A64_DMB_ISH aarch64_insn_gen_dmb(AARCH64_INSN_MB_SY)
>>   
> ISH - This option has the same behavior as SY and is deprecated.
>
> so conversion is valid.
>
> Cheers
> Vladimir
>
Thanks for the review !
Kind regards,
Ada

[1]: 
https://lore.kernel.org/r/20250923174903.76283-1-ada.coupriediaz@arm.com