Re: [PATCH v2 04/13] optee: add OP-TEE mediator skeleton

[Volodymyr Babchuk <volodymyr_babchuk@epam.com>]

Hi Julien,

On 03.09.18 20:38, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 03/09/18 17:54, Volodymyr Babchuk wrote:
>> Add very basic OP-TEE mediator. It can probe for OP-TEE presence,
>> tell it about domain creation/destuction and forward all known
> 
> s/destuction/destruction/
> 
>> 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.
> 
> Shall we expect fireworks? :).
I tried couple of time, but with no success :)

> Anyway, I think this is a call for forbidding DomU access until it is 
> supported. This also has the benefits to allow merging Dom0 support for 
> OP-TEE without the rest.
Some time ago you said that I can't be sure that Dom0 is 1:1 mapped, 
because of grant refs. So, actually, any access should be forbidden. I 
can omit

REGISTER_TEE_MEDIATOR(optee, "OP-TEE", &optee_ops);

till the very last patch in the series.

But then it would not compile, because optee_ops is the static struct...

> 
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/arch/arm/tee/Kconfig            |   4 ++
>>   xen/arch/arm/tee/Makefile           |   1 +
>>   xen/arch/arm/tee/optee.c            | 134 
>> ++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/tee/optee_smc.h |  50 ++++++++++++++
>>   4 files changed, 189 insertions(+)
>>   create mode 100644 xen/arch/arm/tee/optee.c
>>
>> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
>> index e69de29..5b829db 100644
>> --- a/xen/arch/arm/tee/Kconfig
>> +++ 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 c54d479..982c879 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 0000000..7bb84d9
>> --- /dev/null
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -0,0 +1,134 @@
>> +/*
>> + * 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>
>> +
>> +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 ( resp.a0 != OPTEE_MSG_UID_0 ||
>> +         resp.a1 != OPTEE_MSG_UID_1 ||
>> +         resp.a2 != OPTEE_MSG_UID_2 ||
>> +         resp.a3 != OPTEE_MSG_UID_3 )
> 
> I would be extra cautious with the sign-extension here. It would be 
> better to follow the spec regarding UUID by casting resp.a0 & co to 32-bit.
> 
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +static int optee_enable(struct domain *d)
>> +{
>> +    struct arm_smccc_res resp;
>> +
>> +    arm_smccc_smc(OPTEE_SMC_VM_CREATED, d->domain_id + 1, 0, 0, 0, 0, 
>> 0, 0,
>> +                  &resp);
>> +    if ( resp.a0 != OPTEE_SMC_RETURN_OK ) {
>> +        gprintk(XENLOG_WARNIREGISTER_TEE_MEDIATOR(optee, "OP-TEE", &optee_ops);NG, "OP-TEE don't want to support domain: 
>> %d\n",
> 
> This message is slightly odd. It would be better to say:
> 
> "Unable to create OP-TEE client: rc=%d\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),
>> +                  /* client id 0 is reserved for hypervisor itself */
>> +                  current->domain->domain_id + 1,
> 
> I would prefer if the client ID is encoded in a macro. So this could be 
> re-used

Something like

  #define OPTEE_CLIENT_ID(domain) (domain->domain_id + 1)

?

> 
>> +                  &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 */
>> +    arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, d->domain_id + 1, 0, 0, 0, 
>> 0, 0, 0,
>> +                  &resp);
> 
> So this SMC should never fail or even get preempted? I was kind of 
> expecting that it may time some time to destroy a domain.

It is the fast SMCCC call, so it can't be preempted. And it is really 
fast, at lest in OP-TEE case. Basically, OP-TEE just removes some 
entries from the list and frees some memory. And yes, it can't fail.

>> +}
>> +
>> +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 26d100e..1c5a247 100644
>> --- a/xen/include/asm-arm/tee/optee_smc.h
>> +++ b/xen/include/asm-arm/tee/optee_smc.h
>> @@ -305,6 +305,56 @@ struct optee_smc_disable_shm_cache_result {
>>       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)
>>    *
>>    * Call register usage:
>>
> 
> Cheers,
> 

-- 
Volodymyr Babchuk

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel