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

[Suzuki K Poulose]

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@xxxxxxx>
> > [ Switch to RMI_GPT_L1_INFO for checking GPTs and deactivate RMM ]
> > Co-Developed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>
> 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.

> >   	}
> >   
> > +	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 will see if I can improve it.

Cheers
Suzuki