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

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

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@arm.com>
>> Co-Developed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>



>> ---
>>   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