Re: [PATCH v17 3/7] firmware: arm_rmm: Configure the RMM with the host's page size

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 07/09/2026 11:14, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The SMC call leaves parameters X2-X7 uninitialized, violating the RMM's RES0 requirement.
> - [High] The `struct rmm_config` fields are populated using native endianness, which breaks on Big Endian kernels.
> --
> 
> commit 1741e17729c2ff5e23bc8bfb00cc50a61f38ebb7
> Author: Steven Price <steven.price@arm.com>
> firmware: arm_rmm: Configure the RMM with the host's page size
> 
> This commit adds support to configure the RMM's granule size to match the
> host's page size during kernel initialization. It reads the feature
> registers to ensure the requested page size is supported and sends the
> configuration to the RMM via an SMC call.
> 
>> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
>> index 008a783407b4e..76f91c145e1fd 100644
>> --- a/drivers/firmware/arm_rmm/rmi.c
>> +++ b/drivers/firmware/arm_rmm/rmi.c
>> @@ -77,6 +77,60 @@ static int rmi_read_features(void)
>>   	return 0;
>>   }
>>   
>> +static int rmi_configure(void)
>> +{
>> +	unsigned long granule_feature;
>> +	unsigned long granule_size;
>> +	int ret = 0;
>> +	struct rmm_config *config;
>> +
>> +	switch (PAGE_SIZE) {
> [ ... ]
>> +
>> +	config = (struct rmm_config *)get_zeroed_page(GFP_KERNEL);
>> +	if (!config)
>> +		return -ENOMEM;
>> +
>> +	config->rmi_granule_size = granule_size;
>> +
>> +	/*
>> +	 * For now we set the tracking_region_size to 0 which is the only option
>> +	 * for 4KB PAGE_SIZE (1GB for 4KB PAGE_SIZE, 32MB/512MB for 16KB/64KB).
>> +	 * TODO: Support other tracking sizes via Kconfig option for other
>> +	 * PAGE_SIZES
>> +	 */
>> +	config->tracking_region_size = 0;
> 
> [Severity: High]
> Does this code handle endianness correctly?
> 
> The struct rmm_config fields are populated using native u64 assignments.
> On an ARM64 kernel compiled with CONFIG_CPU_BIG_ENDIAN=y, this writes
> big-endian bytes into the shared memory. The RMM, which runs in little-endian
> at EL2, will misinterpret these fields.
> 
> Will this cause the RMM firmware to read invalid configuration values (like a
> byte-swapped granule size) and fail to configure, breaking RMI support on Big
> Endian systems?
> 

We don't plan to support RMM for Big Endian Linux. I have now made the 
ARCH_SUPPORTS_RMM to depend on !CPU_BIG_ENDIAN

>> +
>> +	ret = rmi_rmm_config_set(virt_to_phys(config));
>> +	if (ret) {
>> +		pr_err("RMM config set failed\n");
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	free_page((unsigned long)config);
>> +	return ret;
>> +}
> [ ... ]
>> diff --git a/include/linux/arm-rmi-cmds.h b/include/linux/arm-rmi-cmds.h
>> index 2fb1f7f86d71e..9aa27697e2377 100644
>> --- a/include/linux/arm-rmi-cmds.h
>> +++ b/include/linux/arm-rmi-cmds.h
>> @@ -12,6 +12,23 @@
>>   
>>   unsigned long rmi_feat_reg(unsigned long id);
>>   
>> +/**
>> + * rmi_rmm_config_set() - Configure the RMM
>> + * @cfg_ptr: PA of a struct rmm_config
>> + *
>> + * Sets configuration options on the RMM.
>> + *
>> + * Return: RMI return code
>> + */
>> +static inline int rmi_rmm_config_set(unsigned long cfg_ptr)
>> +{
>> +	struct arm_smccc_res res;
>> +
>> +	arm_smccc_1_1_invoke(SMC_RMI_RMM_CONFIG_SET, cfg_ptr, &res);
> 
> [Severity: High]
> Does this SMC call leave parameters x2 through x7 uninitialized?
> 
> The arm_smccc_1_1_invoke macro only generates inline assembly constraints
> for the explicitly provided arguments. It does not zero-pad omitted arguments,
> leaving registers x2 through x7 with whatever was previously in them.
> 
> Since the RMM firmware expects unused SMC parameters to be zero (RES0),
> could passing uninitialized values cause the RMM to reject the command
> with an INVALID_PARAMETER error and sporadically break RMM initialization?

Correct, I have now moved all RMI calls to smccc_1_2, making sure that
all the undefined arguments are initialised to 0.

Cheers
Suzuki


> 
>> +
>> +	return res.a0;
>> +}
>