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? :).

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.

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_WARNING, "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.

•              &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.

+}

+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,

Hi,

On 09/03/2018 05:54 PM, Volodymyr Babchuk wrote:

OP-TEE meditor needs to store per-domain context, as will be seen

s/meditor/mediator/

in the next patches. At this moment it stores only reference to domain.

This allows us to filter out calls from domains that are not allowed to work with OP-TEE.

Signed-off-by: Volodymyr Babchuk volodymyr_babchuk@epam.com

xen/arch/arm/tee/optee.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 48bff5d..c895a99 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -19,6 +19,14 @@ #include <asm/tee/optee_msg.h> #include <asm/tee/optee_smc.h> +struct domain_ctx {

I would prefer if the structure has optee in its name such as optee_domain.

We also tend to abbreviate context in Xen using the ctxt.

• struct list_head list;
• struct domain *domain;

+};

+static LIST_HEAD(domain_ctx_list); +static DEFINE_SPINLOCK(domain_ctx_list_lock);

• static bool optee_probe(void) { struct dt_device_node *node;

@@ -41,18 +49,49 @@ static bool optee_probe(void) return true; } +static struct domain_ctx *find_domain_ctx(struct domain* d) +{

• struct domain_ctx *ctx;
• spin_lock(&domain_ctx_list_lock);
• list_for_each_entry( ctx, &domain_ctx_list, list )
• {

•    if ( ctx->domain == d )
  

•    {
  

•            spin_unlock(&domain_ctx_list_lock);
  

•            return ctx;
  

•    }
  

• }
• spin_unlock(&domain_ctx_list_lock);
• return NULL;

+}

Why do you need to store the context in a list? As you have a context per domain it would be better to store that directly in struct domain by introduce a field "void * tee".

With that, most of the code in this patch will become unnecessary. So you potentially can fold it into something else.

Cheers,

Hi,

On 05.09.18 17:10, Julien Grall wrote:

Hi,

On 09/03/2018 05:54 PM, Volodymyr Babchuk wrote:

...

OP-TEE meditor needs to store per-domain context, as will be seen

s/meditor/mediator/

...

in the next patches. At this moment it stores only reference to domain.

This allows us to filter out calls from domains that are not allowed to work with OP-TEE.

Signed-off-by: Volodymyr Babchuk volodymyr_babchuk@epam.com

xen/arch/arm/tee/optee.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++   1 file changed, 66 insertions(+)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 48bff5d..c895a99 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -19,6 +19,14 @@   #include <asm/tee/optee_msg.h>   #include <asm/tee/optee_smc.h> +struct domain_ctx {

I would prefer if the structure has optee in its name such as optee_domain.

It is supposed to be internal for this file, so I chose this name. But I can rename it, no problem.

We also tend to abbreviate context in Xen using the ctxt.

...

+    struct list_head list; +    struct domain *domain; +};

+static LIST_HEAD(domain_ctx_list); +static DEFINE_SPINLOCK(domain_ctx_list_lock);

static bool optee_probe(void)   {       struct dt_device_node *node; @@ -41,18 +49,49 @@ static bool optee_probe(void)       return true;   } +static struct domain_ctx *find_domain_ctx(struct domain* d) +{ +    struct domain_ctx *ctx;

+    spin_lock(&domain_ctx_list_lock);

+    list_for_each_entry( ctx, &domain_ctx_list, list ) +    { +        if ( ctx->domain == d ) +        { +                spin_unlock(&domain_ctx_list_lock); +                return ctx; +        } +    }

+    spin_unlock(&domain_ctx_list_lock); +    return NULL; +}

Why do you need to store the context in a list? As you have a context per domain it would be better to store that directly in struct domain by introduce a field "void * tee".

Yes, this was my first intention. But I was unsure on if it is appropriate to add driver-specific fields to struct arch_domain. If you sat that it is okay (under #ifdef CONFIG_TEE), then I'll do in this way.

Hi Julien,

On 05.09.18 18:17, Julien Grall wrote:

Hi,

On 09/03/2018 05:54 PM, Volodymyr Babchuk wrote:

...

Main way to communicate with OP-TEE is to issue standard SMCCC

NIT: The main way

...

call. "Standard" is a SMCCC term and it means that call can be interrupted and OP-TEE can return control to NW before completing the call.

In contranst with fast calls, where arguments and return values

NIT: s/contranst/contrast/

...

are passed in registers, standard calls use shared memory. Register pair r1,r2 holds 64-bit PA of command buffer, where all arguments

Do you mean w1, w2?

Good question. How to call the registers, so it would be valid both for ARMv7 and ARMv8?

...

are stored and which is used to return data. OP-TEE internally copies contents of this buffer into own secure memory before accessing and validating any data in command buffer. This is done to make sure that NW will not change contents of the validated parameters.

Mediator needs to do the same for number of reasons:

1. To make sure that guest will not change data after validation.
2. To translate IPAs to PAs in the command buffer (this is not done

in this patch). 3. To hide translated address from guest, so it will not be able     to do IPA->PA translation by misusing mediator.

Also mediator pins the page with original command buffer because it will write to it later, when returning response from OP-TEE.

I don't think it is necessary to pin the guest command buffer. You can use guestcopy helper to copy to/from the guest memory.

If the guest modify the P2M at the same time, then it is not our business if something wrong happen. The only things we need to prevent here is writing back to an MFN that does not belong to the domain.

You are right. I did this in the same way as OP-TEE does, except that OP-TEE does not copies whole command buffer. So yes, your approach is better. Thank you.

...

During standard call OP-TEE can issue multiple "RPC returns", asking NW to do some work for OP-TEE. NW then issues special call OPTEE_SMC_CALL_RETURN_FROM_RPC to resume handling of the original call. Thus, mediator needs to maintain context for original standard call during multiple SMCCC calls.

Standard call is considered complete, when returned value is not RPC request.

Signed-off-by: Volodymyr Babchuk volodymyr_babchuk@epam.com

xen/arch/arm/tee/optee.c | 319 ++++++++++++++++++++++++++++++++++++++++++++++-   1 file changed, 316 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index c895a99..1008eba 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -12,6 +12,7 @@    */   #include <xen/device_tree.h> +#include <xen/domain_page.h>   #include <xen/sched.h>   #include <asm/smccc.h>   #include <asm/tee/tee.h> @@ -19,9 +20,27 @@   #include <asm/tee/optee_msg.h>   #include <asm/tee/optee_smc.h> +#define MAX_STD_CALLS   16

I suspect this is used to restrict the number of calls in flight. If so, I would appreciate if this is documented in the code and the limitations explained in the commit message.

Yes, you are right. I'll add description.

...

+/*

• • Call context. OP-TEE can issue multiple RPC returns during one call.
• • We need to preserve context during them.
• */

+struct std_call_ctx { +    struct list_head list; +    struct optee_msg_arg *guest_arg; +    struct optee_msg_arg *xen_arg; +    mfn_t guest_arg_mfn; +    int optee_thread_id; +    int rpc_op; +};

struct domain_ctx {       struct list_head list; +    struct list_head call_ctx_list;       struct domain *domain; +    atomic_t call_ctx_count; +    spinlock_t lock;   };   static LIST_HEAD(domain_ctx_list); @@ -49,6 +68,44 @@ static bool optee_probe(void)       return true;   } +static mfn_t lookup_and_pin_guest_ram_addr(paddr_t gaddr, +                                            struct page_info **pg)

