OP-TEE Benchmark #1365

igoropaniuk · 2017-02-22T16:04:15Z

Related PRs:

Benchmark app:
linaro-swg/optee_benchmark#1

Changes in OP-TEE
linaro-swg/linux#38
OP-TEE/build#124
#1365
OP-TEE/optee_client#79

etienne-lms

This change enables benchmarking for all OP-TEE invocations. Is this what you expected ?

etienne-lms · 2017-02-27T12:57:00Z

core/arch/arm/pta/sta_benchmark.c

+		(TEE_PARAM_TYPE_GET(type, 3) != TEE_PARAM_TYPE_NONE)) {
+		return TEE_ERROR_BAD_PARAMETERS;
+	}
+


should register also the buffer size.

etienne-lms · 2017-02-27T12:58:11Z

core/arch/arm/pta/sta_benchmark.c

+	DMSG("Registering stat buffer, addr = %p, paddr = %p\n",
+			p[1].memref.buffer,
+			(void *)virt_to_phys((void *)p[1].memref.buffer));
+	bench_stat_buf = (void *)p[1].memref.buffer;


minor: buffer is already a void *. here and there.

etienne-lms · 2017-02-27T12:59:54Z

core/arch/arm/pta/sta_benchmark.c

+	DMSG("Sending back timestamp buffer paddr, paddr = %p\n",
+		(void *)virt_to_phys(bench_ts_buf));
+	p[0].value.a = virt_to_phys(bench_ts_buf);
+	p[1].value.a = virt_to_phys(bench_stat_buf);


value.a are 32bit, while bench buffers physical addresses might be 64bit.

etienne-lms · 2017-02-27T13:01:58Z

core/arch/arm/pta/sta_benchmark.c

+	DMSG("Unregistering benchmark, timestamp buffer paddr = %p\n",
+		(void *)virt_to_phys(bench_ts_buf));
+	bench_ts_buf = 0;
+	bench_stat_buf = 0;


minor: NULL rather than 0. debug trace displays only the timestamp buffer, not the statistics one.

etienne-lms · 2017-02-27T13:03:19Z

core/arch/arm/pta/sta_benchmark.c

+static void close_session(void *psess __unused)
+{
+
+}


minor: as create_ta/destroy_ta/open_session/close_session are stubs, you can simply discard them.

etienne-lms · 2017-02-27T13:14:51Z

core/include/tee_bench_core.h

