Re: [PATCH v2 03/10] xen/arm: ffa: Fix version negotiation

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
<bertrand.marquis@xxxxxxx> wrote:
>
> Fix FFA version negotiation with the firmware to follow the
> specification guidance more closely (see FF-A Specification Version 1.1
> in chapter 13.2.1).
> When the firmware returns OK we can have several cases:
> - the version requested is accepted but the firmware supports a greater
>   one in the same major.
> - the firmware supports a greater major version. It could still return
>   OK even if the version requested is not accepted. Reject it.
> - the firmware supports a lower version. It will return OK and give that
>   version. Check if we support it and use it or reject it if we do not.
>
> Adapt the code to:
> - reject any version lower than the one we support or not with the same
>   major version
> - use the version returned if in our supported range (currently 1.1
>   only)
> - use 1.1 if the version returned is greater.
>
> Also adapt the handling of version requests from VM:
> - use our version if same major but greater minor is requested
> - use requested version if same major but lower minor is requested
> - do not use if incompatible major is requested
> - always return our version without error to the requester
>
> [1] https://developer.arm.com/documentation/den0077/e/
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes in v2:
> - add link in commit message and code to the version and chapter of
>   the spec.
> - fix comment coding style
> - introduce macros to get the major and minor of a version
> - add BUILD_BUG_ON to validate that the SPMC version we want is
>   compatible with our own version
> - rework version testing and selecting to be clearer by discarding
>   different major or version lower than what we want and then selecting
>   based on the minor version.
> - fix get_version handling to be more generic
> ---
>  xen/arch/arm/tee/ffa.c         | 54 +++++++++++++++++++++++++---------
>  xen/arch/arm/tee/ffa_private.h |  3 ++
>  2 files changed, 43 insertions(+), 14 deletions(-)

Looks good.
Reviewed-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>

Cheers,
Jens

>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index dde932422ecf..1ee6b2895e92 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -148,13 +148,20 @@ static void handle_version(struct cpu_user_regs *regs)
>      struct ffa_ctx *ctx = d->arch.tee;
>      uint32_t vers = get_user_reg(regs, 1);
>
> -    if ( vers < FFA_VERSION_1_1 )
> -        vers = FFA_VERSION_1_0;
> -    else
> -        vers = FFA_VERSION_1_1;
> -
> -    ctx->guest_vers = vers;
> -    ffa_set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
> +    /*
> +     * Guest will use the version it requested if it is our major and minor
> +     * lower or equals to ours. If the minor is greater, our version will be
> +     * used.
> +     * In any case return our version to the caller.
> +     */
> +    if ( FFA_VERSION_MAJOR(vers) == FFA_MY_VERSION_MAJOR )
> +    {
> +        if ( FFA_VERSION_MINOR(vers) > FFA_MY_VERSION_MINOR )
> +            ctx->guest_vers = FFA_MY_VERSION;
> +        else
> +            ctx->guest_vers = vers;
> +    }
> +    ffa_set_regs(regs, FFA_MY_VERSION, 0, 0, 0, 0, 0, 0, 0);
>  }
>
>  static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t 
> fid)
> @@ -537,18 +544,39 @@ static bool ffa_probe(void)
>          goto err_no_fw;
>      }
>
> -    if ( vers < FFA_MIN_SPMC_VERSION || vers > FFA_MY_VERSION )
> +    /* Some sanity check in case we update the version we support */
> +    BUILD_BUG_ON(FFA_MIN_SPMC_VERSION > FFA_MY_VERSION);
> +    BUILD_BUG_ON(FFA_VERSION_MAJOR(FFA_MIN_SPMC_VERSION) !=
> +                                   FFA_MY_VERSION_MAJOR);
> +
> +    major_vers = FFA_VERSION_MAJOR(vers);
> +    minor_vers = FFA_VERSION_MINOR(vers);
> +
> +    if ( major_vers != FFA_MY_VERSION_MAJOR ||
> +         minor_vers < FFA_VERSION_MINOR(FFA_MIN_SPMC_VERSION) )
>      {
> -        printk(XENLOG_ERR "ffa: Incompatible version %#x found\n", vers);
> +        printk(XENLOG_ERR "ffa: Incompatible firmware version %u.%u\n",
> +               major_vers, minor_vers);
>          goto err_no_fw;
>      }
>
> -    major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT)
> -                 & FFA_VERSION_MAJOR_MASK;
> -    minor_vers = vers & FFA_VERSION_MINOR_MASK;
>      printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>             major_vers, minor_vers);
>
> +    /*
> +     * If the call succeed and the version returned is higher or equal to
> +     * the one Xen requested, the version requested by Xen will be the one
> +     * used. If the version returned is lower but compatible with Xen, Xen
> +     * will use that version instead.
> +     * A version with a different major or lower than the minimum version
> +     * we support is rejected before.
> +     * See https://developer.arm.com/documentation/den0077/e/ chapter 13.2.1
> +     */
> +    if ( minor_vers > FFA_MY_VERSION_MINOR )
> +        ffa_fw_version = FFA_MY_VERSION;
> +    else
> +        ffa_fw_version = vers;
> +
>      /*
>       * At the moment domains must support the same features used by Xen.
>       * TODO: Rework the code to allow domain to use a subset of the
> @@ -564,8 +592,6 @@ static bool ffa_probe(void)
>          }
>      }
>
> -    ffa_fw_version = vers;
> -
>      if ( !ffa_rxtx_init() )
>      {
>          printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n");
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index 7c6b06f686fc..045d9c4a0b56 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -35,6 +35,9 @@
>  #define MAKE_FFA_VERSION(major, minor)  \
>          ((((major) & FFA_VERSION_MAJOR_MASK) << FFA_VERSION_MAJOR_SHIFT) | \
>           ((minor) & FFA_VERSION_MINOR_MASK))
> +#define FFA_VERSION_MAJOR(vers) (((vers) >> FFA_VERSION_MAJOR_SHIFT) & \
> +                                 FFA_VERSION_MAJOR_MASK)
> +#define FFA_VERSION_MINOR(vers) ((vers) & FFA_VERSION_MINOR_MASK)
>
>  #define FFA_VERSION_1_0         MAKE_FFA_VERSION(1, 0)
>  #define FFA_VERSION_1_1         MAKE_FFA_VERSION(1, 1)
> --
> 2.47.0
>