TI platforms broken on master

[glneo]

Bisecting was difficult due to #1459 having intermediate patches that do not build. After manually bisecting the offending commit seems to be: 10d13b2

Output:

DEBUG:   TEE-CORE:add_phys_mem:325: CFG_SHMEM_START type NSEC_SHM 0xbf700000 size 0x00400000
DEBUG:   TEE-CORE:add_phys_mem:325: CFG_TA_RAM_START type TA_RAM 0xbdc00000 size 0x01a00000
DEBUG:   TEE-CORE:add_phys_mem:325: VCORE_UNPG_RW_PA type TEE_RAM_RW 0xbdb25000 size 0x000db000
DEBUG:   TEE-CORE:add_phys_mem:325: VCORE_UNPG_RX_PA type TEE_RAM_RX 0xbdb00100 size 0x00024f00
DEBUG:   TEE-CORE:add_phys_mem:325: CONSOLE_UART_BASE type IO_NSEC 0x48000000 size 0x00200000
DEBUG:   TEE-CORE:add_phys_mem:325: GICD_BASE type IO_SEC 0x48200000 size 0x00200000
DEBUG:   TEE-CORE:add_phys_mem:325: GICC_BASE type IO_SEC 0x48200000 size 0x00200000
DEBUG:   TEE-CORE:add_phys_mem:338: Physical mem map overlaps 0x48200000
DEBUG:   TEE-CORE:add_phys_mem:325: SECRAM_BASE type IO_SEC 0x40200000 size 0x00200000
DEBUG:   TEE-CORE:add_phys_mem:325: TZDRAM_BASE type RAM_SEC 0xbdb00000 size 0x00100000
DEBUG:   TEE-CORE:add_phys_mem:325: RNG_BASE type IO_SEC 0x48000000 size 0x00200000
DEBUG:   TEE-CORE:add_va_space:364: type RES_VASPACE size 0x01400000
DEBUG:   TEE-CORE:add_va_space:364: type SHM_VASPACE size 0x01400000
ERROR:   TEE-CORE: Panic

It seems to have to do with our platforms having the start address +0x100 into our TEE_RAM_START area:

#define CFG_TEE_LOAD_ADDR       (CFG_TEE_RAM_START + 0x100)

DEBUG:   TEE-CORE:add_phys_mem:325: VCORE_UNPG_RX_PA type TEE_RAM_RX 0xbdb00100 size 0x00024f00

Removing this offset fixes the issue, I don't see why having this offset is not valid anymore? It seems that this first 0x100 bytes is no longer mapped as we assume the TEE_LOAD address is always the start of TEE_RAM, which is true for other platforms but not ours.

The two possible fixes then are to combine CFG_TEE_LOAD_ADDR and CFG_TEE_RAM_START to force these to be the same. Or set VCORE_UNPG_RO_PA to also include any space before CFG_TEE_LOAD_ADDR but after CFG_TEE_RAM_START.

Unless I'm not understanding the issue correctly (all this memory stuff has gotten very complex).

[glneo]

After further debugging it seems what it is really upset about is that this offset start address is not on a page boundary. CFG_TEE_RAM_START is on a page boundary, but OP-TEE start address is not, if the start address now needs to also aligned then I'll move our start address, but this should be documented.

[etienne-lms]

My apologies, indeed there is a side effect of recently merged #1459 we did not see. In your case, the physical RAM below the load address should be mapped read-only-executable? read-write? both?

Do you need this RAM to be flat map (pa=va) ?
Do you need the RAM to be identified as "TEE_RAM" (from the core_pbuf_is() and friends).

We must fix current optee_os to allow VCORE_UNPG_RX_PA to be CFG_TEE_LOAD_ADDR rounded down to the page alignment. This will fix the TI case with pager enabled.

In case platform expects some flat mapped memory between CFG_TEE_RAM_START and CFG_TEE_LOAD_ADDR, we must find a clean way for platform to define the memory access attributes they expect.

P-R for coherent memory allows to flat map this area but without cache support so that may not fit your needs.

[glneo]

We actually don't need this memory mapped at all, we used to need it for storing the OP-TEE header, but now we do not. The issue seems to be that VCORE_UNPG_RX_PA cannot be un-aligned, but when CFG_TEE_LOAD_ADDR is un-aligned then it will be. I think for us we should be fine with simply moving CFG_TEE_LOAD_ADDR to CFG_TEE_RAM_START, restoring the alignment. RPI3 also has an offset CFG_TEE_LOAD_ADDR but it is page aligned already, so this may not be an issue for them.

I'll do a bit more testing then I'll push a patch for this.

[etienne-lms]

Ok, i will propose something to allow CFG_TEE_LOAD_ADDR to be not paged aligned.

In the meantime, you can set CFG_CORE_RWDATA_NOEXEC=n from plat-ti/conf.mk to get back the legacy mapping layout (TEE_RAM mapping rwx). At least your optee will be functional.

[glneo]

I'm actually okay with having the start address page aligned, it was just unexpected and broke our platforms (and rather hard to debug). I think having this rounding down patch works also as someone will probably do what we did when porting to a new platform and get very confused.

[etienne-lms]

I think load address may not need to be paged aligned. It may help some boot setup. Hence #1558.
In the mean time, with #1557, feel free to close this issue once you feel the optee state is satisfactory.