I don't think there are need to return both an MFN and a page. You can deduce easily from the other. In this context, it would be better to return a page.

Also, I would prefer if this function take a guest frame address over a guest physical address.

Lastly this function is basically a re-implementation of get_page_from_gfn with a restriction on the p2m type. Please re-implement it using that function.

...

+{ +    mfn_t mfn; +    gfn_t gfn; +    p2m_type_t t; +    struct page_info *page; +    struct domain *d = current->domain;

+    gfn = gaddr_to_gfn(gaddr); +    mfn = p2m_lookup(d, gfn, &t);

+    if ( t != p2m_ram_rw || mfn_eq(mfn, INVALID_MFN) ) +        return INVALID_MFN;

+    page = mfn_to_page(mfn);

mfn_to_page can never failed. If you want to check whether an MFN is valid, then you can use mfn_valid(...).

...

+    if ( !page ) +        return INVALID_MFN;

+    if ( !get_page(page, d) ) +        return INVALID_MFN;

+    if ( pg ) +        *pg = page;

+    return mfn; +}

+static void unpin_guest_ram_addr(mfn_t mfn) +{ +    struct page_info *page; +    page = mfn_to_page(mfn); +    if ( !page ) +        return;

+    put_page(page); +}

static struct domain_ctx *find_domain_ctx(struct domain* d)   {       struct domain_ctx *ctx; @@ -87,6 +144,10 @@ static int optee_enable(struct domain *d)       }       ctx->domain = d; +    INIT_LIST_HEAD(&ctx->call_ctx_list);

+    atomic_set(&ctx->call_ctx_count, 0); +    spin_lock_init(&ctx->lock);       spin_lock(&domain_ctx_list_lock);       list_add_tail(&ctx->list, &domain_ctx_list); @@ -134,11 +195,72 @@ static void set_return(struct cpu_user_regs *regs, uint32_t ret)       set_user_reg(regs, 7, 0);   } +static struct std_call_ctx *allocate_std_call_ctx(struct domain_ctx *ctx) +{ +    struct std_call_ctx *call; +    int count;

+    count = atomic_add_unless(&ctx->call_ctx_count, 1, MAX_STD_CALLS);

Please a comment explaining the rationale for this.

E.g. "/* Limit number of calls for the current domain */" ?

...

+    if ( count == MAX_STD_CALLS ) +        return NULL;

+    call = xzalloc(struct std_call_ctx); +    if ( !call ) {

Coding style:

if {

...

+        atomic_dec(&ctx->call_ctx_count); +        return NULL; +    }

+    call->optee_thread_id = -1;

+    spin_lock(&ctx->lock); +    list_add_tail(&call->list, &ctx->call_ctx_list); +    spin_unlock(&ctx->lock);

+    return call; +}

+static void free_std_call_ctx(struct domain_ctx *ctx, struct std_call_ctx *call) +{ +    atomic_dec(&ctx->call_ctx_count);

+    spin_lock(&ctx->lock); +    list_del(&call->list); +    spin_unlock(&ctx->lock);

+    if ( call->xen_arg ) +        free_xenheap_page(call->xen_arg);

free_xenheap_page is able to cope with NULL pointer.

...

+    if ( call->guest_arg ) { +        unmap_domain_page_global(call->guest_arg); +        unpin_guest_ram_addr(call->guest_arg_mfn); +    }

+    xfree(call); +}

+static struct std_call_ctx *find_call_ctx(struct domain_ctx *ctx, int thread_id) +{ +    struct std_call_ctx *call;

+    spin_lock(&ctx->lock); +    list_for_each_entry( call, &ctx->call_ctx_list, list ) +    { +        if ( call->optee_thread_id == thread_id ) +        { +                spin_unlock(&ctx->lock); +                return call; +        } +    } +    spin_unlock(&ctx->lock);

+    return NULL; +}   static void optee_domain_destroy(struct domain *d)   {       struct arm_smccc_res resp;       struct domain_ctx *ctx; +    struct std_call_ctx *call, *call_tmp;       bool found = false;       /* At this time all domain VCPUs should be stopped */ @@ -163,9 +285,201 @@ static void optee_domain_destroy(struct domain *d)       if ( !found )           return; +    ASSERT(!spin_is_locked(&ctx->lock));

+    list_for_each_entry_safe( call, call_tmp, &ctx->call_ctx_list, list ) +        free_std_call_ctx(ctx, call);

+    ASSERT(!atomic_read(&ctx->call_ctx_count));

xfree(ctx);   } +/*

• • Copy command buffer into xen memory to:
• • 1. Hide translated addresses from guest
• • 2. Make sure that guest wouldn't change data in command buffer

during call

• */

+static bool copy_std_request(struct cpu_user_regs *regs, +                             struct std_call_ctx *call) +{ +    paddr_t cmd_gaddr, xen_addr;

+    cmd_gaddr = (paddr_t)get_user_reg(regs, 1) << 32 | +        get_user_reg(regs, 2);

+    /* +     * Command buffer should start at page boundary. +     * This is OP-TEE ABI requirement. +     */ +    if ( cmd_gaddr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1) ) +        return false;

+    call->guest_arg_mfn = lookup_and_pin_guest_ram_addr(cmd_gaddr, NULL); +    if ( mfn_eq(call->guest_arg_mfn, INVALID_MFN) ) +        return false;

+    call->guest_arg = map_domain_page_global(call->guest_arg_mfn); +    if ( !call->guest_arg ) { +        unpin_guest_ram_addr(call->guest_arg_mfn); +        return false; +    }

It look like to me you want to use access_guest_memory_by_ipa here. This would avoid the page to be mapped global for the duration of the call.

Yes, as you said earlier, I can try to copy whole contents of command buffer to shadowed region and then handle it thjere.

...

+    call->xen_arg = alloc_xenheap_page();

I don't see anything in the code making sure that OPTEE_MSG_NONCONTIG_PAGE_SIZE fits in a Xen page-size. You probably want to add a BUILD_BUG_ON(OPTEE_MSG_NONCONTIG_PAGE > PAGE_SIZE).

...

Also, for been more future-proof (i.e Xen using an higher page size), it

would be better to use _xmalloc(OPTEE_MSG_NONCONTIG_PAGE_SIZE, OPTEE_MSG_NONCONTIG_PAGE_SIZE).

_xmalloc will alloc_xenheap_page() if it is necessary.

Thank you for suggestion. It is good to

...

+    if ( !call->xen_arg ) { +        unpin_guest_ram_addr(call->guest_arg_mfn); +        return false; +    }

+    memcpy(call->xen_arg, call->guest_arg, OPTEE_MSG_NONCONTIG_PAGE_SIZE);

+    xen_addr = virt_to_maddr(call->xen_arg);

+    set_user_reg(regs, 1, xen_addr >> 32); +    set_user_reg(regs, 2, xen_addr & 0xFFFFFFFF);

+    return true; +}

