Re: [PATCH 4/4] arm64: insn: clean-up sysregs handling

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

On 10/16/26 17:46, Ada Couprie Diaz wrote:
> System register encodings are already defined in <asm/sysreg.h>, so
> use those definitions in <asm/insn.h> instead of duplicating them.
> This will avoid future errors and deviations.
> 
> Remove the unnecessary shifts for encoding/decoding now that the enum
> values directly match the encoded registers.
> 
> No code using the insn register encodings currently makes a distinction
> between "regular" system registers and special-purpose registers,
> so let's just drop the difference and merge the two enums.
> Remove all unused registers while we are here.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/insn.h          | 36 ++++++++------------------
>  arch/arm64/kernel/probes/decode-insn.c |  2 +-
>  arch/arm64/lib/insn.c                  |  4 +--
>  3 files changed, 14 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index e020f9d8b83c2..6533191638804 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -12,6 +12,7 @@
>  #include <linux/types.h>
>  
>  #include <asm/insn-def.h>
> +#include <asm/sysreg.h>
>  
>  #ifndef __ASSEMBLER__
>  
> @@ -113,38 +114,23 @@ enum aarch64_insn_register {
>  	AARCH64_INSN_REG_SP = 31  /* Stack pointer: as load/store base reg */
>  };
>  
> +/* System register encoding for MRS/MSR (register) instructions. */
>  #define AARCH64_INSN_SYSREG_OFFSET	5
>  #define AARCH64_INSN_SYSREG_MASK	GENMASK(20, AARCH64_INSN_SYSREG_OFFSET)
> +#define AARCH64_INSN_SYSREG_FROM_DEF(sysreg) \

bikeshedding: perhaps __AARCH64_INSN_SYSREG to match other helper macro in this
file?

> +	AARCH64_INSN_SYSREG_##sysreg = SYS_##sysreg
>  
> -enum aarch64_insn_special_register {
> -	AARCH64_INSN_SPCLREG_SPSR_EL1	= 0xC200,
> -	AARCH64_INSN_SPCLREG_ELR_EL1	= 0xC201,
> -	AARCH64_INSN_SPCLREG_SP_EL0	= 0xC208,
> -	AARCH64_INSN_SPCLREG_SPSEL	= 0xC210,
> -	AARCH64_INSN_SPCLREG_CURRENTEL	= 0xC212,
> -	AARCH64_INSN_SPCLREG_NZCV	= 0xDA10,
> -	AARCH64_INSN_SPCLREG_DAIF	= 0xDA11,
> -	AARCH64_INSN_SPCLREG_FPCR	= 0xDA20,
> -	AARCH64_INSN_SPCLREG_DSPSR_EL0	= 0xDA28,
> -	AARCH64_INSN_SPCLREG_DLR_EL0	= 0xDA29,
> -	AARCH64_INSN_SPCLREG_SPSR_EL2	= 0xE200,
> -	AARCH64_INSN_SPCLREG_ELR_EL2	= 0xE201,
> -	AARCH64_INSN_SPCLREG_SP_EL1	= 0xE208,
> -	AARCH64_INSN_SPCLREG_SPSR_IRQ	= 0xE218,
> -	AARCH64_INSN_SPCLREG_SPSR_ABT	= 0xE219,
> -	AARCH64_INSN_SPCLREG_SPSR_UND	= 0xE21A,
> -	AARCH64_INSN_SPCLREG_SPSR_FIQ	= 0xE21B,
> -	AARCH64_INSN_SPCLREG_SPSR_EL3	= 0xF200,
> -	AARCH64_INSN_SPCLREG_ELR_EL3	= 0xF201,
> -	AARCH64_INSN_SPCLREG_SP_EL2	= 0xF208,
> -};
>  

The only register in use was AARCH64_INSN_SPCLREG_DAIF which is
now replaced with direct use of AARCH64_INSN_SYSREG_DAIF.

> +/* Do not duplicate encodings: we already define them in <asm/sysreg.h> */
>  enum aarch64_insn_system_register {
> -	AARCH64_INSN_SYSREG_SP_EL0	= 0xC208,
> -	AARCH64_INSN_SYSREG_TPIDR_EL1	= 0xC684,
> -	AARCH64_INSN_SYSREG_TPIDR_EL2	= 0xE682,
> +	AARCH64_INSN_SYSREG_FROM_DEF(SP_EL0),
> +	AARCH64_INSN_SYSREG_FROM_DEF(TPIDR_EL1),
> +	AARCH64_INSN_SYSREG_FROM_DEF(DAIF),
> +	AARCH64_INSN_SYSREG_FROM_DEF(TPIDR_EL2),
>  };
>  
> +#undef AARCH64_INSN_SYSREG_FROM_DEF
> +
>  enum aarch64_insn_variant {
>  	AARCH64_INSN_VARIANT_32BIT,
>  	AARCH64_INSN_VARIANT_64BIT
> diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c
> index 4137cc5ef031f..bb0af08105885 100644
> --- a/arch/arm64/kernel/probes/decode-insn.c
> +++ b/arch/arm64/kernel/probes/decode-insn.c
> @@ -40,7 +40,7 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn)
>  		 */
>  		if (aarch64_insn_is_mrs(insn))
>  			return aarch64_insn_extract_system_reg(insn)
> -			     != AARCH64_INSN_SPCLREG_DAIF;
> +			     != AARCH64_INSN_SYSREG_DAIF;
>  
>  		/*
>  		 * The HINT instruction is steppable only if it is in whitelist
> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index 2edc5a9c0a9c8..1611c86885c9f 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c
> @@ -1364,7 +1364,7 @@ u32 aarch64_insn_adrp_set_offset(u32 insn, s32 offset)
>   */
>  noinstr u32 aarch64_insn_extract_system_reg(u32 insn)
>  {
> -	return (insn & AARCH64_INSN_SYSREG_MASK) >> AARCH64_INSN_SYSREG_OFFSET;
> +	return insn & AARCH64_INSN_SYSREG_MASK;
>  }
>  
>  bool aarch32_insn_is_wide(u32 insn)
> @@ -1567,7 +1567,7 @@ u32 aarch64_insn_gen_mrs(enum aarch64_insn_register result,
>  	u32 insn = aarch64_insn_get_mrs_value();
>  
>  	insn &= ~AARCH64_INSN_SYSREG_MASK;
> -	insn |= sysreg << AARCH64_INSN_SYSREG_OFFSET;
> +	insn |= sysreg;
>  	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT,
>  					    insn, result);
>  }
> -- 2.43.0
> 

FWIW,

Reviewed-by: Vladimir Murzin <vladimir.murzin@arm.com>