Hi Jens, Thanks a lot for the review. Please find my answers here after.
> On 1 Dec 2025, at 15:43, Jens Wiklander <[email protected]> wrote: > > Hi Bertrand, > > On Thu, Nov 27, 2025 at 4:52 PM Bertrand Marquis > <[email protected]> wrote: >> >> Bring the FF-A headers up to the v1.2 baseline and fix the function-number >> range used for ABI discovery: >> >> - update FFA_FNUM_MAX_VALUE so the FF-A function-number window covers the >> full v1.2 range, and derive the ABI bitmap bounds from >> FFA_FNUM_MIN_VALUE/FFA_FNUM_MAX_VALUE instead of hard-coding >> FFA_ERROR/FFA_MSG_SEND2 >> - define the new v1.2 function IDs; CONSOLE_LOG and >> PARTITION_INFO_GET_REGS are added for ABI discovery even though they are >> not implemented yet >> - extend the firmware ABI table to probe RUN and >> MSG_SEND_DIRECT_REQ2/RESP2 >> - while there, fix an off-by-one in ffa_fw_supports_fid(): the computed bit >> index must be strictly smaller than FFA_ABI_BITMAP_SIZE, so use >= in the >> bounds check >> >> Keep FFA_MY_VERSION at 1.1 for now; we only advertise v1.2 once the >> implementation is fully compliant. >> >> Signed-off-by: Bertrand Marquis <[email protected]> >> --- >> xen/arch/arm/include/asm/tee/ffa.h | 2 +- >> xen/arch/arm/tee/ffa.c | 4 ++++ >> xen/arch/arm/tee/ffa_private.h | 18 +++++++++++------- >> 3 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/tee/ffa.h >> b/xen/arch/arm/include/asm/tee/ffa.h >> index 24cd4d99c8f9..c587f76e63ca 100644 >> --- a/xen/arch/arm/include/asm/tee/ffa.h >> +++ b/xen/arch/arm/include/asm/tee/ffa.h >> @@ -16,7 +16,7 @@ >> #include <asm/types.h> >> >> #define FFA_FNUM_MIN_VALUE _AC(0x60,U) >> -#define FFA_FNUM_MAX_VALUE _AC(0x86,U) >> +#define FFA_FNUM_MAX_VALUE _AC(0x8F,U) > > This is MAX+1, if I'm not mistaken. It depends on how we see it: - FFA v1.2 does not define calls over 8E so in this sense yes - SMCCC is reserving 0x60 to 0xEF for FF-A so in this sense we are not completely covering the SMCCC space I must admit i just used 0x90-1 to cover all 0x60 to 0x90 excluded because i did not wanted to have something to big where we would not use all bits. So I will keep 0x8F adding a comment saying that SMCCC is reserving the space until 0xEF but only number up to 0x8E are defined in ffa v1.2 so that FNUM_MAX_VALUE is the first unused value so that I will also remove the the +1 after. > >> >> static inline bool is_ffa_fid(uint32_t fid) >> { >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >> index 1d0239cf6950..2b4e24750d52 100644 >> --- a/xen/arch/arm/tee/ffa.c >> +++ b/xen/arch/arm/tee/ffa.c >> @@ -11,6 +11,8 @@ >> * https://developer.arm.com/documentation/den0077/a >> * FF-A-1.1-REL0: FF-A specification version 1.1 available at >> * https://developer.arm.com/documentation/den0077/e >> + * FF-A-1.2-REL0: FF-A specification version 1.2 available at >> + * https://developer.arm.com/documentation/den0077/j >> * TEEC-1.0C: TEE Client API Specification version 1.0c available at >> * >> https://globalplatform.org/specs-library/tee-client-api-specification/ >> * >> @@ -102,6 +104,8 @@ static const struct ffa_fw_abi ffa_fw_abi_needed[] = { >> FW_ABI(FFA_MSG_SEND_DIRECT_REQ_32), >> FW_ABI(FFA_MSG_SEND_DIRECT_REQ_64), >> FW_ABI(FFA_MSG_SEND2), >> + FW_ABI(FFA_MSG_SEND_DIRECT_REQ2), >> + FW_ABI(FFA_RUN), >> }; >> >> /* >> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >> index 6dbdb200d840..d7e6b6f5ef45 100644 >> --- a/xen/arch/arm/tee/ffa_private.h >> +++ b/xen/arch/arm/tee/ffa_private.h >> @@ -15,6 +15,7 @@ >> #include <xen/spinlock.h> >> #include <xen/time.h> >> #include <xen/types.h> >> +#include <asm/tee/ffa.h> >> >> /* Error codes */ >> #define FFA_RET_OK 0 >> @@ -42,6 +43,7 @@ >> >> #define FFA_VERSION_1_0 MAKE_FFA_VERSION(1, 0) >> #define FFA_VERSION_1_1 MAKE_FFA_VERSION(1, 1) >> +#define FFA_VERSION_1_2 MAKE_FFA_VERSION(1, 2) >> /* The minimal FF-A version of the SPMC that can be supported */ >> #define FFA_MIN_SPMC_VERSION FFA_VERSION_1_1 >> >> @@ -270,6 +272,10 @@ >> #define FFA_RX_ACQUIRE 0x84000084U >> #define FFA_SPM_ID_GET 0x84000085U >> #define FFA_MSG_SEND2 0x84000086U >> +#define FFA_CONSOLE_LOG 0x8400008AU > > This is the 32-bit version of the interface. There's also a 64-bit version. Ack i will add the missing definition for 64bit and rename that one with a 32 suffix. > >> +#define FFA_PARTITION_INFO_GET_REGS 0x8400008BU >> +#define FFA_MSG_SEND_DIRECT_REQ2 0xC400008DU >> +#define FFA_MSG_SEND_DIRECT_RESP2 0xC400008EU >> >> /** >> * Encoding of features supported or not by the fw in a bitmap: >> @@ -280,11 +286,9 @@ >> #define FFA_ABI_ID(id) ((id) & ARM_SMCCC_FUNC_MASK) >> #define FFA_ABI_CONV(id) (((id) >> ARM_SMCCC_CONV_SHIFT) & BIT(0,U)) >> >> -#define FFA_ABI_MIN FFA_ABI_ID(FFA_ERROR) >> -#define FFA_ABI_MAX FFA_ABI_ID(FFA_MSG_SEND2) >> - >> -#define FFA_ABI_BITMAP_SIZE (2 * (FFA_ABI_MAX - FFA_ABI_MIN + 1)) >> -#define FFA_ABI_BITNUM(id) ((FFA_ABI_ID(id) - FFA_ABI_MIN) << 1 | \ >> +#define FFA_ABI_BITMAP_SIZE (2 * (FFA_FNUM_MAX_VALUE - FFA_FNUM_MIN_VALUE >> \ >> + + 1)) > > Depending on whether FFA_FNUM_MAX_VALUE is MAX+1 or just MAX, we could > drop the +1. Agree i will remove the +1 with the change above. Cheers Bertrand > > Cheers, > Jens > >> +#define FFA_ABI_BITNUM(id) ((FFA_ABI_ID(id) - FFA_FNUM_MIN_VALUE) << 1 | >> \ >> FFA_ABI_CONV(id)) >> >> /* Constituent memory region descriptor */ >> @@ -549,9 +553,9 @@ static inline int32_t ffa_hyp_rx_release(void) >> >> static inline bool ffa_fw_supports_fid(uint32_t fid) >> { >> - BUILD_BUG_ON(FFA_ABI_MIN > FFA_ABI_MAX); >> + BUILD_BUG_ON(FFA_FNUM_MIN_VALUE > FFA_FNUM_MAX_VALUE); >> >> - if ( FFA_ABI_BITNUM(fid) > FFA_ABI_BITMAP_SIZE) >> + if ( FFA_ABI_BITNUM(fid) >= FFA_ABI_BITMAP_SIZE) >> return false; >> return test_bit(FFA_ABI_BITNUM(fid), ffa_fw_abi_supported); >> } >> -- >> 2.51.2
