Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Arvind Sankar <niveditas98@gmail.com>
From:   Arvind Sankar <nivedita@alum.mit.edu>
Date:   Sat, 10 Oct 2020 15:26:24 -0400
To:     Arvind Sankar <nivedita@alum.mit.edu>
Cc:     x86@kernel.org, Joerg Roedel <jroedel@suse.de>,
        Borislav Petkov <bp@alien8.de>, linux-kernel@vger.kernel.org,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier
Message-ID: <20201010192624.GA4062867@rani.riverdale.lan>
References: <20201008191623.2881677-1-nivedita@alum.mit.edu>
 <20201010191110.4060905-1-nivedita@alum.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20201010191110.4060905-1-nivedita@alum.mit.edu>
Precedence: bulk

On Sat, Oct 10, 2020 at 03:11:10PM -0400, Arvind Sankar wrote:
> Commit
>   ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
> started using a new set of pagetables even without KASLR.
> 
> After that commit, initialize_identity_maps() is called before the
> 5-level paging variables are setup in choose_random_location(), which
> will not work if 5-level paging is actually enabled.

Note that I don't have hardware that supports 5-level paging, so this
is not actually tested with 5-level, but based on code inspection, it
shouldn't work.

> 
> Fix this by moving the initialization of __pgtable_l5_enabled,
> pgdir_shift and ptrs_per_p4d into cleanup_trampoline(), which is called
> immediately after the finalization of whether the kernel is executing
> with 4- or 5-level paging. This will be earlier than anything that might
> require those variables, and keeps the 4- vs 5-level paging code all in
> one place.
> 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/ident_map_64.c |  6 ------
>  arch/x86/boot/compressed/kaslr.c        |  8 --------
>  arch/x86/boot/compressed/pgtable_64.c   | 16 ++++++++++++++++
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
> index a3613857c532..505d6299b76e 100644
> --- a/arch/x86/boot/compressed/ident_map_64.c
> +++ b/arch/x86/boot/compressed/ident_map_64.c
> @@ -34,12 +34,6 @@
>  #define __PAGE_OFFSET __PAGE_OFFSET_BASE
>  #include "../../mm/ident_map.c"
>  
> -#ifdef CONFIG_X86_5LEVEL
> -unsigned int __pgtable_l5_enabled;
> -unsigned int pgdir_shift = 39;
> -unsigned int ptrs_per_p4d = 1;
> -#endif
> -
>  /* Used by PAGE_KERN* macros: */
>  pteval_t __default_kernel_pte_mask __read_mostly = ~0;
>  
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 489fabc051d7..9eabd8bc7673 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -835,14 +835,6 @@ void choose_random_location(unsigned long input,
>  		return;
>  	}
>  
> -#ifdef CONFIG_X86_5LEVEL
> -	if (__read_cr4() & X86_CR4_LA57) {
> -		__pgtable_l5_enabled = 1;
> -		pgdir_shift = 48;
> -		ptrs_per_p4d = 512;
> -	}
> -#endif
> -
>  	boot_params->hdr.loadflags |= KASLR_FLAG;
>  
>  	if (IS_ENABLED(CONFIG_X86_32))
> diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
> index 46d761f7faa6..0976c2d2ab2f 100644
> --- a/arch/x86/boot/compressed/pgtable_64.c
> +++ b/arch/x86/boot/compressed/pgtable_64.c
> @@ -9,6 +9,13 @@
>  #define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
>  #define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */
>  
> +#ifdef CONFIG_X86_5LEVEL
> +/* __pgtable_l5_enabled needs to be in .data to avoid being cleared along with .bss */
> +unsigned int __section(.data) __pgtable_l5_enabled;
> +unsigned int __section(.data) pgdir_shift = 39;
> +unsigned int __section(.data) ptrs_per_p4d = 1;
> +#endif
> +
>  struct paging_config {
>  	unsigned long trampoline_start;
>  	unsigned long l5_required;
> @@ -195,4 +202,13 @@ void cleanup_trampoline(void *pgtable)
>  
>  	/* Restore trampoline memory */
>  	memcpy(trampoline_32bit, trampoline_save, TRAMPOLINE_32BIT_SIZE);
> +
> +	/* Initialize variables for 5-level paging */
> +#ifdef CONFIG_X86_5LEVEL
> +	if (__read_cr4() & X86_CR4_LA57) {
> +		__pgtable_l5_enabled = 1;
> +		pgdir_shift = 48;
> +		ptrs_per_p4d = 512;
> +	}
> +#endif
>  }
> -- 
> 2.26.2
>