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

..

>>> +
>>> +struct rec_exit {
>>> +    union { /* 0x000 */
>>> +        u8 exit_reason;
>>> +        u8 padding0[0x100];

...

>>
>>> +    };
>>> +    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?
> 
> Please feel free to point out, if you spot any. I am happy
> to fix them.

I took a look and the padding is for extra "u64" bits in there.
e.g, ripas_value offset = 0x510 and the s2ap_base is really at 0x520
not 0x518. Same applies for the pmu_ovf_status and vsmmu.

So, I would leave them as they are. The rest are consistent.
e.g, num_bps and num_wps. in realm_parameter

Cheers
Suzuki


>