Re: [RFC PATCH 14/16] arm64/insn: always inline aarch64_insn_encode_ldst_size()

[Marc Zyngier <maz@kernel.org>]

On Tue, 23 Sep 2025 18:49:01 +0100,
Ada Couprie Diaz <ada.coupriediaz@arm.com> wrote:
> 
> The type and instruction checks cannot be made at compile time,
> as they are dynamically created. However, we can remove the error print
> as it should never appear in normal operation and will still lead to
> a fault BRK.
> 
> This makes `aarch64_insn_encode_ldst_size()` safe for inlining
> and usage from patching callbacks.
> 
> This is a change of visiblity, as previously the function was private to
> lib/insn.c.
> However, in order to inline more `aarch64_insn_` functions and make
> patching callbacks safe, it needs to be accessible by those functions.
> As it is more accessible than before, add a check so that only loads
> or stores can be affected by the size encoding.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/insn.h | 24 ++++++++++++++++++++++++
>  arch/arm64/lib/insn.c         | 19 +------------------
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 44435eede1f3..46d4d452e2e2 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -717,6 +717,30 @@ static __always_inline u32 aarch64_insn_encode_immediate(
>  
>  	return insn;
>  }
> +
> +extern const u32 aarch64_insn_ldst_size[];
> +static __always_inline u32 aarch64_insn_encode_ldst_size(
> +					 enum aarch64_insn_size_type type,
> +					 u32 insn)
> +{
> +	u32 size;
> +
> +	if (type < AARCH64_INSN_SIZE_8 || type > AARCH64_INSN_SIZE_64) {
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	/* Don't corrput the top bits of other instructions which aren't a size. */
> +	if (!aarch64_insn_is_ldst(insn)) {
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	size = aarch64_insn_ldst_size[type];

Given that we have this:

	enum aarch64_insn_size_type {
		AARCH64_INSN_SIZE_8,
		AARCH64_INSN_SIZE_16,
		AARCH64_INSN_SIZE_32,
		AARCH64_INSN_SIZE_64,
	};

[...]

> +	insn &= ~GENMASK(31, 30);
> +	insn |= size << 30;
> +
> +	return insn;
> +}
> +
>  static __always_inline u32 aarch64_insn_encode_register(
>  				 enum aarch64_insn_register_type type,
>  				 u32 insn,
> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index 71df4d72ac81..63564d236235 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c
> @@ -42,30 +42,13 @@ u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
>  	return (insn >> shift) & mask;
>  }
>  
> -static const u32 aarch64_insn_ldst_size[] = {
> +const u32 aarch64_insn_ldst_size[] = {
>  	[AARCH64_INSN_SIZE_8] = 0,
>  	[AARCH64_INSN_SIZE_16] = 1,
>  	[AARCH64_INSN_SIZE_32] = 2,
>  	[AARCH64_INSN_SIZE_64] = 3,
>  };

[...] this array is completely superfluous, and

	size = aarch64_insn_ldst_size[type];

could be replaced by

	size = type;

But that's only a very minor improvement. On the plus side, your
approach definitely makes it impossible to add a patching callback
using aarch64_insn_encode_ldst_size() in a module!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.