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

[Jonathan Cameron]

> 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?

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

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>