mcuboot: Commit v1.7.0-rc1 #19

oyvindronningstad · 2020-11-09T18:41:47Z

Retrieve mcuboot via west. See zephyrproject-rtos/zephyr#30050.

This PR removes the existing committed mcuboot which is outdated.

~~Commit MCUboot to avoid long download times on each build.~~

CMP0097 doesn't work on 3.16 and 3.17.
https://gitlab.kitware.com/cmake/cmake/-/issues/20579

Add a workaround (request non-existant submodule).
This prevents fetching almost 200 MB of data, improving
pristine build times.

This change was added upstream, but reverted because of unspecified build problems.
Add it here unless it gives problems, since it improves build times 40 seconds.

oyvindronningstad · 2020-11-15T15:27:39Z

This now has a PR upstream again: https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/7023

ioannisg · 2020-11-16T16:11:19Z

FYI @agansari

ioannisg

Looks pretty straight forward to me.

ioannisg · 2020-11-16T18:39:22Z

@oyvindronningstad would we need to manually upmerge the mcuboot in order to move forward from v1.7.0-RC1?

ioannisg · 2020-11-16T18:39:40Z

@galak @microbuilder could you please add feedback?

oyvindronningstad · 2020-11-16T18:59:50Z

@oyvindronningstad would we need to manually upmerge the mcuboot in order to move forward from v1.7.0-RC1?

Yes

microbuilder · 2020-11-16T19:02:33Z

This should stay in sync with the upstream TF-M repo in my opinion, but it looks like you're already working towards that: https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/7023

Edit: I +1'ed the upstream change request.

oyvindronningstad · 2020-11-18T13:41:39Z

This should stay in sync with the upstream TF-M repo in my opinion, but it looks like you're already working towards that: https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/7023

Edit: I +1'ed the upstream change request.

Note that this PR is now different from the ustream one. This one does not change the trusted-firmware-m directory precisely because we might not get the upstream change in for 1.2.

tejlmand · 2020-11-18T14:50:09Z

CMakeLists.txt

               ${TFM_REGRESSION_ARG}
               ${TFM_PROFILE_ARG}
               -DTFM_TEST_REPO_PATH=${ZEPHYR_TFM_MODULE_DIR}/tf-m-tests
+               -DMCUBOOT_PATH=${ZEPHYR_TFM_MODULE_DIR}/mcuboot


Why is it not possible to use:

Suggested change

-DMCUBOOT_PATH=${ZEPHYR_TFM_MODULE_DIR}/mcuboot

-DMCUBOOT_PATH=${ZEPHYR_MCUBOOT_MODULE_DIR}

and thus use the same mcuboot as Zephyr ?
And then completely remove mcuboot from this repository.

Are they not compatible ?

They are, and when I tried it it works, they just aren't identical I think. So I think to keep TFM exactly as released, we don't want to do that yet. Though I'm not entirely convinced myself why we can't.

I understand that reason, but in that case, why not use west ?
That would remove the need to upmerge mcuboot in two places:

This repo

https://github.com/zephyrproject-rtos/mcuboot

Just add this to west.yml: https://github.com/zephyrproject-rtos/zephyr/blob/master/west.yml

- name: tfm_mcuboot repo-path: mcuboot path: modules/tee/tfm/mcuboot revision: 69344636bee1f18a6703dc5a8e7a0edfd8128d45 # revision: 1.7.0-rc1

Note, the 69344636bee1f18a6703dc5a8e7a0edfd8128d45 SHA is part of this PR: zephyrproject-rtos/mcuboot#39
zephyrproject-rtos/mcuboot@6934463

But if you ask @nvlsianpu, then maybe he can include the mcuboot tags in the upmerge, and you will be able to use 1.7.0-rc1 instead of the SHA.

That way we remove the need to upmerge mcuboot inside this repo, but still use a different version for TFM, than what Zephyr is using.
And there is only one place to keep mcuboot in sync with upstream.

But if you ask @nvlsianpu, then maybe he can include the mcuboot tags in the upmerge, and you will be able to use 1.7.0-rc1 instead of the SHA

I can send 1.7.0-rc1 tag to zephyr's mcuboot fork If needed.

Changed to use west now. See if you like it better

