Re: [BROKEN] Re: [PATCH v9 0/5] enable MMU for RISC-V

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-05-31 at 12:45 +0100, Andrew Cooper wrote:
> 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.
Linux RISC-V has the similar approach and it doesn't explode but if it
will be better to use identity map then I'll re-write it.

> 
> 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.
In this case enable_mmu() should be called before start_xen() function
because if start_xen() has local variables then after jumping to VA and
removing identity page we will have an issue with stack pointer as it
will contain addresses where Xen was loaded.
One more enable_mmu() can't have local variable too ( as stack pointer 
can be somewhere outside one page used for identity mapping ) and it
will cause an issue when execute epilogue of enable_mmu() function.

Or am I missing something?

> 
> 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.
Thanks. I'll take it into account.

~ Oleksii