Re: [PATCH v18 6/7] firmware: arm_rmm: Ensure the RMM has GPT entries for memory

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

On 21/09/2026 10:32, Suzuki K Poulose wrote:
> On 19/09/2026 02:27, Jonathan Cameron wrote:
>>> The RMM maintains the state of all the granules in the system to make
>>> sure that the host is abiding by the rules. This state can be maintained
>>> at different granularity, per page (TRACKING_FINE) or per region
>>> (TRACKING_COARSE or TRACKING_INTERMEDIATE). The region size depends 
>>> on the
>>> underlying "RMI_GRANULE_SIZE". For a "coarse"/"intermediate" region, 
>>> all pages
>>> in the region must be of the same state, this implies we need to have 
>>> "fine"
>>> tracking for DRAM, so that we can delegate individual pages.
>>>
>>> For now we only support a statically carved out memory for tracking
>>> granules for the "fine" regions. This can be extended in the future to
>>> allow modifying the tracking granularity and remove the need for a
>>> static allocation by the firmware.
>>>
>>> Similarly, the firmware may create L0 GPT entries describing the total
>>> address space. But if we change the "PAS" (Physical Address Space) of a
>>> granule, then the firmware may need to create L1 tables to track the PAS
>>> at a finer granularity. Linux therefore checks if the platform 
>>> firmware manages
>>> the PAR region. i.e., the firmware is in charge of managing the L1 GPTs
>>> (creation and the required memory for the GPT tables - via static 
>>> carveouts)
>>> without host intervention. Support for dynamic GPT creation by the 
>>> host will be
>>> added later.
>>>
>>> If the firmware requires us to manage the tracking or GPT memory, 
>>> Deactivate
>>> the RMM and reclaim any memory donated at RMM activation.
>>>
>>> Apply the same checks when hotplugged memory is brought online.
>>>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> [ Switch to RMI_GPT_L1_INFO for checking GPTs and deactivate RMM ]
>>> Co-Developed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> A few comments inline.
>>
>>
>>> ---
>>>   Changes since v17:
>>>       * Move wrappers that may not be used elsewhere, out of arm-rmi- 
>>> cmds.h
>>>   Changes since v16:
>>>       * Check fine tracking and create L1 GPTs for hotplug-added memory.
>>>       * Clarify the L1 GPT setup and move the explanatory comment.
>>>       * Switch to using RMI_GPT_INFO command for checking the GPTs.
>>>       * Deactivate the RMM and reclaim the memory if we can't proceed.
>>>   Changes since v15:
>>>       * Skip firmware-reserved NOMAP memory in rmi_init_metadata()
>>>       * Handle negative error codes from wrappers.
>>>   Changes since v14:
>>>       * Move the implementation into drivers/firmware/arm_rmm.
>>>   Changes since v13:
>>>       * Moved out of KVM
>>> ---
>>>   drivers/firmware/arm_rmm/rmi.c | 200 +++++++++++++++++++++++++++++++++
>>>   include/linux/arm-rmi-cmds.h   |   2 +
>>>   2 files changed, 202 insertions(+)
>>>
>>> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/ 
>>> arm_rmm/rmi.c
>>> index ecc89e91d264..583e1aca9b15 100644
>>> --- a/drivers/firmware/arm_rmm/rmi.c
>>> +++ b/drivers/firmware/arm_rmm/rmi.c
>>
>>
>>> + */
>>> +static inline long rmi_gpt_info(unsigned long start, unsigned long end,
>>
>> Why inline vs letting compiler make it's mind up?
>> Same in other places
>>
>>> +                unsigned long *out_top,
>>> +                unsigned long *out_gpt_par_state)
>>> +{
>>> +    struct arm_smccc_1_2_regs regs = {
>>> +        SMC_RMI_GPT_INFO, start, end,
>>> +    };
>>> +
>>> +    rmi_smccc_invoke(&regs);
>>> +    if (regs.a0 != RMI_SUCCESS)
>>> +        return regs.a0;
>>> +
>>> +    if (out_top)
>>> +        *out_top = regs.a1;
>>> +    if (out_gpt_par_state)
>>> +        *out_gpt_par_state = regs.a2;
>>> +
>>> +    return RMI_SUCCESS;
>>> +}
>>
>>
>>
>>>   static int __init arm64_init_rmi(void)
>>>   {
>>>       int ret;
>>> @@ -786,8 +970,24 @@ static int __init arm64_init_rmi(void)
>>>       if (ret) {
>>>           pr_err("RMM activate failed\n");
>>>           ret = ret < 0 ? ret : -ENXIO;
>>> +        return ret;
>>
>> Why did this change?
> 
> Rebase messed up. I will restore it.

Actually this is not. We dont have to check the metadata if
we couldn't activate the RMM. Also, the failure path at the
bottom has "deactivate", which again is not needed. So
it is the right thing to do.

> 
>>
>>>       }
>>> +    ret = rmi_init_metadata();
>>> +    if (ret)
>>
>> And this is hitting another bit of guidance in cleanup.h.
>> Functions shouldn't be mixing __free and friends with
>> gotos.  Again, not a bug here but there are large ugly
>> monsters around this stuff, hence the blanket guidance.
>> I haven't thought that hard on how you avoid it here, but
>> usually it's a combination of suitable helpers and wrappers
>> and resulting code is often more readable as a result.

I could change the hunk to something like, but that looks ugly.

@@ -1010,20 +1010,12 @@ static int __init arm64_init_rmi(void)
                 return ret;
         }

-       ret = rmi_init_metadata();
-       if (ret)
-               goto out_deactivate;
+       if (!rmi_init_metadata() && 
!register_memory_notifier(&rmi_memory_nb)) {
+               arm64_rmi_is_available = true;
+               pr_info("RMI configured\n");
+               return 0;
+       }

-       ret = register_memory_notifier(&rmi_memory_nb);
-       if (ret)
-               goto out_deactivate;
-
-       arm64_rmi_is_available = true;
-       pr_info("RMI configured\n");
-
-       return 0;
-
-out_deactivate:
         WARN_ON(rmi_sro_memxfer_cmd(sro, GFP_KERNEL, 
SMC_RMI_RMM_DEACTIVATE));
         return ret;
  }


Either ways, we have to cleanup the object on return, no matter
the route we take. So the original form is much more readable
for me.

Cheers
Suzuki

>>
> 
> I will see if I can improve it.
> 
> Cheers
> Suzuki
>