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

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

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.

> 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).

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.

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?

> 
> 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>
> ---
>  arch/arm64/include/asm/insn.h | 46 +++++++++++++++++++++++++++++++++--
>  arch/arm64/lib/insn.c         | 44 ---------------------------------
>  arch/arm64/net/bpf_jit.h      |  2 +-
>  3 files changed, 45 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 2d19f48aab81..c91a8431c961 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -703,8 +703,50 @@ u32 aarch64_insn_gen_cas(enum aarch64_insn_register result,
>  			 enum aarch64_insn_register value,
>  			 enum aarch64_insn_size_type size,
>  			 enum aarch64_insn_mem_order_type order);
> -u32 aarch64_insn_gen_dmb(enum aarch64_insn_mb_type type);
> -u32 aarch64_insn_gen_dsb(enum aarch64_insn_mb_type type);
> +
> +static __always_inline u32 aarch64_insn_gen_dmb(enum aarch64_insn_mb_type type)
> +{
> +	u32 insn;
> +
> +	switch (type) {
> +	case AARCH64_INSN_MB_LD:
> +	case AARCH64_INSN_MB_ST:
> +	case AARCH64_INSN_MB_SY:
> +		break;
> +	default:
> +		BUILD_BUG_ON_MSG(1, "invalid dmb type");
> +	}
> +
> +	insn = aarch64_insn_get_dmb_value();
> +	insn &= ~AARCH64_INSN_MB_TYPE_MASK;
> +	insn |= type;
> +
> +	return insn;
> +}
> +
> +static __always_inline u32 aarch64_insn_gen_dsb(enum aarch64_insn_mb_type type)
> +{
> +	u32 insn;
> +
> +	switch (type) {
> +	case AARCH64_INSN_MB_OSH:
> +	case AARCH64_INSN_MB_NSH:
> +	case AARCH64_INSN_MB_ISH:
> +	case AARCH64_INSN_MB_LD:
> +	case AARCH64_INSN_MB_ST:
> +	case AARCH64_INSN_MB_SY:
> +		break;
> +	default:
> +		BUILD_BUG_ON_MSG(1, "invalid dsb type");
> +	}
> +
> +	insn = aarch64_insn_get_dsb_base_value();
> +	insn &= ~AARCH64_INSN_MB_TYPE_MASK;
> +	insn |= type;
> +
> +	return insn;
> +}
> +
>  u32 aarch64_insn_gen_mrs(enum aarch64_insn_register result,
>  			 enum aarch64_insn_system_register sysreg);
>  
> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index fa61a631d687..5c37f3a4c402 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c
> @@ -1561,50 +1561,6 @@ u32 aarch64_insn_gen_extr(enum aarch64_insn_variant variant,
>  	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RM, insn, Rm);
>  }
>  
> -static bool __is_valid_barrier_type(enum aarch64_insn_mb_type type)
> -{
> -	switch (type) {
> -	case AARCH64_INSN_MB_OSH:
> -	case AARCH64_INSN_MB_NSH:
> -	case AARCH64_INSN_MB_ISH:
> -	case AARCH64_INSN_MB_LD:
> -	case AARCH64_INSN_MB_ST:
> -	case AARCH64_INSN_MB_SY:
> -		return true;
> -	default:
> -		pr_err("%s: unknown barrier type %d\n", __func__, type);
> -		return false;
> -	}
> -}
> -
> -u32 aarch64_insn_gen_dmb(enum aarch64_insn_mb_type type)
> -{
> -	u32 insn;
> -
> -	if (!__is_valid_barrier_type(type))
> -		return AARCH64_BREAK_FAULT;
> -
> -	insn = aarch64_insn_get_dmb_value();
> -	insn &= ~AARCH64_INSN_MB_TYPE_MASK;
> -	insn |= type;
> -
> -	return insn;
> -}
> -
> -u32 aarch64_insn_gen_dsb(enum aarch64_insn_mb_type type)
> -{
> -	u32 insn;
> -
> -	if (!__is_valid_barrier_type(type))
> -		return AARCH64_BREAK_FAULT;
> -
> -	insn = aarch64_insn_get_dsb_base_value();
> -	insn &= ~AARCH64_INSN_MB_TYPE_MASK;
> -	insn |= type;
> -
> -	return insn;
> -}
> -
>  u32 aarch64_insn_gen_mrs(enum aarch64_insn_register result,
>  			 enum aarch64_insn_system_register sysreg)
>  {
> 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

>  /* ADR */
>  #define A64_ADR(Rd, offset) \
> -- 2.43.0
>