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

On 9/8/26 8:37 PM, Suzuki K Poulose wrote:

Ok.

quoted

quoted

+#define RMI_SUCCESS            0
+#define RMI_ERROR_INPUT            1
+#define RMI_ERROR_REALM            2
+#define RMI_ERROR_REC            3
+#define RMI_ERROR_RTT            4
+#define RMI_ERROR_NOT_SUPPORTED        5
+#define RMI_ERROR_DEVICE        6
+#define RMI_ERROR_RTT_AUX        7
+#define RMI_ERROR_PSMMU_ST        8
+#define RMI_ERROR_DPT            9
+#define RMI_BUSY            10
+#define RMI_ERROR_GLOBAL        11
+#define RMI_ERROR_TRACKING        12
+#define RMI_INCOMPLETE            13
+#define RMI_BLOCKED            14
+#define RMI_ERROR_GPT            15
+#define RMI_ERROR_GRANULE        16
+
+#define RMI_CONTINUE_KEEP_GOING        0
+#define RMI_CONTINUE_STOP        1
+
+#define RMI_OP_MEM_REQ_NONE        0
+#define RMI_OP_MEM_REQ_DONATE        1
+#define RMI_OP_MEM_REQ_RECLAIM        2
+
+#define RMI_DONATE_SIZE_MASK        3UL
+#define RMI_DONATE_COUNT_MASK        GENMASK(15, 2)
+#define RMI_DONATE_CONTIG_MASK        BIT(16)
+#define RMI_DONATE_STATE_MASK        GENMASK(18, 17)
+
+#define RMI_DONATE_SIZE(req)        FIELD_GET(RMI_DONATE_SIZE_MASK, req)
+#define RMI_DONATE_COUNT(req)        FIELD_GET(RMI_DONATE_COUNT_MASK, req)
+#define RMI_DONATE_CONTIG(req) FIELD_GET(RMI_DONATE_CONTIG_MASK, req)
+#define RMI_DONATE_STATE(req)        FIELD_GET(RMI_DONATE_STATE_MASK, req)
+

As above, RMI_DONATE_{SIZE, COUNT, CONTIG, STATE}_MASK are used for once in this
header file. So their definitions can be dropped by modifying the followup macros,
as below. Some enhancements are also applicable: (a) Use 'BLOCK' to indicate the
unit encoded in the request. (b) The macros are put into order from MSB to LSB.

#define RMI_DONATE_STATE(req)        FIELD_GET(GENMASK(18, 17), req)
#define RMI_DONATE_CONTIG(req)        FIELD_GET(GENMASK(16, 16), req)
#define RMI_DONATE_BLOCK_COUNT(req)    FIELD_GET(GENMASK(15, 2), req)
#define RMI_DONATE_BLOCK_SIZE(req)    FIELD_GET(GENMASK(1, 0), req)

Same as above.

Please rename RMI_DONATE_{SIZE, COUNT} to RMI_DONATE_BLOCK_{SIZE, COUNT} if possible.

quoted

quoted

+#define RMI_OP_MEM_DELEGATED        0
+#define RMI_OP_MEM_UNDELEGATED        1
+#define RMI_OP_MEM_CONDITIONAL        2
+

B4.6.40 RmiOpMemContig type is missed here and they should be used in the c code.

#define RMI_OP_MEM_NON_CONTIG    0
#define RMI_OP_MEM_CONTIG    1

In the c code, we shall have:

if (RMI_DONATE_CONTIG(req) == RMI_OP_MEM_CONTIG)) {
    ...
} else {
    ...
}

Does it make much sense though ? I prefer

if (RMI_DONATE_CONTIG(req))

to


if (RMI_DONATE_CONTIG(req) == RMI_OP_MEM_CONTIG)) {


than having a field for a single bit values. I am happy to change
this if there is strong preference to the other.

The point is RMM-v2.0-bet3 expose RMI_OP_{MEM, MEM_NON}_CONTIG instead of true/false
to indicate if the requested memory blocks are physically contiguous or not. So the
expression 'if (RMI_DONATE_CONTIG(req) == RMI_OP_MEM_CONTIG)' is consistent to the
RMM-v2.0-bet3 spec.

  

quoted

Similarly, B4.6.39 RmiOpCanCancel type is missed here and they should be used in
the c code.

#define RMI_OP_CANNOT_CANCEL    0
#define RMI_OP_CAN_CANCEL    1

In the c code, we shall have:

if (RMI_RETURN_CAN_CANCEL(ret) == RMI_OP_CAN_CANCEL) {
     ...
} else {
     ...
}

Same as above.

Please see above explanation.

quoted

quoted

+#define RMI_ADDR_TYPE_NONE        0
+#define RMI_ADDR_TYPE_SINGLE        1
+#define RMI_ADDR_TYPE_LIST        2
+
+#define RMI_ADDR_RANGE_SIZE_MASK    GENMASK(1, 0)
+#define RMI_ADDR_RANGE_COUNT_MASK    GENMASK(PAGE_SHIFT - 1, 2)
+#define RMI_ADDR_RANGE_ADDR_MASK    (PAGE_MASK & GENMASK(51, 0))
+#define RMI_ADDR_RANGE_STATE_MASK    GENMASK(63, 62)
+
+#define RMI_ADDR_RANGE_SIZE(ar) (FIELD_GET(RMI_ADDR_RANGE_SIZE_MASK, \
+                           (ar)))
+#define RMI_ADDR_RANGE_COUNT(ar) (FIELD_GET(RMI_ADDR_RANGE_COUNT_MASK, \
+                           (ar)))
+#define RMI_ADDR_RANGE_ADDR(ar)        ((ar) & RMI_ADDR_RANGE_ADDR_MASK)
+#define RMI_ADDR_RANGE_STATE(ar) (FIELD_GET(RMI_ADDR_RANGE_STATE_MASK, \
+                           (ar)))
+

