So the patch does the wrong thing anyway - it effectively changes the watermark assertion so that it won't trigger if the TFM_RAM_CODE area goes beyond the end of RAM. That's just broken, because TFM_RAM_CODE is, obviously, supposed to be in RAM.
There may well be a problem here, but this is not the correct fix for it.
Chris
From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Christopher Brand via TF-M Sent: Thursday, February 6, 2020 3:58 PM To: Jamie Fox Jamie.Fox@arm.com; tf-m@lists.trustedfirmware.org Cc: Gabor Abonyi Gabor.Abonyi@arm.com; nd nd@arm.com Subject: Re: [TF-M] PsoC64 build broken
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
I did an armclang build of the head of master and with this patch reverted, and there is a small difference between them - the TFM_RAM_CODE and SRAM_WATERMARK sections are swapped. That feels wrong, because the TFM_RAM_CODE is inside SRAM, so I'd expect the "end of SRAM watermark" to be after it, not before it. I'm not sure exactly how the watermark section is used, though. On the plus side, TFM_RAM_CODE does indeed still end up at the correct address, in SRAM.
I'm not sure exactly which sections you want me to try changing to set the LMA. The line numbers don't seem to match up...
Chris
From: TF-M <tf-m-bounces@lists.trustedfirmware.orgmailto:tf-m-bounces@lists.trustedfirmware.org> On Behalf Of Christopher Brand via TF-M Sent: Thursday, February 6, 2020 3:43 PM To: Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com>; tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: Re: [TF-M] PsoC64 build broken
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
I'd really prefer to see the patch reverted first, and then we can figure out exactly what's going on and push a revised patch that achieves the objectives without breaking anything. It does seem to have some side-effects on the PSoC64 armclang build, too.
It's pretty standard open source process to revert a patch that breaks something like this...
Chris
From: Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com> Sent: Thursday, February 6, 2020 3:15 PM To: tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org; Christopher Brand <chris.brand@cypress.commailto:chris.brand@cypress.com> Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: Re: PsoC64 build broken
Hi Chris,
Sorry about that. It shouldn't have changed the address of the region though because it's explicitly set to S_RAM_CODE_START.
I think this could be down to an "interesting" behaviour in GCC linker scripts where if the LMA is explicitly set with for a region with "AT>", then it won't revert back to being equal to the VMA for the next region in some cases. So before we revert the change, please can you try setting the LMA explicitly for the following region(s)? That is, try "> FLASH AT> FLASH" instead of just "> FLASH" on lines 527 and 563.
Best wishes, Jamie
________________________________ From: TF-M <tf-m-bounces@lists.trustedfirmware.orgmailto:tf-m-bounces@lists.trustedfirmware.org> on behalf of Christopher Brand via TF-M <tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org> Sent: 06 February 2020 20:11 To: tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org <tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org> Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com> Subject: [TF-M] PsoC64 build broken
Hi,
Commit 52182bc5e006752a4d28c3ccd909f93dafee0cf5 ("Build: Fix SRAM sanity check in common scatter file") seems to break the PSoc64 build. This is from https://review.trustedfirmware.org/c/trusted-firmware-m/+/3333
Building with gcc, I get:
/lhome/cbrand/work/trees_2/psoc6_atfm/trusted-firmware-m/build_gcc_psoc64/secure_fw/tfm_s.ld.i:352 cannot move location counter backwards (from 000000000802f578 to 0000000008000000)
collect2: error: ld returned 1 exit status
secure_fw/CMakeFiles/tfm_s.dir/build.make:210: recipe for target 'unit_test/tfm_s.axf' failed
I'm quite surprised that the comments on the review note that "I noticed that this define is currently only used for PSOC6 platform which I don't possess" and yet apparently a Musca B1-only build was considered sufficient to merge it.
I haven't dug into the details, but superficially it seems to move the .ramfunc code to before S_DATA_START, which means that it will no longer be in secure RAM.
Can we please revert this patch for now?
Chris
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
Hi Chris,
I agree there needs to be some rethinking of the way this works. The problem the patch was intended to solve was that the watermark no longer correctly detected overflow in data allocated in RAM because the TFM_RAM_CODE region explicitly set the address before reaching the watermark. My feeling is that data RAM and code RAM should be considered separate areas in the scatter file, each with their own watermark. There is no guarantee that they will be the same physical memory on a particular device after all.
I have pushed the revert for review but it doesn't merge cleanly so will need some fixup in the morning https://review.trustedfirmware.org/c/trusted-firmware-m/+/3368
On the LMAs, my line numbers were off because I wasn't looking at master. To be clear the sections I was referring to adjusting were .ARM.extab and .ER_TFM_CODE.
Kind regards,
Jamie
________________________________ From: Christopher Brand chris.brand@cypress.com Sent: 07 February 2020 00:26 To: tf-m@lists.trustedfirmware.org tf-m@lists.trustedfirmware.org; Jamie Fox Jamie.Fox@arm.com Cc: Gabor Abonyi Gabor.Abonyi@arm.com; nd nd@arm.com Subject: RE: PsoC64 build broken
So the patch does the wrong thing anyway – it effectively changes the watermark assertion so that it won’t trigger if the TFM_RAM_CODE area goes beyond the end of RAM. That’s just broken, because TFM_RAM_CODE is, obviously, supposed to be in RAM.
There may well be a problem here, but this is not the correct fix for it.
Chris
From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Christopher Brand via TF-M Sent: Thursday, February 6, 2020 3:58 PM To: Jamie Fox Jamie.Fox@arm.com; tf-m@lists.trustedfirmware.org Cc: Gabor Abonyi Gabor.Abonyi@arm.com; nd nd@arm.com Subject: Re: [TF-M] PsoC64 build broken
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
I did an armclang build of the head of master and with this patch reverted, and there is a small difference between them – the TFM_RAM_CODE and SRAM_WATERMARK sections are swapped. That feels wrong, because the TFM_RAM_CODE is inside SRAM, so I’d expect the “end of SRAM watermark” to be after it, not before it. I’m not sure exactly how the watermark section is used, though. On the plus side, TFM_RAM_CODE does indeed still end up at the correct address, in SRAM.
I’m not sure exactly which sections you want me to try changing to set the LMA. The line numbers don’t seem to match up…
Chris
From: TF-M <tf-m-bounces@lists.trustedfirmware.orgmailto:tf-m-bounces@lists.trustedfirmware.org> On Behalf Of Christopher Brand via TF-M Sent: Thursday, February 6, 2020 3:43 PM To: Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com>; tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: Re: [TF-M] PsoC64 build broken
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
I’d really prefer to see the patch reverted first, and then we can figure out exactly what’s going on and push a revised patch that achieves the objectives without breaking anything. It does seem to have some side-effects on the PSoC64 armclang build, too.
It’s pretty standard open source process to revert a patch that breaks something like this…
Chris
From: Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com> Sent: Thursday, February 6, 2020 3:15 PM To: tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org; Christopher Brand <chris.brand@cypress.commailto:chris.brand@cypress.com> Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: Re: PsoC64 build broken
Hi Chris,
Sorry about that. It shouldn't have changed the address of the region though because it's explicitly set to S_RAM_CODE_START.
I think this could be down to an "interesting" behaviour in GCC linker scripts where if the LMA is explicitly set with for a region with "AT>", then it won't revert back to being equal to the VMA for the next region in some cases. So before we revert the change, please can you try setting the LMA explicitly for the following region(s)? That is, try "> FLASH AT> FLASH" instead of just "> FLASH" on lines 527 and 563.
Best wishes,
Jamie
________________________________
From: TF-M <tf-m-bounces@lists.trustedfirmware.orgmailto:tf-m-bounces@lists.trustedfirmware.org> on behalf of Christopher Brand via TF-M <tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org> Sent: 06 February 2020 20:11 To: tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org <tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org> Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com> Subject: [TF-M] PsoC64 build broken
Hi,
Commit 52182bc5e006752a4d28c3ccd909f93dafee0cf5 (“Build: Fix SRAM sanity check in common scatter file”) seems to break the PSoc64 build. This is from https://review.trustedfirmware.org/c/trusted-firmware-m/+/3333
Building with gcc, I get:
/lhome/cbrand/work/trees_2/psoc6_atfm/trusted-firmware-m/build_gcc_psoc64/secure_fw/tfm_s.ld.i:352 cannot move location counter backwards (from 000000000802f578 to 0000000008000000)
collect2: error: ld returned 1 exit status
secure_fw/CMakeFiles/tfm_s.dir/build.make:210: recipe for target 'unit_test/tfm_s.axf' failed
I’m quite surprised that the comments on the review note that “I noticed that this define is currently only used for PSOC6 platform which I don't possess” and yet apparently a Musca B1-only build was considered sufficient to merge it.
I haven’t dug into the details, but superficially it seems to move the .ramfunc code to before S_DATA_START, which means that it will no longer be in secure RAM.
Can we please revert this patch for now?
Chris
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
Hi All,
Sorry for the inconvenience. I'll clean the revert and push it soon.
Regards, Gabor
From: Jamie Fox Jamie.Fox@arm.com Sent: 2020. február 7., péntek 2:11 To: Christopher Brand chris.brand@cypress.com; tf-m@lists.trustedfirmware.org Cc: Gabor Abonyi Gabor.Abonyi@arm.com; nd nd@arm.com Subject: Re: PsoC64 build broken
Hi Chris,
I agree there needs to be some rethinking of the way this works. The problem the patch was intended to solve was that the watermark no longer correctly detected overflow in data allocated in RAM because the TFM_RAM_CODE region explicitly set the address before reaching the watermark. My feeling is that data RAM and code RAM should be considered separate areas in the scatter file, each with their own watermark. There is no guarantee that they will be the same physical memory on a particular device after all.
I have pushed the revert for review but it doesn't merge cleanly so will need some fixup in the morning https://review.trustedfirmware.org/c/trusted-firmware-m/+/3368
On the LMAs, my line numbers were off because I wasn't looking at master. To be clear the sections I was referring to adjusting were .ARM.extab and .ER_TFM_CODE.
Kind regards,
Jamie
________________________________ From: Christopher Brand <chris.brand@cypress.commailto:chris.brand@cypress.com> Sent: 07 February 2020 00:26 To: tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org <tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org>; Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com> Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: RE: PsoC64 build broken
So the patch does the wrong thing anyway - it effectively changes the watermark assertion so that it won't trigger if the TFM_RAM_CODE area goes beyond the end of RAM. That's just broken, because TFM_RAM_CODE is, obviously, supposed to be in RAM.
There may well be a problem here, but this is not the correct fix for it.
Chris
From: TF-M <tf-m-bounces@lists.trustedfirmware.orgmailto:tf-m-bounces@lists.trustedfirmware.org> On Behalf Of Christopher Brand via TF-M Sent: Thursday, February 6, 2020 3:58 PM To: Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com>; tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: Re: [TF-M] PsoC64 build broken
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
I did an armclang build of the head of master and with this patch reverted, and there is a small difference between them - the TFM_RAM_CODE and SRAM_WATERMARK sections are swapped. That feels wrong, because the TFM_RAM_CODE is inside SRAM, so I'd expect the "end of SRAM watermark" to be after it, not before it. I'm not sure exactly how the watermark section is used, though. On the plus side, TFM_RAM_CODE does indeed still end up at the correct address, in SRAM.
I'm not sure exactly which sections you want me to try changing to set the LMA. The line numbers don't seem to match up...
Chris
From: TF-M <tf-m-bounces@lists.trustedfirmware.orgmailto:tf-m-bounces@lists.trustedfirmware.org> On Behalf Of Christopher Brand via TF-M Sent: Thursday, February 6, 2020 3:43 PM To: Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com>; tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: Re: [TF-M] PsoC64 build broken
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
I'd really prefer to see the patch reverted first, and then we can figure out exactly what's going on and push a revised patch that achieves the objectives without breaking anything. It does seem to have some side-effects on the PSoC64 armclang build, too.
It's pretty standard open source process to revert a patch that breaks something like this...
Chris
From: Jamie Fox <Jamie.Fox@arm.commailto:Jamie.Fox@arm.com> Sent: Thursday, February 6, 2020 3:15 PM To: tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org; Christopher Brand <chris.brand@cypress.commailto:chris.brand@cypress.com> Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com>; nd <nd@arm.commailto:nd@arm.com> Subject: Re: PsoC64 build broken
Hi Chris,
Sorry about that. It shouldn't have changed the address of the region though because it's explicitly set to S_RAM_CODE_START.
I think this could be down to an "interesting" behaviour in GCC linker scripts where if the LMA is explicitly set with for a region with "AT>", then it won't revert back to being equal to the VMA for the next region in some cases. So before we revert the change, please can you try setting the LMA explicitly for the following region(s)? That is, try "> FLASH AT> FLASH" instead of just "> FLASH" on lines 527 and 563.
Best wishes,
Jamie
________________________________
From: TF-M <tf-m-bounces@lists.trustedfirmware.orgmailto:tf-m-bounces@lists.trustedfirmware.org> on behalf of Christopher Brand via TF-M <tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org> Sent: 06 February 2020 20:11 To: tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org <tf-m@lists.trustedfirmware.orgmailto:tf-m@lists.trustedfirmware.org> Cc: Gabor Abonyi <Gabor.Abonyi@arm.commailto:Gabor.Abonyi@arm.com> Subject: [TF-M] PsoC64 build broken
Hi,
Commit 52182bc5e006752a4d28c3ccd909f93dafee0cf5 ("Build: Fix SRAM sanity check in common scatter file") seems to break the PSoc64 build. This is from https://review.trustedfirmware.org/c/trusted-firmware-m/+/3333
Building with gcc, I get:
/lhome/cbrand/work/trees_2/psoc6_atfm/trusted-firmware-m/build_gcc_psoc64/secure_fw/tfm_s.ld.i:352 cannot move location counter backwards (from 000000000802f578 to 0000000008000000)
collect2: error: ld returned 1 exit status
secure_fw/CMakeFiles/tfm_s.dir/build.make:210: recipe for target 'unit_test/tfm_s.axf' failed
I'm quite surprised that the comments on the review note that "I noticed that this define is currently only used for PSOC6 platform which I don't possess" and yet apparently a Musca B1-only build was considered sufficient to merge it.
I haven't dug into the details, but superficially it seems to move the .ramfunc code to before S_DATA_START, which means that it will no longer be in secure RAM.
Can we please revert this patch for now?
Chris
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
tf-m@lists.trustedfirmware.org
-
Christopher Brand -
Gabor Abonyi -
Jamie Fox