Re: [PATCH v3 04/11] optee: add OP-TEE mediator skeleton

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Julien,

Julien Grall writes:

> Hi Volodymyr,
>
> On 12/18/18 9:11 PM, Volodymyr Babchuk wrote:
>> From: Volodymyr Babchuk <vlad.babchuk@gmail.com>
>>
>> Add very basic OP-TEE mediator. It can probe for OP-TEE presence,
>> tell it about domain creation/destruction and forward all known
>> calls.
>>
>> This is all what is needed for Dom0 to work with OP-TEE as long
>> as Dom0 shares 1:1 mapped pages with OP-TEE. Any attempt to call
>> OP-TEE from DomU will fail and can lead to spectacular results. Also,
>> problems can arise if Dom0 uses pages mapped from other domains.
>> So, this patch should not be merged without next patches in the series.
>
> The last sentence is not necessary in the commit message. You can add it
> after "---". So it is stripped down when the patch is committed, yet we
> know it should not be merged :).
Oh, okay.

>>
>> This code issues two non-preemptible calls to OP-TEE: to create and
>> to destroy client context. They can't block in OP-TEE, but OP-TEE can
>> wait on a splinlocks, so there is no maximal execution time
>> guaranteed.
>
> This paragraph is worrying. From the discussion we had on the previous
> version and some discussions at Linaro Connect with OP-TEE folks, I
> gathered this call should not take a long time.
Yes, this is right. It **should** not take a long time. And currently I
can't see why it would take a long time anyways. But, you know,
theoretically, there is no upper limit on waiting for spinlock.

> In general spinlock should have no contention or wait should be limited.
> In particular, in the context of OP-TEE I would expect something is
> really wrong if you wait on a spinlock for a really long time as you
> block the normal world. So maybe this is just a misunderstanding of the
> commit message?
Yes, I think it is a misunderstanding. OP-TEE uses spinlocks in the same
way as XEN or kernel - for short atomic operations. But as spinlock()
function have no timeout argument, theoretically it would block for a
long time.

I think, I'll remove that clause from the commit message. Just for
clarity :)