I guess it wouldn't a bad idea to explictly indicate 'BLOCK' in the definitions, as
below. Some improvements are applicated either: (a) 'ar' is renamed to 'r'; (b) The
excessive parentheses are dropped to follow the patterns we already had; (c) For
RMI_ADDR_RANGE_ADDR(r), FIELD_GET() is used.

#define RMI_ADDR_RANGE_BLOCK_SIZE_MASK        GENMASK(1, 0)
#define RMI_ADDR_RANGE_BLOCK_COUNT_MASK        GENMASK(PAGE_SHIFT - 1, 2)
#define RMI_ADDR_RANGE_ADDR_MASK        (PAGE_MASK & GENMASK(51, 0))
#define RMI_ADDR_RANGE_STATE_MASK        GENMASK(63, 62)

#define RMI_ADDR_RANGE_BLOCK_SIZE(r) FIELD_GET(RMI_ADDR_RANGE_SIZE_MASK, r)
#define RMI_ADDR_RANGE_BLOCK_COUNT(r) FIELD_GET(RMI_ADDR_RANGE_COUNT_MASK, r)
#define RMI_ADDR_RANGE_ADDR(r) FIELD_GET(RMI_ADDR_RANGE_ADDR_MASK, r)
#define RMI_ADDR_RANGE_STATE(r) FIELD_GET(RMI_ADDR_RANGE_STATE_MASK, r)

Ack

quoted

quoted

+enum rmi_ripas {
+    RMI_EMPTY = 0,
+    RMI_RAM = 1,
+    RMI_DESTROYED = 2,
+    RMI_DEV = 3,
+};

quoted

quoted

+#define RMI_FEATURE_REGISTER_1_RMI_GRAN_SZ_4KB    BIT(0)
+#define RMI_FEATURE_REGISTER_1_RMI_GRAN_SZ_16KB    BIT(1)
+#define RMI_FEATURE_REGISTER_1_RMI_GRAN_SZ_64KB    BIT(2)
+#define RMI_FEATURE_REGISTER_1_HASH_SHA_256    BIT(3)
+#define RMI_FEATURE_REGISTER_1_HASH_SHA_384    BIT(4)
+#define RMI_FEATURE_REGISTER_1_HASH_SHA_512    BIT(5)
+#define RMI_FEATURE_REGISTER_1_MAX_RECS_ORDER    GENMASK(9, 6)
+#define RMI_FEATURE_REGISTER_1_L0GPTSZ        GENMASK(13, 10)
+#define RMI_FEATURE_REGISTER_1_PPS        GENMASK(16, 14)
+
+#define RMI_FEATURE_REGISTER_2_DA        BIT(0)
+#define RMI_FEATURE_REGISTER_2_DA_COH        BIT(1)
+#define RMI_FEATURE_REGISTER_2_VSMMU        BIT(2)
+#define RMI_FEATURE_REGISTER_2_ATS        BIT(3)
+#define RMI_FEATURE_REGISTER_2_PDEV_MAX_VDEVS_ORDER    GENMASK(7, 4)
+#define RMI_FEATURE_REGISTER_2_VDEV_KROU    BIT(8)
+#define RMI_FEATURE_REGISTER_2_NON_TEE_STREAM    BIT(9)
+#define RMI_FEATURE_REGISTER_2_REALM_MAX_VDEVS_ORDER    GENMASK(14, 10)
+
+#define RMI_FEATURE_REGISTER_3_MAX_NUM_AUX_PLANES    GENMASK(3, 0)
+#define RMI_FEATURE_REGISTER_3_RTT_PLANE        GENMASK(5, 4)
+#define RMI_FEATURE_REGISTER_3_RTT_S2AP_INDIRECT    BIT(6)
+
+#define RMI_FEATURE_REGISTER_4_MEC_COUNT        GENMASK(63, 0)
+

We would reorder those feature register definitions from MSB ro LSB :-)

Ack

Thanks for your review.

Cheers
Suzuki

Thanks,
Gavin