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

[Gavin Shan <gshan@redhat.com>]

Hi Suzuki,

On 9/7/26 10:02 PM, Suzuki K Poulose wrote:
> 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
> 

You probably need to avoid referring to CPU_BIG_ENDIAN as it's being dropped
by PATCH[v2 13/14] in Will's series to removed unused big-endian support on
arm64 [1].

[1] [PATCH v2 00/14] arm64: Remove unused big-endian support
     (https://lore.kernel.org/linux-arm-kernel/20260907163726.17104-14-will@kernel.org/)

Thanks,
Gavin

>>> +
>>> +    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;
>>> +}
>>
> 
>