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

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

Hi Ada,

On 10/7/26 17:31, Ada Couprie Diaz wrote:
> 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
> 

Make sense, please send it as separate change. AFAICS, it should not affect bpf
folk, but worth making them aware ;)

Thanks
Vladimir