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: TFM import from master branch #10286

Merged
merged 10 commits into from
Apr 11, 2019
Merged

PSA: TFM import from master branch #10286

merged 10 commits into from
Apr 11, 2019

Conversation

orenc17
Copy link
Contributor

@orenc17 orenc17 commented Apr 1, 2019

Description

Import TF-M sources from master branch at hash 0101fd3766913ff9786b95568cbc69cf58c1946b
Refactor PSA code generators scripts

Pull request type

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

Reviewers

@ARMmbed/mbed-os-psa

Release Notes

@ciarmcom ciarmcom requested review from a team April 1, 2019 17:00
@ciarmcom
Copy link
Member

ciarmcom commented Apr 1, 2019

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

@@ -11,6 +11,8 @@
#include <stdint.h>
#include <stddef.h>
#include <limits.h>
#include "../ext/mcuboot/bootutil/include/bootutil/image.h"
#include "../ext/mcuboot/include/flash_map/flash_map.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This include path doesn't seem right.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now we are not building secure bootloader in tree as it is provided as binary.
Can we tweak importer to skip bl2 folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alzix could you clarify what you mean? Should this include path stay as-is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right about include path is being wrong. The only reason why it does not fail compilation is that this header file is never included. This raises a question why is it here? I suggest tweaking importer configuration file to exclude this header file.

from tools.psa.mbed_spm_tfm_common import *
from tools.psa.generate_mbed_spm_partition_code import *
from tools.psa.generate_partition_code import *
from .test_data import *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we should stay away from import * to avoid bringing everything into the namespace. I realize in this case you're trying to do just that. For consistency with (most) of the tools, could you do one of the following:

  • Import the module and call the functions on the module (this is usually my preferred choice).
    • If you're worries about line length, you can always do something like import tools.psa.generate_partition_code as gpc. This allows you to then call those functions with gpc.my_func().
  • Import each of the functions by name

@@ -11,6 +11,8 @@
#include <stdint.h>
#include <stddef.h>
#include <limits.h>
#include "../ext/mcuboot/bootutil/include/bootutil/image.h"
#include "../ext/mcuboot/include/flash_map/flash_map.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now we are not building secure bootloader in tree as it is provided as binary.
Can we tweak importer to skip bl2 folder.

@orenc17
Copy link
Contributor Author

orenc17 commented Apr 8, 2019

@bridadan @alzix
components/TARGET_PSA/TARGET_TFM/COMPONENT_SPE/bl2/include/boot_record.h was not in use i've removed it in the first patch...updated commit SHA's as well

Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments!

My previous comment about the import * statements can be addressed in a later pull request. I didn't realize that many of those statements were already present in a release.

@cmonr
Copy link
Contributor

cmonr commented Apr 9, 2019

CI started while reviews are finished.

@cmonr
Copy link
Contributor

cmonr commented Apr 9, 2019

CI restarted since results are now stale

Wrong PR.

@cmonr
Copy link
Contributor

cmonr commented Apr 9, 2019

Also started PSA/job/PSA-SPM-tests Jenkins test.

@mbed-ci
Copy link

mbed-ci commented Apr 9, 2019

Test run: SUCCESS

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

@mbed-ci
Copy link

mbed-ci commented Apr 10, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@cmonr
Copy link
Contributor

cmonr commented Apr 10, 2019

@orenc17 Can you check the results of the PSA/job/PSA-SPM-tests job in Jenkins and let us know what the results are here? Currently not on VPN.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2019

@orenc17 Can you check the results of the PSA/job/PSA-SPM-tests job in Jenkins and let us know what the results are here? Currently not on VPN.

Waiting for the confirmation.

Oren Cohen and others added 5 commits April 10, 2019 14:20
- Remove un-needed files
- Disable printf and uart
- Modify include paths
- Guard macros from mbed_lib with ifndef

(cherry picked from commit 1f30b52)
(cherry picked from commit 71cd34d)
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Apr 10, 2019

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR

@alekla01
Copy link
Contributor

Restarted jenkins-ci

@orenc17 orenc17 changed the title PSA: TFM import and code generation refactor PSA: TFM import from master branch Apr 10, 2019
@mbed-ci
Copy link

mbed-ci commented Apr 10, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 5
Build artifacts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants