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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Mon, 21 Sep 2026 11:02:56 +0100
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:

> 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

Too obscure for me!  So if not too painful cleaner to just fix it.

Jonathan

> 
> Cheers
> Suzuki
> 
>