ftrace: Add 32 bit apps [and function execution time support -- discarded] #3080

b49020 · 2019-06-12T11:09:54Z

This patch-set adds ftrace support for 32 bit apps as well as function execution time. So after this patch-set function graph would look something like:

              | __ta_entry() {
              |  __utee_entry() {
     1.168 us |   ta_header_get_session();
     1.392 us |   __utee_to_param();
              |   TA_InvokeCommandEntryPoint() {
              |    ta_entry_params_access_rights() {
    31.392 us |     TEE_CheckMemoryAccessRights();
    28.176 us |     TEE_CheckMemoryAccessRights();
   173.984 us |    }
   202.448 us |   }
     1.152 us |   __utee_from_param();
              |   ta_header_save_params() {
      .656 us |    memset();
     5.488 us |   }
   275.824 us |  }
   281.424 us | }

Looking forward to your valuable feedback/comments.

Regards,
Sumit

jforissier · 2019-06-12T11:23:19Z

That is quite cool 👍

I'd like to make sure I understand correctly the meaning of the time values. From what I can tell, each value is placed at a function exit point and indicates the elapsed time between the entry and the exit of that particular function.
For instance, in the example above:

ta_header_get_session() took 1.168 us
TA_InvokeCommandEntryPoint() took 202.448 us
__ta_entry() took 281.424 us

Is this correct?

I'm not sure the time values can be called timestamps, they are actually deltas (elapsed time).

b49020 · 2019-06-12T11:41:14Z

Is this correct?

Yes.

I'm not sure the time values can be called timestamps, they are actually deltas (elapsed time).

Agree. Timestamps are basically used to calculate these deltas. So should I use deltas or elapsed time in commit messages? Or something like function execution time?

jbech-linaro · 2019-06-12T11:41:44Z

I'd like to make sure I understand correctly the meaning of the time values. From what I can tell, each value is placed at a function exit point and indicates the elapsed time between the entry and the exit of that particular function.
For instance, in the example above:

ta_header_get_session() took 1.168 us

TA_InvokeCommandEntryPoint() took 202.448 us

__ta_entry() took 281.424 us

Is this correct?

I'm not sure the time values can be called timestamps, they are actually deltas (elapsed time).

I guess it mimics the function tracer in Linux and they simply say "measure of a function’s time execution". So, change commit message to "... enable function runtime statistics" or something like that?

jforissier · 2019-06-12T11:43:02Z

"Execution time" sounds good (to me at least!)

b49020 · 2019-06-12T11:45:31Z

I guess it mimics the function tracer in Linux and they simply say "measure of a function’s time execution".

Yeah I have tried to keep this implementation on similar lines as Linux function tracer.

b49020 · 2019-06-12T11:46:43Z

"Execution time" sounds good (to me at least!)

Ok then, will use it instead.

b49020 · 2019-06-12T11:56:55Z

Updated.

jforissier

Reviewing "ftrace: Enable support for 32 bit apps".

core/arch/arm/arm.mk

lib/libutils/isoc/arch/arm/sub.mk

lib/libutils/isoc/include/setjmp.h

ta/arch/arm/user_ta_header.c

b49020 · 2019-06-17T06:33:24Z

Addressed comments.

jforissier

For commits "ftrace: Enable support for 32 bit apps" and "core: ftrace: Enable user-space access to counter regs":
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Why is "ftrace: Add 32 bit apps and function execution time support" not squashed with "libutee: ftrace: Add function execution time support" (and libutee: removed from the subject)? Without that, the first commit does not work as expected it seems.

core/arch/arm/kernel/ftrace.c

jforissier · 2019-06-17T07:28:49Z

lib/libutee/arch/arm/utee_mcount_a32.S

@@ -57,6 +57,16 @@ FUNC __ftrace_return, :
 	ldmia		sp!, {r0-r3}
 	bx		lr
 END_FUNC __ftrace_return
+
+FUNC read_cntpct, :


This is arguably not the best place to have these functions, because they could be useful outside of ftrace. So they could have their own CFG_ and be moved somewhere else.
Not a big deal however, could be done later.

They are already generated and defined in <out-dir>/core/include/generated/arm32_sysreg.h and as assembly macros in arm32_sysreg.S

@jenswi-linaro excellent 👍

@jenswi-linaro These seems to used for core build only and not exported for TA library builds. I get following error when I tried to use them:

AS out/arm/ta_arm32-lib/libutee/arch/arm/utee_mcount_a32.o lib/libutee/arch/arm/utee_mcount_a32.S:7:10: fatal error: generated/arm32_sysreg.S: No such file or directory #include <generated/arm32_sysreg.S> ^~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. mk/compile.mk:147: recipe for target 'out/arm/ta_arm32-lib/libutee/arch/arm/utee_mcount_a32.o' failed make[1]: *** [out/arm/ta_arm32-lib/libutee/arch/arm/utee_mcount_a32.o] Error 1 make[1]: Leaving directory '/home/sumit/build/optee/optee_os' common.mk:396: recipe for target 'optee-os-common' failed make: *** [optee-os-common] Error 2

Also, similar is the case with arm64.

Yes, perhaps we can generate a subset with the registers which can be accessed from EL0 into <out>/include/generated/arm32_user_sysreg.h. One way could be to copy the relevant lines from core/arch/arm/kernel/arm32_sysreg.txt.

For arm64 it's a bit easier due to the convenient assembly mnemonic, there we can just hand code all the EL0 accessible registers in lib/libutee/include/arm64_user_sysreg.h.

Ok, let me try to create a sysreg subset for user access.

