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

[Jonathan Cameron]

> 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@xxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>

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?

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.

> +#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.

> +#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.

> +/*
> + * 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.

> + */
> +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.
> +
> +struct rec_params {
> +	union { /* 0x0 */
> +		u64 flags;
> +		u8 padding0[0x100];
> +	};
> +	union { /* 0x100 */
> +		u64 mpidr;
> +		u8 padding1[0x100];
> +	};
> +	union { /* 0x200 */
> +		u64 pc;
> +		u8 padding2[0x100];
> +	};
> +	union { /* 0x300 */
> +		u64 gprs[REC_CREATE_NR_GPRS];
> +		u8 padding3[0xd00];
> +	};
> +};
> +
> +static_assert(sizeof(struct rec_params) == SZ_4K);
Whilst the assert works and is need to prevent oversized the
dos never seem to provide any indication of the final trailing
padding other than indirectly and I don't like maths on Fridays ;).
Maybe union the inner union set with a u8 [SZ_4K]?

> +
> +struct rec_exit {
> +	union { /* 0x000 */
> +		u8 exit_reason;
> +		u8 padding0[0x100];
> +	};
> +	union { /* 0x100 */
> +		struct {
> +			u64 esr;
> +			u64 far;
> +			u64 hpfar;
> +			u64 rtt_tree;
> +		};
> +		u8 padding1[0x100];
> +	};
> +	union { /* 0x200 */
> +		u64 gprs[REC_RUN_GPRS];
> +		u8 padding2[0x100];
> +	};
> +	union { /* 0x300 */
> +		u8 padding3[0x100];

Why does this one exist rather than padding above
by 0x200?  Other structures don't seem to keep
to a specific stride so I'm not sure what this gives
you here.

> +	};
> +	union { /* 0x400 */
> +		struct {
> +			u64 cntp_ctl;
> +			u64 cntp_cval;
> +			u64 cntv_ctl;
> +			u64 cntv_cval;
> +		};
> +		u8 padding4[0x100];
> +	};
> +	union { /* 0x500 */
> +		struct {
> +			u64 ripas_base;
> +			u64 ripas_top;
> +			u8 ripas_value;
> +			u8 padding5[0xf];

In various other places you just use a u64 for a u8
+ padding. Why is this one special? And for that matter
various other fields later in this particular structure?

> +			u64 s2ap_base;
> +			u64 s2ap_top;
> +			u64 vdev_id_1;
> +			u64 vdev_id_2;
> +			u64 dev_mem_base;
> +			u64 dev_mem_top;
> +			u64 dev_mem_pa;
> +		};
> +		u8 padding6[0x100];
> +	};
> +	union { /* 0x600 */
> +		struct {
> +			u16 imm;
> +			u8 padding7[0x6];
> +			u64 plane;
> +		};
> +		u8 padding8[0x100];
> +	};
> +	union { /* 0x700 */
> +		struct {
> +			u8 pmu_ovf_status;
> +			u8 padding9[0xf];
> +			u64 vsmmu;
> +		};
> +		u8 padding10[0x100];
> +	};
> +};
> +
> +static_assert(sizeof(struct rec_exit) == SZ_2K);

> +
> +/* RMI_RTT_UNPROT_MAP_FLAGS definitions */

How this counts as a 'flags' field is slightly beyond me, but
indeed matches the spec naming even though it's a grab bag
of small fields...

> +#define RMI_RTT_UNPROT_MAP_FLAGS_OADDR_TYPE	GENMASK(1, 0)
> +#define RMI_RTT_UNPROT_MAP_FLAGS_LIST_COUNT	GENMASK(15, 2)
> +#define RMI_RTT_UNPROT_MAP_FLAGS_MEMATTR	GENMASK(18, 16)
> +#define RMI_RTT_UNPROT_MAP_FLAGS_S2AP		GENMASK(22, 19)

Nothing signficant enough in here to absolutely require changes so
Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>