Split the optee image into three images to support optee pager in arm-tf #1589

Summer-ARM · 2017-06-09T11:14:43Z

In order to adapt to the optee pager supporting, it needs to split the
original tee.bin into three images, that is tee-header-v2.bin,
tee-pager-v2.bin and tee-pageable-v2.bin.

Change-Id: I8bff3d3aa7b03a39cda21b28e1b1a1e624675ee6
Signed-off-by: Summer Qin summer.qin@arm.com

jforissier · 2017-06-09T11:20:52Z

core/arch/arm/kernel/link.mk

@@ -219,6 +219,9 @@ $(link-out-dir)/tee.bin: $(link-out-dir)/tee-pager.bin \
 		--init_mem_usage `cat $(link-out-dir)/tee-init_mem_usage.txt` \
 		--tee_pager_bin $(link-out-dir)/tee-pager.bin \
 		--tee_pageable_bin $(link-out-dir)/tee-pageable.bin \
+		--tee_header_v2_out $(link-out-dir)/tee-header-v2.bin\
+		--tee_pager_v2_out $(link-out-dir)/tee-pager-v2.bin\
+		--tee_pageable_v2_out $(link-out-dir)/tee-pageable-v2.bin\


Please add a space before the backslashes

*-v2.bin are output files, so:

they should appear as targets somehow

they should be added to $(cleanfiles)

they should be added as prerequisites of all:

I think it's best to invoke gen_hashed_bin.py once per output file. All sorts of problems may arise otherwise, i.e., when a command generates several output files (for instance, make -jN).

So here is how I would write the rules:

gen_hash_bin_deps := $(link-out-dir)/tee-pager.bin \ $(link-out-dir)/tee-pageable.bin \ $(link-out-dir)/tee-init_size.txt \ $(link-out-dir)/tee-init_load_addr.txt \ $(link-out-dir)/tee-init_mem_usage.txt \ ./scripts/gen_hashed_bin.py define gen_hash_bin_cmd @$(cmd-echo-silent) ' GEN $@' $(q)load_addr=`cat $(link-out-dir)/tee-init_load_addr.txt` && \ ./scripts/gen_hashed_bin.py \ --arch $(if $(filter y,$(CFG_ARM64_core)),arm64,arm32) \ --init_size `cat $(link-out-dir)/tee-init_size.txt` \ --init_load_addr_hi $$(($$load_addr >> 32 & 0xffffffff)) \ --init_load_addr_lo $$(($$load_addr & 0xffffffff)) \ --init_mem_usage `cat $(link-out-dir)/tee-init_mem_usage.txt` \ --tee_pager_bin $(link-out-dir)/tee-pager.bin \ --tee_pageable_bin $(link-out-dir)/tee-pageable.bin endef all: $(link-out-dir)/tee.bin cleanfiles += $(link-out-dir)/tee.bin $(link-out-dir)/tee.bin: $(gen_hashed_bin_deps) $(gen_hash_bin_cmd) --out $@ all: $(link-out-dir)/tee-header-v2.bin cleanfiles += $(link-out-dir)/tee-header-v2.bin $(link-out-dir)/tee-header-v2.bin: $(gen_hashed_bin_deps) $(gen_hash_bin_cmd) --tee_header_v2_out $@ # And same for the two other v2 binaries

jforissier · 2017-06-09T11:44:47Z

scripts/gen_hashed_bin.py

@@ -120,6 +125,18 @@ def get_args():
 		required=True, type=argparse.FileType('wb'), \
 		help='The output tee.bin')

