Re: [PATCH 5/6] arm64: insn: operate on MSR/MRS sysreg field via defines

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

Hi Vladimir,

On 07/10/2026 07:53, Vladimir Murzin wrote:
> Hi Ada,
>
> On 9/28/26 14:30, Ada Couprie Diaz wrote:
>> Replace the few instances of hard-coded system register offset and masks
>> used to operate on MSR/MRS instructions with defines.
>> This will allow re-use in future commits while making the connection
>> between those values more explicit.
>>
>> While we are here, mark `aarch64_insn_extract_system_reg()` `noinstr`
>> so it can be safe to use in alternative pacthing callbacks.
>>
>> Changing the mask used in `aarch64_insn_gen_mrs()` to exclude
>> the lower bits does not change behaviour,
>> as `aarch64_insn_encode_register()` already clears the bits
>> used to encode the target register.
>>
>> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> ---
>>   arch/arm64/include/asm/insn.h | 3 +++
>>   arch/arm64/lib/insn.c         | 8 ++++----
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 40f13d28a5fd7..03620f67a3e47 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -753,6 +753,9 @@ static __always_inline u32 aarch64_insn_gen_dsb(enum aarch64_insn_mb_type type)
>>   	return insn;
>>   }
>>   
>> +#define AARCH64_INSN_SYSREG_OFFSET	5
>> +#define AARCH64_INSN_SYSREG_MASK	GENMASK(19, 5)
>> +
> Sashiko has raised comment [1]
>
> | Does this mask unintentionally exclude bit 20?
> | The previous hardcoded mask was 0x1FFFE0, which is equivalent to
> | GENMASK(20, 5) and includes 16 bits. Defining it as GENMASK(19, 5) yields
> | a 15-bit mask.
>
> [1] https://sashiko.dev/#/patchset/20260928133034.243541-1-ada.coupriediaz%40arm.com
>
> Cheers
> Vladimir

Thanks for bringing the report up, it does raise an interesting issue.

This does change the mask used and does indeed change the returned value,
potentially breaking comparisons to the return value of `aarch64_insn_extract_system_reg()`. However, the existing mask is 
incorrect as far as I can tell : the function is supposed to extract the 
Op and CR corresponding to the system registers of the MSR/MRS 
instructions. Those fields are encoded in bits 19-5, which corresponds 
to the GENMASK I used. I think it would make more sense to fix the `enum 
aarch64_insn_special_register` values to represent the registers 
properly and drop bit 20, which is 1 in all cases anyway... I would be 
happy to send a quick v2 with this change ! Thanks for bringing it up, 
Kind regards Ada