PSA tools: Find secure image at post-build #9894

mikisch81 · 2019-02-28T20:04:15Z

Description

Add a common function which finds the relevant secure image of a non-secure target.
This function will be called during the post-build hook which merges the secure and non-secure images together.

The logic was taken from the PSOC6 post-build hook

Tested with Musca_a1.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@theotherjimmy @orenc17

Release Notes

ciarmcom · 2019-02-28T22:00:41Z

@mikisch81, thank you for your changes.
@orenc17 @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

theotherjimmy · 2019-03-01T14:55:30Z

tools/psa/__init__.py

+    secure_image = next((f for f in image_files if basename(f) == basename(ns_image_path)), secure_image)
+
+    if secure_image:
+        toolchain.notify.debug("Secure image file found: %s." % secure_image)


Resources has a notifier too, and if you're only using the notifier from the toolchain, pass in the notifier please.

Well it didn't work for resources notifier:

Traceback (most recent call last): File "C:\miki_dev\mbed-os_backup\tools\test_api.py", line 2210, in build_test_worker bin_file, _ = build_project(*args, **kwargs) File "C:\miki_dev\mbed-os_backup\tools\build_api.py", line 584, in build_project res, _ = toolchain.link_program(resources, build_path, name) File "C:\miki_dev\mbed-os_backup\tools\toolchains\__init__.py", line 654, in link_program self.binary(r, elf, bin) File "C:\miki_dev\mbed-os_backup\tools\hooks.py", line 54, in wrapper post_res = tooldesc["post"](t_self, *args, **kwargs) File "C:\miki_dev\mbed-os_backup\tools\targets\__init__.py", line 604, in binary_hook secure_bin = find_secure_image(t_self, resources, binf, configured_secure_image_filename, FileType.BIN) File "C:\miki_dev\mbed-os_backup\tools\psa\__init__.py", line 12, in find_secure_image resources.notify.debug("Secure image file found: %s." % secure_image) AttributeError: 'Resources' object has no attribute 'notify'

Probably private then. Pass in a notifier instead.

theotherjimmy · 2019-03-01T14:55:37Z

tools/psa/__init__.py

+    if secure_image:
+        toolchain.notify.debug("Secure image file found: %s." % secure_image)
+    else:
+        toolchain.notify.debug("Secure image file %s not found. Aborting." % configured_s_image_path)


Resources has a notifier too, and if you're only using the notifier from the toolchain, pass in the notifier please.

theotherjimmy · 2019-03-01T14:56:16Z

@mikisch81 I'm not seeing a change that calls this new function. How does this get invoked?

mikisch81 · 2019-03-01T15:51:24Z

@mikisch81 I'm not seeing a change that calls this new function. How does this get invoked?

By the musca post-build hook

This is currently in a branch and will be added to #9221

theotherjimmy · 2019-03-01T17:09:02Z

@mikisch81 That code is not on master or in the diff, so this looks like you're adding dead code, because from the perspective of this branch you are adding dead code. Could you include the modification to the post-build hook too?

mikisch81 · 2019-03-01T17:26:53Z

Do you suggest adding the musca post build hook here?

theotherjimmy · 2019-03-01T20:35:11Z

@mikisch81 That could work, and I would be alright with that. but:

The logic was taken from the PSOC6 post-build hook

So why is the PSOC6 post-build hook not refactored to use this?

theotherjimmy

Notifier changes requested offline.

theotherjimmy · 2019-03-01T20:39:03Z

tools/psa/__init__.py

+    secure_image = next((f for f in image_files if basename(f) == basename(ns_image_path)), secure_image)
+
+    if secure_image:
+        toolchain.notify.debug("Secure image file found: %s." % secure_image)


Suggested change

toolchain.notify.debug("Secure image file found: %s." % secure_image)

notify.debug("Secure image file found: %s." % secure_image)

theotherjimmy · 2019-03-01T20:39:13Z

tools/psa/__init__.py

+    if secure_image:
+        toolchain.notify.debug("Secure image file found: %s." % secure_image)
+    else:
+        toolchain.notify.debug("Secure image file %s not found. Aborting." % configured_s_image_path)


