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

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

On 14/09/2026 13:50, Sudeep Holla wrote:
> On Sat, Sep 12, 2026 at 09:36:07AM +0100, Suzuki K Poulose wrote:
>> RMM v2.0 introduces the concept of "Stateful RMI Operations" (SRO). This
>> means that an SMC can return with an operation still in progress. The
>> host is expected to continue the operation until it reaches a conclusion
>> (either success or failure). During this process the RMM can request
>> additional memory ('donate') or hand memory back to the host
>> ('reclaim'). The host can request an in progress operation is cancelled,
>> but still continue the operation until it has completed (otherwise the
>> incomplete operation may cause future RMM operations to fail).
>>
>> The SRO is tracked using a struct rmi_sro_state object which keeps track
>> of any memory which has been allocated but not yet consumed by the RMM
>> or reclaimed from the RMM. This allows the memory to be reused in a
>> future request within the same operation. It will also permit an
>> operation to be done in a context where memory allocation may be
>> difficult (e.g. atomic context) with the option to abort the operation
>> and retry the memory allocation outside of the atomic context. The
>> memory stored in the struct rmi_sro_state object can then be reused on
>> the subsequent attempt.
>>
>> Wrappers for SRO RMI commands are also provided here because they depend
>> on the rmi_sro_execute() implementation added by this patch.
>> Delegate/undelegate handles are also added here because they now use the
>> SRO/stateful command infrastructure and are also used for the memory
>> DONATE/RECLAIM flows.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> Co-Developed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> [...]
> 
>> +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;
>> +
>> +	for (int i = 0; i < addr_list_start && found < count; i++) {
>> +		unsigned long entry = sro->addr_list[i];
>> +
>> +		if (RMI_ADDR_RANGE_BLOCK_SIZE(entry) == block_size_fld &&
>> +		    RMI_ADDR_RANGE_COUNT(entry) == 1 &&
>> +		    RMI_ADDR_RANGE_STATE(entry) == state) {
>> +			addr_list_start--;
>> +			swap(sro->addr_list[addr_list_start],
>> +			     sro->addr_list[i]);
>> +			found++;
>> +			i--;
>> +		}
>> +	}
>> +
>> +	ret = rmi_sro_ensure_capacity(sro, count - found);
>> +	if (ret)
>> +		return ret;
>> +
>> +	while (found < count) {
>> +		unsigned long addr_range;
>> +		void *virt = alloc_pages_exact(block_size, gfp);
>> +		phys_addr_t phys;
>> +
>> +		if (!virt)
>> +			return -ENOMEM;
>> +
>> +		phys = virt_to_phys(virt);
>> +
>> +		if (state == RMI_OP_MEM_DELEGATED) {
> 
> Based on my understanding, rmi_sro_memxfer_execute() is an exported function
> and can be invoked by any module. The donatereq argument appears to accept one
> of three operations:
> 
> RMI_OP_MEM_DELEGATED
> RMI_OP_MEM_UNDELEGATED
> RMI_OP_MEM_CONDITIONAL

Ack

> 
> Currently, the check confirming the state is RMI_OP_MEM_DELEGATED occurs
> relatively late in the function execution. It seems this function is
> explicitly designed to handle only RMI_OP_MEM_DELEGATED.

No, that is not correct. The function handles both OP_MEM_DELEGATED and
OP_MEM_UNDELEGATED. In the former case, we explicitly "delegate" the
pages before donating. The "UNDELEGATED" case doesn't need to do that
extra step.

> 
> Given that this is an exported interface, would it make sense to
> fail-fast by moving this validation to the very beginning of the function?
> Even if RMI_OP_MEM_CONDITIONAL is intended for future use, it should

Yep, agree. We can rejec the CONDITIONAL ones.

> probably be rejected as invalid for now. Also, it is not clear why the
> current check is inside the loop while the state itself doesn't get
> modified.

As above, that check is additionally preparing the memory for RMM
consumption.


> 
> If this is a valid concern, the same architectural pattern should likely be
> applied to rmi_sro_donate_contig().
> 

Ack, we can reject the CONDITIONAL ones.

Suzuki