Re: [PATCH v19 4/7] firmware: arm_rmm: Add support for SRO

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 25/09/2026 12:50, Catalin Marinas wrote:
> On Thu, Sep 24, 2026 at 02:51:58PM +0100, Suzuki K Poulose wrote:
>> +static int rmi_sro_donate_contig(struct rmi_sro_state *sro,
>> +				 unsigned long sro_handle,
>> +				 unsigned long donatereq,
>> +				 struct arm_smccc_1_2_regs *out_regs,
>> +				 gfp_t gfp)
>> +{
>> +	unsigned long block_size_fld = RMI_DONATE_BLOCK_SIZE(donatereq);
>> +	unsigned long block_size = rmi_addr_block_size_to_bytes(block_size_fld);
>> +	unsigned long count = RMI_DONATE_COUNT(donatereq);
>> +	unsigned long state = RMI_DONATE_STATE(donatereq);
>> +	unsigned long size = block_size * count;
>> +	unsigned long addr_range;
>> +	unsigned long donated_granules;
>> +	unsigned long donated_size;
>> +	int ret;
>> +	void *virt;
>> +	phys_addr_t phys;
>> +
>> +	/*
>> +	 * The RMM specification requires contiguous allocations are always a
>> +	 * power of 2
>> +	 */
>> +	if (WARN_ON_ONCE(!is_power_of_2(size)))
>> +		return -EINVAL;
>> +
>> +	/* Reuse the cached address range if we have one */
>> +	for (int i = 0; i < sro->addr_count; i++) {
>> +		unsigned long entry = sro->addr_list[i];
>> +
>> +		if (RMI_ADDR_RANGE_BLOCK_SIZE(entry) == block_size_fld &&
>> +		    RMI_ADDR_RANGE_COUNT(entry) == count &&
>> +		    RMI_ADDR_RANGE_STATE(entry) == state &&
>> +		    IS_ALIGNED(RMI_ADDR_RANGE_ADDR(entry), size)) {
>> +			sro->addr_count--;
>> +			swap(sro->addr_list[sro->addr_count],
>> +			     sro->addr_list[i]);
>> +
>> +			goto mem_donate;
>> +		}
>> +	}
>> +
>> +	ret = rmi_sro_ensure_capacity(sro, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	virt = alloc_pages_exact(size, gfp);
>> +	if (!virt)
>> +		return -ENOMEM;
>> +	phys = virt_to_phys(virt);
>> +
>> +	if (state == RMI_OP_MEM_DELEGATED) {
>> +		phys_addr_t delegated_phys;
>> +
>> +		if (rmi_delegate_range(phys, size, &delegated_phys)) {
>> +			if (!rmi_undelegate_range(phys, delegated_phys - phys))
>> +				free_pages_exact(virt, size);
>> +			return -ENXIO;
>> +		}
>> +	}
>> +
>> +	addr_range = phys & RMI_ADDR_RANGE_ADDR_MASK;
>> +	FIELD_MODIFY(RMI_ADDR_RANGE_BLOCK_SIZE_MASK, &addr_range, block_size_fld);
>> +	FIELD_MODIFY(RMI_ADDR_RANGE_COUNT_MASK, &addr_range, count);
> 
> Courtesy of an LLM - if we get a contiguous/4K pages request for 4MB
> (1024 pages), the above 10-bit field becomes 0. Should we reject the
> request or the updated spec will guarantee this won't happen?

The spec is being updated to clamp the "maximum" count to match the
granule size. (Also called out in the Cover letter as "Known Issues")
This will be available with beta4 version of the spec.

> 
> Somewhat related, with RMI_BLOCK_L2 or higher and appropriate count we
> can easily go over the MAX_PAGE_ORDER allocation and fail alloc_pages()
> (the contig case). IIUC, the kernel can reject the donation but current
> TF-RMM does not report RMI_OP_CAN_CANCEL (R_SZVNK says it can be
> cancelled). It gets complicated if we want to support large contiguous

Ack, I will get this fixed in the TF-RMM.

> allocations here (e.g. alloc_contig_pages() can sleep). I'd rather just
> cancel the request, not support such large sizes.
> 
> [...]
> 
>> +static int rmi_sro_donate_noncontig(struct rmi_sro_state *sro,
>> +				    unsigned long sro_handle,
>> +				    unsigned long donatereq,
>> +				    struct arm_smccc_1_2_regs *out_regs,
>> +				    gfp_t gfp)
>> +{
>> +	unsigned long block_size_fld = RMI_DONATE_BLOCK_SIZE(donatereq);
>> +	unsigned long block_size = rmi_addr_block_size_to_bytes(block_size_fld);
>> +	unsigned long count = RMI_DONATE_COUNT(donatereq);
>> +	unsigned long state = RMI_DONATE_STATE(donatereq);
>> +	unsigned long found = 0;
>> +	unsigned long donated_granules;
>> +	unsigned long granules_per_block = block_size >> PAGE_SHIFT;
>> +	unsigned long consumed_blocks;
>> +	int addr_list_start = sro->addr_count;
>> +	int ret, i;
> 
> [...]
> 
>> +	for (int i = 0, src = addr_list_start + consumed_blocks;
> 
> Nit: we have 'int ret, i' earlier already, so you can drop the 'int'
> here.
> 
> [...]
> 
>> +/*
>> + * rmi_sro_execute: Execute an RMI command that is Stateful but not memory
>> + * tranfserring. Takes regs, filled with the FIDs and the arguments in place.
>> + *
>> + * Returns :
>> + *  -ECANCELLED - If the operation had to be aborted and SRO was cancellable.
> 
> Nit: -ECANCELED.
> 

Ack

>> + *  Otherwise, returns the result of the RMI command.
>> + */
>> +long rmi_sro_execute(struct arm_smccc_1_2_regs *regs)
>> +{
>> +	bool cancelled = false;
>> +	unsigned long sro_handle = regs->a1;
>> +
>> +	rmi_smccc_invoke(regs);
>> +
>> +	sro_handle = regs->a1;
> 
> Nit: drop the first initialisation maybe?

Of course, I will do that. Thanks for spotting.

Suzuki

>