+static bool copy_std_request_back(struct domain_ctx *ctx, +                                  struct cpu_user_regs *regs, +                                  struct std_call_ctx *call) +{ +    unsigned int i; +    uint32_t attr;

This function might be a bit tricky to implement using access_guest_memory_by_ipa. However, you can just map the region temporarily. This would avoid to use map_domain_page_global() within then code and avoid one less potentially error in the cleanup path.

I think, I can make all needed manipulations in the shadowed buffer and then copy it as a whole to the guest's one.

...

+    call->guest_arg->ret = call->xen_arg->ret; +    call->guest_arg->ret_origin = call->xen_arg->ret_origin; +    call->guest_arg->session = call->xen_arg->session; +    for ( i = 0; i < call->xen_arg->num_params; i++ ) {

for ( ... ) {

...

+        attr = call->xen_arg->params[i].attr;

+        switch ( attr & OPTEE_MSG_ATTR_TYPE_MASK ) {

switch ( ... ) {

...

+        case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: +        case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: +            call->guest_arg->params[i].u.tmem.size = +                call->xen_arg->params[i].u.tmem.size; +            continue; +        case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: +        case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: +            call->guest_arg->params[i].u.value.a = +                call->xen_arg->params[i].u.value.a; +            call->guest_arg->params[i].u.value.b = +                call->xen_arg->params[i].u.value.b; +            continue; +        case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: +        case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: +            call->guest_arg->params[i].u.rmem.size = +                call->xen_arg->params[i].u.rmem.size; +            continue; +        case OPTEE_MSG_ATTR_TYPE_NONE: +        case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: +        case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT: +            continue; +        } +    }

+    return true; +}

+static bool execute_std_call(struct domain_ctx *ctx, +                             struct cpu_user_regs *regs, +                             struct std_call_ctx *call) +{ +    register_t optee_ret;

+    forward_call(regs);

I find a bit odd that you introduce a return for forward_call in the previous patch. But you barely use it.

You see, division of original big patch into smaller ones is a bit arbitrary, so you can spot artifacts like this. I'll try to fix them.

I think I would prefer if the function does not return a value and you just rely on get_user_reg(regs, 0).

...

+    optee_ret = get_user_reg(regs, 0); +    if ( OPTEE_SMC_RETURN_IS_RPC(optee_ret) ) +    { +        call->optee_thread_id = get_user_reg(regs, 3); +        call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(optee_ret); +        return true; +    }

+    copy_std_request_back(ctx, regs, call);

Here another function return a value but it is not used. If you don't plan to use the return, then please drop. If you plan to use it, then do it everywhere.

The same thing. It is used later in the series, when there will be additional logic.

...

+    free_std_call_ctx(ctx, call);

+    return true; +}

+static bool handle_std_call(struct domain_ctx *ctx, struct cpu_user_regs *regs) +{ +    struct std_call_ctx *call; +    bool ret;

+    call = allocate_std_call_ctx(ctx);

+    if (!call) +        return false;

+    ret = copy_std_request(regs, call); +    if ( !ret ) +        goto out;

+    /* Now we can safely examine contents of command buffer */ +    if ( OPTEE_MSG_GET_ARG_SIZE(call->xen_arg->num_params) > +         OPTEE_MSG_NONCONTIG_PAGE_SIZE ) {

if ( ... ) {

...

+        ret = false; +        goto out; +    }

+    switch ( call->xen_arg->cmd ) +    { +    case OPTEE_MSG_CMD_OPEN_SESSION: +    case OPTEE_MSG_CMD_CLOSE_SESSION: +    case OPTEE_MSG_CMD_INVOKE_COMMAND: +    case OPTEE_MSG_CMD_CANCEL: +    case OPTEE_MSG_CMD_REGISTER_SHM: +    case OPTEE_MSG_CMD_UNREGISTER_SHM: +        ret = true; +        break; +    default: +        ret = false; +    }

+    if (!ret) +        goto out;

+    ret = execute_std_call(ctx, regs, call);

+out: +    if (!ret) +        free_std_call_ctx(ctx, call);

+    return ret; +}

+static bool handle_rpc(struct domain_ctx *ctx, struct cpu_user_regs *regs) +{ +    struct std_call_ctx *call;

No need for the newline here.

...

+    int optee_thread_id = get_user_reg(regs, 3);

+    call = find_call_ctx(ctx, optee_thread_id);

+    if ( !call ) +        return false;

+    switch ( call->rpc_op ) {

switch ( ... ) {

...

+    case OPTEE_SMC_RPC_FUNC_ALLOC: +        /* TODO: Add handling */ +        break; +    case OPTEE_SMC_RPC_FUNC_FREE: +        /* TODO: Add handling */ +        break; +    case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR: +        break; +    case OPTEE_SMC_RPC_FUNC_CMD: +        /* TODO: Add handling */ +        break; +    }

+    return execute_std_call(ctx, regs, call); +}

static bool handle_exchange_capabilities(struct cpu_user_regs *regs)   {       uint32_t caps; @@ -225,10 +539,9 @@ static bool optee_handle_call(struct cpu_user_regs *regs)       case OPTEE_SMC_EXCHANGE_CAPABILITIES:           return handle_exchange_capabilities(regs);       case OPTEE_SMC_CALL_WITH_ARG: +        return handle_std_call(ctx, regs);       case OPTEE_SMC_CALL_RETURN_FROM_RPC: -        /* TODO: Add proper handling for this calls */ -        forward_call(regs); -        return true; +        return handle_rpc(ctx, regs);       default:           return false;       }

Cheers,

Hi Julien,

On 10.09.18 16:01, Julien Grall wrote:

Hi Volodymyr,

On 03/09/18 17:54, Volodymyr Babchuk wrote:

...

OP-TEE usually uses the same idea with command buffers (see previous commit) to issue RPC requests. Problem is that initially it has no buffer, where it can write request. So the first RPC request it makes is special: it requests NW to allocate shared buffer for other RPC requests. Usually this buffer is allocated only once for every OP-TEE thread and it remains allocated all the time until shutdown.

Mediator needs to pin this buffer(s) to make sure that domain can't transfer it to someone else. Also it should be mapped into XEN address space, because mediator needs to check responses from guests.

Can you explain why you always need to keep the shared buffer mapped in Xen? Why not using access_guest_memory_by_ipa every time you want to get information from the guest?

Sorry, I just didn't know about this mechanism. But for performance reasons, I'd like to keep this buffers always mapped. You see, RPC returns are very frequent (for every IRQ, actually). So I think, it will be costly to map/unmap this buffer every time.

...

Signed-off-by: Volodymyr Babchuk volodymyr_babchuk@epam.com

xen/arch/arm/tee/optee.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-   1 file changed, 119 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 1008eba..6d6b51d 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -21,6 +21,7 @@   #include <asm/tee/optee_smc.h>   #define MAX_STD_CALLS   16 +#define MAX_RPC_SHMS    16   /*    * Call context. OP-TEE can issue multiple RPC returns during one call. @@ -35,11 +36,22 @@ struct std_call_ctx {       int rpc_op;   }; +/* Pre-allocated SHM buffer for RPC commands */ +struct shm_rpc { +    struct list_head list; +    struct optee_msg_arg *guest_arg; +    struct page *guest_page; +    mfn_t guest_mfn; +    uint64_t cookie; +};

struct domain_ctx {       struct list_head list;       struct list_head call_ctx_list; +    struct list_head shm_rpc_list;       struct domain *domain;       atomic_t call_ctx_count; +    atomic_t shm_rpc_count;       spinlock_t lock;   }; @@ -145,8 +157,10 @@ static int optee_enable(struct domain *d)       ctx->domain = d;       INIT_LIST_HEAD(&ctx->call_ctx_list); +    INIT_LIST_HEAD(&ctx->shm_rpc_list);       atomic_set(&ctx->call_ctx_count, 0); +    atomic_set(&ctx->shm_rpc_count, 0);       spin_lock_init(&ctx->lock);       spin_lock(&domain_ctx_list_lock); @@ -256,11 +270,81 @@ static struct std_call_ctx *find_call_ctx(struct domain_ctx *ctx, int thread_id)       return NULL;   } +static struct shm_rpc *allocate_and_map_shm_rpc(struct domain_ctx *ctx, paddr_t gaddr,

I would prefer if you pass a gfn instead of the address here.

...

+                                        uint64_t cookie)

NIT: Indentation

...

+{ +    struct shm_rpc *shm_rpc; +    int count;

+    count = atomic_add_unless(&ctx->shm_rpc_count, 1, MAX_RPC_SHMS); +    if ( count == MAX_RPC_SHMS ) +        return NULL;

+    shm_rpc = xzalloc(struct shm_rpc); +    if ( !shm_rpc ) +        goto err;

+    shm_rpc->guest_mfn = lookup_and_pin_guest_ram_addr(gaddr, NULL);

+    if ( mfn_eq(shm_rpc->guest_mfn, INVALID_MFN) ) +        goto err;

+    shm_rpc->guest_arg = map_domain_page_global(shm_rpc->guest_mfn); +    if ( !shm_rpc->guest_arg ) +    { +        gprintk(XENLOG_INFO, "Could not map domain page\n");

You don't unpin the guest page if Xen can't map the page.

...

+        goto err; +    } +    shm_rpc->cookie = cookie;

+    spin_lock(&ctx->lock); +    list_add_tail(&shm_rpc->list, &ctx->shm_rpc_list); +    spin_unlock(&ctx->lock);

+    return shm_rpc;

+err: +    atomic_dec(&ctx->shm_rpc_count); +    xfree(shm_rpc); +    return NULL; +}

+static void free_shm_rpc(struct domain_ctx *ctx, uint64_t cookie) +{ +    struct shm_rpc *shm_rpc; +    bool found = false;

+    spin_lock(&ctx->lock);

+    list_for_each_entry( shm_rpc, &ctx->shm_rpc_list, list ) +    { +        if ( shm_rpc->cookie == cookie )

What does guarantee you the cookie will be uniq?

Normal World guarantees. This is the part of the protocol.

...

+        { +            found = true; +            list_del(&shm_rpc->list); +            break; +        } +    } +    spin_unlock(&ctx->lock);

At this point you have a shm_rpc in hand to free. But what does guarantee you no-one will use it?

This is valid point. I'll revisit this part of the code, thank you. Looks like I need some refcount there.

...

+    if ( !found ) { +        return; +    }

No need for the {} in a one-liner.

...

+    if ( shm_rpc->guest_arg ) {

Coding style:

if ( ... ) {

...

+        unpin_guest_ram_addr(shm_rpc->guest_mfn); +        unmap_domain_page_global(shm_rpc->guest_arg); +    }

+    xfree(shm_rpc); +}

static void optee_domain_destroy(struct domain *d)   {       struct arm_smccc_res resp;       struct domain_ctx *ctx;       struct std_call_ctx *call, *call_tmp; +    struct shm_rpc *shm_rpc, *shm_rpc_tmp;       bool found = false;       /* At this time all domain VCPUs should be stopped */ @@ -290,7 +374,11 @@ static void optee_domain_destroy(struct domain *d)       list_for_each_entry_safe( call, call_tmp, &ctx->call_ctx_list, list )           free_std_call_ctx(ctx, call); +    list_for_each_entry_safe( shm_rpc, shm_rpc_tmp, &ctx->shm_rpc_list, list ) +        free_shm_rpc(ctx, shm_rpc->cookie);

ASSERT(!atomic_read(&ctx->call_ctx_count)); +    ASSERT(!atomic_read(&ctx->shm_rpc_count));       xfree(ctx);   } @@ -452,6 +540,32 @@ out:       return ret;   } +static void handle_rpc_func_alloc(struct domain_ctx *ctx, +                                  struct cpu_user_regs *regs) +{ +    paddr_t ptr = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2);

+    if ( ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1) ) +        gprintk(XENLOG_WARNING, "Domain returned invalid RPC command buffer\n");

+    if ( ptr ) { +        uint64_t cookie = get_user_reg(regs, 4) << 32 | get_user_reg(regs, 5); +        struct shm_rpc *shm_rpc;

+        shm_rpc = allocate_and_map_shm_rpc(ctx, ptr, cookie); +        if ( !shm_rpc ) +        { +            gprintk(XENLOG_WARNING, "Failed to allocate shm_rpc object\n"); +            ptr = 0; +        } +        else +            ptr = mfn_to_maddr(shm_rpc->guest_mfn);

+        set_user_reg(regs, 1, ptr >> 32); +        set_user_reg(regs, 2, ptr & 0xFFFFFFFF); +    } +}

static bool handle_rpc(struct domain_ctx *ctx, struct cpu_user_regs *regs)   {       struct std_call_ctx *call; @@ -465,11 +579,14 @@ static bool handle_rpc(struct domain_ctx *ctx, struct cpu_user_regs *regs)       switch ( call->rpc_op ) {       case OPTEE_SMC_RPC_FUNC_ALLOC: -        /* TODO: Add handling */ +        handle_rpc_func_alloc(ctx, regs);           break;       case OPTEE_SMC_RPC_FUNC_FREE: -        /* TODO: Add handling */ +    { +        uint64_t cookie = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2);

Newline here.

...

+        free_shm_rpc(ctx, cookie);

It feels quite suspicious to free the memory in Xen before calling OP-TEE. I think this need to be done afterwards.

No, it is OP-TEE asked to free buffer. This function is called, when NW returns from the RPC. So at this moment NW freed the buffer.

...

break; +    }       case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:           break;       case OPTEE_SMC_RPC_FUNC_CMD:

Cheers,

Hi,

On 10.09.18 17:02, Julien Grall wrote:

Hi,

On 03/09/18 17:54, Volodymyr Babchuk wrote:

...

Shared memory is widely used by NW to communicate with TAs in OP-TEE. NW can share part of own memory with TA or OP-TEE core, by registering it OP-TEE, or by providing a temporal refernce. Anyways, information about such memory buffers are sent to OP-TEE as a list of pages. This mechanism is descripted optee_msg.h.

Mediator should step in when NW tries to share memory with OP-TEE for two reasons:

1. Do address translation from IPA to PA.
2. Pin domain pages till they are mapped into OP-TEE or TA

address space, so domain can't transfer this pages to     other domain or baloon out them.

s/baloon/balloon/

...

Address translation is done by translate_noncontig(...) function. It allocates new buffer from xenheap and then walks on guest provided list of pages, translates addresses and stores PAs into newly allocated buffer. This buffer will be provided to OP-TEE instead of original buffer from the guest. This buffer will be free at the end of sdandard call.

In the same time this function pins pages and stores them in struct shm_buf object. This object will live all the time, when given SHM buffer is known to OP-TEE. It will be freed after guest unregisters shared buffer. At this time pages will be unpinned.

Signed-off-by: Volodymyr Babchuk volodymyr_babchuk@epam.com

xen/arch/arm/tee/optee.c | 245 ++++++++++++++++++++++++++++++++++++++++++++++-   1 file changed, 244 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 6d6b51d..8bfcfdc 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -22,6 +22,8 @@   #define MAX_STD_CALLS   16   #define MAX_RPC_SHMS    16 +#define MAX_TOTAL_SMH_BUF_PG    16384

So that's 64MB worth of guest memory. Do we expect them to be mapped in Xen? Or just pinned?

Just pinned. We are not interested in contents of this memory.

...

+#define MAX_NONCONTIG_ENTRIES   5   /*    * Call context. OP-TEE can issue multiple RPC returns during one call. @@ -31,6 +33,9 @@ struct std_call_ctx {       struct list_head list;       struct optee_msg_arg *guest_arg;       struct optee_msg_arg *xen_arg; +    /* Buffer for translated page addresses, shared with OP-TEE */ +    void *non_contig[MAX_NONCONTIG_ENTRIES]; +    int non_contig_order[MAX_NONCONTIG_ENTRIES];

Can you please introduce a structure with the order and mapping?

...

mfn_t guest_arg_mfn;       int optee_thread_id;       int rpc_op; @@ -45,13 +50,24 @@ struct shm_rpc {       uint64_t cookie;   }; +/* Shared memory buffer for arbitrary data */ +struct shm_buf { +    struct list_head list; +    uint64_t cookie; +    int max_page_cnt; +    int page_cnt;

AFAICT, max_page_cnt and page_cnt should never but negative. If so, then they should be unsigned.

...

+    struct page_info *pages[]; +};

struct domain_ctx {       struct list_head list;       struct list_head call_ctx_list;       struct list_head shm_rpc_list; +    struct list_head shm_buf_list;       struct domain *domain;       atomic_t call_ctx_count;       atomic_t shm_rpc_count; +    atomic_t shm_buf_pages;       spinlock_t lock;   }; @@ -158,9 +174,12 @@ static int optee_enable(struct domain *d)       ctx->domain = d;       INIT_LIST_HEAD(&ctx->call_ctx_list);       INIT_LIST_HEAD(&ctx->shm_rpc_list); +    INIT_LIST_HEAD(&ctx->shm_buf_list);       atomic_set(&ctx->call_ctx_count, 0);       atomic_set(&ctx->shm_rpc_count, 0); +    atomic_set(&ctx->shm_buf_pages, 0);

spin_lock_init(&ctx->lock);       spin_lock(&domain_ctx_list_lock); @@ -339,12 +358,76 @@ static void free_shm_rpc(struct domain_ctx *ctx, uint64_t cookie)       xfree(shm_rpc);   } +static struct shm_buf *allocate_shm_buf(struct domain_ctx *ctx, +                                        uint64_t cookie, +                                        int pages_cnt)

Ditto.

...

+{ +    struct shm_buf *shm_buf;

+    while(1) +    { +        int old = atomic_read(&ctx->shm_buf_pages); +        int new = old + pages_cnt; +        if ( new >= MAX_TOTAL_SMH_BUF_PG ) +            return NULL; +        if ( likely(old == atomic_cmpxchg(&ctx->shm_buf_pages, old, new)) ) +            break; +    }

+    shm_buf = xzalloc_bytes(sizeof(struct shm_buf) + +                            pages_cnt * sizeof(struct page *)); +    if ( !shm_buf ) {

Coding style:

if ( ... ) {

...

+        atomic_sub(pages_cnt, &ctx->shm_buf_pages); +        return NULL; +    }

+    shm_buf->cookie = cookie; +    shm_buf->max_page_cnt = pages_cnt;

+    spin_lock(&ctx->lock); +    list_add_tail(&shm_buf->list, &ctx->shm_buf_list); +    spin_unlock(&ctx->lock);

+    return shm_buf; +}

+static void free_shm_buf(struct domain_ctx *ctx, uint64_t cookie) +{ +    struct shm_buf *shm_buf; > +    bool found = false;

+    spin_lock(&ctx->lock); +    list_for_each_entry( shm_buf, &ctx->shm_buf_list, list ) +    { +        if ( shm_buf->cookie == cookie )

What does guarantee you the cookie will be uniq?

...

+        { +            found = true; +            list_del(&shm_buf->list); +            break; +        } +    } +    spin_unlock(&ctx->lock);

At this point you have a shm_buf in hand to free. But what does guarantee you no-one will use it?

...

+    if ( !found ) { +        return; +    }

+    for ( int i = 0; i < shm_buf->page_cnt; i++ )

Please define int i before hand.

...

+        if ( shm_buf->pages[i] ) +            put_page(shm_buf->pages[i]);

+    atomic_sub(shm_buf->max_page_cnt, &ctx->shm_buf_pages);

+    xfree(shm_buf); +}

static void optee_domain_destroy(struct domain *d)   {       struct arm_smccc_res resp;       struct domain_ctx *ctx;       struct std_call_ctx *call, *call_tmp;       struct shm_rpc *shm_rpc, *shm_rpc_tmp; +    struct shm_buf *shm_buf, *shm_buf_tmp;       bool found = false;       /* At this time all domain VCPUs should be stopped */ @@ -377,12 +460,163 @@ static void optee_domain_destroy(struct domain *d)       list_for_each_entry_safe( shm_rpc, shm_rpc_tmp, &ctx->shm_rpc_list, list )           free_shm_rpc(ctx, shm_rpc->cookie); +    list_for_each_entry_safe( shm_buf, shm_buf_tmp, &ctx->shm_buf_list, list ) +        free_shm_buf(ctx, shm_buf->cookie);

ASSERT(!atomic_read(&ctx->call_ctx_count));       ASSERT(!atomic_read(&ctx->shm_rpc_count)); +    ASSERT(!atomic_read(&ctx->shm_buf_pages));       xfree(ctx);   } +#define PAGELIST_ENTRIES_PER_PAGE                       \ +    ((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(u64)) - 1)

+static size_t get_pages_list_size(size_t num_entries) +{ +    int pages = DIV_ROUND_UP(num_entries, PAGELIST_ENTRIES_PER_PAGE);

+    return pages * OPTEE_MSG_NONCONTIG_PAGE_SIZE; +}

+static bool translate_noncontig(struct domain_ctx *ctx, +                                struct std_call_ctx *call, +                                struct optee_msg_param *param, +                                int idx)

Most likely this should be unsigned.

...

+{ +    /* +     * Refer to OPTEE_MSG_ATTR_NONCONTIG description in optee_msg.h for details. +     */ +    uint64_t size; +    int page_offset; +    int num_pages; +    int order; +    int entries_on_page = 0; +    paddr_t gaddr; +    mfn_t guest_mfn;

I don't like the terminology guest_mfn. This is misleading because of the past usage in Xen. It would be better if you call this just mfn.

Okay, will do.

...

+    struct { +        uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE]; +        uint64_t next_page_data; +    } *pages_data_guest, *pages_data_xen, *pages_data_xen_start; +    struct shm_buf *shm_buf;

+    page_offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);

+    size = ROUNDUP(param->u.tmem.size + page_offset, +                   OPTEE_MSG_NONCONTIG_PAGE_SIZE);

+    num_pages = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE);

