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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Usage of GENMASK and BIT macros for 64-bit masks causes Undefined Behavior on 32-bit architectures
--

commit cfdd0e3f2fb70671ab2dbb64f06ca56946732456
Author: Steven Price <steven.price@arm.com>

firmware: arm_rmm: Add SMC definitions for calling the RMM

This commit introduces SMC definitions for calling the Realm Management Monitor
(RMM) from the host. The definitions are based on the ARM RMM specification
version 2.0-bet3.

Link: https://developer.arm.com/documentation/den0137/2-0bet3/

> diff --git a/include/linux/arm-smccc-rmi.h b/include/linux/arm-smccc-rmi.h
> new file mode 100644
> index 0000000000000..214d6228dfc22
> --- /dev/null
> +++ b/include/linux/arm-smccc-rmi.h

[ ... ]

> +#define RMI_ADDR_TYPE_NONE		0
> +#define RMI_ADDR_TYPE_SINGLE		1
> +#define RMI_ADDR_TYPE_LIST		2
> +
> +#define RMI_ADDR_RANGE_STATE_MASK	GENMASK(63, 62)
> +#define RMI_ADDR_RANGE_ADDR_MASK	GENMASK(51, PAGE_SHIFT)

[Severity: Low]
Can the use of GENMASK for bit positions >= 32 cause undefined behavior on
32-bit architectures? 

The GENMASK macro evaluates to unsigned long, which is 32 bits wide on 32-bit
architectures. Shifting by 32 or more results in truncation or an
out-of-bounds shift.

Would it be better to use GENMASK_ULL here for these 64-bit masks?

> +#define RMI_ADDR_RANGE_COUNT_MASK	GENMASK(PAGE_SHIFT - 1, 2)
> +#define RMI_ADDR_RANGE_BLOCK_SIZE_MASK	GENMASK(1, 0)

[ ... ]

> +enum rmi_ripas {
> +	RMI_EMPTY = 0,
> +	RMI_RAM = 1,
> +	RMI_DESTROYED = 2,
> +	RMI_DEV = 3,
> +};
> +
> +#define RMI_NO_MEASURE_CONTENT	0
> +#define RMI_MEASURE_CONTENT	1
> +
> +#define RMI_FEATURE_REGISTER_0_S2OASZ			GENMASK(40, 33)
> +#define RMI_FEATURE_REGISTER_0_L0GPT_BLOCK_DELEGATE	BIT(32)

[Severity: Low]
Similarly, will this GENMASK and BIT usage trigger the same undefined behavior
when evaluated as unsigned long? Using GENMASK_ULL and BIT_ULL might avoid this.

> +#define RMI_FEATURE_REGISTER_0_PMU_NUM_CTRS		GENMASK(31, 27)
> +#define RMI_FEATURE_REGISTER_0_PMU			BIT(26)

[ ... ]

> +#define RMI_FEATURE_REGISTER_3_RTT_S2AP_INDIRECT	BIT(6)
> +#define RMI_FEATURE_REGISTER_3_RTT_PLANE		GENMASK(5, 4)
> +#define RMI_FEATURE_REGISTER_3_MAX_NUM_AUX_PLANES	GENMASK(3, 0)
> +
> +#define RMI_FEATURE_REGISTER_4_MEC_COUNT		GENMASK(63, 0)

[Severity: Low]
Could GENMASK_ULL(63, 0) be used here as well?

> +
> +#define RMI_MEM_CATEGORY_CONVENTIONAL		0
> +#define RMI_MEM_CATEGORY_DEV_NCOH		1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912083611.2513845-1-suzuki.poulose@arm.com?part=1