Re: [PATCH 1/3] x86/mm/encrypt: Move sme_populate_pgd*() into separate translation unit

[Tom Lendacky]

On 1/23/2018 11:19 AM, Kirill A. Shutemov wrote:
> sme_populate_pgd() and sme_populate_pgd_large() operate on the identity
> mapping, which means they want virtual addresses to be equal to physical
> one, without PAGE_OFFSET shift.
> 
> We also need to avoid paravirtualizaion call there.
> 
> Getting this done is tricky. We cannot use usual page table helpers.
> It forces us to open-code a lot of things. It makes code ugly and hard
> to modify.
> 
> We can get it work with the page table helpers, but it requires few
> preprocessor tricks. These tricks may have side effects for the rest of
> the file.
> 
> Let's isolate sme_populate_pgd() and sme_populate_pgd_large() into own
> translation unit.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/mm/Makefile               |  13 ++--
>  arch/x86/mm/mem_encrypt.c          | 129 -----------------------------------
>  arch/x86/mm/mem_encrypt_identity.c | 134 +++++++++++++++++++++++++++++++++++++
>  arch/x86/mm/mm_internal.h          |  14 ++++
>  4 files changed, 156 insertions(+), 134 deletions(-)
>  create mode 100644 arch/x86/mm/mem_encrypt_identity.c
> 
> diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
> index 27e9e90a8d35..51e364ef12d9 100644
> --- a/arch/x86/mm/Makefile
> +++ b/arch/x86/mm/Makefile
> @@ -1,12 +1,14 @@
>  # SPDX-License-Identifier: GPL-2.0
> -# Kernel does not boot with instrumentation of tlb.c and mem_encrypt.c
> -KCOV_INSTRUMENT_tlb.o		:= n
> -KCOV_INSTRUMENT_mem_encrypt.o	:= n
> +# Kernel does not boot with instrumentation of tlb.c and mem_encrypt*.c
> +KCOV_INSTRUMENT_tlb.o			:= n
> +KCOV_INSTRUMENT_mem_encrypt.o		:= n
> +KCOV_INSTRUMENT_mem_encrypt_identity.o	:= n
>  
> -KASAN_SANITIZE_mem_encrypt.o	:= n
> +KASAN_SANITIZE_mem_encrypt.o		:= n
> +KASAN_SANITIZE_mem_encrypt_identity.o	:= n
>  
>  ifdef CONFIG_FUNCTION_TRACER
> -CFLAGS_REMOVE_mem_encrypt.o	= -pg

You found this one already.

> +CFLAGS_REMOVE_mem_encrypt_identity.o	= -pg
>  endif
>  
>  obj-y	:=  init.o init_$(BITS).o fault.o ioremap.o extable.o pageattr.o mmap.o \
> @@ -47,4 +49,5 @@ obj-$(CONFIG_RANDOMIZE_MEMORY)			+= kaslr.o
>  obj-$(CONFIG_PAGE_TABLE_ISOLATION)		+= pti.o
>  
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt.o
> +obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_identity.o
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_boot.o
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index e1d61e8500f9..740b8a54f616 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -464,18 +464,6 @@ void swiotlb_set_mem_attributes(void *vaddr, unsigned long size)
>  	set_memory_decrypted((unsigned long)vaddr, size >> PAGE_SHIFT);
>  }
>  
> -struct sme_populate_pgd_data {
> -	void	*pgtable_area;
> -	pgd_t	*pgd;
> -
> -	pmdval_t pmd_flags;
> -	pteval_t pte_flags;
> -	unsigned long paddr;
> -
> -	unsigned long vaddr;
> -	unsigned long vaddr_end;
> -};
> -
>  static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)

If you're going to move some of the functions, did you look at moving the
sme_enable(), sme_encrypt_kernel(), etc., too?  I believe everything below
the sme_populate_pgd_data structure is used during early identity-mapped
boot time. If you move everything, then mm_internal.h doesn't need to be
updated and all of the identity-mapped early boot code ends up in one
file.

You'd have to move the command line declarations and make sev_enabled
not a static, but it should be doable.

Thanks,
Tom