Changed to use west now. See if you like it better

You need a commit with:

git rm -r mcuboot

for this to work.

@microbuilder that would be the optimal, and if this guide is followed:

Just verified, zephyrproject-rtos/mcuboot#39, does have all the upstream SHAs.

Should I change the west entry to a sha, or should we bring in the tag?

You need a commit with:

git rm -r mcuboot

for this to work.

Done

But if you ask @nvlsianpu, then maybe he can include the mcuboot tags in the upmerge, and you will be able to use 1.7.0-rc1 instead of the SHA

I can send 1.7.0-rc1 tag to zephyr's mcuboot fork If needed.

@nvlsianpu that would be highly appreciated 😄

tejlmand

Much nicer.

oyvindronningstad · 2020-11-29T19:22:12Z

This should stay in sync with the upstream TF-M repo in my opinion, but it looks like you're already working towards that: https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/7023

Edit: I +1'ed the upstream change request.

Thanks @microbuilder, but now this change is entirely outside of the upstream repo. Can you take another look with this in mind?

oyvindronningstad · 2020-12-04T13:28:02Z

@microbuilder @galak Please review

microbuilder · 2020-12-04T14:10:33Z

@d3zd3z Care to give your feedback on this as well?

.gitignore

galak

I don't think it makes sense to assume that the mcuboot from zephyr west will be in sync with TFM.

microbuilder · 2020-12-04T14:45:39Z

I don't think it makes sense to assume that the mcuboot from zephyr west will be in sync with TFM.

I sort of dislike the assumption here as well, and the additional maintenance burden to keep Zephyr's MCUBoot requirements in sync with TF-M's.

oyvindronningstad · 2020-12-07T07:58:46Z

I don't think it makes sense to assume that the mcuboot from zephyr west will be in sync with TFM.

I sort of dislike the assumption here as well, and the additional maintenance burden to keep Zephyr's MCUBoot requirements in sync with TF-M's.

@galak @microbuilder This solution was agreed on a while ago in this thread: #19 (comment)

It seems there's no optimal way to do this, so I suggest that we merge this. We can always redo it later.

I really want to stop wasting a whole minute every time I build TFM.

galak · 2020-12-07T15:05:23Z

@galak @microbuilder This solution was agreed on a while ago in this thread: #19 (comment)

It seems there's no optimal way to do this, so I suggest that we merge this. We can always redo it later.

I really want to stop wasting a whole minute every time I build TFM.

I suggest looking for other solutions to the issue. I don't agree with this approach of coupling the mcuboot. @d3zd3z do you mind commenting as well.

d3zd3z

I'm a bit uncomfortable having TFM use the mcuboot out of the Zephyr repository rather than the upstream. The fork that is in Zephyr has the purpose of holding onto Zephyr-specific support before it can be upstreamed. There is no testing for whether that repo works in any non-zephyr configuration, including TF-M. In fact, it is likely to lag behind the upstream, where TF-M-specific fixes will end up.

Upstream TF-M will be removing the mcuboot code from the repo, and referencing upstream. I think we should mirror that. If we have timing issues with builds, maybe that can be addressed another way. Pointing at the wrong repo is not the right answer, though.

oyvindronningstad · 2020-12-07T19:28:04Z

@oyvindronningstad what is the whole minute you are referring to associated with?

Mcuboot is being downloaded, with all its submodules. This is approx 200MB, taking around 1 min on my connection.

oyvindronningstad · 2020-12-07T19:30:24Z

I misunderstood the branch being used. I'm ok with using this, then.

Glad we cleared it up 👍

galak · 2020-12-07T19:34:19Z

Mcuboot is being downloaded, with all its submodules. This is approx 200MB, taking around 1 min on my connection.

If I understand tfm's cmake is downloading mcuboot in the case you are describing for every new build you do. Is that correct?

We just need a solution to point to a "pre-cloned" version of mcuboot, so that every tfm build doesn't checkout a new version of mcuboot. We could do this by just updating the files in zephyrproject-rtos/trusted-firmware-m for mcuboot.

oyvindronningstad · 2020-12-07T20:16:06Z

