Re: [PATCH 01/10] xen/arm: ffa: add FF-A v1.2 function IDs

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 1 Dec 2025, at 16:19, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
> 
> Hi Jens,
> 
> Thanks a lot for the review.
> Please find my answers here after.
> 
>> On 1 Dec 2025, at 15:43, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>> 
>> Hi Bertrand,
>> 
>> On Thu, Nov 27, 2025 at 4:52 PM Bertrand Marquis
>> <bertrand.marquis@xxxxxxx> 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 <bertrand.marquis@xxxxxxx>
>>> ---
>>> 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.

Actually this would not be coherent with how psci.h is using FNUM so
I will set the value to 0x8E with a comment that it is the range used
as of FF-A v1.2 and ...

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

I will keep this one as is as a consequence.

Cheers
Bertrand