+	parser.add_argument('--tee_header_v2_out', \
+		required=True, type=argparse.FileType('wb'), \


Obviously, if we have several output rules as recommended above, no output parameter should be mandatory.

jforissier · 2017-06-09T11:46:20Z

scripts/gen_hashed_bin.py

+	tee_pager_v2_outf  = args.tee_pager_v2_out
+	append_to(tee_pager_v2_outf, 0, tee_pager_fname)
+        append_to(tee_pager_v2_outf, 0, tee_pageable_fname, init_bin_size)
+        append_hashes(tee_pager_v2_outf, tee_pageable_fname)


Leading spaces should be tabs

jforissier · 2017-06-09T11:53:04Z

I find the commit description a bit unclear. Why is this needed? You have to say it is to support a new format introduced in upstream ARM-TF or something. And please, no Change-Id: in the description.

A quick note about our usual way of doing reviews on pull requests: you may address review comments as incremental patches that you push to this PR (it's easier to review), then address the commit message issue in the end when everyone is happy with the code. At this point you squash all commits together and update the description. Then I merge it into master.

Thanks!

Summer-ARM · 2017-06-09T12:36:36Z

Hi Jforissier,
Thanks for your detail review; I will update it soon and try to update one.
BTW, can you help to delete a closed branch ?
https://github.com/OP-TEE/optee_os/pull/1587
It is over committed by mistake and may cause serious internal checking about this...
Maybe this patch will gone since they are using same branch name; I just collected all your comments and it is OK for me...

Thanks in advance...

-Summer

jforissier · 2017-06-09T12:53:41Z

About 1587 - no, I can't delete the pull request, even with admin rights to the repository. Are you concerned with some code that you pushed that should not have been published? If that is the reason why you would like the PR deleted, I can certainly understand. I believe only GitHub support can delete a PR, but I can ask them to do so.
However if there is no compelling reason for deleting the PR, I'd rather let it alone (in the closed state).

Please let me know if I should ask GitHub support for help on this matter.

Summer-ARM · 2017-06-09T15:19:12Z

Yes, you are right; I concerned with some code should not have been published.

I just googled the answer:
https://github.com/blog/1335-tidying-up-after-pull-requests
so I asked for your help...

Please help to ask support on this, thanks...

jforissier · 2017-06-09T15:48:16Z

OK

Summer-ARM · 2017-06-10T07:48:22Z

Just checked previous pull request was gone.
Thank you very much; and I will update a new patch soon. :)

Summer-ARM · 2017-06-19T08:50:07Z

Hi @jforissier
I have pushed the second patch according your kind comments.
But I don't know why the travls-CI always failed : (
Locally, I delete some tests in the travis.yml, it can succss.

Thanks.

etienne-lms · 2017-06-19T09:03:15Z

Hi all,
I would rather have the same script tool generating the legacy and the "new" image files, based on arguments, as in previous proposal. The legacy and new format are almost the same: only the header changes and that binary blobs are appended in the same file or stored in specific files. So it would be better to have the same script sequence to generate the binaries.

jforissier

Thanks @Summer-ARM. Please see my comments below. With them addressed, I'm happy with how this looks. But I'd like Acks from @etienne-lms and @jenswi-linaro before merging.

jforissier · 2017-06-19T09:02:49Z

core/arch/arm/kernel/link.mk

+$(link-out-dir)/tee-header-v2.bin: $(link-out-dir)/tee.bin
+	./scripts/split_hashed_bin.py \
+		--in_file $< \
+		--out_header_v2 $@


Missing $(q)

Minor: could we get rid of --in_file? That is: $(q)./scripts/split_hashed_bin.py --out_header_v2 $@ $<

jforissier · 2017-06-19T09:07:49Z

core/arch/arm/kernel/link.mk

@@ -221,6 +221,26 @@ $(link-out-dir)/tee.bin: $(link-out-dir)/tee-pager.bin \
 		--tee_pageable_bin $(link-out-dir)/tee-pageable.bin \
 		--out $@

+all: $(link-out-dir)/tee-header-v2.bin
+cleanfiles += $(link-out-dir)/tee-header-v2.bin
+$(link-out-dir)/tee-header-v2.bin: $(link-out-dir)/tee.bin


Missing:

@$(cmd-echo-silent) ' GEN $@'

I'd prefer if this wasn't generated from tee.bin since that makes it hard to get rid tee.bin once we've switched to this new format. Generating from tee.bin also has the drawback that there's yet another step where you need to double check when debugging errors related to the generated binary.

@jenswi-linaro good points indeed

Hello, let me explain it; and please give comments :)
We make tee.bin as input and created another scripts due to:

init_size for 'tee_pager' in script actually depends on files size of tee.bin in gen_hashed_bin.py; or we need to add more variables to calculate it size out while we generating other binaries (while generating these binaries tee.bin is not there so we need calculation);

Too much modification in .py and link.mk will make it looks quite a mess? We want to make the difference tiny :)

We will update another version which uses one .py file for final result and let's compare the difference?

Yes, please make such a version. Once this new format is in place and established I see no reason to keep the old for very long.

jforissier · 2017-06-19T09:19:04Z

