[v3,03/10] xen/arm: ffa: Fix version negotiation

Commit Message

Bertrand Marquis Nov. 27, 2024, 4:07 p.m. UTC

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 <[e-mail]>
Reviewed-by: Jens Wiklander <[e-mail]>
---
Changes in v3:
- add Jens R-b
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         | 53 +++++++++++++++++++++++++---------
 xen/arch/arm/tee/ffa_private.h |  3 ++
 2 files changed, 43 insertions(+), 13 deletions(-)

Patch

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index ade6aaa6fd1d..c5dcb4fe240c 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,17 +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
@@ -563,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)