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

[Suzuki K Poulose]

On 29/09/2026 12:01, Catalin Marinas wrote:
> On Thu, Sep 24, 2026 at 02:52:00PM +0100, Suzuki K Poulose wrote:
> > +/*
> > + * Make sure the area is tracked by RMM at FINE granularity.
> > + * We do not support changing the tracking yet.
> > + */
> > +static int rmi_verify_memory_tracking(phys_addr_t start, phys_addr_t end)
> > +{
> > +	while (start < end) {
> > +		unsigned long ret, category, state, next;
> > +
> > +		ret = rmi_granule_tracking_get(start, end, &category, &state, &next);
> > +		if (ret != RMI_SUCCESS)
> > +			return -ENOMEM;
> > +
> > +		if (WARN_ON(next <= start))
> > +			return -ENXIO;
> > +
> > +		if (state != RMI_TRACKING_FINE ||
> > +		    category != RMI_MEM_CATEGORY_CONVENTIONAL) {
> > +			/* TODO: Set granule tracking in this case */
> > +			pr_err("Granule tracking for region isn't fine/conventional: %llx-%lx\n",
> > +			       start, next);
> > +			return -ENODEV;
> > +		}
> > +		start = next;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * rmi_gpt_info - Query the GPT info for the given PAR.
> > + * @start: Base of the physical address region
> > + * @end: Top of the physical address region
> > + * @out_top: Top of the physical address region for which
> > + *		the GPT @out_gpt_par_state is valid
> > + * @out_gpt_par_state: State of the GPT covered by [start, out_top)
> > + */
> > +static long rmi_gpt_info(unsigned long start, unsigned long end,
> > +			 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;
> > +}
> > +
> > +/*
> > + * 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)
> > +			return -ENOMEM;
>
> Nit: what does -ENOMEM actually mean here? Was there any allocation?
> Maybe -ENXIO is better suited, as the one below. Also, is it useful to
> print something like we do further down?
>
> Same comment for rmi_verify_memory_tracking().
>
> > +		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 -ENOMEM;
>
> Nit: in v18, Gavin suggested returning -ENODEV here (the hunk he
> proposed). I think it makes more sense and it matches the -ENODEV in
> rmi_verify_memory_tracking().

Ack, I have incorporated both suggestions.

Suzuki

>