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

[Suzuki K Poulose]

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@xxxxxxx>
> > Co-Developed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>
> [...]
>
> > +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