Suggested change

toolchain.notify.debug("Secure image file %s not found. Aborting." % configured_s_image_path)

notify.debug("Secure image file %s not found. Aborting." % configured_s_image_path)

theotherjimmy · 2019-03-01T20:39:29Z

tools/psa/__init__.py

+
+
+# Find secure image.
+def find_secure_image(toolchain, resources, ns_image_path, configured_s_image_path, image_type):


Suggested change

def find_secure_image(toolchain, resources, ns_image_path, configured_s_image_path, image_type):

def find_secure_image(notify, resources, ns_image_path, configured_s_image_path, image_type):

theotherjimmy · 2019-03-01T20:43:26Z

tools/psa/__init__.py

+from os.path import basename
+
+
+# Find secure image.


Could you change this into a doc comment? You would place this in a string as the first statement inside the body of the function.

E.G.

def find_secure_image(toolchain, resources, ns_image_path, configured_s_image_path, image_type): """ Find secure image. """

orenc17 · 2019-03-01T20:43:48Z

@mikisch81 That could work, and I would be alright with that. but:

The logic was taken from the PSOC6 post-build hook

So why is the PSOC6 post-build hook not refactored to use this?

@theotherjimmy
Psoc6 postbuild hook is being used by a variety of platforms some support PSA some not
This change comes to provide a unified solution for PSA targets only

theotherjimmy · 2019-03-01T20:45:16Z

@orenc17 That does not make sense. If the PSOC6 has the same logic within it, then replacing that logic with this logic works regardless of PSA or any other three letter acronym you care to use.

theotherjimmy · 2019-03-02T00:28:46Z

My tone on my comments has been rather harsh. My apologies, I'll be editing my comments better in the future.

I had a discussion with the authors outside of this thread, and we will be working to wire this new method into the tools in the next few days so that it's tested and verified in this PR, or the PR that contains it.

- Used by Non-secure targets post-build hooks to find the relevant secure image.

NirSonnenschein · 2019-03-03T14:26:27Z

starting CI

mikisch81 · 2019-03-03T14:26:59Z

@ARMmbed/mbed-os-maintainers This PR needs to be for 5.12.0, #9910 will need this.

NirSonnenschein · 2019-03-03T14:27:31Z

@theotherjimmy , please re-review this PR

mbed-ci · 2019-03-03T15:55:07Z

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

orenc17 · 2019-03-03T16:13:38Z

This PR is required for #9910

mikisch81 · 2019-03-03T18:08:26Z

@theotherjimmy see this comment to see the proposed usage of this function in #9910

mikisch81 · 2019-03-03T18:58:10Z

@0xc0170 is it possible to merge, and if any new comments from @theotherjimmy come up a new PR will be opened?

mikisch81 · 2019-03-04T13:22:05Z

@theotherjimmy I added tests

theotherjimmy

Have test. Looks great

mikisch81 · 2019-03-04T14:44:49Z

@0xc0170 Is it possible to start CI? @theotherjimmy approved

theotherjimmy · 2019-03-04T15:11:06Z

@cmonr Your timezone starts soon. running CI appreciated. (this passed travis, so it should be the same result as current master)

0xc0170 · 2019-03-04T16:10:50Z

@0xc0170 Is it possible to start CI? @theotherjimmy approved

We are now focusing on rc1 PRs and nightly job has some failures we are investigating. We will start when able.

mikisch81 · 2019-03-04T16:17:14Z

Adding @maclobdell

0xc0170 · 2019-03-05T13:05:40Z

I'll run one job here (exporters will fail as master has one bug) but at least to get the rest tested now

0xc0170 · 2019-03-05T13:06:44Z

CI started

mbed-ci · 2019-03-05T15:11:35Z

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_exporter

0xc0170 · 2019-03-05T15:15:02Z

Failure in the exporters will be fixed, tracked here #9938

mikisch81 · 2019-03-06T07:00:36Z

@ARMmbed/mbed-os-maintainers Can we re-run the exporter job?

NirSonnenschein · 2019-03-06T07:36:25Z

restarted exporter to check if issue is resolved

0xc0170 · 2019-03-06T08:20:09Z