>  {
>  	unsigned long pgd_start, pgd_end, pgd_size;
> @@ -491,11 +479,6 @@ static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
>  	memset(pgd_p, 0, pgd_size);
>  }
>  
> -#define PGD_FLAGS		_KERNPG_TABLE_NOENC
> -#define P4D_FLAGS		_KERNPG_TABLE_NOENC
> -#define PUD_FLAGS		_KERNPG_TABLE_NOENC
> -#define PMD_FLAGS		_KERNPG_TABLE_NOENC
> -
>  #define PMD_FLAGS_LARGE		(__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
>  
>  #define PMD_FLAGS_DEC		PMD_FLAGS_LARGE
> @@ -512,118 +495,6 @@ static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
>  
>  #define PTE_FLAGS_ENC		(PTE_FLAGS | _PAGE_ENC)
>  
> -static pmd_t __init *sme_prepare_pgd(struct sme_populate_pgd_data *ppd)
> -{
> -	pgd_t *pgd_p;
> -	p4d_t *p4d_p;
> -	pud_t *pud_p;
> -	pmd_t *pmd_p;
> -
> -	pgd_p = ppd->pgd + pgd_index(ppd->vaddr);
> -	if (native_pgd_val(*pgd_p)) {
> -		if (IS_ENABLED(CONFIG_X86_5LEVEL))
> -			p4d_p = (p4d_t *)(native_pgd_val(*pgd_p) & ~PTE_FLAGS_MASK);
> -		else
> -			pud_p = (pud_t *)(native_pgd_val(*pgd_p) & ~PTE_FLAGS_MASK);
> -	} else {
> -		pgd_t pgd;
> -
> -		if (IS_ENABLED(CONFIG_X86_5LEVEL)) {
> -			p4d_p = ppd->pgtable_area;
> -			memset(p4d_p, 0, sizeof(*p4d_p) * PTRS_PER_P4D);
> -			ppd->pgtable_area += sizeof(*p4d_p) * PTRS_PER_P4D;
> -
> -			pgd = native_make_pgd((pgdval_t)p4d_p + PGD_FLAGS);
> -		} else {
> -			pud_p = ppd->pgtable_area;
> -			memset(pud_p, 0, sizeof(*pud_p) * PTRS_PER_PUD);
> -			ppd->pgtable_area += sizeof(*pud_p) * PTRS_PER_PUD;
> -
> -			pgd = native_make_pgd((pgdval_t)pud_p + PGD_FLAGS);
> -		}
> -		native_set_pgd(pgd_p, pgd);
> -	}
> -
> -	if (IS_ENABLED(CONFIG_X86_5LEVEL)) {
> -		p4d_p += p4d_index(ppd->vaddr);
> -		if (native_p4d_val(*p4d_p)) {
> -			pud_p = (pud_t *)(native_p4d_val(*p4d_p) & ~PTE_FLAGS_MASK);
> -		} else {
> -			p4d_t p4d;
> -
> -			pud_p = ppd->pgtable_area;
> -			memset(pud_p, 0, sizeof(*pud_p) * PTRS_PER_PUD);
> -			ppd->pgtable_area += sizeof(*pud_p) * PTRS_PER_PUD;
> -
> -			p4d = native_make_p4d((pudval_t)pud_p + P4D_FLAGS);
> -			native_set_p4d(p4d_p, p4d);
> -		}
> -	}
> -
> -	pud_p += pud_index(ppd->vaddr);
> -	if (native_pud_val(*pud_p)) {
> -		if (native_pud_val(*pud_p) & _PAGE_PSE)
> -			return NULL;
> -
> -		pmd_p = (pmd_t *)(native_pud_val(*pud_p) & ~PTE_FLAGS_MASK);
> -	} else {
> -		pud_t pud;
> -
> -		pmd_p = ppd->pgtable_area;
> -		memset(pmd_p, 0, sizeof(*pmd_p) * PTRS_PER_PMD);
> -		ppd->pgtable_area += sizeof(*pmd_p) * PTRS_PER_PMD;
> -
> -		pud = native_make_pud((pmdval_t)pmd_p + PUD_FLAGS);
> -		native_set_pud(pud_p, pud);
> -	}
> -
> -	return pmd_p;
> -}
> -
> -static void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
> -{
> -	pmd_t *pmd_p;
> -
> -	pmd_p = sme_prepare_pgd(ppd);
> -	if (!pmd_p)
> -		return;
> -
> -	pmd_p += pmd_index(ppd->vaddr);
> -	if (!native_pmd_val(*pmd_p) || !(native_pmd_val(*pmd_p) & _PAGE_PSE))
> -		native_set_pmd(pmd_p, native_make_pmd(ppd->paddr | ppd->pmd_flags));
> -}
> -
> -static void __init sme_populate_pgd(struct sme_populate_pgd_data *ppd)
> -{
> -	pmd_t *pmd_p;
> -	pte_t *pte_p;
> -
> -	pmd_p = sme_prepare_pgd(ppd);
> -	if (!pmd_p)
> -		return;
> -
> -	pmd_p += pmd_index(ppd->vaddr);
> -	if (native_pmd_val(*pmd_p)) {
> -		if (native_pmd_val(*pmd_p) & _PAGE_PSE)
> -			return;
> -
> -		pte_p = (pte_t *)(native_pmd_val(*pmd_p) & ~PTE_FLAGS_MASK);
> -	} else {
> -		pmd_t pmd;
> -
> -		pte_p = ppd->pgtable_area;
> -		memset(pte_p, 0, sizeof(*pte_p) * PTRS_PER_PTE);
> -		ppd->pgtable_area += sizeof(*pte_p) * PTRS_PER_PTE;
> -
> -		pmd = native_make_pmd((pteval_t)pte_p + PMD_FLAGS);
> -		native_set_pmd(pmd_p, pmd);
> -	}
> -
> -	pte_p += pte_index(ppd->vaddr);
> -	if (!native_pte_val(*pte_p))
> -		native_set_pte(pte_p, native_make_pte(ppd->paddr | ppd->pte_flags));
> -}
> -
>  static void __init __sme_map_range_pmd(struct sme_populate_pgd_data *ppd)
>  {
>  	while (ppd->vaddr < ppd->vaddr_end) {
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> new file mode 100644
> index 000000000000..dbf7a98f657d
> --- /dev/null
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -0,0 +1,134 @@
> +/*
> + * AMD Memory Encryption Support
> + *
> + * Copyright (C) 2016 Advanced Micro Devices, Inc.
> + *
> + * Author: Tom Lendacky <thomas.lendacky@xxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define DISABLE_BRANCH_PROFILING
> +
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include "mm_internal.h"
> +
> +#define PGD_FLAGS		_KERNPG_TABLE_NOENC
> +#define P4D_FLAGS		_KERNPG_TABLE_NOENC
> +#define PUD_FLAGS		_KERNPG_TABLE_NOENC
> +#define PMD_FLAGS		_KERNPG_TABLE_NOENC
> +
> +static pmd_t __init *sme_prepare_pgd(struct sme_populate_pgd_data *ppd)
> +{
> +	pgd_t *pgd_p;
> +	p4d_t *p4d_p;
> +	pud_t *pud_p;
> +	pmd_t *pmd_p;
> +
> +	pgd_p = ppd->pgd + pgd_index(ppd->vaddr);
> +	if (native_pgd_val(*pgd_p)) {
> +		if (IS_ENABLED(CONFIG_X86_5LEVEL))
> +			p4d_p = (p4d_t *)(native_pgd_val(*pgd_p) & ~PTE_FLAGS_MASK);
> +		else
> +			pud_p = (pud_t *)(native_pgd_val(*pgd_p) & ~PTE_FLAGS_MASK);
> +	} else {
> +		pgd_t pgd;
> +
> +		if (IS_ENABLED(CONFIG_X86_5LEVEL)) {
> +			p4d_p = ppd->pgtable_area;
> +			memset(p4d_p, 0, sizeof(*p4d_p) * PTRS_PER_P4D);
> +			ppd->pgtable_area += sizeof(*p4d_p) * PTRS_PER_P4D;
> +
> +			pgd = native_make_pgd((pgdval_t)p4d_p + PGD_FLAGS);
> +		} else {
> +			pud_p = ppd->pgtable_area;
> +			memset(pud_p, 0, sizeof(*pud_p) * PTRS_PER_PUD);
> +			ppd->pgtable_area += sizeof(*pud_p) * PTRS_PER_PUD;
> +
> +			pgd = native_make_pgd((pgdval_t)pud_p + PGD_FLAGS);
> +		}
> +		native_set_pgd(pgd_p, pgd);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_X86_5LEVEL)) {
> +		p4d_p += p4d_index(ppd->vaddr);
> +		if (native_p4d_val(*p4d_p)) {
> +			pud_p = (pud_t *)(native_p4d_val(*p4d_p) & ~PTE_FLAGS_MASK);
> +		} else {
> +			p4d_t p4d;
> +
> +			pud_p = ppd->pgtable_area;
> +			memset(pud_p, 0, sizeof(*pud_p) * PTRS_PER_PUD);
> +			ppd->pgtable_area += sizeof(*pud_p) * PTRS_PER_PUD;
> +
> +			p4d = native_make_p4d((pudval_t)pud_p + P4D_FLAGS);
> +			native_set_p4d(p4d_p, p4d);
> +		}
> +	}
> +
> +	pud_p += pud_index(ppd->vaddr);
> +	if (native_pud_val(*pud_p)) {
> +		if (native_pud_val(*pud_p) & _PAGE_PSE)
> +			return NULL;
> +
> +		pmd_p = (pmd_t *)(native_pud_val(*pud_p) & ~PTE_FLAGS_MASK);
> +	} else {
> +		pud_t pud;
> +
> +		pmd_p = ppd->pgtable_area;
> +		memset(pmd_p, 0, sizeof(*pmd_p) * PTRS_PER_PMD);
> +		ppd->pgtable_area += sizeof(*pmd_p) * PTRS_PER_PMD;
> +
> +		pud = native_make_pud((pmdval_t)pmd_p + PUD_FLAGS);
> +		native_set_pud(pud_p, pud);
> +	}
> +
> +	return pmd_p;
> +}
> +
> +void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
> +{
> +	pmd_t *pmd_p;
> +
> +	pmd_p = sme_prepare_pgd(ppd);
> +	if (!pmd_p)
> +		return;
> +
> +	pmd_p += pmd_index(ppd->vaddr);
> +	if (!native_pmd_val(*pmd_p) || !(native_pmd_val(*pmd_p) & _PAGE_PSE))
> +		native_set_pmd(pmd_p, native_make_pmd(ppd->paddr | ppd->pmd_flags));
> +}
> +
> +void __init sme_populate_pgd(struct sme_populate_pgd_data *ppd)
> +{
> +	pmd_t *pmd_p;
> +	pte_t *pte_p;
> +
> +	pmd_p = sme_prepare_pgd(ppd);
> +	if (!pmd_p)
> +		return;
> +
> +	pmd_p += pmd_index(ppd->vaddr);
> +	if (native_pmd_val(*pmd_p)) {
> +		if (native_pmd_val(*pmd_p) & _PAGE_PSE)
> +			return;
> +
> +		pte_p = (pte_t *)(native_pmd_val(*pmd_p) & ~PTE_FLAGS_MASK);
> +	} else {
> +		pmd_t pmd;
> +
> +		pte_p = ppd->pgtable_area;
> +		memset(pte_p, 0, sizeof(*pte_p) * PTRS_PER_PTE);
> +		ppd->pgtable_area += sizeof(*pte_p) * PTRS_PER_PTE;
> +
> +		pmd = native_make_pmd((pteval_t)pte_p + PMD_FLAGS);
> +		native_set_pmd(pmd_p, pmd);
> +	}
> +
> +	pte_p += pte_index(ppd->vaddr);
> +	if (!native_pte_val(*pte_p))
> +		native_set_pte(pte_p, native_make_pte(ppd->paddr | ppd->pte_flags));
> +}
> diff --git a/arch/x86/mm/mm_internal.h b/arch/x86/mm/mm_internal.h
> index 4e1f6e1b8159..b3ab82ae9b12 100644
> --- a/arch/x86/mm/mm_internal.h
> +++ b/arch/x86/mm/mm_internal.h
> @@ -19,4 +19,18 @@ extern int after_bootmem;
>  
>  void update_cache_mode_entry(unsigned entry, enum page_cache_mode cache);
>  
> +struct sme_populate_pgd_data {
> +	void	*pgtable_area;
> +	pgd_t	*pgd;
> +
> +	pmdval_t pmd_flags;
> +	pteval_t pte_flags;
> +	unsigned long paddr;
> +
> +	unsigned long vaddr;
> +	unsigned long vaddr_end;
> +};
> +
> +void __init sme_populate_pgd(struct sme_populate_pgd_data *ppd);
> +void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd);
>  #endif	/* __X86_MM_INTERNAL_H */
>