Re: [PATCH v18 1/7] firmware: arm_rmm: Add SMC definitions for calling the RMM

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

On 21/09/2026 10:27, Suzuki K Poulose wrote:
> On 19/09/2026 02:27, Jonathan Cameron wrote:
>>> The RMM (Realm Management Monitor) provides functionality that can be
>>> accessed by SMC calls from the host.
>>>
>>> The SMC definitions are based on DEN0137[1] version 2.0-bet3
>>>
>>> [1] https://developer.arm.com/documentation/den0137/2-0bet3/
>>>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> With Gavin's nitpicks and the GENMASK_ULL() from sashiko, just a few
>> comments inline.  Mostly on subtle inconsistencies that really don't
>> matter that much.
>>
>>>   include/linux/arm-smccc-rmi.h | 497 ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 497 insertions(+)
>>>   create mode 100644 include/linux/arm-smccc-rmi.h
>>>
>>> diff --git a/include/linux/arm-smccc-rmi.h b/include/linux/arm-smccc- 
>>> rmi.h
>>> new file mode 100644
>>> index 000000000000..214d6228dfc2
>>> --- /dev/null
>>> +++ b/include/linux/arm-smccc-rmi.h
>>> @@ -0,0 +1,497 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (C) 2023-2026 ARM Ltd.
>>> + *
>>> + * The values and structures in this file are from the Realm 
>>> Management Monitor
>>> + * specification (DEN0137) version 2.0-bet3:
>>> + * https://developer.arm.com/documentation/den0137/2-0bet3/
>>> + */
>>> +
>>> +#ifndef __LINUX_ARM_SMCCC_RMI_H_
>>> +#define __LINUX_ARM_SMCCC_RMI_H_
>>> +
>>> +#include <linux/arm-smccc.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bits.h>
>>> +#include <linux/build_bug.h>
>>> +#include <linux/sizes.h>
>>> +
>>> +#include <asm/page.h>
>>> +
>>> +#define SMC_RMI_CALL(func)                \
>>> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,        \
>>> +               ARM_SMCCC_SMC_64,        \
>>> +               ARM_SMCCC_OWNER_STANDARD,    \
>>> +               (func))
>>
>> Obviously it is v18 so probably a future thing but nothing about this
>> is RMI specific.  Could be used for ARM_SMCCC_TRNG_RND64 for instance.
>> I'm not entirely sure what we'd call such a macro
>>
>> ARM_SMCCC_CALL_VAL64_STD() maybe?
> 
> ARM_SMCCC_STD_CALL64_VAL() ?
> 
> But, I would leave it as a wider cleanup in the tree as a separate
> series.
> 
>>
>> I'm not just not keen on macros whose names to me hint at something
>> special. FWIW this also matches SMC_RSI_FID() and FFA_SMC64().
>> Isn't it nice when we have a predictable naming scheme :)
>>
>> FID (for Function IDentifier) is in the spec, so maybe?
>>
>> Anyhow, I don't really care that much.
>>
>>> +
>>> +#define SMC_RMI_VERSION                SMC_RMI_CALL(0x0150)
>>> +
>>
>>
>>> +#define RMI_ABI_MAJOR_VERSION    2
>>> +#define RMI_ABI_MINOR_VERSION    0
>>> +
>>> +#define RMI_ABI_VERSION_GET_MAJOR(version) ((version) >> 16)
>>
>> I'd mask it.  Mostly because that would shout that it is only 15 bits.
>>
> 
> Ack
> 
>>> +#define RMI_ABI_VERSION_GET_MINOR(version) ((version) & 0xFFFF)
>>> +#define RMI_ABI_VERSION(major, minor)      (((major) << 16) | (minor))
>>
>> I'd go all in on FIELD_PREP() / FIELD_GET() + GENMASK just for the
>> sake of consistency + not having to be careful that everything is
>> checked for fit to keep the LLM bots happy.
>>
>>> +
>>> +#define RMI_RETURN_STATUS_MASK        GENMASK(7, 0)
>>> +#define RMI_RETURN_INDEX_MASK        GENMASK(15, 8)
>>> +#define RMI_RETURN_MEMREQ_MASK        GENMASK(9, 8)
>>> +#define RMI_RETURN_CAN_CANCEL_MASK    BIT(10)
>>> +
>>> +#define RMI_RETURN_STATUS(ret)        
>>> FIELD_GET(RMI_RETURN_STATUS_MASK, ret)
>>> +#define RMI_RETURN_INDEX(ret)        
>>> FIELD_GET(RMI_RETURN_INDEX_MASK, ret)
>>
>> What's this one?  I can't find anything in the spec that matches it
>> and as far as I can tell you don't use it in this series.
> 
> This is coming from RmiResultDataLevel. See RmiResult type.
> This was renamed after we introduce the RmiResultDataIncomplete.
> It is used in the KVM code to find the "level" where a command
> failed/walked.
> 
> I could rename it to RMI_RESULT_DATA_LEVEL() ?
> Similarly RMI_RESULT_STATUS instead of RMI_RETURN_*
> 
>>
>>> +#define RMI_RETURN_MEMREQ(ret)        
>>> FIELD_GET(RMI_RETURN_MEMREQ_MASK, ret)
>>> +#define RMI_RETURN_CAN_CANCEL(ret)    
>>> FIELD_GET(RMI_RETURN_CAN_CANCEL_MASK, ret)
> 
>> These are obscure enough to find in the spec I'd give a comment just
>> to save the sanity of anyone looking for them.
> 
> As above, they are really RMI_RESULT_DATA_INCOMPLETE_*
> 
>>
>>> +/*
>>> + * Note many of these fields are smaller than u64 but all fields 
>>> have u64
>>> + * alignment, so use u64 to ensure correct alignment.
>>
>> Obviously this is only going to run on arm64 so it's not critical, but
>> more generally u64s aren't always 64 bit aligned.  So if you 'really'
>> care aligned_u64 is there to ensure it.  Meh, arm64 so fine.
> 
> Agreed, I am worried about the churn in the consumer code. Also, like
> you said, this is only for ARM64. So, I would pass it.
> 
>>
>>> + */
>>> +struct rmm_config {
>>> +    union { /* 0x0 */
>>> +        struct {
>>> +            u64 tracking_region_size;
>>> +            u64 rmi_granule_size;
>>> +        };
>>> +        u8 sizer[SZ_4K];
>>> +    };
>>> +};
>>> +
>>> +static_assert(sizeof(struct rmm_config) == SZ_4K);
>>> +
>>> +#define RMI_REALM_PARAM_FLAG_SVE        BIT(1)
>>> +#define RMI_REALM_PARAM_FLAG_PMU        BIT(2)
>>> +#define RMI_REALM_PARAM_FLAG_DA            BIT(3)
>>> +#define RMI_REALM_PARAM_FLAG_LFA_POLICY        GENMASK(6, 5)
>>> +#define RMI_REALM_PARAM_FLAG_MEC_POLICY        GENMASK(8, 7)
>>
>> Tiny bit inconsistent. When do you decide _MASK is needed and when
>> not?  Seems a little too random for multibit fields.
> 
> I will try to clean this up.

Actually, this is used when we don't consume them from RMM. i.e.,
we never call FIELD_GET() on them. But thats not a reason for
not being consistent in naming. I can fix it

Cheers
Suzuki