fix(mem): fix hypervisor self-coloring boot hang - #391
Conversation
Enabling hypervisor coloring aborts in mem_color_hypervisor(), before the console is up, so the failure shows as a silent hang. The bitmap is copied as a standalone region and mapped in the new address space at its original virtual address. That address, however, belongs to the image, as pp_bitmap_alloc() carves the bitmap out of a static pool, so it is already taken by the SEC_HYP_IMAGE mapping. mem_alloc_vpage() returns INVALID_VA and the "Can't allocate address" error is reached. The matching cleanup at the end of the function is stale for the same reason: it frees a range past the CPU regions where no bitmap was ever placed, which belongs to the colored page pool by then. Since the bitmap lives inside the image, it is relocated by the image copy already. What that copy misses is the page tables allocated afterwards to map the image in the new address space, which the bitmap must account for. Refresh it by re-copying the image over the mapping copy_space() left behind, once every CPU is past the barrier and no allocation is left to be done. This keeps the property that the bitmap is the last thing copied and needs no allocation of its own, which would not be tracked by the bitmap being copied. Also check that the bitmap is part of the image, so that a future change to how it is allocated does not silently reintroduce the problem. Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>
With hypervisor coloring enabled, the boot hangs before any console output on armv8-a aarch64. mem_color_hypervisor() relocates the image, .text included, with a plain data-side copy, and then calls switch_space() to repoint TTBR0_EL2 at the colored pages and return into that copy. Nothing makes the relocated instructions visible to instruction fetch: the copy may still sit in the data cache above the PoC, and the PE may hold instruction cache lines for the new physical pages, so the fetches after the switch execute stale memory. The existing cache_flush_range() after switch_space() cannot serve this purpose, as execution already resumed from those pages, and it only cleans the data side. Clean the relocated image to the PoC through the temporary mapping, since the image virtual addresses still translate to the original pages at that point. Invalidate the instruction cache in switch_space(), so that every PE does it, just before writing TTBR0_EL2, while the code being executed still comes from the original pages. Builds without hypervisor coloring are unaffected, as mem_color_hypervisor() only runs when config.hyp.colors is set. Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>
91dc4a6 to
1f2a8a0
Compare
|
Thanks @rabara! This feature has not been used that much lately, so the issue slipped through when we modified the memory layout and allocations to accommodate MCU architectures. Just about to add a test infrastructure finally #363, so we should probably add tests which include setting up hypervisor colors. Both fixes look good to me. I went through the allocation flow and the invariant the recopy leans on holds: every allocation happens before the barrier, so the refreshed image picks up the final bitmap along with the rest of the pool bookkeeping. The removed cleanup was also zeroing pages that by now belong to the free pool, so that was a corruption waiting to happen. The check that the bitmap lives inside the image is a nice tripwire for future changes to how it is allocated. On the cache side, the sequence is the right one: clean through the temporary mapping while the image virtual addresses still translate to the old pages, and the per PE icache invalidate in switch_space before the TTBR write. |
| /** | ||
| * Refresh the root page pool bitmap in the colored image. | ||
| * | ||
| * The bitmap lives inside the image, so the copy above already relocated it, but that | ||
| * snapshot is stale: it predates the page tables allocated to map the image in the new | ||
| * address space. Every CPU is past the barrier and no allocation is left to be done, so | ||
| * re-copy the image over the mapping copy_space() left behind to pick up the final bitmap, | ||
| * along with the rest of the page pool bookkeeping. Copying the image again takes no | ||
| * allocation, which a separate bitmap copy would, and that allocation would no longer be | ||
| * tracked by the bitmap being copied. | ||
| */ | ||
| memcpy(image_cpy, &_image_start, image_size); |
There was a problem hiding this comment.
With this, we memcpy the whole image twice. Once in copy_space(), then again after the barrier to pick up the final bitmap. I think we can skip the first copy altogether (without skipping the allocation and mapping).
So I'm proposing we split the operations of copy_space() (alloc+map and copy) to save an additional image memcpy on boot.
| /** | ||
| * Invalidate the instruction cache. | ||
| * | ||
| * This routine returns into a copy of the image the caller relocated with a data-side copy and | ||
| * cleaned to the PoC. The image virtual addresses are about to be repointed at that copy, so | ||
| * drop any instruction cache lines this PE holds for the new physical pages, otherwise the | ||
| * fetches after the switch may hit stale instructions. | ||
| */ | ||
| ic iallu | ||
| dsb ish | ||
| isb | ||
|
|
There was a problem hiding this comment.
Could the ic iallu just go after the tlbi alle2 in the existing block? They don't depend on each other and both do a dsb+isb
3 participants
Summary
pp_bitmap_alloc(), not a standalone SEC_HYP_GLOBAL region).ic ialluinswitch_space()).Test
PLATFORM=qemu-aarch64-virt DEMO=baremetalwithconfig.hyp.colorssetgo 0x50000000; fixed build prints Bao banner and boots guest