Re: [PATCH v18 4/7] firmware: arm_rmm: Add support for SRO
From: Suzuki K Poulose
Date: Mon Sep 14 2026 - 02:29:23 EST
Hi Gavin
Thank you for the review, I will address most of them. Responses inline.
On 14/09/2026 06:04, Gavin Shan wrote:
On 9/12/26 6:36 PM, 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>
---
drivers/firmware/arm_rmm/rmi.c | 586 +++++++++++++++++++++++++++++++++
include/linux/arm-rmi-cmds.h | 41 +++
2 files changed, 627 insertions(+)
diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/ arm_rmm/rmi.c
index 5b0e342ce3d58..4f9898ece7547 100644
--- a/drivers/firmware/arm_rmm/rmi.c
+++ b/drivers/firmware/arm_rmm/rmi.c
I would drop rmi_granule_range_{delegate, undelegate}() by combining their logics to
their only callers rmi_{delegate, undelegate}_range(). More details are provided for
rmi_{delegate, undelegate}_range() in the below.
Ack
+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) {
+ /* Buggy RMM ? */
+ if (WARN_ON(out_top <= phys)) {
+ rmi_undelegate_range(top - size, size);
[top - size, size] is incorrect because we may be delegating a sub-range of the
range of granules. It's actually the caller's responsibility to undelegate the
graunles that have been delegated.
if (WARN_ON(out_top <= phys)) {
ret = -ENXIO;
break;
}
Agree, I have done this already based on Sashiko review, and added a comment too.
+/*
+ * 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));
+}
+
+/*
+ * free_addr_range: Free memory described by the address range entry, which may
+ * be partially consumed by RMM.
+ *
+ * @entry: RMI_ADDR_RANGE descriptor
+ * @consumed_size: Page aligned size consumed by the RMM from the address range.
+ *
+ * If the state of the address is DELEGATED, undelegate it back, before freeing.
+ * Leaks the memory if we cannot undelegate the range.
+ */
+static void free_addr_range(unsigned long entry, unsigned long consumed_size)
+{
+ unsigned long phys = RMI_ADDR_RANGE_ADDR(entry);
+ unsigned long block_size_fld = RMI_ADDR_RANGE_BLOCK_SIZE(entry);
+ unsigned long count = RMI_ADDR_RANGE_COUNT(entry);
+ unsigned long state = RMI_ADDR_RANGE_STATE(entry);
+ unsigned long size = rmi_addr_block_size_to_bytes(block_size_fld) * count;
+
+ WARN_ON(!PAGE_ALIGNED(phys) || !PAGE_ALIGNED(consumed_size));
+
+ /* Adjust the address and size for partially consumed entry */
+ phys += consumed_size;
+ size -= consumed_size;
+ /*
+ * Undelegate the pages back if required. If we can't
+ * change them back, leak the pages.
+ */
+ if (state == RMI_OP_MEM_DELEGATED &&
+ WARN_ON(rmi_undelegate_range(phys, size)))
+ return;
+ free_pages_exact(phys_to_virt(phys), size);
+}
+
+static void rmi_op_continue(unsigned long sro_handle, unsigned long flags,
+ struct arm_smccc_1_2_regs *out_regs)
+{
+ *out_regs = (struct arm_smccc_1_2_regs) {
+ SMC_RMI_OP_CONTINUE, sro_handle, flags
+ };
+
+ rmi_smccc_invoke(out_regs);
+}
+
The pattern 'regs' is used in some of the 'struct arm_smccc_1_2_regs' arguments
or variables in this series, which is incosistent to the existing patterns which
is either 'args' or 'res' by searching the source files using 'git grep arm_smccc_1_2_regs'.
So I would suggest we have the fixed the pattern 'args' :-)
I would prefer to keep it "regs" as, unlike the smccc_1_1 calls, we
pass "arm_smccc_1_2_regs" for both arguments and results. In this case
we are using a single structure, so, to avoid the confusion, I
intentionally used regs
+
+int rmi_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(rmi_free_delegated_page);
+
I would move rmi_free_delegated_page() right after rmi_undelegate_range().
Ack
+^^^^^^
+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;
+
Unecessary blank line.
Removed
...
+
+void rmi_sro_free(struct rmi_sro_state *sro)
+{
+ /* Handle the worse */
+ if (WARN_ON(sro->addr_count < 0))
+ return;
+
+ if (WARN_ON(sro->addr_count > RMI_MAX_ADDR_LIST))
+ sro->addr_count = RMI_MAX_ADDR_LIST;
+
+ for (int i = 0; i < sro->addr_count; i++)
+ free_addr_range(sro->addr_list[i], 0);
+
+ sro->addr_count = 0;
+}
+EXPORT_SYMBOL_GPL(rmi_sro_free);
+
+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_RETURN_STATUS(regs->a0) == RMI_INCOMPLETE) {
+ bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0) == RMI_OP_CAN_CANCEL;
+ int ret = 0;
+
+ switch (RMI_RETURN_MEMREQ(regs->a0)) {
+ case RMI_OP_MEM_REQ_NONE:
+ rmi_op_continue(sro_handle, RMI_CONTINUE_KEEP_GOING,
+ regs);
+ break;
+ case RMI_OP_MEM_REQ_DONATE:
+ ret = rmi_sro_donate(sro, sro_handle, regs->a2, regs,
+ gfp);
+ break;
+ case RMI_OP_MEM_REQ_RECLAIM:
+ ret = rmi_sro_reclaim(sro, sro_handle, regs);
+ break;
+ default:
+ ret = WARN_ON_ONCE(1);
+ break;
"ret = WARN_ON_ONCE(1)" is same to "ret = true". I guess we would return
-EINVAL here.
WARN_ON_ONCE(1);
ret = -EINVAL;
break;
Ack, this should be -ENXIO
Cheers