On Fri, Nov 10, 2017 at 10:29:33AM +0100, Ingo Molnar wrote:
> 
> * Kirill A. Shutemov <[email protected]> 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.

Makes sense.

-- 
 Kirill A. Shutemov

Reply via email to