Re: [PATCH 1/6] firmware: arm_rmm: Add SMC definitions for calling the RMM

[Steven Price <steven.price@arm.com>]

On 15/07/2026 15:38, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] `struct rec_exit` contains 4 bytes of implicit compiler padding between `padding9` and `plane`.
> - [Low] Discrepancy between the `PROT_MAP` macro prefix and the `DATA_MAP` comment and SMC command name.
> --
> 
> --- Patch [1]: [PATCH 1/6] firmware: arm_rmm: Add SMC definitions for calling the RMM ---
> commit c161f24da1669945cb4dc961f3a3c4ef452450b6
> Author: Steven Price <steven.price@arm.com>
> firmware: arm_rmm: Add SMC definitions for calling the RMM
> 
> The RMM (Realm Management Monitor) provides functionality that can be
> accessed by SMC calls from the host.
> 
>> diff --git a/include/linux/arm-smccc-rmi.h b/include/linux/arm-smccc-rmi.h
>> new file mode 100644
>> index 0000000000000..c965e8867518f
>> --- /dev/null
>> +++ b/include/linux/arm-smccc-rmi.h
> 
> [ ... ]
> 
>> +	union { /* 0x600 */
>> +		struct {
>> +			u16 imm;
>> +			u16 padding9;
>> +			u64 plane;
> 
> [Severity: Low]
> Does this struct layout contain implicit padding? The sequence of two u16
> fields followed by a u64 field forces the compiler to inject 4 bytes of
> padding to align the plane field to an 8-byte boundary. While deterministic
> on ARM64, implicit padding in ABI structures shared with firmware is usually
> avoided. Should an explicit 32-bit padding field be added here?

Indeed this padding is broken. Amusingly I'd actually asked AI to write
a script to generate this file from the specification source (which had
identified a few minor issues that I'd already fix), but it managed to
preserve this bug :(

I've fixed the script and this now produces:

                struct {
                        u16 imm;
                        u8 padding9[6];
                        u64 plane;
                };

>> +		};
>> +		u8 padding6[0x100];
>> +	};
> 
> [ ... ]
> 
>> +/* RMI_RTT_DATA_MAP_FLAGS definitions */
>> +#define RMI_RTT_PROT_MAP_FLAGS_OADDR_TYPE	GENMASK(1, 0)
>> +#define RMI_RTT_PROT_MAP_FLAGS_LIST_COUNT	GENMASK(15, 2)
> 
> [Severity: Low]
> This isn't a bug, but there seems to be a naming discrepancy. The comment
> mentions RMI_RTT_DATA_MAP_FLAGS, which corresponds to the
> SMC_RMI_RTT_DATA_MAP command, but the macros are prefixed with
> RMI_RTT_PROT_MAP_FLAGS. Should these macros use the DATA_MAP prefix instead?

These comments were manually generated - again I've fixed the script to
generate the comments automatically to avoid this mismatch in the future.

Thanks,
Steve

>> +
>> +/* S2AP Direct Encodings, used in RMI_RTT_UNPROT_MAP_FLAGS_S2AP */
>> +#define RMI_S2AP_DIRECT_WRITE			BIT(0)
>> +#define RMI_S2AP_DIRECT_READ			BIT(1)
>> +
>> +#endif /* __LINUX_ARM_SMCCC_RMI_H_ */
>