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

[Julien Grall <julien@xxxxxxx>]

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