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

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

On 24/09/2026 20:13, Jonathan Cameron wrote:
> On Thu, 24 Sep 2026 14:51:58 +0100
> Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
>> From: Steven Price <steven.price@arm.com>
>>
>> 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>
> 
> Nice.  Everything I spotted this time around is pretty trivial.
> So assuming you'll clean up and bits that make sense to you for v20
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> 

Thanks, respones inline.

...


>> +
>> +int rmi_undelegate_range(phys_addr_t phys,
>> +			 unsigned long size)
>> +{
>> +	long ret = 0;
>> +	unsigned long top = phys + size;
>> +	unsigned long out_top;
>> +
>> +	while (phys < top) {
>> +		ret = rmi_granule_range_undelegate(phys, top, &out_top);
>> +
>> +		if (ret == RMI_SUCCESS) {
>> +			/* Buggy RMM ? Let the caller leak the pages */
>> +			if (WARN_ON(out_top <= phys))
>> +				return -ENXIO;
>> +			phys = out_top;
>> +		} else {
> 
> Similar to below, why not deal with error case first and reduce indent of
> the good path.
> 

...

>> + */
>> +int rmi_delegate_range(phys_addr_t phys,
>> +		       unsigned long size,
>> +		       phys_addr_t *out_phys)
>> +{
>> +	long ret = 0;
>> +	unsigned long top = phys + size;
>> +	unsigned long out_top;
>> +
>> +	while (phys < top) {
>> +		ret = rmi_granule_range_delegate(phys, top, &out_top);
>> +
>> +		if (ret == RMI_SUCCESS) {
> 
> My instinct here would be to flip this and have the error out of line given
> it breaks anyway and that gives you smaller indent for that ocmment block.
> 
> 		if (ret != RMI_SUCCESS)
> 			break;
> 
> 		/*
> 		 * Buggy RMM ? Let the caller handle the failure. We can't know
> 		 * how far the RMM delegated in this iteration, so we return
>   		 * the best known good limit. RMM can deal with granules
> 		 * already in "undelegated" in a given range. So, it is fine
> 		 * for the caller to try the range we return.
> 		 */
> 		if (WARN_ON...
> 

Agree, it looks much cleaner. I have done that.
>> +
>> +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)

...

>> +
>> +	/* Gather the suitable entries to the end of the list */
>> +	i = 0;
>> +	while (i <  addr_list_start && found < count) {
>> +		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++;
>> +			/* Continue from the swapped in entry */
>> +			continue;
>> +		}
>> +		/* skip past the entry */
> 
> Bit random on comment capitalization.  Have a quick final look through.
> For instance I think this one is Skip to match Continue above.
> 

Ack

>> +		i++;
>> +	}
> 
> ...
> 
> 
>> +
>> +static int rmi_sro_reclaim(struct rmi_sro_state *sro,
>> +			   unsigned long sro_handle,
>> +			   struct arm_smccc_1_2_regs *out_regs)
>> +{
>> +	unsigned long capacity;
>> +
>> +	/*
>> +	 * We don't do a partial free of the entries. So for
>> +	 * now free the entire address list as we prepare
>> +	 * to reclaim more from the RMM.
> 
> Rewrap to use all that nice space up to 80 chars!  I guess a refactoring
> side effect.
> 

Ack

>> +}
> 
>> +
>> +long rmi_sro_memxfer_execute(struct rmi_sro_state *sro, gfp_t gfp)
>> +{
>> +	struct arm_smccc_1_2_regs *regs = &sro->regs;
>> +	bool cancelled = false;
>> +	unsigned long sro_handle;
>> +
>> +	rmi_smccc_invoke(regs);
>> +
>> +	sro_handle = regs->a1;
>> +	while (RMI_RESULT_STATUS(regs->a0) == RMI_INCOMPLETE) {
>> +		bool can_cancel = RMI_RESULT_CAN_CANCEL(regs->a0) == RMI_OP_CAN_CANCEL;
> 
> For a flag that is "can" or "cannot", do we need the RMI_OP_CAN_CANCEL (1) / RMI_OP_CANNOT_CANCEL (0)
> defines?  Doesn't feel like we'll ever get RMI_OP_UNKNOWN_IF_IT_CAN_CANCEL and I can't think
> of any other more reasonable options that would justify needing the explicit field value
> match.
> 
> To me
> 
> 		bool can_cancel = RMI_RESULT_CAN_CANCEL(regs->a0);
> 
> is obvious enough.  I don't care that much though so up to you.
> 

Yep, this was the original form and I changed it based on Gavin's
feedback. I would leave it as it is, to avoid another churn.



>> +	if (cancelled)
>> +		return -ECANCELED;
>> +
>> +	return regs->a0;
>> +}
>> +EXPORT_SYMBOL_GPL(rmi_sro_memxfer_execute);
>> +
>> +/*
>> + * 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.
> 
> Spell check. Transferring.  Also why does Stateful get a capital letter and
> Memory Transferring does not.  They seem to both be properties of the comman
> so I'd expect some consistency.

Ack

Thank you for the review !

Suzuki