tools/psa/__init__.py

@@ -0,0 +1,23 @@
+from os.path import basename


missing license header here

0xc0170

license addition needed

mikisch81 · 2019-03-06T08:42:24Z

@0xc0170 Added missing license header

- For cases where non-secure image is HEX and secure image is BIN

0xc0170 · 2019-03-06T15:07:17Z

CI started

ghost

Meets post freeze criteria, PSA related. Approved.

mbed-ci · 2019-03-06T17:35:11Z

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

orenc17 approved these changes Feb 28, 2019

View reviewed changes

cmonr requested a review from theotherjimmy February 28, 2019 20:59

ciarmcom requested review from orenc17 and a team February 28, 2019 22:00

ciarmcom added the needs: review label Feb 28, 2019

cmonr added the release-version: 5.12.1 label Mar 1, 2019

theotherjimmy suggested changes Mar 1, 2019

View reviewed changes

theotherjimmy reviewed Mar 1, 2019

View reviewed changes

tools: Add find_secure_image function

e7341a9

- Used by Non-secure targets post-build hooks to find the relevant secure image.

orenc17 approved these changes Mar 3, 2019

View reviewed changes

mikisch81 mentioned this pull request Mar 3, 2019

Add support for LPC55S59 #9910

Merged

0xc0170 requested a review from a user March 4, 2019 07:31

Add assertions and tests

a9fdf1f

theotherjimmy approved these changes Mar 4, 2019

View reviewed changes

ghost added the PM_ACCEPTED label Mar 4, 2019

0xc0170 added needs: CI and removed needs: review labels Mar 5, 2019

0xc0170 reviewed Mar 6, 2019

View reviewed changes

0xc0170 requested changes Mar 6, 2019

View reviewed changes

Add missing license header

f155057

0xc0170 approved these changes Mar 6, 2019

View reviewed changes

Compare non-secure image name without extension

1f7a829

- For cases where non-secure image is HEX and secure image is BIN

ghost approved these changes Mar 6, 2019

View reviewed changes

orenc17 mentioned this pull request Mar 6, 2019

PSA tools docs #9963

Merged

cmonr added ready for merge and removed needs: CI labels Mar 6, 2019

cmonr merged commit f05a343 into ARMmbed:master Mar 6, 2019

cmonr removed the ready for merge label Mar 6, 2019

	toolchain.notify.debug("Secure image file found: %s." % secure_image)
	notify.debug("Secure image file found: %s." % secure_image)

	toolchain.notify.debug("Secure image file %s not found. Aborting." % configured_s_image_path)
	notify.debug("Secure image file %s not found. Aborting." % configured_s_image_path)



		# Find secure image.
		def find_secure_image(toolchain, resources, ns_image_path, configured_s_image_path, image_type):

	def find_secure_image(toolchain, resources, ns_image_path, configured_s_image_path, image_type):
	def find_secure_image(notify, resources, ns_image_path, configured_s_image_path, image_type):

PSA tools: Find secure image at post-build #9894

PSA tools: Find secure image at post-build #9894

Uh oh!

Conversation

mikisch81 commented Feb 28, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Mar 1, 2019

Uh oh!

mikisch81 commented Mar 1, 2019

Uh oh!

theotherjimmy commented Mar 1, 2019

Uh oh!

mikisch81 commented Mar 1, 2019

Uh oh!

theotherjimmy commented Mar 1, 2019

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenc17 commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Mar 1, 2019

Uh oh!

theotherjimmy commented Mar 2, 2019

Uh oh!

NirSonnenschein commented Mar 3, 2019

Uh oh!

mikisch81 commented Mar 3, 2019

Uh oh!

NirSonnenschein commented Mar 3, 2019

Uh oh!

mbed-ci commented Mar 3, 2019

Uh oh!

orenc17 commented Mar 3, 2019

Uh oh!

mikisch81 commented Mar 3, 2019

Uh oh!

mikisch81 commented Mar 3, 2019

Uh oh!

mikisch81 commented Mar 4, 2019

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

mikisch81 commented Mar 4, 2019

orenc17 commented Mar 1, 2019 •

edited

Loading