Re: [PATCH v22 06/10] firmware: arm_rmm: Ensure the RMM has GPT entries for memory

[Kohei Enju]

Hi Suzuki,

I've tested this series and found a possible TF-RMM issue.
Please  below.

On 10/02 07:15, Suzuki K Poulose wrote:
> From: Steven Price <steven.price@xxxxxxx>
> 
> 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.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>
> Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>
> Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> Tested-by: Gavin Shan <gshan@xxxxxxxxxx>
> Signed-off-by: Steven Price <steven.price@xxxxxxx>
> Co-developed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> ---
[...]
> +/*
> + * We do not support creating L1 GPTs yet. So, make sure that all the regions
> + * are managed by the firmware.
> + */
> +static int rmi_verify_gpt_firmware_managed(phys_addr_t start, phys_addr_t end)
> +{
> +	unsigned long l0gpt_sz;
> +	unsigned long next, par_state;
> +
> +	l0gpt_sz = 1UL << (30 + FIELD_GET(RMI_FEATURE_REGISTER_1_L0GPTSZ,
> +					  rmi_feat_reg(1)));
> +	start = ALIGN_DOWN(start, l0gpt_sz);
> +	end = ALIGN(end, l0gpt_sz);
> +
> +	while (start < end) {
> +		long ret = rmi_gpt_info(start, end, &next, &par_state);
> +
> +		if (ret != RMI_SUCCESS) {
> +			pr_err("RMI_GPT_INFO failed for %llx-%llx: (%ld)\n",
> +				start, end, ret);
> +			return -ENXIO;
> +		}
> +
> +		if (WARN_ON(next <= start))
> +			return -ENXIO;
> +
> +		if (par_state != RMI_GPT_PAR_PLAT) {
> +			pr_err("GPT for the region is not managed by firmware %llx-%lx\n",
> +				start, next);
> +			return -ENODEV;

When system RAM starts at a non-L0GPTSZ-aligned address, the alignment
in rmi_verify_gpt_firmware_managed() may cause the queried L0GPT range
to include a memory hole.

On out platform, the relevant memory layout is:

    800c0000000-800dc3fffff : memory hole
    800dc400000-82fffffffff : System RAM

Consequently, Linux queries the following L0GPT range:

    800c0000000-80100000000

The query fails with RMI_GPT_PAR_RESERVED:

    [    8.551855] GPT for the region is not managed by firmware 800c0000000-80100000000

and arm64_init_rmi() subsequently fails.

As I understand it, smc_gpt_info() in TF-RMM checks only the beginning
of the aligned L0GPT range. Since that address is in the hole, the whole
range is reported as RMI_GPT_PAR_RESERVED, even though it contains
platform-managed System RAM.

I believe this should be addressed in TF-RMM. Do you have any thoughts
on the expected RMI_GPT_INFO behavior in this case?

As a side note, we tested this series and the v23 integration branch
with a Linux-side workaround to avoid the problem above, and we have not
observed any other major issues. Sorry for the delay in testing this
series.

Thanks,
Kohei

> +		}
> +		start = next;
> +	}
> +
> +	return 0;
> +}