We just need a solution to point to a "pre-cloned" version of mcuboot, so that every tfm build doesn't checkout a new version of mcuboot. We could do this by just updating the files in zephyrproject-rtos/trusted-firmware-m for mcuboot.

Exactly, and that was my 1st rewrite. It was rejected in #19 (comment).

I suggest we merge this. The MCUboot situation is not figured out yet, so we will have plenty of opportunity to change things later.

galak · 2020-12-08T16:03:39Z

@oyvindronningstad can we tweak this to introduce a west.yml in the trusted-firmware-m repo and use import in zephyr/west.yml

so trusted-firmware-m/west.yml would look like:

manifest:
  projects:
    - name: tfm-mcuboot
      url: https://github.com/zephyrproject-rtos/mcuboot
      revision: 69344636bee1f18a6703dc5a8e7a0edfd8128d45

galak · 2020-12-08T16:06:50Z

Also, I think if we are going down this path, we should do it for mbed-crypto as well. I can setup the zephyrproject mirror if you are in agreement.

oyvindronningstad · 2020-12-08T16:12:42Z

@oyvindronningstad can we tweak this to introduce a west.yml in the trusted-firmware-m repo and use import in zephyr/west.yml

That sounds like a good plan. I'll add it.

Also, I think if we are going down this path, we should do it for mbed-crypto as well. I can setup the zephyrproject mirror if you are in agreement.

Sounds good as well. Note (in case you are not aware) that even though they call it mbedcrypto they are using mbedtls. Also: they modify the repo with a number of patches. These modifications should be committed to the repo mirror.

Thanks!

galak · 2020-12-08T16:12:49Z

Also, I pushed the 'v1.7.0', 1.7.0-rc1 and v1.7.0-rc2 tags to the zephyr mcuboot repo so we can use those now.

galak · 2020-12-08T16:24:36Z

Sounds good as well. Note (in case you are not aware) that even though they call it mbedcrypto they are using mbedtls. Also: they modify the repo with a number of patches. These modifications should be committed to the repo mirror.

I was not aware. Looking at the tfm buildsystem, I think we need to tweak things so that tfm does the patch'ing.

We can deal with mbedtls/mbed-crypto after we get mcuboot working.

It will be added to Zephyr's west.yml Signed-off-by: Øyvind Rønningstad <[email protected]>

ioannisg · 2020-12-09T21:51:01Z

@galak could you please take a final look?

west.yml

galak

In general looks good:

Just the 2 questions:

the .gitignore with /mcuboot seems weird, and maybe wrong or not needed
can we tweak the path of where mcuboot gets put by west.

ioannisg · 2020-12-10T11:07:28Z

README.rst

    commit: 3d986e72d9e084bb45763849699718f474fb10ba
    license: Apache 2.0
+
+See also west.yml for more dependencies.


I wonder if you could add a more detailed comment here, to explain what happens with MCUboot, but it is not a must-do for this PR.

Pointing to Zephyr's mcuboot fork. Signed-off-by: Øyvind Rønningstad <[email protected]>

This improves build times by skipping the download. Signed-off-by: Øyvind Rønningstad <[email protected]>

Remove mcuboot from init-git.sh and README.rst. Signed-off-by: Øyvind Rønningstad <[email protected]>

galak · 2020-12-10T12:51:38Z

Guess we need a zephyr side PR now.

oyvindronningstad force-pushed the mcuboot branch from 8993857 to 211e9e7 Compare November 15, 2020 15:24

ioannisg self-requested a review November 16, 2020 12:39

oyvindronningstad force-pushed the mcuboot branch from 211e9e7 to 9ac6264 Compare November 16, 2020 13:17

oyvindronningstad changed the title ~~mcuboot: Add workaround to not fetch submodules~~ mcuboot: Commit v1.7.0-rc1 Nov 16, 2020

ioannisg requested review from galak, microbuilder and tejlmand November 16, 2020 16:11

oyvindronningstad mentioned this pull request Nov 16, 2020

west.yml: Update TFM module SHA to bring in the TFM MCUboot update zephyrproject-rtos/zephyr#30050

Merged

ioannisg approved these changes Nov 16, 2020

View reviewed changes

tejlmand suggested changes Nov 18, 2020

View reviewed changes

