Re: [PATCH v17 4/7] firmware: arm_rmm: Add support for SRO
From: Suzuki K Poulose
Date: Thu Sep 10 2026 - 06:00:03 EST
Hi Gavin
Thank you for the the review, much appreciated. Responses inline.
On 09/09/2026 05:10, Gavin Shan wrote:
Hi Suzuki,
On 9/7/26 7:59 PM, Suzuki K Poulose wrote:
From: Steven Price <steven.price@xxxxxxx>
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.
Signed-off-by: Steven Price <steven.price@xxxxxxx>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
---
...
diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/ arm_rmm/rmi.c
index 76f91c145e1fd..42c973c3a98bb 100644
--- a/drivers/firmware/arm_rmm/rmi.c
+++ b/drivers/firmware/arm_rmm/rmi.c
...
+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)
+ phys = out_top;
+ else if (ret == RMI_BUSY || ret == RMI_BLOCKED)
+ cpu_relax();
+ else
+ break;
+ }
+
+ if (out_phys)
+ *out_phys = phys;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(rmi_delegate_range);
+
rmi_granule_range_delegate() can't return RMI_BUSY or RMI_BLOCKED as those two
error statuses are filtered out by inner call rmi_smccc_invoke(). So it's not
needed to have "else if (ret == RMI_BUSY || ret == RMI_BLOCKED) cpu_relax()"
here.
Ack
Besides, 'long ret' is truncated to 'int' by 'return ret'. I think we need a
helper to convert RMI error status to the linux error code, something like
below. The newly added helper rmi_to_linux_errno() is used by rmi_sro_memxfer_execute()
and rmi_sro_execute() where the return values are 'int' (not 'long' any more).
SRO operations return the full set of results from the execution in the out_regs/sro->regs. So they can make the full use of the results, if
required.
As for the truncation, we don't return the partial results to the
callers of the rmi_*delegate_range helpers, but they are consumed
internally (e.g., by the SRO execution). The callers care about the
return "status" which is still an 8bit field. We could extend this
if we need it in the future.
static int rmi_errno_map[] = {
[RMI_SUCCESS] = 0,
[RMI_ERROR_INPUT] = -EINVAL,
[RMI_ERROR_REALM] = -EBADF,
[RMI_ERROR_REC] = -EBADF,
[RMI_ERROR_RTT] = -EBADF,
[RMI_ERROR_NOT_SUPPORTED] = -EOPNOTSUPP,
[RMI_ERROR_DEVICE] = -EBADF,
[RMI_ERROR_RTT_AUX] = -EBADF,
[RMI_ERROR_PSMMU_ST] = -EBADF,
[RMI_ERROR_DPT] = -EBADF,
[RMI_BUSY] = -EBUSY,
[RMI_ERROR_GLOBAL] = -ENOSYS,
[RMI_ERROR_TRACKING] = -EBADF,
[RMI_INCOMPLETE] = -EINPROGRESS,
[RMI_BLOCKED] = -EAGAIN,
[RMI_ERROR_GPT] = -EBADF,
[RMI_ERROR_GRANULE] = -EBADF,
};
This may be useful, but don't see the need for that now.
int rmi_to_linux_errno(unsigned long status)
{
status = RMI_RETURN_STATUS(status);
return (status < ARRAY_SIZE(rmi_errno_maps)) ? rmi_errno_map[status] : -EINVAL;
}
EXPORT_SYMBOL_GPL(rmi_to_linux_errno);
+EXPORT_SYMBOL_GPL(rmi_undelegate_range);
+
+static unsigned long donate_req_to_size(unsigned long donatereq)
+{
+ unsigned long unit_size = RMI_DONATE_SIZE(donatereq);
+
+ return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(3 - unit_size));
+}
+
I would rename this helper to explicitly indicate it's going to get
the block size.
static unsigned long donate_req_to_block_size(unsigned long req)
{
unsigned long block_size_encode = RMI_DONATE_BLOCK_SIZE(req);
return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(3 - block_size_encode));
}
Ack:
I have cleaned this up a little bit by explicitly calling out the conversion of RmiAddresBlockSize in a wrapper. i.e.,
/*
* Convert the RmiAddrBlockSize to actual size. This is used in RmiDonateReq
* and RmiAddrRangeDesc*.
*/
static unsigned long rmi_addr_block_size_to_bytes(unsigned long block_size_fld)
{
return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(3 - block_size_fld));
}
static unsigned long donate_req_to_block_size(unsigned long donatereq)
{
return rmi_addr_block_size_to_bytes(RMI_DONATE_BLOCK_SIZE(donatereq));
}
So that it doesn't look awkward to do something like :
>> + unsigned long size = donate_req_to_size(unit_size) * count;
instead:
unsigned long size = rmi_addr_block_size_to_bytes(unit_size) * count;
...+static void rmi_smccc_invoke(struct arm_smccc_1_2_regs *regs_in,
+}
+
The local variable @regs in rmi_op_{continue, cancel}() and rmi_op_mem_{donate, recliam}() can
be avoided since rmi_smccc_invoke() has two variables for input and output separately. So
rmi_op_continue() can be improved as below. Other 3 functions can be improved in similar ways.
static void rmi_op_continue(unsigned long sro_handle, unsigned long flags,
struct arm_smccc_1_2_regs *out_regs)
{
out_regs->a0 = SMC_RMI_OP_CONTINUE;
out_regs->a1 = sro_handle;
out_regs->a2 = flags;
This may not be sufficient, as we need to zero out the entire arguments
for RES0 requirement. But could do something like :
*out_regs = (struct arm_smccc_1_2_regs) { SMC_RMI_OP_CONTINUE, sro_handle, flags };
rmi_smccc_invoke(out_regs, out_regs);
}
+int free_delegated_page(phys_addr_t phys)
+{
+ if (WARN_ON_ONCE(rmi_undelegate_page(phys))) {
+ /* Undelegate failed: leak the page */
+ return -EBUSY;
+ }
+
+ free_page((unsigned long)phys_to_virt(phys));
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(free_delegated_page);
+
How about renaming this to rmi_free_delegated_page()? It seems all functions exposed by
rmi.c have prefix 'rmi'.
This is in my internal tree for the next version already, thanks for pointing that out.
+static int rmi_sro_ensure_capacity(struct rmi_sro_state *sro,
+ unsigned long count)
+{
+ if (WARN_ON_ONCE(sro->addr_count > RMI_MAX_ADDR_LIST))
+ return -EOVERFLOW;
+
+ if (count > RMI_MAX_ADDR_LIST - sro->addr_count)
+ return -ENOSPC;
+
+ return 0;
+}
+
+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 unit_size = RMI_DONATE_SIZE(donatereq);
+ unsigned long unit_size_bytes = donate_req_to_size(donatereq);
+ unsigned long count = RMI_DONATE_COUNT(donatereq);
+ unsigned long state = RMI_DONATE_STATE(donatereq);
+ unsigned long size = unit_size_bytes * count;
+ unsigned long addr_range;
+ int ret;
+ void *virt;
+ phys_addr_t phys;
+
s/unit_size/block_size_encode
chose, block_size_fld
s/unit_size_bytes/block_size
Please move 'donated_size' to the begining of this function.
unsigned long addr_range, donated_size;
Ack
+ /*
+ * The RMM specification requires contiguous allocations are always a
+ * power of 2
+ */
+ if (WARN_ON_ONCE(!is_power_of_2(size)))
+ return -EINVAL;
+
+ for (int i = 0; i < sro->addr_count; i++) {
+ unsigned long entry = sro->addr_list[i];
+
+ if (RMI_ADDR_RANGE_SIZE(entry) == unit_size &&
+ 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 out;
+ }
+ }
+
The search in the address range array may deserve a comment, but I doubt how much
benefits (hit ratio) the array can give to us :-)
/* Reuse the cached address range if we have one */
Ack
Besides, 'int i' needs to be 'unsigned long i' because 'struct mi_sro_state::addr_count'
is 'unsigned long'. Alternative, we may change 'struct mi_sro_state::addr_count' to
'int'.
I have changed the addr_count to "int". That gives us a better way to
handle corruptions in the sro->addr_count. i.e, addr_count < 0 vs addr_count > RMI_MAX_ADDR_LIST
+ 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_SIZE_MASK, &addr_range, unit_size);
+ FIELD_MODIFY(RMI_ADDR_RANGE_COUNT_MASK, &addr_range, count);
+ FIELD_MODIFY(RMI_ADDR_RANGE_STATE_MASK, &addr_range, state);
+
+ sro->addr_list[sro->addr_count] = addr_range;
+
We actually requires that 'phys' in the range specified by RMI_ADDR_RANGE_ADDR_MASK, so:
WARN_ON_ONCE(phys & ~RMI_ADDR_RANGE_ADDR_MASK);
addr_range = phys;
Do we support more than 52bit ? Also this comes from virt_to_phys(),
which means the kernel has to first support the phys > 52bit. And the
ADDR_MASK is based on PAGE_SHIFT. So, if that breaks, we have bigger
problems.
+out:
+ rmi_op_mem_donate(sro_handle,
+ virt_to_phys(&sro->addr_list[sro->addr_count]), 1,
+ 0, out_regs);
+
+ unsigned long donated_granules = out_regs->a1;
+ unsigned long donated_size = donated_granules << PAGE_SHIFT;
+
+ if (donated_granules == 0) {
+ /* No pages used by the RMM */
+ sro->addr_count++;
+ } else if (donated_size < size) {
+ phys = sro->addr_list[sro->addr_count] & RMI_ADDR_RANGE_ADDR_MASK;
+
+ /* Not all granules used by the RMM, free the remaining pages */
+ for (long i = donated_size; i < size; i += PAGE_SIZE) {
+ if (state == RMI_OP_MEM_DELEGATED)
+ free_delegated_page(phys + i);
+ else
+ __free_page(phys_to_page(phys + i));
+ }
+ }
+
'i' was used previouly and I would avoid using it again. I would suggest to simplify
this chunk of code, as below.
I have dropped that hunk and replaced them with a new helper :
free_addr_range_entry(), that can free an address range, bit more
efficiently (batching granule_undelegate) and even handle a partially
consumed entry. This will be reused for contig/non-contig and the
rmi_sro_free().
Another question is if we need to check if
RMI_SUCCESS
is returned from rmi_op_mem_donate()?
donated_size = PFN_PHYS(out_regs->a1);
/* All granules are consumed by RMM */
if (donated_size == size)
return 0;
/* No granules are consumed by RMM, cache all granules */
if (donated_size == 0) {
sro->addr_count++;
return 0;
}
Ack, that is much clearer.
/*
* The granules are partially consumed by RMM, delegate and release
* the unused granules.
*/
phys = sro->addr_list[sro->addr_count] & RMI_ADDR_RANGE_ADDR_MASK;
while (donated_size < size) {
if (state == RMI_OP_MEM_DELEGATED)
rmi_free_delegated_page(phys + donated_size);
else
__free_page(phys_to_page(phys + donated_size));
donated_size += PAGE_SIZE;
}
return 0;
+ return 0;
+}
+
+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 unit_size = RMI_DONATE_SIZE(donatereq);
+ unsigned long unit_size_bytes = donate_req_to_size(donatereq);
+ unsigned long count = RMI_DONATE_COUNT(donatereq);
+ unsigned long state = RMI_DONATE_STATE(donatereq);
+ unsigned long found = 0;
+ unsigned long addr_list_start = sro->addr_count;
+ int ret;
+
s/unit_size/block_size_encode
s/unit_size/block_size
Ack, same as above.
+ for (int i = 0; i < addr_list_start && found < count; i++) {
+ unsigned long entry = sro->addr_list[i];
+
+ if (RMI_ADDR_RANGE_SIZE(entry) == unit_size &&
+ 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--;
+ }
+ }
+
The type of 'i' is different to 'addr_list_start' and 'sro->addr_count'.
As above, switched to i, and I will move addr_list_start to int.
+ ret = rmi_sro_ensure_capacity(sro, count - found);
+ if (ret)
+ return ret;
+
+ while (found < count) {
+ unsigned long addr_range;
+ void *virt = alloc_pages_exact(unit_size_bytes, gfp);
+ phys_addr_t phys;
+
+ 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, unit_size_bytes,
+ &delegated_phys)) {
+ if (!rmi_undelegate_range(phys, delegated_phys - phys))
+ free_pages_exact(virt, unit_size_bytes);
+ return -ENXIO;
+ }
+ }
+
+ addr_range = phys & RMI_ADDR_RANGE_ADDR_MASK;
+ FIELD_MODIFY(RMI_ADDR_RANGE_SIZE_MASK, &addr_range, unit_size);
+ FIELD_MODIFY(RMI_ADDR_RANGE_COUNT_MASK, &addr_range, 1);
+ FIELD_MODIFY(RMI_ADDR_RANGE_STATE_MASK, &addr_range, state);
+
+ sro->addr_list[sro->addr_count++] = addr_range;
+ found++;
+ }
+
+ rmi_op_mem_donate(sro_handle,
+ virt_to_phys(&sro->addr_list[addr_list_start]),
+ found, 0, out_regs);
+
The local variable 'found' looks redundant and can be dropped. With 'found' dropped,
we need:
May be, but it is easier to reason with that variable. So I prefer to
keep it for the loop.
while (sro->addr_count - addr_list_start < count) {
:
}
rmi_op_mem_donate(sro_handle,
virt_to_phys(&sro->addr_list[addr_list_start]),
count, 0, out_regs);
Ack for the switch to count here.
+ unsigned long donated_granules = out_regs->a1;
+ unsigned long granules_per_unit = unit_size_bytes >> PAGE_SHIFT;
+ unsigned long consumed_units;
+
s/granules_per_unit/granules_per_block
s/consumed_units/consumed_blocks
Ack
+void rmi_sro_free(struct rmi_sro_state *sro)
+{
+ for (int i = 0; i < sro->addr_count; i++) {
+ unsigned long entry = sro->addr_list[i];
+ unsigned long addr = RMI_ADDR_RANGE_ADDR(entry);
+ unsigned long unit_size = RMI_ADDR_RANGE_SIZE(entry);
+ unsigned long count = RMI_ADDR_RANGE_COUNT(entry);
+ unsigned long state = RMI_ADDR_RANGE_STATE(entry);
+ unsigned long size = donate_req_to_size(unit_size) * count;
+
+ if (state == RMI_OP_MEM_DELEGATED) {
+ if (WARN_ON_ONCE(rmi_undelegate_range(addr, size))) {
+ /* Leak the pages */
+ continue;
+ }
+ }
+ free_pages_exact(phys_to_virt(addr), size);
+ }
The nested if statements can be avoided by:
if (state == RMI_OP_MEM_DELEGATED &&
WARN_ON_ONCE(rmi_undelegate_range(addr, size)) {
/* Leak the granules */
continue;
}
free_pages_exact(phys_to_virt(addr), size);
As mentioned above, this is now in a common helper.
+
+ sro->addr_count = 0;
+}
+EXPORT_SYMBOL_GPL(rmi_sro_free);
+
+long rmi_sro_memxfer_execute(struct rmi_sro_state *sro, gfp_t gfp)
+{
+ unsigned long sro_handle;
+ struct arm_smccc_1_2_regs *regs = &sro->regs;
+ bool cancelled = false;
+
+ rmi_smccc_invoke(regs, regs);
+
+ sro_handle = regs->a1;
+
+ while (RMI_RETURN_STATUS(regs->a0) == RMI_INCOMPLETE) {
+ bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0);
+ int ret = 0;
+
Strictly speaking, we need to refresh the SRO handle after every RMI call.
bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0);
unsigned long sro_handle = regs->a1;
int ret = 0;
Ack for both instances
+
+ rmi_smccc_invoke(regs, regs);
+
+ sro_handle = regs->a1;
+
+ while (RMI_RETURN_STATUS(regs->a0) == RMI_INCOMPLETE) {
+ bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0);
+
Strictly speaking, we need to refresh the SRO handle after every RMI call.
bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0);
unsigned long sro_handle = regs->a1;
as above.
/types.h>
+#define RMI_MAX_ADDR_LIST 256
+
ng rmi_sro_execute(struct arm_smccc_1_2_regs *regs);
+
+#define rmi_sro_memxfer_cmd(sro, gfp, ...) ({ \
+ struct rmi_sro_state *__sro = (sro); \
+ *__sro = (struct rmi_sro_state){ .regs = {__VA_ARGS__} }; \
+ long __ret = rmi_sro_memxfer_execute(__sro, gfp); \
+ rmi_sro_free(__sro); \
+ __ret; \
+})
+
The temporary storage space for 'struct rmi_sro_state' in the stack due to
'*__sro = (struct rmi_sro_state){ .regs = {__VA_ARGS__} };' can be avoided
by:
__sro->regs = {__VA_ARGS__};
Have got this already in, based on Sashiko review.
+static inline long rmi_granule_range_delegate(unsigned long base,
+ unsigned long top,
+ unsigned long *out_top)
+{
+ struct arm_smccc_1_2_regs regs = {
+ SMC_RMI_GRANULE_RANGE_DELEGATE, base, top
+ };
+ long ret = rmi_sro_execute(®s);
+
+ if (ret == RMI_SUCCESS && out_top)
+ *out_top = regs.a1;
+
+ return ret;
+}
+
It seems rmi_granule_range_delegate() is used for once by rmi.c::rmi_delegate_range().
If so, we needn't to expose the function. The logic here can be combined to
rmi.c::rmi_delegate_range().
+/**
+ * rmi_granule_range_undelegate() - Undelegate a range of granules
...
+
It seems rmi_granule_range_undelegate() is used for once by rmi.c::rmi_undelegate_range().
If so, we needn't expose the function. The logic here can be combined to rmi_undelegate_range().
Ack
Thanks
Suzuki