Re: [PATCH v19 4/7] firmware: arm_rmm: Add support for SRO
flat view
From: Catalin Marinas <catalin.marinas@arm.com>
Date: 2026-09-25 11:50:44
Also in:
kvm, kvmarm, linux-coco, lkml
On Thu, Sep 24, 2026 at 02:51:58PM +0100, Suzuki K Poulose wrote:
quoted hunk ↗ jump to hunk
+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 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 size = block_size * count; + unsigned long addr_range; + unsigned long donated_granules; + unsigned long donated_size; + int ret; + void *virt; + phys_addr_t phys; + + /* + * The RMM specification requires contiguous allocations are always a + * power of 2 + */ + if (WARN_ON_ONCE(!is_power_of_2(size))) + return -EINVAL; + + /* Reuse the cached address range if we have one */ + for (int i = 0; i < sro->addr_count; i++) { + unsigned long entry = sro->addr_list[i]; + + if (RMI_ADDR_RANGE_BLOCK_SIZE(entry) == block_size_fld && + 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 mem_donate; + } + } + + 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_BLOCK_SIZE_MASK, &addr_range, block_size_fld); + FIELD_MODIFY(RMI_ADDR_RANGE_COUNT_MASK, &addr_range, count);
Courtesy of an LLM - if we get a contiguous/4K pages request for 4MB (1024 pages), the above 10-bit field becomes 0. Should we reject the request or the updated spec will guarantee this won't happen? Somewhat related, with RMI_BLOCK_L2 or higher and appropriate count we can easily go over the MAX_PAGE_ORDER allocation and fail alloc_pages() (the contig case). IIUC, the kernel can reject the donation but current TF-RMM does not report RMI_OP_CAN_CANCEL (R_SZVNK says it can be cancelled). It gets complicated if we want to support large contiguous allocations here (e.g. alloc_contig_pages() can sleep). I'd rather just cancel the request, not support such large sizes. [...]
quoted hunk ↗ jump to hunk
+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, i;
[...]
+ for (int i = 0, src = addr_list_start + consumed_blocks;
Nit: we have 'int ret, i' earlier already, so you can drop the 'int' here. [...]
quoted hunk ↗ jump to hunk
+/* + * 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. + * + * Returns : + * -ECANCELLED - If the operation had to be aborted and SRO was cancellable.
Nit: -ECANCELED.
quoted hunk ↗ jump to hunk
+ * Otherwise, returns the result of the RMI command. + */ +long rmi_sro_execute(struct arm_smccc_1_2_regs *regs) +{ + bool cancelled = false; + unsigned long sro_handle = regs->a1; + + rmi_smccc_invoke(regs); + + sro_handle = regs->a1;
Nit: drop the first initialisation maybe? -- Catalin