+    order = get_order_from_bytes(get_pages_list_size(num_pages));

+    pages_data_xen_start = alloc_xenheap_pages(order, 0);

This could be replaced by a _xmalloc and would avoid to allocate more memory than necessary when the order is getting bigger.

Thanks. Would it allocate page-aligned buffer? This is crucial in this case. I can't find any documentation on it so I don't know which alignment it guarantees.

...

+    if ( !pages_data_xen_start ) +        return false;

+    shm_buf = allocate_shm_buf(ctx, param->u.tmem.shm_ref, num_pages);

In alocate_shm_buf you are now globally limiting the number of pages ( (16384) to pin. However, this does not limit per call.

With the current limit, you would could call up to 16384 times lookup_and_pin_guest_ram_addr(...). On Arm, for p2m related operation, we limit to 512 iterations in one go before checking the preemption. So I think 16384 times is far too much.

So, in other words, I can translate only 2MB buffer (if 4096KB pages are used), is it right? I think, it will be okay to implement such limitation for this initial version of mediator. In the future, it would be possible to do RPC return from XEN (as OP-TEE does) to finish this request later.

...

+    if ( !shm_buf ) +        goto err_free;

+    gaddr = param->u.tmem.buf_ptr & ~(OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1); +    guest_mfn = lookup_and_pin_guest_ram_addr(gaddr, NULL); +    if ( mfn_eq(guest_mfn, INVALID_MFN) ) +        goto err_free;

+    pages_data_guest = map_domain_page(guest_mfn); +    if ( !pages_data_guest ) +        goto err_free;

+    pages_data_xen = pages_data_xen_start; +    while ( num_pages ) { +        struct page_info *page; +        mfn_t entry_mfn = lookup_and_pin_guest_ram_addr( +            pages_data_guest->pages_list[entries_on_page], &page);

+        if ( mfn_eq(entry_mfn, INVALID_MFN) ) +            goto err_unmap;

+        shm_buf->pages[shm_buf->page_cnt++] = page; +        pages_data_xen->pages_list[entries_on_page] = mfn_to_maddr(entry_mfn); +        entries_on_page++;

+        if ( entries_on_page == PAGELIST_ENTRIES_PER_PAGE ) { +            pages_data_xen->next_page_data = virt_to_maddr(pages_data_xen + 1); +            pages_data_xen++; +            gaddr = pages_data_guest->next_page_data;

next_page_data is not a guest address but a machine address. For anything related to address, the variable should be named accordingly to avoid confusion.

Why? In this case it is IPA that comes from the guest.

...

+            unmap_domain_page(pages_data_guest); +            unpin_guest_ram_addr(guest_mfn);

+            guest_mfn = lookup_and_pin_guest_ram_addr(gaddr, NULL); +            if ( mfn_eq(guest_mfn, INVALID_MFN) ) +                goto err_free;

+            pages_data_guest = map_domain_page(guest_mfn); +            if ( !pages_data_guest ) +                goto err_free; +            /* Roll over to the next page */ +            entries_on_page = 0; +        } +        num_pages--; +    }

+    param->u.tmem.buf_ptr = virt_to_maddr(pages_data_xen_start) | page_offset;

+    call->non_contig[idx] = pages_data_xen_start; +    call->non_contig_order[idx] = order;

If you use _xmalloc, then you don't need to store the order. But, who is going to free non_contig?

Thank you for pointing out. It should be done in free_std_call_ctx() but seems I missed this part of the code when split the big patch into smaller ones.

...

+    unmap_domain_page(pages_data_guest); +    unpin_guest_ram_addr(guest_mfn); +    return true;

+err_unmap: +    unmap_domain_page(pages_data_guest); +    unpin_guest_ram_addr(guest_mfn); +    free_shm_buf(ctx, shm_buf->cookie);

+err_free: +    free_xenheap_pages(pages_data_xen_start, order);

+    return false; +}

