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.org> On Behalf Of Christopher Brand via TF-M
Sent: Thursday, February 6, 2020 3:43 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’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.com>
Sent: Thursday, February 6, 2020 3:15 PM
To: tf-m@lists.trustedfirmware.org; Christopher Brand <chris.brand@cypress.com>
Cc: Gabor Abonyi <Gabor.Abonyi@arm.com>; nd <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.org> on behalf of Christopher Brand via TF-M <tf-m@lists.trustedfirmware.org>
Sent: 06 February 2020 20:11
To: tf-m@lists.trustedfirmware.org <tf-m@lists.trustedfirmware.org>
Cc: Gabor Abonyi <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.