@etienne-lms I mostly agree with you, that was my initial expectation. Although, I don't really mind having a separate script.

etienne-lms · 2017-06-19T09:27:51Z

@jforissier, if you are fine with the dual scripts, then ok. It's a matter of maintenance... and you're the maintainer!

jforissier

Looks good, a few comments about the documentation.

jforissier · 2017-06-20T13:36:12Z

documentation/optee_design.md

+(`pager` in general) gets launched after all images loaded.
+Header image structure is updated to support separate loading:
+```c
+typedef struct optee_image {


We don't like typedefs very much (see https://www.kernel.org/doc/html/v4.10/process/coding-style.html#typedefs). Please drop it and write struct optee_image optee_image[]; below.

jforissier · 2017-06-20T13:37:42Z

documentation/optee_design.md

+        uint8_t version;
+        uint8_t arch;
+        uint16_t flags;
+	uint32_t nb_images;


You're mixing spaces and tabs => use spaces everywhere

jforissier · 2017-06-20T13:38:22Z

documentation/optee_design.md

@@ -451,6 +451,36 @@ OP-TEE to be able to initialize at all. The loader supplies in `r0/x0` the
 address of the first byte following what was not copied and jumps to the load
 address to start OP-TEE.

+Extra image files are generated by loaders who support loading separate images.


s/by/for/?

jforissier · 2017-06-20T13:42:31Z

scripts/gen_hashed_bin.py

+			     hashlib.sha256().digest_size
+
+	if paged_input_size % (4 * 1024) != 0:
+		print("Error: pageable size not aligned" + \


"Error: pageable size not a multiple of 4K: "

jforissier · 2017-06-20T13:46:29Z

documentation/optee_design.md

+
+Magic number and architecture are identical as original. Version is increased
+to 2. `load_addr_hi` and `load_addr_lo` may be ZERO for pageable image since
+pageable part may get loaded by loader into dynamic available position.


What about image_id? It is not documented.

jforissier · 2017-06-20T13:54:34Z

documentation/optee_design.md

+image, pager image and pageable image. Loaders load header image first to get
+image list and information of each image; and then load each of them into
+different load address assigned in structure; finally main image binary
+(`pager` in general) gets launched after all images loaded.


Is the "main image" always the first one? (i.e., optee_header_v2:optee_image[0]). Or does it depend on image_id?

I think maybe we don't need to distinguish the main image and secondary image. It will confuse the reader. So I will delete this sentence.

jenswi-linaro · 2017-06-21T10:02:20Z

scripts/gen_hashed_bin.py

-	write_header(outf, init_size, args, paged_size)
-
-	outf.close()
+	header_v1_size     = get_header_v1_size();


Please move this line below if args.out is not None:

jenswi-linaro · 2017-06-21T10:02:51Z

scripts/gen_hashed_bin.py

+
+	if args.out is not None:
+		outf = args.out
+		write_header_v2(outf, 0, args, 0)


Shouldn't this be a call to write_header_v1()?

jenswi-linaro · 2017-06-21T10:50:37Z

scripts/gen_hashed_bin.py

+def get_header_v1_size():
+	return struct.calcsize('<IBBHIIIII')
+
+def write_header_v2(outf, init_size, args,paged_size):


Please add a space before paged_size

jenswi-linaro · 2017-06-21T10:52:40Z

scripts/gen_hashed_bin.py

+			repr(paged_input_size))
+		sys.exit(1)
+
+	init_size	   = pager_input_size


These two assignments are redundant.

Can you address this comment from Jens ?

My fault: discard my comment....

Maybe it could be more clear to simply compute:

init_size = pager_input_size + min(init_bin_size, paged_input_size) + hash_size paged_size = paged_input_size - min(init_bin_size, paged_input_size)

Sure; refine done.

jenswi-linaro · 2017-06-21T10:56:23Z

scripts/gen_hashed_bin.py

+	init_size	   = pager_input_size
+	paged_size	   = paged_input_size
+
+	init_size	   = min(init_bin_size, paged_input_size) + hash_size


These two variables are later reassigned in the if args.out is not None: below, is that intentional?

etienne-lms · 2017-06-22T12:54:55Z

documentation/optee_design.md

+```
+
+Magic number and architecture are identical as original. Version is increased
+to 2. `load_addr_hi` and `load_addr_lo` may be ZERO for pageable image since


