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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Host-side memory allocation failures are incorrectly parsed as RMI firmware errors.
- [Low] Empty or sub-page memblock regions are expanded into 1GB GPT allocations.
- [Low] Missing trailing newline in success message.
--

--- Patch [6]: [PATCH 6/6] firmware: arm_rmm: Ensure the RMM has GPT entries for memory ---
commit dfe33cca468028175b080bbcfc671eba1604716c
Author: Steven Price <steven.price@arm.com>

firmware: arm_rmm: Ensure the RMM has GPT entries for memory
    
This commit configures RMM memory tracking to ensure DRAM regions are 
tracked at FINE granularity. It statically allocates tracking granules and 
creates Level 1 GPT tables for these regions when needed.

> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> index 6a9f61760fd77..d0c083bdf251f 100644
> --- a/drivers/firmware/arm_rmm/rmi.c
> +++ b/drivers/firmware/arm_rmm/rmi.c

[ ... ]

> @@ -604,6 +606,89 @@ static int rmi_configure(void)
>  	return 0;
>  }
>  
> +/*
> + * Make sure the area is tracked by RMM at FINE granularity.
> + * We do not support changing the tracking yet.
> + */

[ ... ]

> +static int rmi_create_gpts(phys_addr_t start, phys_addr_t end)
> +{
> +	struct rmi_sro_state *sro;
> +	unsigned long l0gpt_sz;
> +
> +	sro = kmalloc_obj(*sro, GFP_KERNEL);
> +	if (!sro)
> +		return -ENOMEM;
> +
> +	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);

[Severity: Low]
If a memblock region is smaller than a page or misaligned, is it possible
for the initial start physical address to be greater than or equal to end?

If so, applying ALIGN_DOWN() to start and ALIGN() to end could force 
start < end to evaluate to true for an originally empty region. Could this
cause the loop to inadvertently execute and allocate an L1 GPT table for a
1GB block that contains no full pages?

> +
> +	while (start < end) {
> +		int ret = rmi_gpt_l1_create(start, sro, GFP_KERNEL);
> +
> +		/*
> +		 * Make sure the L1 GPT tables are created for the region.
> +		 * RMI_ERROR_GPT indicates the L1 table already exists.
> +		 */
> +		if (ret != RMI_SUCCESS && RMI_RETURN_STATUS(ret) != RMI_ERROR_GPT) {

[Severity: Medium]
Since rmi_gpt_l1_create() can return a standard negative POSIX error code
like -ENOMEM from a host page allocation failure, does passing it to
RMI_RETURN_STATUS(ret) misinterpret the error?

For -ENOMEM (-12), the macro extracts the lower bits (244), which won't
match RMI_ERROR_GPT. Could this result in printing a misleading "GPT Level1
table missing" error when the real issue is an out-of-memory condition in
the host kernel?

> +			pr_err("GPT Level1 table missing for %llx\n", start);
> +			kfree(sro);
> +			return -ENOMEM;
> +		}
> +		start += l0gpt_sz;
> +	}
> +
> +	kfree(sro);
> +	return 0;
> +}

[ ... ]

> @@ -621,6 +706,13 @@ static int __init arm64_init_rmi(void)
>  	if (ret)
>  		return ret;
>  
> +	ret = rmi_init_metadata();
> +	if (ret)
> +		return ret;
> +
> +	arm64_rmi_is_available = true;
> +	pr_info("RMI configured");

[Severity: Low]
This isn't a bug, but should this include a trailing newline ("\n") to 
prevent subsequent kernel logs from being appended to the same line?

> +
>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715142739.80398-1-steven.price@arm.com?part=6