>
>>
>> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
>> ---
>>
>>   Changes from v2:
>>    - Fixed coding style
>>    - Introduced tee/Kconfig
>>    - Fixed error messages
>>
>>   xen/arch/arm/Kconfig                |   2 +
>>   xen/arch/arm/tee/Kconfig            |   4 +
>>   xen/arch/arm/tee/Makefile           |   1 +
>>   xen/arch/arm/tee/optee.c            | 151 ++++++++++++++++++++++++++++
>>   xen/include/asm-arm/tee/optee_smc.h |  50 +++++++++
>>   5 files changed, 208 insertions(+)
>>   create mode 100644 xen/arch/arm/tee/Kconfig
>>   create mode 100644 xen/arch/arm/tee/optee.c
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index e527b2f885..99e6f0ebb2 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -237,3 +237,5 @@ source "arch/arm/platforms/Kconfig"
>>   source "common/Kconfig"
>>
>>   source "drivers/Kconfig"
>> +
>> +source "arch/arm/tee/Kconfig"
>> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
>> new file mode 100644
>> index 0000000000..5b829db2e9
>> --- /dev/null
>> +++ b/xen/arch/arm/tee/Kconfig
>> @@ -0,0 +1,4 @@
>> +config OPTEE
>> +	bool "Enable OP-TEE mediator"
>> +	default n
>> +	depends on TEE
>> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
>> index c54d4796ff..982c879684 100644
>> --- a/xen/arch/arm/tee/Makefile
>> +++ b/xen/arch/arm/tee/Makefile
>> @@ -1 +1,2 @@
>>   obj-y += tee.o
>> +obj-$(CONFIG_OPTEE) += optee.o
>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>> new file mode 100644
>> index 0000000000..73ad25ee0b
>> --- /dev/null
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -0,0 +1,151 @@
>> +/*
>> + * xen/arch/arm/tee/optee.c
>> + *
>> + * OP-TEE mediator
>> + *
>> + * Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> + * Copyright (c) 2018 EPAM Systems.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#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>
>> +
>> +/* Client ID 0 is reserved for hypervisor itself */
>> +#define OPTEE_CLIENT_ID(domain) (domain->domain_id + 1)
>
> The second 'domain' should be between () to avoid macro-expansion issue.
>
>> +
>> +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_enable(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.
>> +     * Also, interrupts are disabled.
>> +     * Right now OP-TEE just frees allocated memory, so it should be
>> +     * really fast.
>
> The last sentence looks wrong in this context.
>
>> +     */
>> +    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",
>> +                (uint32_t)resp.a0);
>> +        return -ENODEV;
>> +    }
>> +
>> +    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 void optee_domain_destroy(struct domain *d)
>> +{
>> +    struct arm_smccc_res resp;
>> +
>> +    /* At this time all domain VCPUs should be stopped */
>> +
>> +    /*
>> +     * 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.
>> +     */
>> +    arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
>> +                  &resp);
>> +}
>> +
>> +static bool optee_handle_call(struct cpu_user_regs *regs)
>> +{
>> +    switch ( get_user_reg(regs, 0) )
>> +    {
>> +    case OPTEE_SMC_CALLS_COUNT:
>> +    case OPTEE_SMC_CALLS_UID:
>> +    case OPTEE_SMC_CALLS_REVISION:
>> +    case OPTEE_SMC_CALL_GET_OS_UUID:
>> +    case OPTEE_SMC_FUNCID_GET_OS_REVISION:
>> +    case OPTEE_SMC_ENABLE_SHM_CACHE:
>> +    case OPTEE_SMC_DISABLE_SHM_CACHE:
>> +    case OPTEE_SMC_GET_SHM_CONFIG:
>> +    case OPTEE_SMC_EXCHANGE_CAPABILITIES:
>> +    case OPTEE_SMC_CALL_WITH_ARG:
>> +    case OPTEE_SMC_CALL_RETURN_FROM_RPC:
>> +        forward_call(regs);
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
>> +}
>> +
>> +static const struct tee_mediator_ops optee_ops =
>> +{
>> +    .probe = optee_probe,
>> +    .enable = optee_enable,
>> +    .domain_destroy = optee_domain_destroy,
>> +    .handle_call = optee_handle_call,
>> +};
>> +
>> +REGISTER_TEE_MEDIATOR(optee, "OP-TEE", &optee_ops);
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/asm-arm/tee/optee_smc.h b/xen/include/asm-arm/tee/optee_smc.h
>> index 26d100e215..1c5a2479e9 100644
>> --- a/xen/include/asm-arm/tee/optee_smc.h
>> +++ b/xen/include/asm-arm/tee/optee_smc.h
>> @@ -304,6 +304,56 @@ struct optee_smc_disable_shm_cache_result {
>>   #define OPTEE_SMC_ENABLE_SHM_CACHE \
>>   	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_SHM_CACHE)
>>
>> +/*
>> + * Inform OP-TEE about a new virtual machine
>> + *
>> + * Hypervisor issues this call during virtual machine (guest) creation.
>> + * OP-TEE records VM_ID of new virtual machine and makes self ready
>> + * to receive requests from it.
>> + *
>> + * Call requests usage:
>> + * a0	SMC Function ID, OPTEE_SMC_VM_CREATED
>> + * a1	VM_ID of newly created virtual machine
>> + * a2-6 Not used
>> + * a7	Hypervisor Client ID register. Must be 0, because only hypervisor
>> + *      can issue this call
>> + *
>> + * Normal return register usage:
>> + * a0	OPTEE_SMC_RETURN_OK
>> + * a1-7	Preserved
>> + *
>> + * Error return:
>> + * a0	OPTEE_SMC_RETURN_ENOTAVAIL	OP-TEE has no resources for
>> + *					another VM
>> + * a1-7	Preserved
>> + *
>> + */
>> +#define OPTEE_SMC_FUNCID_VM_CREATED	13
>> +#define OPTEE_SMC_VM_CREATED \
>> +	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_VM_CREATED)
>> +
>> +/*
>> + * Inform OP-TEE about shutdown of a virtual machine
>> + *
>> + * Hypervisor issues this call during virtual machine (guest) destruction.
>> + * OP-TEE will clean up all resources associated with this VM.
>> + *
>> + * Call requests usage:
>> + * a0	SMC Function ID, OPTEE_SMC_VM_DESTROYED
>> + * a1	VM_ID of virtual machine being shutted down
>> + * a2-6 Not used
>> + * a7	Hypervisor Client ID register. Must be 0, because only hypervisor
>> + *      can issue this call
>> + *
>> + * Normal return register usage:
>> + * a0	OPTEE_SMC_RETURN_OK
>> + * a1-7	Preserved
>> + *
>> + */
>> +#define OPTEE_SMC_FUNCID_VM_DESTROYED	14
>> +#define OPTEE_SMC_VM_DESTROYED \
>> +	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_VM_DESTROYED)
>> +
>>   /*
>>    * Resume from RPC (for example after processing a foreign interrupt)
>>    *
>>
>
> Cheers,


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