Re: [XEN PATCH v7 09/20] xen/arm: ffa: add support for FFA_ID_GET

[Julien Grall]

Hi,

On 27/02/2023 14:48, Bertrand Marquis wrote:
On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
Adds support for the FF-A function FFA_ID_GET to return the ID of the
calling client.

Signed-off-by: Jens Wiklander <jens.wiklan...@linaro.org>
---
xen/arch/arm/tee/ffa.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 8b0b80ce1ff5..463fd7730573 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -167,6 +167,12 @@ static bool ffa_get_version(uint32_t *vers)
     return true;
}

+static uint16_t get_vm_id(const struct domain *d)
+{
+    /* +1 since 0 is reserved for the hypervisor in FF-A */
+    return d->domain_id + 1;
+}
+
static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
                      register_t v2, register_t v3, register_t v4, register_t 
v5,
                      register_t v6, register_t v7)
@@ -181,6 +187,12 @@ static void set_regs(struct cpu_user_regs *regs, 
register_t v0, register_t v1,
         set_user_reg(regs, 7, v7);
}

+static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
+                             uint32_t w3)
+{
+    set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
+}
+
static void handle_version(struct cpu_user_regs *regs)
{
     struct domain *d = current->domain;
@@ -210,6 +222,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     case FFA_VERSION:
         handle_version(regs);
         return true;
+    case FFA_ID_GET:
+        set_regs_success(regs, get_vm_id(d), 0);
+        return true;

     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -221,7 +236,11 @@ static int ffa_domain_init(struct domain *d)
{
     struct ffa_ctx *ctx;

-    if ( !ffa_version )
+     /*
+      * We can't use that last possible domain ID or get_vm_id() would cause
+      * an overflow.
+      */
+    if ( !ffa_version || d->domain_id == UINT16_MAX)
         return -ENODEV;

In reality the overflow could only happen if this is called by the IDLE domain 
right now.
Anyway this could change and this is making the code more robust at no real 
cost.

I would just suggest here to return a different error code like ERANGE for 
example to
prevent missing ENODEV with other cases not related to FFA not being available.

+1. I would also like to suggest to use >= rather than == in case we 
decide to support more than 16-bit domid.

Cheers,

--
Julien Grall