LinuxLists.cc - [PATCHv2 0/3] x86/mm/encrypt: Cleanup and switching between paging modes

2018-01-23 17:20:18

Subject: [PATCHv2 0/3] x86/mm/encrypt: Cleanup and switching between paging modes

This patcheset is a preparation set for boot-time switching between
paging modes. Please review and consider applying.

Code around sme_populate_pgd() is unnecessary complex and hard to modify.

This patchset rewrites it in more stream-lined way to add support of
boot-time switching between paging modes.

I haven't tested the patchset on hardware capable of memory encryption.

v2:
- Rebased to up-to-date tip

Kirill A. Shutemov (3):
x86/mm/encrypt: Move sme_populate_pgd*() into separate translation
unit
x86/mm/encrypt: Rewrite sme_populate_pgd() and
sme_populate_pgd_large()
x86/mm/encrypt: Rewrite sme_pgtable_calc()

arch/x86/mm/Makefile | 13 +--
arch/x86/mm/mem_encrypt.c | 171 +++----------------------------------
arch/x86/mm/mem_encrypt_identity.c | 118 +++++++++++++++++++++++++
arch/x86/mm/mm_internal.h | 14 +++
4 files changed, 152 insertions(+), 164 deletions(-)
create mode 100644 arch/x86/mm/mem_encrypt_identity.c

--
2.15.1

2018-01-23 17:20:00

by Kirill A. Shutemov

[permalink] [raw]

Subject: [PATCH 3/3] x86/mm/encrypt: Rewrite sme_pgtable_calc()

sme_pgtable_calc() is unnecessary complex. It can be re-written in a
more stream-lined way.

As a side effect, we would get the code ready to boot-time switching
between paging modes.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/mm/mem_encrypt.c | 42 ++++++++++++------------------------------
1 file changed, 12 insertions(+), 30 deletions(-)

diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 740b8a54f616..88e9d4b1a233 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -556,8 +556,7 @@ static void __init sme_map_range_decrypted_wp(struct sme_populate_pgd_data *ppd)

static unsigned long __init sme_pgtable_calc(unsigned long len)
{
- unsigned long p4d_size, pud_size, pmd_size, pte_size;
- unsigned long total;
+ unsigned long entries = 0, tables = 0;

/*
* Perform a relatively simplistic calculation of the pagetable
@@ -571,42 +570,25 @@ static unsigned long __init sme_pgtable_calc(unsigned long len)
* Incrementing the count for each covers the case where the addresses
* cross entries.
*/
- if (IS_ENABLED(CONFIG_X86_5LEVEL)) {
- p4d_size = (ALIGN(len, PGDIR_SIZE) / PGDIR_SIZE) + 1;
- p4d_size *= sizeof(p4d_t) * PTRS_PER_P4D;
- pud_size = (ALIGN(len, P4D_SIZE) / P4D_SIZE) + 1;
- pud_size *= sizeof(pud_t) * PTRS_PER_PUD;
- } else {
- p4d_size = 0;
- pud_size = (ALIGN(len, PGDIR_SIZE) / PGDIR_SIZE) + 1;
- pud_size *= sizeof(pud_t) * PTRS_PER_PUD;
- }
- pmd_size = (ALIGN(len, PUD_SIZE) / PUD_SIZE) + 1;
- pmd_size *= sizeof(pmd_t) * PTRS_PER_PMD;
- pte_size = 2 * sizeof(pte_t) * PTRS_PER_PTE;

- total = p4d_size + pud_size + pmd_size + pte_size;
+ /* PGDIR_SIZE is equal to P4D_SIZE on 4-level machine. */
+ if (PTRS_PER_P4D > 1)
+ entries += (DIV_ROUND_UP(len, PGDIR_SIZE) + 1) * sizeof(p4d_t) * PTRS_PER_P4D;
+ entries += (DIV_ROUND_UP(len, P4D_SIZE) + 1) * sizeof(pud_t) * PTRS_PER_PUD;
+ entries += (DIV_ROUND_UP(len, PUD_SIZE) + 1) * sizeof(pmd_t) * PTRS_PER_PMD;
+ entries += 2 * sizeof(pte_t) * PTRS_PER_PTE;

/*
* Now calculate the added pagetable structures needed to populate
* the new pagetables.
*/
- if (IS_ENABLED(CONFIG_X86_5LEVEL)) {
- p4d_size = ALIGN(total, PGDIR_SIZE) / PGDIR_SIZE;
- p4d_size *= sizeof(p4d_t) * PTRS_PER_P4D;
- pud_size = ALIGN(total, P4D_SIZE) / P4D_SIZE;
- pud_size *= sizeof(pud_t) * PTRS_PER_PUD;
- } else {
- p4d_size = 0;
- pud_size = ALIGN(total, PGDIR_SIZE) / PGDIR_SIZE;
- pud_size *= sizeof(pud_t) * PTRS_PER_PUD;
- }
- pmd_size = ALIGN(total, PUD_SIZE) / PUD_SIZE;
- pmd_size *= sizeof(pmd_t) * PTRS_PER_PMD;

- total += p4d_size + pud_size + pmd_size;
+ if (PTRS_PER_P4D > 1)
+ tables += DIV_ROUND_UP(entries, PGDIR_SIZE) * sizeof(p4d_t) * PTRS_PER_P4D;
+ tables += DIV_ROUND_UP(entries, P4D_SIZE) * sizeof(pud_t) * PTRS_PER_PUD;
+ tables += DIV_ROUND_UP(entries, PUD_SIZE) * sizeof(pmd_t) * PTRS_PER_PMD;

- return total;
+ return entries + tables;
}

void __init __nostackprotector sme_encrypt_kernel(struct boot_params *bp)
--
2.15.1

2018-01-23 17:21:09

by Kirill A. Shutemov

[permalink] [raw]

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

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 <[email protected]>
---
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
+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)
{
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 <[email protected]>
+ *
+ * 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 */
--
2.15.1

2018-01-23 17:21:15

by Kirill A. Shutemov

[permalink] [raw]

Subject: [PATCH 2/3] x86/mm/encrypt: Rewrite sme_populate_pgd() and sme_populate_pgd_large()

2018-01-24 10:37:59

by Kirill A. Shutemov

[permalink] [raw]

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

On Tue, Jan 23, 2018 at 08:19:08PM +0300, 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 <[email protected]>
> ---
> 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
> +CFLAGS_REMOVE_mem_encrypt_identity.o = -pg
> endif

0day found a boot issue with the commit.

We need to add line on mem_encrypt_identity.o, not replace existing one.

Fixup is below.

diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index 51e364ef12d9..03c6c8561623 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -8,6 +8,7 @@ KASAN_SANITIZE_mem_encrypt.o := n
KASAN_SANITIZE_mem_encrypt_identity.o := n

ifdef CONFIG_FUNCTION_TRACER
+CFLAGS_REMOVE_mem_encrypt.o = -pg
CFLAGS_REMOVE_mem_encrypt_identity.o = -pg
endif

--
Kirill A. Shutemov

2018-01-24 15:01:11

by Tom Lendacky

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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 */
>

2018-01-24 15:53:10

by Kirill A. Shutemov

[permalink] [raw]

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

On Wed, Jan 24, 2018 at 02:59:58PM +0000, Tom Lendacky wrote:
> > 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.

I moved minimum of the code to get the trick work, but I'll look into
moving move there.

--
Kirill A. Shutemov