b49020 · 2019-06-17T07:57:28Z

Why is "ftrace: Add 32 bit apps and function execution time support" not squashed with "libutee: ftrace: Add function execution time support" (and libutee: removed from the subject)? Without that, the first commit does not work as expected it seems.

I guess you meant core: ftrace: exclude TA suspend time from timestamps instead of ftrace: Add 32 bit apps and function execution time support.

And yes they could be squashed together. The only reason I avoided squash was to ease the review process. So is it fine if I squash after review is complete?

jforissier · 2019-06-17T08:01:59Z

I guess you meant [...]

Correct, bad copy/paste sorry :-/

core/arch/arm/arm.mk

By default 32 bit trusted applications are compiled in thumb mode but thumb mode doesn't support function graph tracing due to missing frame pointer support required to trace function call chain. So rather compile trusted applications in ARM mode in case function tracing is enabled. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

b49020 · 2019-07-01T13:13:55Z

@jforissier @jbech-linaro @jenswi-linaro Currently I have just rebased "ftrace: Enable support for 32 bit apps" on top of master after ldelf PR merge. Is it fine with you to incorporate this patch only for OP-TEE 3.6.0 release? I think it completes ftrace feature.

Regarding timestamp support, I am rebasing stuff for which I can create a separate PR.

jbech-linaro · 2019-07-01T13:16:25Z

Is it fine with you to incorporate this patch only for OP-TEE 3.6.0 release?

OK for me.

jforissier · 2019-07-01T15:15:52Z

@b49020 fine with me, too. I will merge this patch now and let's proceed with the timestamp stuff in another PR.

b49020 changed the title ~~ftrace: Add 32 bit apps and timestamps support~~ ftrace: Add 32 bit apps and function execution time support Jun 12, 2019

b49020 force-pushed the master branch from b8f44ce to a271c99 Compare June 12, 2019 11:56

jforissier reviewed Jun 12, 2019

View reviewed changes

core/arch/arm/arm.mk Outdated Show resolved Hide resolved

lib/libutils/isoc/arch/arm/sub.mk Show resolved Hide resolved

lib/libutils/isoc/include/setjmp.h Show resolved Hide resolved

ta/arch/arm/user_ta_header.c Show resolved Hide resolved

jforissier reviewed Jun 17, 2019

View reviewed changes

jenswi-linaro reviewed Jun 18, 2019

View reviewed changes

core/arch/arm/arm.mk Outdated Show resolved Hide resolved

b49020 mentioned this pull request Jun 27, 2019

Load TAs using a user space ELF loader #3036

Merged

b49020 force-pushed the master branch from cc6d943 to 07eba44 Compare July 1, 2019 12:51

jforissier merged commit c96d709 into OP-TEE:master Jul 1, 2019

jforissier changed the title ~~ftrace: Add 32 bit apps and function execution time support~~ ftrace: Add 32 bit apps [and function execution time support -- discarded] Jul 1, 2019

b49020 mentioned this pull request Jul 4, 2019

ftrace: Add function execution time support #3117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ftrace: Add 32 bit apps [and function execution time support -- discarded] #3080

ftrace: Add 32 bit apps [and function execution time support -- discarded] #3080

b49020 commented Jun 12, 2019 •

edited

Loading

jforissier commented Jun 12, 2019

b49020 commented Jun 12, 2019

jbech-linaro commented Jun 12, 2019

jforissier commented Jun 12, 2019

b49020 commented Jun 12, 2019

b49020 commented Jun 12, 2019

b49020 commented Jun 12, 2019

jforissier left a comment

b49020 commented Jun 17, 2019

jforissier left a comment

jforissier Jun 17, 2019

b49020 Jun 17, 2019

jenswi-linaro Jun 18, 2019

jforissier Jun 18, 2019

b49020 Jun 19, 2019 •

edited

Loading

jenswi-linaro Jun 19, 2019

b49020 Jun 19, 2019

b49020 commented Jun 17, 2019

jforissier commented Jun 17, 2019

b49020 commented Jul 1, 2019

jbech-linaro commented Jul 1, 2019

jforissier commented Jul 1, 2019

ftrace: Add 32 bit apps [and function execution time support -- discarded] #3080

ftrace: Add 32 bit apps [and function execution time support -- discarded] #3080

Conversation

b49020 commented Jun 12, 2019 • edited Loading

jforissier commented Jun 12, 2019

b49020 commented Jun 12, 2019

jbech-linaro commented Jun 12, 2019

jforissier commented Jun 12, 2019

b49020 commented Jun 12, 2019

b49020 commented Jun 12, 2019

b49020 commented Jun 12, 2019

jforissier left a comment

Choose a reason for hiding this comment

b49020 commented Jun 17, 2019

jforissier left a comment

Choose a reason for hiding this comment

jforissier Jun 17, 2019

Choose a reason for hiding this comment

b49020 Jun 17, 2019

Choose a reason for hiding this comment

jenswi-linaro Jun 18, 2019

Choose a reason for hiding this comment

jforissier Jun 18, 2019

Choose a reason for hiding this comment

b49020 Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

jenswi-linaro Jun 19, 2019

Choose a reason for hiding this comment

b49020 Jun 19, 2019

Choose a reason for hiding this comment

b49020 commented Jun 17, 2019

jforissier commented Jun 17, 2019

b49020 commented Jul 1, 2019

jbech-linaro commented Jul 1, 2019

jforissier commented Jul 1, 2019

b49020 commented Jun 12, 2019 •

edited

Loading

b49020 Jun 19, 2019 •

edited

Loading