I would rather use an all-ones value for unspecified load addresses, in case, some day, the null physical address become a valid address in optee (which is not the current case).

etienne-lms · 2017-06-22T12:57:14Z

documentation/optee_design.md

@@ -451,6 +451,36 @@ OP-TEE to be able to initialize at all. The loader supplies in `r0/x0` the
 address of the first byte following what was not copied and jumps to the load
 address to start OP-TEE.

+Extra image files are generated for loaders who support loading separate images.
+Legacy OP-TEE image tee.bin is split into several v2 images; typically header


The sentence "Legacy OP-TEE image tee.bin is split into several v2 images." sounds strange to me.
The full section "Partitioning of the binary" could be split into 2 sub-sections:, i.e "Generated binary image v1" and "Binary image v2", each independently providing it own description, even if some description will be redundant.

A comment out of scope of the current P-R: This section is part of the "Pager" section. Maybe we should move it to a dedicated section outside the "Pager" description and add a note stating that when pager is used, optee core is made of several images.

We put it here just after original header to show difference; how about the section movement into another PR ?

The word 'split' is wrong since we have move back to one script file; will refine this.

etienne-lms · 2017-06-23T07:47:50Z

On 23 June 2017 at 04:15, Ken Liu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In documentation/optee_design.md <#1589 (comment)>: > @@ -451,6 +451,36 @@ OP-TEE to be able to initialize at all. The loader supplies in `r0/x0` the address of the first byte following what was not copied and jumps to the load address to start OP-TEE. +Extra image files are generated for loaders who support loading separate images. +Legacy OP-TEE image tee.bin is split into several v2 images; typically header We put it here just after original header to show difference; how about the section movement into another PR ?

Agree.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATEHJkORE3dkj1LRYcBvWsKg1oZ89ezuks5sGx_CgaJpZM4N1OJ0> .

etienne-lms · 2017-06-23T08:10:18Z

scripts/gen_hashed_bin.py

+	outf.write(struct.pack('<IBBHI', \
+		magic, version, arch_id[args.arch], args.flags, nb_images))
+	outf.write(struct.pack('<IIII', \
+		args.init_load_addr_hi, args.init_load_addr_lo, 0, init_size))


I would rather replace magic value 0 (and 1 below) with explicit IDs, whatever they being 0 or not.

etienne-lms · 2017-06-23T08:11:35Z

documentation/optee_design.md

+Magic number and architecture are identical as original. Version is increased
+to 2. `load_addr_hi` and `load_addr_lo` may be 0xFFFFFFFF for pageable image
+since pageable part may get loaded by loader into dynamic available position.
+`image_id` shows the order of the current image in the `optee_image[]` array.


Is the image_id really about ordering? I though it would be an ID that specifies how loader shall handle the image, whatever ordering is used (even if build will always place images in the same order). This would leave flexibility for future evolution without break compatibility.
As for now, 0 would mean "optee bootable image" and 1 means "optee pagestore image".

Yes, you are right, it should be something like 'type'. Will update.

etienne-lms · 2017-06-23T08:14:25Z

documentation/optee_design.md

