[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator



Hi Jens,

Mainly reviewing this patch from a Xen PoV. I will leave the others to review the patch from a spec-compliance PoV.

On 13/04/2023 08:14, Jens Wiklander wrote:
+struct ffa_ctx {
+    /* FF-A version used by the guest */
+    uint32_t guest_vers;
+};
+
+/* Negotiated FF-A version to use with the SPMC */
+static uint32_t ffa_version __ro_after_init;

Coding style: We tend to add attributes just after the type. E.g.:

static uint32_t __ro_after_init ffa_version;

I can deal with this one on commit if there is nothing else to change.

[...]

+static int ffa_domain_init(struct domain *d)
+{
+    struct ffa_ctx *ctx;
+
+    if ( !ffa_version )
+        return -ENODEV;
+
+    ctx = xzalloc(struct ffa_ctx);
+    if ( !ctx )
+        return -ENOMEM;
+
+    d->arch.tee = ctx;
+
+    return 0;
+}
+
+/* This function is supposed to undo what ffa_domain_init() has done */

I think there is a problem in the TEE framework. The callback .relinquish_resources() will not be called if domain_create() failed. So this will result to a memory leak.

We also can't call .relinquish_resources() on early domain creation failure because relinquishing resources can take time and therefore needs to be preemptible.

So I think we need to introduce a new callback domain_free() that will be called arch_domain_destroy(). Is this something you can look at?

+static int ffa_relinquish_resources(struct domain *d)
+{
+    struct ffa_ctx *ctx = d->arch.tee;
+
+    if ( !ctx )
+        return 0;
+
+    XFREE(d->arch.tee);
+
+    return 0;
+}

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.