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 23:40, Gavin Shan wrote:
> 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 for the heads up, I will just ignore this one from Sashiko.

Cheers
Suzuki


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