Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Mar 6, 2019

Conversation

Projects
None yet
9 participants
@mikisch81
Copy link
Contributor

commented Feb 28, 2019

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

@cmonr cmonr requested a review from theotherjimmy Feb 28, 2019

@ciarmcom ciarmcom requested review from orenc17 and ARMmbed/mbed-os-maintainers Feb 28, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

tools/psa/__init__.py Outdated
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)

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 1, 2019

Contributor

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

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 1, 2019

Author Contributor

Ok

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 1, 2019

Author Contributor

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'

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 1, 2019

Contributor

Probably private then. Pass in a notifier instead.

tools/psa/__init__.py Outdated
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)

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 1, 2019

Contributor

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

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 1, 2019

Author Contributor

Ok

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

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

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Do you suggest adding the musca post build hook here?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@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
Copy link
Contributor

left a comment

Notifier changes requested offline.

tools/psa/__init__.py Outdated
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)

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 1, 2019

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

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 3, 2019

Author Contributor

done

tools/psa/__init__.py Outdated
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)

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 1, 2019

Contributor
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)

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 3, 2019

Author Contributor

done

tools/psa/__init__.py Outdated


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

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 1, 2019

Contributor
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):

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 3, 2019

Author Contributor

done

tools/psa/__init__.py Outdated
from os.path import basename


# Find secure image.

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 1, 2019

Contributor

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

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 3, 2019

Author Contributor

done

@orenc17

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

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.

tools: Add find_secure_image function
- Used by Non-secure targets post-build hooks to find
  the relevant secure image.

@mikisch81 mikisch81 force-pushed the kfnta:find_secure_image branch to e7341a9 Mar 3, 2019

@orenc17

orenc17 approved these changes Mar 3, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

starting CI

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

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

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

@theotherjimmy , please re-review this PR

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 3, 2019

Test run: SUCCESS

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

@orenc17

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

This PR is required for #9910

@mikisch81 mikisch81 referenced this pull request Mar 3, 2019

Merged

Add support for LPC55S59 #9910

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

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

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

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

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@theotherjimmy I added tests

@theotherjimmy
Copy link
Contributor

left a comment

Have test. Looks great

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

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

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Adding @maclobdell

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

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

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 5, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

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

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

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

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

restarted exporter to check if issue is resolved

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

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Mar 6, 2019

Member

missing license header here

This comment has been minimized.

Copy link
@mikisch81

mikisch81 Mar 6, 2019

Author Contributor

Done

@0xc0170
Copy link
Member

left a comment

license addition needed

@mikisch81

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@0xc0170 Added missing license header

@0xc0170

0xc0170 approved these changes Mar 6, 2019

Compare non-secure image name without extension
- For cases where non-secure image is HEX and secure image is BIN
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

CI started

@ChiefBureaucraticOfficer
Copy link

left a comment

Meets post freeze criteria, PSA related. Approved.

@orenc17 orenc17 referenced this pull request Mar 6, 2019

Merged

PSA tools docs #9963

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 6, 2019

Test run: SUCCESS

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

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

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

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10091 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.