On 25/05/2023 4:28 pm, Oleksii Kurochko wrote: > Oleksii Kurochko (5): > xen/riscv: add VM space layout > xen/riscv: introduce setup_initial_pages > xen/riscv: align __bss_start > xen/riscv: setup initial pagetables > xen/riscv: remove dummy_bss variable
These have just been committed. But as I fed back to early drafts of this series, patch 2 is sufficiently fragile and unwise as to be unacceptable in this form. enable_mmu() is unsafe in multiple ways, from the compiler reordering statements (the label needs to be in the asm statement for that to work correctly), and because it * depends* on hooking all exceptions and pagefault. Any exception other than pagefault, or not taking a pagefault causes it to malfunction, which means you will fail to boot depending on where Xen was loaded into memory. It may not explode inside Qemu right now, but it will not function reliably in the general case. Furthermore, a combination of patch 2 and 4 breaks the CI integration of looking for "All set up" at the end of start_xen(). It's not ok, from a code quality point of view, to defer 99% of start_xen()'s functionality into unrelated function. Please do not do anything else until you've addressed these issues. enable_mmu() needs to return normally, cont_after_mmu_is_enabled() needs deleting entirely, and there needs to be an identity page for Xen to land on so it isn't jumping into the void and praying not to explode. Other minor issues include page.h not having __ASSEMBLY__ guards, mm.c locally externing cpu0_boot_stack[] from setup.c when the declaration needs to be in a header file somewhere, and SPDX tags. ~Andrew