+static inline uint64_t read_ccounter(void)
+{
+	uint64_t ccounter = 0;
+#ifdef __aarch64__


minor: add empty line

etienne-lms · 2017-02-27T13:15:01Z

core/include/tee_bench_core.h

+static inline __attribute__((always_inline)) uintptr_t read_pc(void)
+{
+	uintptr_t pc = 0;
+#ifdef __aarch64__


minor: add empty line

This is ARM specific, please store it below core/arch/arm

etienne-lms · 2017-02-27T13:15:57Z

core/include/tee_bench_core.h

+	struct tee_time_buf *timeb = NULL;
+	uint64_t ts_i;
+
+	if ((cmd_id & BENCHMARK_CMD(0)) == BENCHMARK_CMD(0))


What about TAs that want to use such command IDs ? they won't be able to get benchmark stats.

etienne-lms · 2017-02-27T13:16:12Z

core/include/tee_bench_core.h

+static inline __attribute__((always_inline))
+void bm_timestamp(uint32_t source, uint32_t cmd_id)
+{
+	struct tee_time_buf *timeb = NULL;


minor: useless init

etienne-lms · 2017-02-27T13:17:18Z

core/include/tee_bench_core.h

+static void bm_timestamp(uint32_t source __unused,
+				uint32_t cmd_id __unused)
+{
+	;


add inline

minor: need this ; ?

jenswi-linaro · 2017-02-27T13:37:15Z

core/arch/arm/pta/sta_benchmark.c

+	DMSG("Registering timestamp buffer, addr = %p, paddr = %p\n",
+			p[0].memref.buffer,
+			(void *)virt_to_phys((void *)p[0].memref.buffer));
+	bench_ts_buf = (void *)p[0].memref.buffer;


Cast not needed

jenswi-linaro · 2017-02-27T13:38:45Z

core/arch/arm/pta/sta_benchmark.c

+
+	DMSG("Registering timestamp buffer, addr = %p, paddr = %p\n",
+			p[0].memref.buffer,
+			(void *)virt_to_phys((void *)p[0].memref.buffer));


Cast not needed
Use PRIxPA instead of p to print paddr_t values.

jenswi-linaro · 2017-02-27T13:40:35Z

core/arch/arm/pta/sta_benchmark.c

+		uint32_t nCommandID, uint32_t nParamTypes,
+		TEE_Param pParams[TEE_NUM_PARAMS])
+{
+	DMSG("command entry point for static ta \"%s\"", TA_NAME);


How useful is this?

Not useful at all, just forgot to delete it :)

jenswi-linaro · 2017-02-27T13:44:11Z

core/arch/arm/pta/sta_benchmark.c

@@ -0,0 +1,160 @@
+/*
+ * Copyright (c) 2015, Linaro Limited


... or drop the sta_ prefix entirely.

jenswi-linaro · 2017-02-27T13:55:09Z

core/include/tee_bench_core.h

+ */
+
+#ifndef TEE_BENCH_H
+#define TEE_BENCH_H


Both the tee and core part of this filename doesn't seem a bit redundant.

jenswi-linaro · 2017-02-27T13:56:27Z

core/include/tee_bench_core.h

+static inline __attribute__((always_inline)) uintptr_t read_pc(void)
+{
+	uintptr_t pc = 0;
+#ifdef __aarch64__


This is ARM specific, please store it below core/arch/arm

jenswi-linaro · 2017-02-27T14:11:50Z

lib/libutee/include/tee_api_defines.h

@@ -450,8 +450,7 @@
 *  }
 */
 #define TEE_PARAM_TYPES(t0,t1,t2,t3) \
-   ((t0) | ((t1) << 4) | ((t2) << 8) | ((t3) << 12))
-
+		((t0) | ((t1) << 4) | ((t2) << 8) | ((t3) << 12))


Change not needed

jenswi-linaro · 2017-02-27T14:18:35Z

core/include/tee_bench_core.h

+
+	if (timeb->tm_ind >= TEE_BENCH_MAX_STAMPS)
+		return;
+	ts_i = timeb->tm_ind++;


timeb->tm_ind should be copied before checking the value as normal world may have a window where it can update it unchecked.

What about concurrent updates?

jenswi-linaro · 2017-02-27T14:18:47Z

core/include/tee_bench_core.h

+	if (!bench_ts_buf)
+		return;
+
+	timeb = (struct tee_time_buf *) bench_ts_buf;


Cast not needed

jenswi-linaro · 2017-02-27T14:23:48Z

core/include/tee_bench_core.h

+}
+
+/* Adding timestamp to buffer */
+static inline __attribute__((always_inline))


With __builtin_return_address(0) instead below, or some clever macro to supply the return address as an argument to this function it wouldn't need to be inlined.

jenswi-linaro · 2017-04-18T08:18:49Z

core/arch/arm/pta/benchmark.c

+{
+	uint64_t ccounter = 0;
+#ifdef __aarch64__
+	asm volatile("mrs %0, PMCCNTR_EL0" : "=r"(ccounter));


Please add a read_pmccntr_el0() or similar in arm64.h and use that instead, and something similar for the ARM32 version below.

Already moving it there :)

etienne-lms · 2017-04-18T08:34:48Z

core/arch/arm/pta/benchmark.c

+	DMSG("Registering timestamp buffer, addr = %p, paddr = %" PRIxPA "\n",
+			p[0].memref.buffer,
+			virt_to_phys(p[0].memref.buffer));
+	bench_ts_global = p[0].memref.buffer;


Should check p[0].memref.buffer is mapped non-secure (case secure client) ?

If the client is secure, the buffer virtual address is likely to be temporary (to this TA invocation). Core should get the paddr and gets it core mapping to get a reliable virtual address for the buffer.

@etienne-lms II didn't get it. Could you please provide any example of secure client case?
Or you mean TA-to-TA calls?

If a TA invokes your pTA, the memref parameters can be SHM references or secure memory reference (specific case when TA use a memref parameter on a TA private buffer).
As we expect here a SHM buffer (later RPC to tee-supplicant), maybe you can only check tee_pbuf_is_non_sec(p[0].memref.buffer, p[0].memref.size).

added check

etienne-lms · 2017-04-18T08:41:47Z

core/arch/arm/pta/benchmark.c

+
+	/* fill timestamp data */
+	if (cur_cpu >= bench_ts_global->cores)
+		return;


thread_set_foreign_intr(true);

See line 199

or I can move call of read_ccounter(); a bit higher (to cur_cpu = get_core_pos(); ) to re-enable interrupt handling as soon as possible

@etienne-lms

if (cur_cpu >= bench_ts_global->cores) { thread_set_foreign_intr(true); return; }

etienne-lms · 2017-04-18T08:43:00Z

core/arch/arm/pta/benchmark.c

+	cpu_buf = &bench_ts_global->cpu_buf[cur_cpu];
+	ts_i = __sync_fetch_and_add(&cpu_buf->head, 1);
+	ts_data.cnt = read_ccounter();
+	ts_data.addr = (uintptr_t) ret_addr;


minor: remove space before ret_addr

etienne-lms · 2017-04-18T08:47:37Z

core/arch/arm/tee/entry_std.c

@@ -287,6 +288,8 @@ static void entry_invoke_command(struct thread_smc_args *smc_args,
 	struct tee_ta_session *s;
 	struct tee_ta_param param;

+	bm_timestamp();


This should be a known ID on which REE gets comprehensive.
I think this should be like bm_timestamp(CORE_PRE/POST_IMVOKE_TA);
Maybe this can come later... Specified bm points and free bm points.

saving __builtin_return_address(0); in bm_timestamp() isn't sufficient for these purposes?

@etienne-lms

if the PC gives a clear view of trace point when rendering the output data to a human readable format, then it is fine.

it's going to be mapped to specific file:line and function (the same as addr2line does)

I think this is OK as it is for now.

etienne-lms · 2017-04-19T14:30:34Z

core/arch/arm/pta/benchmark.c

+	if (!bench_ts_global)
+		return;
+
+	thread_set_foreign_intr(false);


What if called with foreign interruptions not enabled? If will be enabled at routine exit.

@etienne-lms good question, my first intention was to use thread_restore_foreign_intr(), but, actually, I didn't fully get where and how the previous CPSR value is saved (based on only one example of thread_restore_foreign_intr() I found in void tee_svc_handler(struct thread_svc_regs *regs) implementation), so the only one option was just to do it manually with thread_get_exceptions/thread_set_exceptions.

uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL);
...
thread_unmask_exceptions(exceptions);

Or at least, mask THREAD_EXCP_FOREIGN_INTR as it is the noe that can makes thread to migrate to another cpu.

etienne-lms · 2017-04-19T14:30:50Z

core/arch/arm/pta/benchmark.c

+
+	/* fill timestamp data */
+	if (cur_cpu >= bench_ts_global->cores)
+		return;


if (cur_cpu >= bench_ts_global->cores) { thread_set_foreign_intr(true); return; }

igoropaniuk · 2017-04-19T15:33:45Z

@etienne-lms @jenswi-linaro Could check please also https://github.com/linaro-swg/optee_benchmark/pull/1/files, it includes almost 40-50% of the functionality. Thanks

jbech-linaro · 2017-04-26T11:37:22Z

core/arch/arm/pta/benchmark.c

+
+	mutex_lock(&bench_reg_mu);
+
+	/* we accept only non-secure buffers, as we later perform


Nitpick: But I think checkpatch will complain here (multiline comment shall start with /* on a single line).

jbech-linaro · 2017-04-26T11:37:47Z

core/arch/arm/pta/benchmark.c

+
+	/* we accept only non-secure buffers, as we later perform
+	 * registration of this buffer in NS layers
+	 * (optee linx kmod/optee client)


s/linx/Linux/

jbech-linaro · 2017-04-26T12:02:20Z

core/arch/arm/pta/benchmark.c

+	exceptions = thread_mask_exceptions(THREAD_EXCP_ALL);
+	cur_cpu = get_core_pos();
+
+	/* fill timestamp data */


This comment seems a bit malplaced.

jbech-linaro · 2017-04-26T12:37:19Z

core/include/bench.h

+#define TEE_BENCH_DIVIDER		64
+
+/* max amount of timestamps per buffer */
+#define TEE_BENCH_MAX_STAMPS	32


Isn't this a rather low default value? Does it mean what it says? I.e, there is room for 32 unique timestamps, then it will wrap?

I.e, there is room for 32 unique timestamps, then it will wrap?

right

jbech-linaro · 2017-04-26T12:39:07Z

core/include/bench.h

+#define OPTEE_MSG_RPC_CMD_BENCH_REG_NEW		0
+#define OPTEE_MSG_RPC_CMD_BENCH_REG_DEL		1
+
+/* OP-TEE susbsystems ids */


Now I've started look into the PR in optee_os, but if this isn't a shared h-file (which I suspect it is), then we should maybe consider remove the ones not in use in optee_os. I'll update this comment when/if I've reviewed the other PRs.

Each rep(besides optee_benchmark, it uses optee_client's 'public/tee_bench.h` file) has it's own version of bench.h (and it's not just a copy-paste), with our defines which are related only to that specific subsystem.
Or maybe I just didn't get your comment

jbech-linaro · 2017-04-26T12:40:25Z

core/include/bench.h

+#ifdef CFG_TEE_BENCHMARK
+void bm_timestamp(void);
+#else
+/* stub */


It's already rather obvious that this is a stub.

jbech-linaro · 2017-04-26T12:41:28Z

core/include/optee_msg.h

@@ -419,4 +419,5 @@ struct optee_msg_arg {
 */
 #define OPTEE_MSG_RPC_CMD_SHM_FREE	7

+#define OPTEE_MSG_RPC_CMD_BENCH_REG	20


I think this command also deserves a comment just like the other cmd's here.

jbech-linaro · 2017-04-26T12:48:21Z

It'd be nice to have "some" documentation in a markdown file or similar, giving a quick high level overview of this. In the acceptance criteria (Jira), we have:

At optee_os/documentation there should be a file "benchmark.md". That describes, how to run the benchmark, how to interpret the data

That doesn't necessarily have to go into this PR, it could be done on a separate PR also.

etienne-lms · 2017-04-26T14:54:11Z

core/arch/arm/pta/benchmark.c

+	ret_addr = __builtin_return_address(0);
+
+	cpu_buf = &bench_ts_global->cpu_buf[cur_cpu];
+	ts_i = __sync_fetch_and_add(&cpu_buf->head, 1);


__sync_fetch_and_add ?

Foreign interruptions (actually all interrupts) are already masked, so you can be pretty sure you can atomically update head value with even a complex SW sequence.

Also, if head is lower than tail an reaches tail when incremented, it should update tail and maybe raise an overrun flag.

Foreign interruptions (actually all interrupts) are already masked, so you can be pretty sure you can atomically update head value with even a complex SW sequence.

Agree. This came from the version, when there was only common timestamp buffer for all CPUs. I'll leave __sync_fetch_and_add only for NW EL0 apps (optee_client).

Also, if head is lower than tail an reaches tail when incremented, it should update tail and maybe raise an overrun flag.

Right, we have already discussed it with Joakim, and he suggested to leave it for now as it is, and add support of overrun detection in further PRs. As for me, it's better just to add additioanl field to struct tee_time_st with will contain timestamp id (actually, it can be the head value, as it's incrementing all the time), so we can detect an over-run when parsing results by the actual timestamp analyzing app, which will detect a bit gap in timestamp IDs.

etienne-lms · 2017-04-26T15:01:09Z

core/arch/arm/pta/benchmark.c

+
+	/* fill timestamp data */
+	if (cur_cpu >= bench_ts_global->cores) {
+		thread_unmask_exceptions(exceptions);


This could panic the TEE. I guess we don't expect benchmark support in end product, so panicking here is not a issue.

etienne-lms · 2017-04-26T15:05:58Z

core/include/bench.h

+#define BENCHMARK_CMD(id)	(0xFA190000 | ((id) & 0xFFFF))
+#define BENCHMARK_CMD_REGISTER_MEMREF	BENCHMARK_CMD(1)
+#define BENCHMARK_CMD_GET_MEMREF		BENCHMARK_CMD(2)
+#define BENCHMARK_CMD_UNREGISTER		BENCHMARK_CMD(3)


The 'pure' benchmark pTA API could go in a specific header file exported to TA devkit for being shared.
(I think Joakim had a comment about it)

@etienne-lms Timestamping within TAs isn't supported yet, do we really need to move this header for now?

This is minor. Yet it is a good habit to locate pTA info definitions in the devkit so it can be used even by non-secure applications that already gets pTA info from the devkit, from include/pta_xxx.h, basically the pTA UUID and its command IDs.
See comment on BENCHMARK_UUID above.

etienne-lms · 2017-04-26T15:06:38Z

core/include/bench.h

+#define BENCHMARK_CMD_GET_MEMREF		BENCHMARK_CMD(2)
+#define BENCHMARK_CMD_UNREGISTER		BENCHMARK_CMD(3)
+
+#define TEE_BENCH_DIVIDER		64


Maybe add a comment about what this macro is about.

etienne-lms · 2017-05-02T08:13:34Z

core/arch/arm/include/arm64.h

+	return val;
+}
+
+


minor: empty line

etienne-lms · 2017-05-02T08:14:16Z

core/arch/arm/pta/benchmark.c

+#include <string.h>
+#include <string_ext.h>
+#include <stdio.h>
+#include <trace.h>


minor: sort those includes.

etienne-lms · 2017-05-02T08:17:46Z

core/arch/arm/pta/benchmark.c

+
+#define BENCHMARK_UUID \
+		{ 0x0b9a63b0, 0xb4c6, 0x4c85, \
+		{ 0xa2, 0x84, 0xa2, 0x28, 0xef, 0x54, 0x7b, 0x4e } }


minor: this could go in lib/libutee/include/pta_benchmark.h as other pTAs header files.
See also comment on BENCHMARK_CMD() below.

etienne-lms · 2017-05-02T08:26:15Z

core/include/bench.h

+#define BENCHMARK_CMD(id)	(0xFA190000 | ((id) & 0xFFFF))
+#define BENCHMARK_CMD_REGISTER_MEMREF	BENCHMARK_CMD(1)
+#define BENCHMARK_CMD_GET_MEMREF		BENCHMARK_CMD(2)
+#define BENCHMARK_CMD_UNREGISTER		BENCHMARK_CMD(3)


This is minor. Yet it is a good habit to locate pTA info definitions in the devkit so it can be used even by non-secure applications that already gets pTA info from the devkit, from include/pta_xxx.h, basically the pTA UUID and its command IDs.
See comment on BENCHMARK_UUID above.

etienne-lms · 2017-05-02T08:30:48Z

core/arch/arm/pta/benchmark.c

+	ret_addr = __builtin_return_address(0);
+
+	cpu_buf = &bench_ts_global->cpu_buf[cur_cpu];
+	ts_i = __sync_fetch_and_add(&cpu_buf->head, 1);


Is __sync_fetch_and_add() reliable ? A simple cpu_buf->head++; will fit.

etienne-lms · 2017-05-05T12:19:54Z

core/arch/arm/include/arm32.h

@@ -601,6 +601,15 @@ static __always_inline uint32_t read_r7(void)
 	asm volatile ("mov %0, r7" : "=r" (val));
 	return val;
 }
+
+/* PMU cycle counter */


minor: useless comment

etienne-lms · 2017-05-05T12:20:22Z

core/arch/arm/include/arm64.h

@@ -238,6 +238,15 @@ static __always_inline uint64_t read_fp(void)
 	return val;
 }

+/* PMU cycle counter */


minor: useless comment

etienne-lms · 2017-05-05T12:23:44Z

core/arch/arm/pta/benchmark.c

+
+	res = rpc_reg_global_buf(OPTEE_MSG_RPC_CMD_BENCH_REG_DEL, 0, 0);
+
+	mutex_unlock(&bench_reg_mu);


can release lock before the RPC.

same comment as : useless (from core) but armless mutex hold. ok like this.

@etienne-lms see https://github.com/OP-TEE/optee_os/pull/1365/files#r117725764

etienne-lms · 2017-05-05T12:29:44Z

core/arch/arm/pta/benchmark.c

+	 * registration of this buffer in NS layers
+	 * (optee linux kmod/optee client)
+	 */
+	if (!tee_pbuf_is_non_sec(p[0].memref.buffer, p[0].memref.size))


p[0].memref.buffer is a virtual address. Should use tee_vbuf_is_non_sec().

etienne-lms · 2017-05-05T12:31:01Z

core/arch/arm/pta/benchmark.c

+			sizeof(struct tee_ts_cpu_buf) *
+			bench_ts_global->cores);
+
+	mutex_unlock(&bench_reg_mu);


i think the lock does not need to be held during the RPC.

there is no any synchronization on that side (in the RPC handler in optee linux driver)

@etienne-lms

there is no any synchronization on that side (in the RPC handler in optee linux driver)

As soon as bench_ts_global is stored, it is safe from core to use it, no ?
Anyway, holding the mutex a bit longer is not really armful. ok like this.

I was trying to handle the case, when register_benchmark_memref is called twice (for the case, when someone has executed optee_benchmark app at the same time), so if don't to wrap rpc_reg_global_buf with mutex, it's possible to have a race condition in static void handle_rpc_func_cmd_bm_reg(struct optee_msg_arg *arg) L312

I think it's better to add condition in the beginning of register_benchmark_memref which will check if we have registered a buffer before, and then we can avoid such situation and unlock mutex before rpc call

igoropaniuk · 2017-05-19T11:57:56Z

Rebased on latest master and tested

jforissier · 2017-05-19T14:57:14Z

The commit description cannot be empty. Please explain what is meant by "benchmarking", how it is useful, how it is enabled (looks like it is always enabled? what if we want to reduce the size of the TEE? it is a debug feature after all, isn't it?), how one typically uses it, how it is implemented (high-level description, just to understand the interactions between the Secure/Non-secure kernel and user modes).

etienne-lms

Maybe add CFG_TEE_BENCHMARK ?= n with few explanations in mk/config.mk.

The current bench support is a bit touchy when more than 1 clients invokes the TEE while bench is being enabled. I've understood we can live with this constraint, but it should be stated somewhere.

Another tricky part: in SMP, a single invocation of the TEE can switch from 1 cpu to another resulting in the timestamp structures being stored in various 'core' related sub-buffers. Not easy to analyse. I think the client API should set the cpu affinity for the requested invocation when bench is enabled by client.

etienne-lms · 2017-05-19T14:59:59Z

core/arch/arm/pta/benchmark.c

+
+static TEE_Result invoke_command(void *pSessionContext __unused,
+		uint32_t nCommandID, uint32_t nParamTypes,
+		TEE_Param pParams[TEE_NUM_PARAMS])


style: prefer lowercase for variable.

etienne-lms · 2017-05-19T15:00:47Z

core/arch/arm/pta/benchmark.c

+		return;
+	}
+
+	/* fill timestamp data */


style: useless comment.

etienne-lms · 2017-05-19T15:02:36Z

core/arch/arm/tee/entry_std.c

 	res = copy_in_params(arg->params, num_params, &param);
+


remove this empty line.

etienne-lms · 2017-05-19T15:05:23Z

core/include/bench.h

+ */
+#define TEE_BENCH_DIVIDER		64
+
+/* max amount of timestamps per buffer */


style: start comment with a uppercase character.

igoropaniuk · 2017-05-19T15:18:45Z

@jforissier

The commit description cannot be empty. Please explain what is meant by "benchmarking", how it is useful, how it is enabled (looks like it is always enabled? what if we want to reduce the size of the TEE? it is a debug feature after all, isn't it?), how one typically uses it, how it is implemented (high-level description, just to understand the interactions between the Secure/Non-secure kernel and user modes).

I'm going to add some basic information to this commit, that I'll create additional PR with extended Markdown doc with detailed information about its implementation, usage etc. Deal? :)

jforissier · 2017-05-19T15:33:35Z

@igoropaniuk yep sounds good to me, thanks.

jforissier · 2017-05-19T15:35:14Z

BTW @igoropaniuk if you want you can create and push an empty commit (git commit --allow-empty) so that we can review the description.

igoropaniuk · 2017-05-19T15:43:52Z

@jforissier I've already amended current commit, and added description there (I hope it can be called a "description" :) )

igoropaniuk · 2017-05-22T10:25:18Z

@etienne-lms @jforissier
Any updates?

etienne-lms

Comment on commit comment:

(can be specified globally in build/common.mk)

Maybe remove this. This reference is out of optee_os scope.

etienne-lms · 2017-05-22T11:27:30Z

core/arch/arm/pta/benchmark.c

+			sizeof(struct tee_ts_cpu_buf) *
+			bench_ts_global->cores);
+
+	mutex_unlock(&bench_reg_mu);


As soon as bench_ts_global is stored, it is safe from core to use it, no ?
Anyway, holding the mutex a bit longer is not really armful. ok like this.

etienne-lms · 2017-05-22T11:29:00Z

core/arch/arm/pta/benchmark.c

+
+	res = rpc_reg_global_buf(OPTEE_MSG_RPC_CMD_BENCH_REG_DEL, 0, 0);
+
+	mutex_unlock(&bench_reg_mu);


same comment as : useless (from core) but armless mutex hold. ok like this.

igoropaniuk · 2017-05-22T14:25:46Z

@etienne-lms all comments addressed

igoropaniuk · 2017-05-23T10:42:31Z

I think I've addressed all comments so far and now think this is ready for a final review and if there are no more comments we should consider merging this now

igoropaniuk · 2017-05-29T14:03:57Z

@etienne-lms @jbech-linaro @jenswi-linaro @jforissier
Thanks for all your comments. Is there anything else worth mentioning/fixing?

jforissier · 2017-05-29T16:24:27Z

Abbreviating "benchmark" as "bench" sounds weird I think (bench.h, TEE_BENCH_* etc.) and is not done consistently, so it would be better to write "benchmark" always.
Anyway:
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

etienne-lms · 2017-05-30T07:15:14Z

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

OP-TEE Benchmark feature provides timestamp data for the roundtrip time from libteec to OP-TEE OS core. Benchmark PTA handles registration/unregistration commands of timestamp buffer, invoked by optee_benchmark NW application, and performs registration of timestamp buffer in the linux kernel optee driver via RPC call. To enable this feature set CFG_TEE_BENCHMARK compile flag to "y". Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>

igoropaniuk · 2017-05-30T12:28:00Z

Applied tags and rebased

igoropaniuk · 2017-05-30T16:13:14Z

@jforissier all these PRs are rebased on master, tags applied and ready for merge

linaro-swg/optee_benchmark#1
linaro-swg/linux#38
OP-TEE/build#124
#1365
OP-TEE/optee_client#79

thanks!

jforissier · 2017-05-30T16:15:47Z

All merged, thanks.

jbech-linaro · 2017-05-30T19:14:01Z

Thanks all reviewers and big thanks to you @igoropaniuk for implementing this and having the patience of reworking it a couple of times.

This was referenced Feb 22, 2017

OP-TEE Benchmark OP-TEE/optee_client#79

Merged

OP-TEE benchmark OP-TEE/build#124

Merged

OP-TEE Benchmark linaro-swg/linux#38

Merged

OP-TEE benchmark: initial benchmark app linaro-swg/optee_benchmark#1

Merged

igoropaniuk force-pushed the optee_benchmark branch 6 times, most recently from f283897 to 6b613c8 Compare February 22, 2017 20:27

etienne-lms reviewed Feb 27, 2017

View reviewed changes

jenswi-linaro reviewed Feb 27, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 6b613c8 to 8afc92f Compare April 9, 2017 16:47

igoropaniuk force-pushed the optee_benchmark branch from 0079d7b to 7cc94f0 Compare April 17, 2017 22:01

igoropaniuk changed the title ~~[RFC] OP-TEE Benchmark~~ OP-TEE Benchmark Apr 18, 2017

jenswi-linaro reviewed Apr 18, 2017

View reviewed changes

etienne-lms reviewed Apr 18, 2017

View reviewed changes

etienne-lms reviewed Apr 19, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 10e96b1 to 5e1556b Compare April 21, 2017 11:20

jbech-linaro reviewed Apr 26, 2017

View reviewed changes

etienne-lms reviewed Apr 26, 2017

View reviewed changes

etienne-lms reviewed May 2, 2017

View reviewed changes

etienne-lms reviewed May 5, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch 2 times, most recently from e89356b to 467fea7 Compare May 19, 2017 11:15

etienne-lms reviewed May 19, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 467fea7 to 155aa3b Compare May 19, 2017 15:17

igoropaniuk force-pushed the optee_benchmark branch from 155aa3b to e70ab0e Compare May 19, 2017 15:23

etienne-lms reviewed May 22, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from e70ab0e to 373534a Compare May 22, 2017 14:24

igoropaniuk force-pushed the optee_benchmark branch from 373534a to 43891c3 Compare May 22, 2017 14:46

igoropaniuk force-pushed the optee_benchmark branch from 43891c3 to 72f97fc Compare May 24, 2017 21:37

igoropaniuk force-pushed the optee_benchmark branch from 72f97fc to 6138e8a Compare May 30, 2017 12:13

jforissier merged commit d5b65f3 into OP-TEE:master May 30, 2017

igoropaniuk deleted the optee_benchmark branch July 4, 2017 17:11


		mutex_lock(&bench_reg_mu);

		/* we accept only non-secure buffers, as we later perform


		res = rpc_reg_global_buf(OPTEE_MSG_RPC_CMD_BENCH_REG_DEL, 0, 0);

		mutex_unlock(&bench_reg_mu);

OP-TEE Benchmark #1365

OP-TEE Benchmark #1365

Conversation

igoropaniuk commented Feb 22, 2017 • edited Loading

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoropaniuk Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoropaniuk Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoropaniuk Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoropaniuk commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoropaniuk Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbech-linaro Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbech-linaro commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igoropaniuk commented Feb 22, 2017 •

edited

Loading

igoropaniuk Apr 18, 2017 •

edited

Loading

igoropaniuk Apr 18, 2017 •

edited

Loading

igoropaniuk Apr 19, 2017 •

edited

Loading

igoropaniuk Apr 26, 2017 •

edited

Loading

jbech-linaro Apr 26, 2017 •

edited

Loading