oyvindronningstad force-pushed the mcuboot branch 2 times, most recently from 056fe85 to f4da9e7 Compare November 19, 2020 12:51

tejlmand approved these changes Nov 25, 2020

View reviewed changes

microbuilder requested a review from d3zd3z December 4, 2020 14:10

galak reviewed Dec 4, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

galak requested changes Dec 4, 2020

View reviewed changes

d3zd3z requested changes Dec 7, 2020

View reviewed changes

oyvindronningstad force-pushed the mcuboot branch 4 times, most recently from a6dfc53 to 14850a7 Compare December 9, 2020 09:57

oyvindronningstad requested a review from galak December 9, 2020 10:02

mcuboot: Remove so it can be fetched with west instead

0eaa935

It will be added to Zephyr's west.yml Signed-off-by: Øyvind Rønningstad <[email protected]>

oyvindronningstad force-pushed the mcuboot branch from 14850a7 to fe10881 Compare December 9, 2020 10:56

galak reviewed Dec 9, 2020

View reviewed changes

west.yml Outdated Show resolved Hide resolved

galak requested changes Dec 9, 2020

View reviewed changes

oyvindronningstad force-pushed the mcuboot branch from fe10881 to a2ff742 Compare December 10, 2020 10:50

ioannisg reviewed Dec 10, 2020

View reviewed changes

ioannisg requested a review from galak December 10, 2020 11:08

oyvindronningstad added 3 commits December 10, 2020 12:33

west.yml: Add initial west.yml with mcuboot v1.7.0-rc1

5eaf95c

Pointing to Zephyr's mcuboot fork. Signed-off-by: Øyvind Rønningstad <[email protected]>

CMakeLists.txt: Use MCUBOOT_PATH downloaded via west

88c51f6

This improves build times by skipping the download. Signed-off-by: Øyvind Rønningstad <[email protected]>

tfm: Clean up mcuboot references

a247c0e

Remove mcuboot from init-git.sh and README.rst. Signed-off-by: Øyvind Rønningstad <[email protected]>

oyvindronningstad force-pushed the mcuboot branch from a2ff742 to a247c0e Compare December 10, 2020 11:34

galak approved these changes Dec 10, 2020

View reviewed changes

galak merged commit 867d714 into zephyrproject-rtos:master Dec 10, 2020

oyvindronningstad deleted the mcuboot branch December 10, 2020 13:07

	-DMCUBOOT_PATH=${ZEPHYR_TFM_MODULE_DIR}/mcuboot
	-DMCUBOOT_PATH=${ZEPHYR_MCUBOOT_MODULE_DIR}

mcuboot: Commit v1.7.0-rc1 #19

mcuboot: Commit v1.7.0-rc1 #19

Uh oh!

Conversation

oyvindronningstad commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oyvindronningstad commented Nov 15, 2020

Uh oh!

ioannisg commented Nov 16, 2020

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

ioannisg commented Nov 16, 2020

Uh oh!

ioannisg commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oyvindronningstad commented Nov 16, 2020

Uh oh!

microbuilder commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oyvindronningstad commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejlmand Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

oyvindronningstad commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oyvindronningstad commented Dec 4, 2020

Uh oh!

microbuilder commented Dec 4, 2020

Uh oh!

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

microbuilder commented Dec 4, 2020

Uh oh!

oyvindronningstad commented Dec 7, 2020

Uh oh!

galak commented Dec 7, 2020

Uh oh!

d3zd3z left a comment

Choose a reason for hiding this comment

Uh oh!

oyvindronningstad commented Dec 7, 2020

Uh oh!

oyvindronningstad commented Dec 7, 2020

Uh oh!

galak commented Dec 7, 2020

Uh oh!

oyvindronningstad commented Dec 7, 2020

Uh oh!

galak commented Dec 8, 2020

oyvindronningstad commented Nov 9, 2020 •

edited

Loading

ioannisg commented Nov 16, 2020 •

edited

Loading

microbuilder commented Nov 16, 2020 •

edited

Loading

tejlmand Nov 18, 2020 •

edited

Loading

oyvindronningstad commented Nov 29, 2020 •

edited

Loading

oyvindronningstad commented Dec 8, 2020 •

edited

Loading