Re: [PATCH 3/4] x86/boot/compressed/64: Introduce place_trampoline()

[Ingo Molnar]

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

> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -315,6 +315,18 @@ ENTRY(startup_64)
>  	 * The first step is go into compatibility mode.
>  	 */
>  
> +	/*
> +	 * Find suitable place for trampoline and populate it.
> +	 * The address will be stored in RCX.
> +	 *
> +	 * RSI holds real mode data and need to be preserved across
> +	 * a function call.
> +	 */
> +	pushq	%rsi
> +	call	place_trampoline
> +	popq	%rsi
> +	movq	%rax, %rcx
> +
>  	/* Clear additional page table */
>  	leaq	lvl5_pgtable(%rbx), %rdi
>  	xorq	%rax, %rax

So in the final version of this code we now have:

	pushq	%rsi
	call	need_to_enabled_l5
	popq	%rsi

	/* If need_to_enabled_l5() returned zero, we're done here. */
	cmpq	$0, %rax
	je	lvl5

	/*
	 * At this point we are in long mode with 4-level paging enabled,
	 * but we want to enable 5-level paging.
	 *
	 * The problem is that we cannot do it directly. Setting LA57 in
	 * long mode would trigger #GP. So we need to switch off long mode
	 * first.
	 *
	 * We use trampoline in lower memory to handle situation when
	 * bootloader put the kernel image above 4G.
	 *
	 * The first step is go into compatibility mode.
	 */

	/*
	 * Find suitable place for trampoline and populate it.
	 * The address will be stored in RCX.
	 *
	 * RSI holds real mode data and need to be preserved across
	 * a function call.
	 */
	pushq	%rsi
	call	place_trampoline
	popq	%rsi
	movq	%rax, %rcx

Firstly, the 'need_to_enabled_l5' name sucks because it includes a typo, but also 
because the prefix is way too generic.

Something like:

	l5_paging_required()

would read a lot better - and would also provide a namespace for all L5 paging 
related functions.

Secondly, couldn't this be combined into a single .c function, named accordingly:

	l5_paging_prepare()

which would return true if L5 paging is available and should be enabled. In this 
case the trampoline copying function would be called in C, by l5_paging_prepare().

This further reduces the amount of assembly code.

Thanks,

	Ingo