HiKey960, pager: SDP test hangs (xtest 1014.3)

[jforissier]

Building for HiKey960 with command: make -j10 CFG_WITH_PAGER=y CFG_TEE_CORE_EMBED_INTERNAL_TESTS=y CFG_TEE_CORE_LOG_LEVEL=2
SDP test 1014.3 hangs and cannot be interrupted with Ctrl+C:

buildroot login: test
$ xtest 1014
[...]
o regression_1014.3 SDP: SDP TA invokes a test pTA (invoke_tests.pta)
^C^C^C

Bisected down to commit d6ad67f ("core: mm: change vm_pa2va() to return a virtual address").

[jforissier]

So apparently commit d6ad67f ("core: mm: change vm_pa2va() to return a virtual address") did not only change the return type, it changed the logic too. The following patches fixes the issue for me, @etienne-lms can you please check if that makes sense? Thanks.

diff --git a/core/mm/vm.c b/core/mm/vm.c
index a6c59861..4cd491c5 100644
--- a/core/mm/vm.c
+++ b/core/mm/vm.c
@@ -1198,8 +1198,6 @@ void *vm_pa2va(const struct user_mode_ctx *uctx, paddr_t pa)
 		assert(!granule || IS_POWER_OF_TWO(granule));
 
 		for (ofs = region->offset; ofs < region->size; ofs += size) {
-			if (mobj_get_pa(region->mobj, ofs, granule, &p))
-				continue;
 
 			if (granule) {
 				/* From current offset to buffer/granule end */
@@ -1211,6 +1209,9 @@ void *vm_pa2va(const struct user_mode_ctx *uctx, paddr_t pa)
 				size = region->size;
 			}
 
+			if (mobj_get_pa(region->mobj, ofs, granule, &p))
+				return NULL;
+
 			if (core_is_buffer_inside(pa, 1, p, size)) {
 				/* Remove region offset (mobj phys offset) */
 				ofs -= region->offset;

[etienne-lms]

thanks for the report. Indeed, i was too quick on moving if (mobj_get_pa(...)) before size update.
However

			if (mobj_get_pa(region->mobj, ofs, granule, &p))
				return NULL;

should be replaced with:

			if (mobj_get_pa(region->mobj, ofs, granule, &p))
				continue;

I think, since since phys address can be found for the area, the loop should try next areas, eventually not find any suitable one and ending with the final return NULL; at function bottom.
Makes sense to you?

I'll test that on Qemu with SDP.

[jforissier]

@etienne-lms yes that makes sense and works in my tests too.

[jforissier]

#4508

I have tested xtest 1014 on QEMU too (with CFG_WITH_PAGER=y CFG_TEE_CORE_EMBED_INTERNAL_TESTS=y CFG_SECURE_DATA_PATH=y).