+| Pageable |
+----------+
+```
+In this case, loaders load header images first to get image list and information


typo: "header images first" (singular)

kenlsoft · 2017-06-27T02:38:45Z

Summer, please refine commit msg and remove change-id from commit msg.

kenlsoft · 2017-06-27T13:44:38Z

Oops, time out in CI again....

@jforissier @etienne-lms @jenswi-linaro Hello guys, can you help to take look at latest patches and give comments? I and Summer collect all review in one patch... we are trying to merge before code freeze so that next arm-tf release would have full function support...

etienne-lms

I am fine with these changes and the proposed header structure.

etienne-lms · 2017-06-27T14:15:48Z

scripts/gen_hashed_bin.py

+			repr(paged_input_size))
+		sys.exit(1)
+
+	init_size	   = pager_input_size


Can you address this comment from Jens ?

kenlsoft · 2017-06-27T14:27:52Z

I am sorry but do you mean “init_size = pager_input_size” ？

It is used for calculating pager part size (pager + init_size + hash sizes) by these four lines:
init_size = pager_input_size
paged_size = paged_input_size
init_size += min(init_bin_size, paged_input_size) + hash_size
paged_size -= min(init_bin_size, paged_input_size)

I guess Jens comment on this due to in our last commit we missed += and -= in following two lines; so Jens thought we assigned 'init_size' with twice times and he thought it is redundant.

jforissier · 2017-06-28T06:17:56Z

How can this be tested? Do you have a branch of arm-trusted-firmware that supports this new binary format for HiKey or QEMU?

david-wang-2015 · 2017-06-28T10:32:43Z

@jforissier , the patches in arm-tf for paging/header support are in review by arm-tf team (not in Github). We expect these can be visible in 2-3 weeks in Github. :)
We did the tests on Juno board which actually runs pager/paged in DRAM. It works fine.

jforissier · 2017-06-28T11:01:48Z

@davwan01 okay, thanks.
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

vchong · 2017-06-28T11:16:35Z

@davwan01 Does this mean things will not work if we use this patch with arm-tf master/integration branch today on github?

etienne-lms · 2017-06-28T12:50:28Z

Acked-by: Etienne Carriere <etienne.carriere@linaro.org> (thanks @jforissier for mastering those python stuff).

jenswi-linaro · 2017-06-28T14:16:58Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

david-wang-2015 · 2017-06-28T15:13:46Z

@vchong The paging won't work with the master branch of arm-tf at the moment. But it won't (shouldn't) break any non-paging function.

Generate three binaries tee-header_v2.bin, tee-pager_v2.bin and tee-pageable_v2.bin for loaders supporting separate binary loading. This kind of loader loads and parses header binary first and then loads rest two binaries under specified manners header information implies. Generic loaders who don't support separate binary loading just ignore these binaries; and this change will not affect all existing design. Signed-off-by: Summer Qin <summer.qin@arm.com> Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jforissier · 2017-06-28T17:00:53Z

Thanks @Summer-ARM @davwan01.

vchong · 2017-07-10T09:46:17Z

@davwan01 It looks like the recent arm-tf v1.4 release doesn't yet support the parsing of this, right? Just want to confirm.

jforissier reviewed Jun 9, 2017

View reviewed changes

jforissier reviewed Jun 19, 2017

View reviewed changes

jforissier reviewed Jun 20, 2017

View reviewed changes

jenswi-linaro reviewed Jun 21, 2017

View reviewed changes

etienne-lms reviewed Jun 22, 2017

View reviewed changes

etienne-lms reviewed Jun 23, 2017

View reviewed changes

Summer-ARM force-pushed the sq/optee-header branch from c3a8915 to e045854 Compare June 27, 2017 12:46

etienne-lms reviewed Jun 27, 2017

View reviewed changes

Summer-ARM force-pushed the sq/optee-header branch from 3ae4112 to 3c1d40f Compare June 28, 2017 16:23

Summer-ARM force-pushed the sq/optee-header branch from 3c1d40f to 6eecea5 Compare June 28, 2017 16:29

jforissier merged commit ac3cc6c into OP-TEE:master Jun 28, 2017

Split the optee image into three images to support optee pager in arm-tf #1589

Split the optee image into three images to support optee pager in arm-tf #1589

Conversation

Summer-ARM commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Jun 9, 2017

Summer-ARM commented Jun 9, 2017

jforissier commented Jun 9, 2017

Summer-ARM commented Jun 9, 2017

jforissier commented Jun 9, 2017

Summer-ARM commented Jun 10, 2017

Summer-ARM commented Jun 19, 2017

etienne-lms commented Jun 19, 2017

jforissier 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

kenlsoft Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Jun 19, 2017

etienne-lms commented Jun 19, 2017

jforissier 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

jforissier Jun 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Jun 27, 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

etienne-lms commented Jun 23, 2017 via email

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

kenlsoft commented Jun 27, 2017

kenlsoft commented Jun 27, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenlsoft commented Jun 27, 2017

jforissier commented Jun 28, 2017

david-wang-2015 commented Jun 28, 2017

jforissier commented Jun 28, 2017

vchong commented Jun 28, 2017 • edited Loading

etienne-lms commented Jun 28, 2017

jenswi-linaro commented Jun 28, 2017

david-wang-2015 commented Jun 28, 2017

jforissier commented Jun 28, 2017

vchong commented Jul 10, 2017

kenlsoft Jun 19, 2017 •

edited

Loading

jforissier Jun 20, 2017 •

edited

Loading

etienne-lms Jun 27, 2017 •

edited

Loading

vchong commented Jun 28, 2017 •

edited

Loading