|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 04/11] optee: add OP-TEE mediator skeleton
Hi Julien,
Julien Grall writes:
> Hi Volodymyr,
>
> On 12/18/18 9:11 PM, Volodymyr Babchuk wrote:
>> From: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxx>
>> + * 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |