Re: [PATCHv3, RFC] x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G

[Ingo Molnar]

* Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx> wrote:

> [
>   The patch is based on my boot-time switching patchset and would not apply
>   directly to current upstream, but I would appreciate early feedback.
> ]
> 
> This patch addresses shortcoming in current boot process on machines
> that supports 5-level paging.
> 
> If bootloader enables 64-bit mode with 4-level paging, we need to
> switch over to 5-level paging. The switching requires disabling paging.
> It works fine if kernel itself is loaded below 4G.
> 
> If bootloader put the kernel above 4G (not sure if anybody does this),
> we would loose control as soon as paging is disabled as code becomes
> unreachable.
> 
> This patch implements trampoline in lower memory to handle this
> situation.
> 
> Apart from trampoline itself we also need place to store top level page
> table in lower memory as we don't have a way to load 64-bit value into
> CR3 from 32-bit mode. We only really need 8-bytes there as we only use
> the very first entry of the page table. but we allocate whole page
> anyway. We cannot have the code in the same because, there's hazard that
> a CPU would read page table speculatively and get confused seeing
> garbage.
> 
> We only need the memory for very short time, until main kernel image
> setup its own page tables.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/boot/compressed/head_64.S   | 83 ++++++++++++++++++++++--------------
>  arch/x86/boot/compressed/pagetable.c | 45 +++++++++++++++++++
>  arch/x86/boot/compressed/pagetable.h | 16 +++++++
>  3 files changed, 111 insertions(+), 33 deletions(-)
>  create mode 100644 arch/x86/boot/compressed/pagetable.h

Yeah, things like this is what I'd like to see, but could we please structure it a 
bit differently. Splitting it up more is very important, as 100+ lines difficult 
commits are difficult to debug after the fact. I'd suggest the following split-up:

 patch 1: introduce place_trampoline(), call it from the assembly - but don't do anything
 patch 2: move as much existing assembly code to C code in place_trampoline() as possible
 patch 3: modify remaining assembly code for dynamic 5-level pagetable support
 patch 4: modify place_trampoline() for dynamic 5-level pagetable support

Also, if you can think of more boot code in that file to move to C reasonably, 
please do it, even if it's unrelated to 5-level paging at the moment. The more 
boot assembly code you manage to move to C, the better x86 maintainers will like 
your patch-set. We are easy to corrupt with such patches! :-)

Thanks,

	Ingo