Re: [Xen-devel] [PATCH v4 03/10] xen/arm: optee: add OP-TEE mediator skeleton

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Julien,

Thank you for the review.

Julien Grall writes:

>> + */
>> +
>> +#include <xen/device_tree.h>
>> +#include <xen/sched.h>
>> +
>> +#include <asm/smccc.h>
>> +#include <asm/tee/tee.h>
>> +#include <asm/tee/optee_msg.h>
>> +#include <asm/tee/optee_smc.h>
>> +
>> +#define OPTEE_ENABLED ((void*)0x1)
>
> Please don't do that. Introduce a dummy structure instead and fill it
> up when needed.
>

It will be removed in the patch #5. But probably yes, I can
create empty optee_domain structure in this patch.

>> +
>> +/* Client ID 0 is reserved for hypervisor itself */
>
> s/for/for the/
>
>> +#define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1)
>> +
>> +static bool optee_probe(void)
>> +{
>> +    struct dt_device_node *node;
>> +    struct arm_smccc_res resp;
>> +
>> +    /* Check for entry in dtb */
>> +    node = dt_find_compatible_node(NULL, NULL, "linaro,optee-tz");
>> +    if ( !node )
>> +        return false;
>> +
>> +    /* Check UID */
>> +    arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END), &resp);
>> +
>> +    if ( (uint32_t)resp.a0 != OPTEE_MSG_UID_0 ||
>> +         (uint32_t)resp.a1 != OPTEE_MSG_UID_1 ||
>> +         (uint32_t)resp.a2 != OPTEE_MSG_UID_2 ||
>> +         (uint32_t)resp.a3 != OPTEE_MSG_UID_3 )
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +static int optee_domain_init(struct domain *d)
>> +{
>> +    struct arm_smccc_res resp;
>> +
>> +    /*
>> +     * Inform OP-TEE about a new guest.
>> +     * This is a "Fast" call in terms of OP-TEE. This basically
>> +     * means that it can't be preempted, because there is no
>> +     * thread allocated for it in OP-TEE. It is close to atomic
>> +     * context in linux kernel: E.g. no blocking calls can be issued.
>
> This does not really make sense to describe Linux here. Can't you just
> make the wording OS agnostic?
>
It was just as an example. But okay, I'll remove mention of Linux.


>> +     * Also, interrupts are disabled.
>> +     *
>> +     * a7 should be 0, so we can't skip last 6 parameters of arm_smccc_smc()
>> +     */
>> +    arm_smccc_smc(OPTEE_SMC_VM_CREATED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 
>> 0,
>> +                  &resp);
>> +    if ( resp.a0 != OPTEE_SMC_RETURN_OK )
>> +    {
>> +        gprintk(XENLOG_WARNING, "Unable to create OPTEE client: rc = 
>> 0x%X\n",
>
> gprintk will print the current vCPU and not the domain created. This
> is not very useful to know the current domain. So it would be better
> to use:
>
> printk(XENLOG_G_WARNING, "%pd: Unable to create OPTEE client: rc ...", d);
>
Good point, thank you.

>> +                (uint32_t)resp.a0);
>> +
>> +        return -ENODEV;
>> +    }
>> +
>> +    d->arch.tee = OPTEE_ENABLED;
>> +
>> +    return 0;
>> +}
>> +
>> +static void forward_call(struct cpu_user_regs *regs)
>> +{
>> +    struct arm_smccc_res resp;
>> +
>> +    arm_smccc_smc(get_user_reg(regs, 0),
>> +                  get_user_reg(regs, 1),
>> +                  get_user_reg(regs, 2),
>> +                  get_user_reg(regs, 3),
>> +                  get_user_reg(regs, 4),
>> +                  get_user_reg(regs, 5),
>> +                  get_user_reg(regs, 6),
>> +                  OPTEE_CLIENT_ID(current->domain),
>> +                  &resp);
>> +
>> +    set_user_reg(regs, 0, resp.a0);
>> +    set_user_reg(regs, 1, resp.a1);
>> +    set_user_reg(regs, 2, resp.a2);
>> +    set_user_reg(regs, 3, resp.a3);
>> +    set_user_reg(regs, 4, 0);
>> +    set_user_reg(regs, 5, 0);
>> +    set_user_reg(regs, 6, 0);
>> +    set_user_reg(regs, 7, 0);
>> +}
>> +
>> +static int optee_relinquish_resources(struct domain *d)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void optee_domain_destroy(struct domain *d)
>> +{
>> +    struct arm_smccc_res resp;
>> +
>> +    if ( !d->arch.tee )
>> +        return;
>> +
>> +    /*
>> +     * Inform OP-TEE that domain is shutting down. This is
>> +     * also a fast SMC call, like OPTEE_SMC_VM_CREATED, so
>> +     * it is also non-preemptible.
>> +     * At this time all domain VCPUs should be stopped. OP-TEE
>> +     * relies on this.
>> +     *
>> +     * a7 should be 0, so we can't skip last 6 parameters od arm_smccc_smc()
>
> NIT: s/od/of/
>
>> +     */
>> +    arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 
>> 0, 0,
>> +                  &resp);
>
> Your split between domain_destroy and relinquish_resources looks
> wrong. If you relinquish resources before telling OP-TEE then you are
> at risk that OP-TEE will use those resources.
>
> Instead you should first tell OP-TEE the domain is shutting down, then
> release the resources.

Both this calls are supposed to be called after all guest's VCPUs are
stopped, so OP-TEE will be not able to use any resources allocated to
the domain, because OP-TEE is schedule by the Normal World. I assume,
this is true for any other TEE.

[...]


-- 
Best regards,Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel