libmbedtls: use mempool_calloc() for temporary memory

[jenswi-linaro]

mbedtls_mpi_exp_mod_optionally_safe() need a large chunk of temporary memory for the mbedtls_mpi_core_exp_mod() function. The amount of memory is too much to to reliably allocate from the heap. So use mempool_calloc() instead of mbedtls_calloc(), similar to using mbedtls_mpi_init_mempool() instead of mbedtls_mpi_init().

[jenswi-linaro]

This should fix the recent random errors with MEASURED_BOOT_FTPM=y

[jforissier]

Good catch!

Acked-by: Jerome Forissier <[email protected]>

Would you mind also pushing a temporary commit to enable fTPM in the CI? I mean similar to this one: 718c357 but with MEASURED_BOOT_FTPM=y instead of =n. If all is good then we know we can re-instate =y by default in build.git.

I am also running my release tests. I will post a Tested-by once they're good. Thanks!

[jenswi-linaro]

It looks like fTPM uses a lot of heap:

xtest --stats --alloc (with MEASURED_BOOT_FTPM=n)
Pool:                Heap       
Bytes allocated:     20704

xtest --stats --alloc (with MEASURED_BOOT_FTPM=y)
Pool:                Heap
Bytes allocated:     44288

I'm trying to figure out where the memory went.

[etienne-lms]

fTPM has quite many objects kept opened. See microsoft/ms-tpm-20-ref@1b288eb. IHIH.

[jenswi-linaro]

It looks like fTPM has a few objects open in secure storage. There are many small allocations that may have caused fragmentation of the heap.

[jenswi-linaro]

Thanks, Etienne, there are those 20kB I also noted.

[jenswi-linaro]

@etienne-lms, can you create a PR against https://github.com/OP-TEE/optee_ftpm with those patches?

[etienne-lms]

Ok, i'll do.

[jenswi-linaro]

Replacing another malloc() with mempool_alloc().

[jforissier]

For "core: pta: secstore: use mempool_alloc()":

Reviewed-by: Jerome Forissier <[email protected]>

[etienne-lms]

Reviewed-by: Etienne Carriere <[email protected]> for commits
"libmbedtls: use mempool_calloc() for temporary memory" (s/need/needs/ in commit message)
and "core: pta: secstore: use mempool_alloc()".

[etienne-lms]

Enable BestFit or increase the heap size?

[jenswi-linaro]

I'll try to increase the heap also.

[etienne-lms]

I think using the secure DDR mempool for allocating a 8kByte buffer makes sense, especially when the heap is quite small, likely to be when pager is enabled. I would be in favor of not reverting "core: pta: secstore: use mempool_alloc()".

[jenswi-linaro]

That commit introduced a possible deadlock discovered with CFG_LOCKDEP=y. The problem is that normally, while the tadb_mutex is held, we also use the mempool (taking mempool_default->mu), but in install_ta(), the order is reversed.

I'm waiting for the CI checks to pass. Assuming they do what should be the next step?
The fTPM TA is active due to probing by the kernel during a few of the first tests with make check so fTPM is likely to peak in memory usage. I've just tried with a 1kB buffer in install_ta() and I didn't notice that the function took much more time. So 1Kb should be fine and that's also a reasonable heap allocation.

[etienne-lms]

I agree 1kB in the heap is affordable. It would multiply by 8 the TEE/REE world switches when installing a TA but I think it's ok since installing a TA is not performance critical.

[jenswi-linaro]

The "CI / make check (QEMUv8) 1 / 2 (pull_request)" error is with CFG_FTRACE_SUPPORT=y CFG_SYSCALL_FTRACE=y and the secure world is quite slow with this. Probing fTPM takes a long time and keyctl tests times out because of this. I think we should disable fTPM with CFG_FTRACE_SUPPORT=y CFG_SYSCALL_FTRACE=y.

[jforissier]

The "CI / make check (QEMUv8) 1 / 2 (pull_request)" error is with CFG_FTRACE_SUPPORT=y CFG_SYSCALL_FTRACE=y and the secure world is quite slow with this. Probing fTPM takes a long time and keyctl tests times out because of this. I think we should disable fTPM with CFG_FTRACE_SUPPORT=y CFG_SYSCALL_FTRACE=y.

Sounds reasonable.

[jforissier]

For "ci: QEMUv8: disable fTPM with ftrace":
Reviewed-by: Jerome Forissier <[email protected]>

[jforissier]

For "core: pta: secstore: decrease TA buffer":
Acked-by: Jerome Forissier <[email protected]>

[jenswi-linaro]

Tags applied and fingers crossed. :-)

[jenswi-linaro]

The core heap usage is increased by around 20kB with fTPM enabled so it makes sense if this has to be compensated.

[jforissier]

For "plat-vexpress: increase QEMU heap size":
Reviewed-by: Jerome Forissier <[email protected]>

The core heap usage is increased by around 20kB with fTPM enabled so it makes sense if this has to be compensated.

Perhaps mention this in the commit message?

[etienne-lms]

Reviewed-by: Etienne Carriere <[email protected]> for commit
"core: pta: secstore: decrease TA buffer".

Acked-by: Etienne Carriere <[email protected]> for commit
"ci: QEMUv8: disable fTPM with ftrace".

Regarding "plat-vexpress: increase QEMU heap size": better explicit why it's increased as @jforissier suggests in case you want to step back becasue ftpm P-R#4 (or like) gets merged and this extra heap will no longer be needed.

[jforissier]

I could not reproduce the CI failure locally (timeout in regression_1006 when pager is enabled). I restarted the job. We might have some race condition or yet another memory shortage perhaps?

[jenswi-linaro]

regression_1006 might be executed while fTPM is still performing self-tests, which will put some pressure on the pager. Can we increase the timeout value when the pager is enabled?

[jenswi-linaro]

Comments addressed and tags applied. I've dropped "[DO NOT MERGE] ci: QEMUv8: set MEASURED_BOOT_FTPM=y".
We could disable tests with fTPM and paging if it turns out that we have sporadic timeouts with that configuration.

[jforissier]

This is OK for merging. @jenswi-linaro could you please enable fTPM again in build.git? Thanks!

[jenswi-linaro]

Sure, OP-TEE/build#797