+static bool translate_params(struct domain_ctx *ctx, +                             struct std_call_ctx *call) +{ +    unsigned int i; +    uint32_t attr;

+    for ( i = 0; i < call->xen_arg->num_params; i++ ) {

Please pay attention to Xen coding style. I haven't pointed out everywhere, but I would all of them to be fixed in the next version.

Yes, I'm sorry for that. I simultaneously work with different projects and sometimes it is hard to track coding style. I'll fix all such problems.

...

+        attr = call->xen_arg->params[i].attr;

+        switch ( attr & OPTEE_MSG_ATTR_TYPE_MASK ) { +        case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT: +        case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: +        case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: +            if ( attr & OPTEE_MSG_ATTR_NONCONTIG ) { +                if ( !translate_noncontig(ctx, call, +                                          call->xen_arg->params + i, i) ) +                    return false; +            } +            else { +                gprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n"); +                return false; +            } +            break; +        case OPTEE_MSG_ATTR_TYPE_NONE: +        case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: +        case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: +        case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: +        case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: +        case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: +        case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: +            continue; +        } +    } +    return true; +}

/*    * Copy command buffer into xen memory to:    * 1) Hide translated addresses from guest @@ -488,6 +722,15 @@ static bool execute_std_call(struct domain_ctx *ctx,       copy_std_request_back(ctx, regs, call); +    /* +     * If guest successfully unregistered own shared memory, +     * then we can unpin it's pages +     */ +    if ( call->xen_arg->cmd == OPTEE_MSG_CMD_UNREGISTER_SHM && +         call->xen_arg->ret == 0 ) { +        free_shm_buf(ctx, call->xen_arg->params[0].u.rmem.shm_ref); +    }

free_std_call_ctx(ctx, call);       return true; @@ -522,7 +765,7 @@ static bool handle_std_call(struct domain_ctx *ctx, struct cpu_user_regs *regs)       case OPTEE_MSG_CMD_CANCEL:       case OPTEE_MSG_CMD_REGISTER_SHM:       case OPTEE_MSG_CMD_UNREGISTER_SHM: -        ret = true; +        ret = translate_params(ctx, call);           break;       default:           ret = false;

Cheers,

Hi,

On 10.09.18 18:34, Julien Grall wrote:

Hi Volodymyr,

On 03/09/18 17:54, Volodymyr Babchuk wrote:

...

OP-TEE can issue multiple RPC requests. We are interested mostly in request that asks NW to allocate/free shared memory for OP-TEE needs, becuase mediator need to do address translation in the same

s/becuase/because/ s/need/needs/

the mediator

...

way as it was done for shared buffers registered by NW.

As mediator now accesses shared command buffer, we need to shadow

"As the"

...

it in the same way, as we shadow request buffers for STD calls.

Signed-off-by: Volodymyr Babchuk volodymyr_babchuk@epam.com

xen/arch/arm/tee/optee.c | 137 +++++++++++++++++++++++++++++++++++++++++++----   1 file changed, 126 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 8bfcfdc..b2d795e 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -45,6 +45,7 @@ struct std_call_ctx {   struct shm_rpc {       struct list_head list;       struct optee_msg_arg *guest_arg; +    struct optee_msg_arg *xen_arg;       struct page *guest_page;       mfn_t guest_mfn;       uint64_t cookie; @@ -303,6 +304,10 @@ static struct shm_rpc *allocate_and_map_shm_rpc(struct domain_ctx *ctx, paddr_t       if ( !shm_rpc )           goto err; +    shm_rpc->xen_arg = alloc_xenheap_page(); +    if ( !shm_rpc->xen_arg ) +        goto err;

shm_rpc->guest_mfn = lookup_and_pin_guest_ram_addr(gaddr, NULL);       if ( mfn_eq(shm_rpc->guest_mfn, INVALID_MFN) ) @@ -324,6 +329,10 @@ static struct shm_rpc *allocate_and_map_shm_rpc(struct domain_ctx *ctx, paddr_t   err:       atomic_dec(&ctx->shm_rpc_count);

+    if ( shm_rpc->xen_arg ) +        free_xenheap_page(shm_rpc->xen_arg);

xfree(shm_rpc);       return NULL;   } @@ -346,9 +355,10 @@ static void free_shm_rpc(struct domain_ctx *ctx, uint64_t cookie)       }       spin_unlock(&ctx->lock); -    if ( !found ) { +    if ( !found )           return; -    }

That change should be folded into the patch adding the {}.

...

+    free_xenheap_page(shm_rpc->xen_arg);       if ( shm_rpc->guest_arg ) {           unpin_guest_ram_addr(shm_rpc->guest_mfn); @@ -358,6 +368,24 @@ static void free_shm_rpc(struct domain_ctx *ctx, uint64_t cookie)       xfree(shm_rpc);   } +static struct shm_rpc *find_shm_rpc(struct domain_ctx *ctx, uint64_t cookie) +{ +    struct shm_rpc *shm_rpc;

+    spin_lock(&ctx->lock); +    list_for_each_entry( shm_rpc, &ctx->shm_rpc_list, list ) +    { +        if ( shm_rpc->cookie == cookie ) +        { +                spin_unlock(&ctx->lock); +                return shm_rpc; +        } +    } +    spin_unlock(&ctx->lock);

+    return NULL; +}

static struct shm_buf *allocate_shm_buf(struct domain_ctx *ctx,                                           uint64_t cookie,                                           int pages_cnt) @@ -704,6 +732,28 @@ static bool copy_std_request_back(struct domain_ctx *ctx,       return true;   } +static void handle_rpc_return(struct domain_ctx *ctx, +                             struct cpu_user_regs *regs, +                             struct std_call_ctx *call) +{ +    call->optee_thread_id = get_user_reg(regs, 3); +    call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(get_user_reg(regs, 0));

+    if ( call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD ) +    { +        /* Copy RPC request from shadowed buffer to guest */ +        uint64_t cookie = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2); +        struct shm_rpc *shm_rpc = find_shm_rpc(ctx, cookie);

Newline between declaration and code.

Sorry, another habit from kernel coding style :(

...

+        if ( !shm_rpc ) +        { +            gprintk(XENLOG_ERR, "Can't find SHM-RPC with cookie %lx\n", cookie); +            return; +        } +        memcpy(shm_rpc->guest_arg, shm_rpc->xen_arg, +               OPTEE_MSG_GET_ARG_SIZE(shm_rpc->xen_arg->num_params)); +    } +}

static bool execute_std_call(struct domain_ctx *ctx,                                struct cpu_user_regs *regs,                                struct std_call_ctx *call) @@ -715,8 +765,7 @@ static bool execute_std_call(struct domain_ctx *ctx,       optee_ret = get_user_reg(regs, 0);       if ( OPTEE_SMC_RETURN_IS_RPC(optee_ret) )       { -        call->optee_thread_id = get_user_reg(regs, 3); -        call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(optee_ret); +        handle_rpc_return(ctx, regs, call);

It would make sense to introduce handle_rpc_return where you actually add those 2 lines.

...

return true;       } @@ -783,6 +832,74 @@ out:       return ret;   }

+static void handle_rpc_cmd_alloc(struct domain_ctx *ctx, +                                 struct cpu_user_regs *regs, +                                 struct std_call_ctx *call, +                                 struct shm_rpc *shm_rpc) +{ +    if ( shm_rpc->xen_arg->params[0].attr != (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT | +                                            OPTEE_MSG_ATTR_NONCONTIG) ) +    { +        gprintk(XENLOG_WARNING, "Invalid attrs for shared mem buffer\n"); +        return; +    }

+    /* Last entry in non_contig array is used to hold RPC-allocated buffer */ +    if ( call->non_contig[MAX_NONCONTIG_ENTRIES - 1] ) +    { +        free_xenheap_pages(call->non_contig[MAX_NONCONTIG_ENTRIES - 1],

call->non_contig_order[MAX_NONCONTIG_ENTRIES - 1]); +        call->non_contig[MAX_NONCONTIG_ENTRIES - 1] = NULL; +    }

This is quite odd. Why don't you just deny allocating information in the non_config array? This would avoid to silently dropped any page that may have been linked together and potentially used still in use.

No, this, actually is part of the protocol. OP-TEE can ask to allocate more shared buffers, one per RPC return. call->non_contig[x] is needed to hold list of pages until OP_TEE consumes them. The it can be freed and reused to allocate next buffer. Consider this:

1. OP-TEE issues RPC "allocate buffer" 2. NW returns list of pages 3. Mediator translates and stores address in non_contig[x] 4. OP-TEE begins to consume this list 5. IRQ arrives and OP-TEE forced to break the work 6. Mediator receives control back, but it should not free non_contig[x], because it is not sure of OP-TEE finished reading from it 7. Xen/guest handles the IRQ and returns control back to OP-TEE 8. OP-TEE finishes processing this buffers and asks for another one 9. NW returns list of pages for the next buffer 10. At this point mediator is sure that OP-TEE finished processing old non_contig[x], so it can free it and allocated another.

I would also prefer if you introduce a define for that RCP-allocated buffer.

...

+    translate_noncontig(ctx, call, shm_rpc->xen_arg->params + 0,

What does the "+ 0" stand for?

...

+                        MAX_NONCONTIG_ENTRIES - 1); +}

+static void handle_rpc_cmd(struct domain_ctx *ctx, struct cpu_user_regs *regs, +                           struct std_call_ctx *call) +{ +    struct shm_rpc *shm_rpc; +    uint64_t cookie; +    int num_params;

+    cookie = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2);

+    shm_rpc = find_shm_rpc(ctx, cookie);

+    if ( !shm_rpc ) +    { +        gprintk(XENLOG_ERR, "Can't find SHM-RPC with cookie %lx\n", cookie); +        return; +    }

+    num_params = shm_rpc->guest_arg->num_params;

+    barrier(); /* Ensure that num_params is read once */

I would use:

num_params = ACCESS_ONCE(shm_rpc->guest_arg->num_params);

I looked for this macro, but somehow missed it. Thanks for pointing out.

...

+    if ( OPTEE_MSG_GET_ARG_SIZE(num_params) > OPTEE_MSG_NONCONTIG_PAGE_SIZE ) +        return;

+    memcpy(shm_rpc->xen_arg, shm_rpc->guest_arg, +           OPTEE_MSG_GET_ARG_SIZE(num_params));

+    switch (shm_rpc->xen_arg->cmd) { +    case OPTEE_MSG_RPC_CMD_GET_TIME: +        break; +    case OPTEE_MSG_RPC_CMD_WAIT_QUEUE: +        break; +    case OPTEE_MSG_RPC_CMD_SUSPEND: +        break; +    case OPTEE_MSG_RPC_CMD_SHM_ALLOC: +        handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc); +        break; +    case OPTEE_MSG_RPC_CMD_SHM_FREE: +        free_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b); +        break; +    default: +        break; +    } +}

static void handle_rpc_func_alloc(struct domain_ctx *ctx,                                     struct cpu_user_regs *regs)   { @@ -796,13 +913,11 @@ static void handle_rpc_func_alloc(struct domain_ctx *ctx,           struct shm_rpc *shm_rpc;           shm_rpc = allocate_and_map_shm_rpc(ctx, ptr, cookie); -        if ( !shm_rpc ) -        { +        if ( !shm_rpc ) {

Please try to avoid changing the coding style in different patch. But this one is wrong.

Yep :( this is the artifact from splitting the big patch.

...

gprintk(XENLOG_WARNING, "Failed to allocate shm_rpc object\n"); -            ptr = 0; -        } -        else -            ptr = mfn_to_maddr(shm_rpc->guest_mfn); +            ptr = ~0;

Can you explain why you change from 0 to ~0?

I had to introduce this in the original patch, actually.

...

+        } else +            ptr = virt_to_maddr(shm_rpc->xen_arg);           set_user_reg(regs, 1, ptr >> 32);           set_user_reg(regs, 2, ptr & 0xFFFFFFFF); @@ -833,7 +948,7 @@ static bool handle_rpc(struct domain_ctx *ctx, struct cpu_user_regs *regs)       case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:           break;       case OPTEE_SMC_RPC_FUNC_CMD: -        /* TODO: Add handling */ +        handle_rpc_cmd(ctx, regs, call);           break;       }

Cheers,

Hi,

On 11.09.18 16:56, Julien Grall wrote:

On 10/09/18 19:14, Volodymyr Babchuk wrote:

...

On 10.09.18 18:34, Julien Grall wrote:

...

On 03/09/18 17:54, Volodymyr Babchuk wrote:

...

static struct shm_buf *allocate_shm_buf(struct domain_ctx *ctx,                                           uint64_t cookie,                                           int pages_cnt) @@ -704,6 +732,28 @@ static bool copy_std_request_back(struct domain_ctx *ctx,       return true;   } +static void handle_rpc_return(struct domain_ctx *ctx, +                             struct cpu_user_regs *regs, +                             struct std_call_ctx *call) +{ +    call->optee_thread_id = get_user_reg(regs, 3); +    call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(get_user_reg(regs, 0));

+    if ( call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD ) +    { +        /* Copy RPC request from shadowed buffer to guest */ +        uint64_t cookie = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2); +        struct shm_rpc *shm_rpc = find_shm_rpc(ctx, cookie);

Newline between declaration and code.

Sorry, another habit from kernel coding style :(

I think you need to modify your habit because I am pretty sure Linux folks would not allow:

Ah, yes, I read your previous message incorrectly.

...

...

...

+        if ( !shm_rpc ) +        { +            gprintk(XENLOG_ERR, "Can't find SHM-RPC with cookie %lx\n", cookie); +            return; +        } +        memcpy(shm_rpc->guest_arg, shm_rpc->xen_arg, +               OPTEE_MSG_GET_ARG_SIZE(shm_rpc->xen_arg->num_params)); +    } +}

static bool execute_std_call(struct domain_ctx *ctx,                                struct cpu_user_regs *regs,                                struct std_call_ctx *call) @@ -715,8 +765,7 @@ static bool execute_std_call(struct domain_ctx *ctx,       optee_ret = get_user_reg(regs, 0);       if ( OPTEE_SMC_RETURN_IS_RPC(optee_ret) )       { -        call->optee_thread_id = get_user_reg(regs, 3); -        call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(optee_ret); +        handle_rpc_return(ctx, regs, call);

It would make sense to introduce handle_rpc_return where you actually add those 2 lines.

...

return true;       } @@ -783,6 +832,74 @@ out:       return ret;   }

+static void handle_rpc_cmd_alloc(struct domain_ctx *ctx, +                                 struct cpu_user_regs *regs, +                                 struct std_call_ctx *call, +                                 struct shm_rpc *shm_rpc) +{ +    if ( shm_rpc->xen_arg->params[0].attr != (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |

OPTEE_MSG_ATTR_NONCONTIG) ) +    { +        gprintk(XENLOG_WARNING, "Invalid attrs for shared mem buffer\n"); +        return; +    }

+    /* Last entry in non_contig array is used to hold RPC-allocated buffer */ +    if ( call->non_contig[MAX_NONCONTIG_ENTRIES - 1] ) +    { +        free_xenheap_pages(call->non_contig[MAX_NONCONTIG_ENTRIES - 1],

• call->non_contig_order[MAX_NONCONTIG_ENTRIES - 1]);

+        call->non_contig[MAX_NONCONTIG_ENTRIES - 1] = NULL; +    }

This is quite odd. Why don't you just deny allocating information in the non_config array? This would avoid to silently dropped any page that may have been linked together and potentially used still in use.

No, this, actually is part of the protocol. OP-TEE can ask to allocate more shared buffers, one per RPC return.

Please a give link to the spec and the paragraph.

optee_msg.h, above #define OPTEE_MSG_RPC_CMD_SHM_ALLOC 6 (line 405 in my case). It also defines fragmented buffers, which currently aren't supported in OP-TEE.

...

call->non_contig[x] is needed to hold list of pages until OP_TEE consumes them. The it can be freed and reused to allocate next buffer. Consider this:

What is x?

Index in non_contig array. The same principle works not only for buffers allocated by RPC, but for all other buffers as well. See below.

...

1. OP-TEE issues RPC "allocate buffer"
2. NW returns list of pages
3. Mediator translates and stores address in non_contig[x]
4. OP-TEE begins to consume this list
5. IRQ arrives and OP-TEE forced to break the work
6. Mediator receives control back, but it should not free non_contig[x],

because it is not sure of OP-TEE finished reading from it 7. Xen/guest handles the IRQ and returns control back to OP-TEE 8. OP-TEE finishes processing this buffers and asks for another one 9. NW returns list of pages for the next buffer 10. At this point mediator is sure that OP-TEE finished processing      old non_contig[x], so it can free it and allocated another.

Thank you for the description of the protocol. However, it is still does not explain why you decided to free MAX_NONCONTIG_ENTRIES - 1. Why not 0 or 1 or n?

Okay. You can pass up to 4 arguments for TA in command buffer. Any of that argument can be a shared memory reference, so we need at least 4 items in non_contig array. Sadly, this constant (TEE_NUM_PARAMS) is defined not optee_msg.h or optee_smc.h, but in tee_api_defines.h, which is user-space (or TA) part of OP-TEE. But, now I'm thinking, that maybe it is not so bad idea to add this file to XEN... What is your opinion?

Anyways, we need at least 4 items for arguments. But I defined MAX_NONCONTIG_ENTRIES as 5, because last item in array is used for RPC-allocated buffer. I also added comment. I'll copy it there:

/* Last entry in non_contig array is used to hold RPC-allocated buffer */

So, first 4 items are used for arguments and last one used for RPC requests. Right way to define MAX_NONCONTIG_ENTRIES is (TEE_NUM_PARAMS + 1), but then I should add another header file, which defines GlobalPlatform TEE core API.

Overall, it feels like to me you want to write more documentation about how the mediator is supposed to work.

Looks like I need to start on how OP-TEE protocol is supposed to work... I tried to cover this in the commit messages, but looks like it is not sufficient.

...

...

...

gprintk(XENLOG_WARNING, "Failed to allocate shm_rpc object\n"); -            ptr = 0; -        } -        else -            ptr = mfn_to_maddr(shm_rpc->guest_mfn); +            ptr = ~0;

Can you explain why you change from 0 to ~0?

I had to introduce this in the original patch, actually.

What do you mean?

Please ignore previous sentence. There should be 0. I'll remove this change.