[PATCH v7 RESEND 0/4] Support kdump for AMD secure memory encryption(SME)

[PATCH v7 RESEND 0/4] Support kdump for AMD secure memory encryption(SME)

[Lianbo Jiang]

When SME is enabled on AMD machine, we also need to support kdump. Because
the memory is encrypted in the first kernel, we will remap the old memory
to the kdump kernel for dumping data, and SME is also enabled in the kdump
kernel, otherwise the old memory can not be decrypted.

For the kdump, it is necessary to distinguish whether the memory is encrypted.
Furthermore, we should also know which part of the memory is encrypted or
decrypted. We will appropriately remap the memory according to the specific
situation in order to tell cpu how to access the memory.

As we know, a page of memory that is marked as encrypted, which will be
automatically decrypted when read from DRAM, and will also be automatically
encrypted when written to DRAM. If the old memory is encrypted, we have to
remap the old memory with the memory encryption mask, which will automatically
decrypt the old memory when we read those data.

For kdump(SME), there are two cases that doesn't support:

 ----------------------------------------------
| first-kernel | second-kernel | kdump support |
|      (mem_encrypt=on|off)    |   (yes|no)    |
|--------------+---------------+---------------|
|     on       |     on        |     yes       |
|     off      |     off       |     yes       |
|     on       |     off       |     no        |
|     off      |     on        |     no        |
|______________|_______________|_______________|

1. SME is enabled in the first kernel, but SME is disabled in kdump kernel
In this case, because the old memory is encrypted, we can't decrypt the
old memory.

2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
It is unnecessary to support in this case, because the old memory is
unencrypted, the old memory can be dumped as usual, we don't need to enable
SME in kdump kernel. Another, If we must support the scenario, it will
increase the complexity of the code, we will have to consider how to pass
the SME flag from the first kernel to the kdump kernel, in order to let the
kdump kernel know that whether the old memory is encrypted.

There are two methods to pass the SME flag to the kdump kernel. The first
method is to modify the assembly code, which includes some common code and
the path is too long. The second method is to use kexec tool, which could
require the SME flag to be exported in the first kernel by "proc" or "sysfs",
kexec tools will read the SME flag from "proc" or "sysfs" when we use kexec
tools to load image, subsequently the SME flag will be saved in boot_params,
we can properly remap the old memory according to the previously saved SME
flag. But it is too expensive to do this.

This patches are only for SME kdump, the patches don't support SEV kdump.

Test tools:
makedumpfile[v1.6.3]: https://github.com/LianboJ/makedumpfile
commit <e1de103eca8f> "A draft for kdump vmcore about AMD SME"
Note: This patch can only dump vmcore in the case of SME enabled.

crash-7.2.3: https://github.com/crash-utility/crash.git
commit <001f77a05585> "Fix for Linux 4.19-rc1 and later kernels that contain kernel commit <7290d5809571>"

kexec-tools-2.0.17: git://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git
commit <b9de21ef51a7> "kexec: fix for "Unhandled rela relocation: R_X86_64_PLT32" error"

Note:
Before you load the kernel and initramfs for kdump, this patch(
http://lists.infradead.org/pipermail/kexec/2018-September/021460.html) must be merged
to kexec-tools, and then the kdump kernel will work well. Because there is a patch
which is removed based on v6(x86/ioremap: strengthen the logic in
early_memremap_pgprot_adjust() to adjust encryption mask).

Test environment:
HP ProLiant DL385Gen10 AMD EPYC 7251
8-Core Processor
32768 MB memory
600 GB disk space

Linux 4.19-rc5:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
commit <6bf4ca7fbc85> "Linux 4.19-rc5"

Reference:
AMD64 Architecture Programmer's Manual
https://support.amd.com/TechDocs/24593.pdf

Changes since v6:
1. There is a patch which is removed based on v6.
(x86/ioremap: strengthen the logic in early_memremap_pgprot_adjust() to adjust encryption mask)
Dave Young suggests that this patch can be removed and fix the kexec-tools.
Reference: http://lists.infradead.org/pipermail/kexec/2018-September/021460.html)
2. Update the patch log.

Changes since v7:
1. Improve patch log for patch 1/4(Suggested by Baoquan He)
2. Add Reviewed-by for all patches(Tom Lendacky <thomas.lendacky@amd.com>)
3. Add Acked-by for patch 3/4(Joerg Roedel <jroedel@suse.de>)

Some known issues:
1. about SME
Upstream kernel will hang on HP machine(DL385Gen10 AMD EPYC 7251) when
we execute the kexec command as follow:

# kexec -l /boot/vmlinuz-4.19.0-rc5+ --initrd=/boot/initramfs-4.19.0-rc5+.img --command-line="root=/dev/mapper/rhel_hp--dl385g10--03-root ro mem_encrypt=on rd.lvm.lv=rhel_hp-dl385g10-03/root rd.lvm.lv=rhel_hp-dl385g10-03/swap console=ttyS0,115200n81 LANG=en_US.UTF-8 earlyprintk=serial debug nokaslr"
# kexec -e (or reboot)

But this issue can not be reproduced on speedway machine, and this issue
is irrelevant to my posted patches.

The kernel log:
[ 1248.932239] kexec_core: Starting new kernel
early console in extract_kernel
input_data: 0x000000087e91c3b4
input_len: 0x000000000067fcbd
output: 0x000000087d400000
output_len: 0x0000000001b6fa90
kernel_total_size: 0x0000000001a9d000
trampoline_32bit: 0x0000000000099000

Decompressing Linux...
Parsing ELF...        [---Here the system will hang]

Lianbo Jiang (4):
  x86/ioremap: add a function ioremap_encrypted() to remap kdump old
    memory
  kexec: allocate unencrypted control pages for kdump in case SME is
    enabled
  iommu/amd: Remap the device table of IOMMU with the memory encryption
    mask for kdump
  kdump/vmcore: support encrypted old memory with SME enabled

 arch/x86/include/asm/io.h            |  3 ++
 arch/x86/kernel/Makefile             |  1 +
 arch/x86/kernel/crash_dump_encrypt.c | 53 ++++++++++++++++++++++++++++
 arch/x86/mm/ioremap.c                | 25 ++++++++-----
 drivers/iommu/amd_iommu_init.c       | 14 ++++++--
 fs/proc/vmcore.c                     | 21 +++++++----
 include/linux/crash_dump.h           | 12 +++++++
 kernel/kexec_core.c                  | 12 +++++++
 8 files changed, 125 insertions(+), 16 deletions(-)
 create mode 100644 arch/x86/kernel/crash_dump_encrypt.c

-- 
2.17.1

[PATCH v7 RESEND 1/4] x86/ioremap: add a function ioremap_encrypted() to remap kdump old memory

[Lianbo Jiang]

When SME is enabled on AMD machine, the memory is encrypted in the first
kernel. In this case, SME also needs to be enabled in kdump kernel, and
we have to remap the old memory with the memory encryption mask.

Here we only talk about the case that SME is active in the first kernel,
and only care it's active too in kdump kernel. there are four cases we
need considered.

a. dump vmcore
   It is encrypted in the first kernel, and needs be read out in kdump
   kernel.

b. crash notes
   When dumping vmcore, the people usually need to read the useful
   information from notes, and the notes is also encrypted.

c. iommu device table
   It is allocated by kernel, need fill its pointer into mmio of amd iommu.
   It's encrypted in the first kernel, need read the old content to analyze
   and get useful information.

d. mmio of amd iommu
   Register reported by amd firmware, it's not RAM, we don't encrypt in
   both the first kernel and kdump kernel.

To achieve the goal, the solution is:
1. add a new bool parameter "encrypted" to __ioremap_caller()
   It is a low level function, and check the newly added parameter, if it's
   true and in kdump kernel, will remap the memory with sme mask.

2. add a new function ioremap_encrypted() to explicitly passed in a "true"
   value for "encrypted".
   For above a, b, c, we will call ioremap_encrypted();

3. adjust all existed ioremap wrapper functions, passed in "false" for
   encrypted to make them an before.

   ioremap_encrypted()\
   ioremap_cache()     |
   ioremap_prot()      |
   ioremap_wt()        |->__ioremap_caller()
   ioremap_wc()        |
   ioremap_uc()        |
   ioremap_nocache()  /

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/include/asm/io.h |  3 +++
 arch/x86/mm/ioremap.c     | 25 +++++++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 6de64840dd22..f8795f9581c7 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -192,6 +192,9 @@ extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
 #define ioremap_cache ioremap_cache
 extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
 #define ioremap_prot ioremap_prot
+extern void __iomem *ioremap_encrypted(resource_size_t phys_addr,
+					unsigned long size);
+#define ioremap_encrypted ioremap_encrypted
 
 /**
  * ioremap     -   map bus memory into CPU space
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index c63a545ec199..e01e6c695add 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -24,6 +24,7 @@
 #include <asm/pgalloc.h>
 #include <asm/pat.h>
 #include <asm/setup.h>
+#include <linux/crash_dump.h>
 
 #include "physaddr.h"
 
@@ -131,7 +132,8 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
  * caller shouldn't need to know that small detail.
  */
 static void __iomem *__ioremap_caller(resource_size_t phys_addr,
-		unsigned long size, enum page_cache_mode pcm, void *caller)
+		unsigned long size, enum page_cache_mode pcm,
+		void *caller, bool encrypted)
 {
 	unsigned long offset, vaddr;
 	resource_size_t last_addr;
@@ -199,7 +201,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	 * resulting mapping.
 	 */
 	prot = PAGE_KERNEL_IO;
-	if (sev_active() && mem_flags.desc_other)
+	if ((sev_active() && mem_flags.desc_other) || encrypted)
 		prot = pgprot_encrypted(prot);
 
 	switch (pcm) {
@@ -291,7 +293,7 @@ void __iomem *ioremap_nocache(resource_size_t phys_addr, unsigned long size)
 	enum page_cache_mode pcm = _PAGE_CACHE_MODE_UC_MINUS;
 
 	return __ioremap_caller(phys_addr, size, pcm,
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_nocache);
 
@@ -324,7 +326,7 @@ void __iomem *ioremap_uc(resource_size_t phys_addr, unsigned long size)
 	enum page_cache_mode pcm = _PAGE_CACHE_MODE_UC;
 
 	return __ioremap_caller(phys_addr, size, pcm,
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL_GPL(ioremap_uc);
 
@@ -341,7 +343,7 @@ EXPORT_SYMBOL_GPL(ioremap_uc);
 void __iomem *ioremap_wc(resource_size_t phys_addr, unsigned long size)
 {
 	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WC,
-					__builtin_return_address(0));
+					__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_wc);
 
@@ -358,14 +360,21 @@ EXPORT_SYMBOL(ioremap_wc);
 void __iomem *ioremap_wt(resource_size_t phys_addr, unsigned long size)
 {
 	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WT,
-					__builtin_return_address(0));
+					__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_wt);
 
+void __iomem *ioremap_encrypted(resource_size_t phys_addr, unsigned long size)
+{
+	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WB,
+				__builtin_return_address(0), true);
+}
+EXPORT_SYMBOL(ioremap_encrypted);
+
 void __iomem *ioremap_cache(resource_size_t phys_addr, unsigned long size)
 {
 	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WB,
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_cache);
 
@@ -374,7 +383,7 @@ void __iomem *ioremap_prot(resource_size_t phys_addr, unsigned long size,
 {
 	return __ioremap_caller(phys_addr, size,
 				pgprot2cachemode(__pgprot(prot_val)),
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_prot);
 
-- 
2.17.1

[PATCH v7 RESEND 2/4] kexec: allocate unencrypted control pages for kdump in case SME is enabled

[Lianbo Jiang]

When SME is enabled in the first kernel, we will allocate unencrypted pages
for kdump in order to be able to boot the kdump kernel like kexec.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 kernel/kexec_core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 23a83a4da38a..e7efcd1a977b 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -471,6 +471,16 @@ static struct page *kimage_alloc_crash_control_pages(struct kimage *image,
 		}
 	}
 
+	if (pages) {
+		/*
+		 * For kdump, we need to ensure that these pages are
+		 * unencrypted pages if SME is enabled.
+		 * By the way, it is unnecessary to call the arch_
+		 * kexec_pre_free_pages(), which will make the code
+		 * become more simple.
+		 */
+		arch_kexec_post_alloc_pages(page_address(pages), 1 << order, 0);
+	}
 	return pages;
 }
 
@@ -867,6 +877,7 @@ static int kimage_load_crash_segment(struct kimage *image,
 			result  = -ENOMEM;
 			goto out;
 		}
+		arch_kexec_post_alloc_pages(page_address(page), 1, 0);
 		ptr = kmap(page);
 		ptr += maddr & ~PAGE_MASK;
 		mchunk = min_t(size_t, mbytes,
@@ -884,6 +895,7 @@ static int kimage_load_crash_segment(struct kimage *image,
 			result = copy_from_user(ptr, buf, uchunk);
 		kexec_flush_icache_page(page);
 		kunmap(page);
+		arch_kexec_pre_free_pages(page_address(page), 1);
 		if (result) {
 			result = -EFAULT;
 			goto out;
-- 
2.17.1

[PATCH v7 RESEND 3/4] iommu/amd: Remap the device table of IOMMU with the memory encryption mask for kdump

[Lianbo Jiang]

In kdump kernel, it will copy the device table of IOMMU from the old device
table, which is encrypted when SME is enabled in the first kernel. So we
have to remap the old device table with the memory encryption mask.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Acked-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu_init.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 84b3e4445d46..3931c7de7c69 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -902,12 +902,22 @@ static bool copy_device_table(void)
 		}
 	}
 
-	old_devtb_phys = entry & PAGE_MASK;
+	/*
+	 * When SME is enabled in the first kernel, the entry includes the
+	 * memory encryption mask(sme_me_mask), we must remove the memory
+	 * encryption mask to obtain the true physical address in kdump kernel.
+	 */
+	old_devtb_phys = __sme_clr(entry) & PAGE_MASK;
+
 	if (old_devtb_phys >= 0x100000000ULL) {
 		pr_err("The address of old device table is above 4G, not trustworthy!\n");
 		return false;
 	}
-	old_devtb = memremap(old_devtb_phys, dev_table_size, MEMREMAP_WB);
+	old_devtb = (sme_active() && is_kdump_kernel())
+		    ? (__force void *)ioremap_encrypted(old_devtb_phys,
+							dev_table_size)
+		    : memremap(old_devtb_phys, dev_table_size, MEMREMAP_WB);
+
 	if (!old_devtb)
 		return false;
 
-- 
2.17.1

[PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

[Lianbo Jiang]

In kdump kernel, we need to dump the old memory into vmcore file,if SME
is enabled in the first kernel, we have to remap the old memory with the
memory encryption mask, which will be automatically decrypted when we
read from DRAM.

For SME kdump, there are two cases that doesn't support:

 ----------------------------------------------
| first-kernel | second-kernel | kdump support |
|      (mem_encrypt=on|off)    |   (yes|no)    |
|--------------+---------------+---------------|
|     on       |     on        |     yes       |
|     off      |     off       |     yes       |
|     on       |     off       |     no        |
|     off      |     on        |     no        |
|______________|_______________|_______________|

1. SME is enabled in the first kernel, but SME is disabled in kdump kernel
In this case, because the old memory is encrypted, we can't decrypt the
old memory.

2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
On the one hand, the old memory is unencrypted, the old memory can be dumped
as usual, we don't need to enable SME in kdump kernel; On the other hand, it
will increase the complexity of the code, we will have to consider how to
pass the SME flag from the first kernel to the kdump kernel, it is really
too expensive to do this.

This patches are only for SME kdump, the patches don't support SEV kdump.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/kernel/Makefile             |  1 +
 arch/x86/kernel/crash_dump_encrypt.c | 53 ++++++++++++++++++++++++++++
 fs/proc/vmcore.c                     | 21 +++++++----
 include/linux/crash_dump.h           | 12 +++++++
 4 files changed, 81 insertions(+), 6 deletions(-)
 create mode 100644 arch/x86/kernel/crash_dump_encrypt.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 8824d01c0c35..dfbeae0e35ce 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -97,6 +97,7 @@ obj-$(CONFIG_KEXEC_CORE)	+= machine_kexec_$(BITS).o
 obj-$(CONFIG_KEXEC_CORE)	+= relocate_kernel_$(BITS).o crash.o
 obj-$(CONFIG_KEXEC_FILE)	+= kexec-bzimage64.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_$(BITS).o
+obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= crash_dump_encrypt.o
 obj-y				+= kprobes/
 obj-$(CONFIG_MODULES)		+= module.o
 obj-$(CONFIG_DOUBLEFAULT)	+= doublefault.o
diff --git a/arch/x86/kernel/crash_dump_encrypt.c b/arch/x86/kernel/crash_dump_encrypt.c
new file mode 100644
index 000000000000..e1b1a577f197
--- /dev/null
+++ b/arch/x86/kernel/crash_dump_encrypt.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *	Memory preserving reboot related code.
+ *
+ *	Created by: Lianbo Jiang (lijiang@redhat.com)
+ *	Copyright (C) RedHat Corporation, 2018. All rights reserved
+ */
+
+#include <linux/errno.h>
+#include <linux/crash_dump.h>
+#include <linux/uaccess.h>
+#include <linux/io.h>
+
+/**
+ * copy_oldmem_page_encrypted - copy one page from "oldmem encrypted"
+ * @pfn: page frame number to be copied
+ * @buf: target memory address for the copy; this can be in kernel address
+ *	space or user address space (see @userbuf)
+ * @csize: number of bytes to copy
+ * @offset: offset in bytes into the page (based on pfn) to begin the copy
+ * @userbuf: if set, @buf is in user address space, use copy_to_user(),
+ *	otherwise @buf is in kernel address space, use memcpy().
+ *
+ * Copy a page from "oldmem encrypted". For this page, there is no pte
+ * mapped in the current kernel. We stitch up a pte, similar to
+ * kmap_atomic.
+ */
+
+ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
+		size_t csize, unsigned long offset, int userbuf)
+{
+	void  *vaddr;
+
+	if (!csize)
+		return 0;
+
+	vaddr = (__force void *)ioremap_encrypted(pfn << PAGE_SHIFT,
+						  PAGE_SIZE);
+	if (!vaddr)
+		return -ENOMEM;
+
+	if (userbuf) {
+		if (copy_to_user((void __user *)buf, vaddr + offset, csize)) {
+			iounmap((void __iomem *)vaddr);
+			return -EFAULT;
+		}
+	} else
+		memcpy(buf, vaddr + offset, csize);
+
+	set_iounmap_nonlazy();
+	iounmap((void __iomem *)vaddr);
+	return csize;
+}
diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index cbde728f8ac6..3065c8bada6a 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -25,6 +25,9 @@
 #include <linux/pagemap.h>
 #include <linux/uaccess.h>
 #include <asm/io.h>
+#include <linux/io.h>
+#include <linux/mem_encrypt.h>
+#include <asm/pgtable.h>
 #include "internal.h"
 
 /* List representing chunks of contiguous memory areas and their offsets in
@@ -98,7 +101,8 @@ static int pfn_is_ram(unsigned long pfn)
 
 /* Reads a page from the oldmem device from given offset. */
 static ssize_t read_from_oldmem(char *buf, size_t count,
-				u64 *ppos, int userbuf)
+				u64 *ppos, int userbuf,
+				bool encrypted)
 {
 	unsigned long pfn, offset;
 	size_t nr_bytes;
@@ -120,8 +124,11 @@ static ssize_t read_from_oldmem(char *buf, size_t count,
 		if (pfn_is_ram(pfn) == 0)
 			memset(buf, 0, nr_bytes);
 		else {
-			tmp = copy_oldmem_page(pfn, buf, nr_bytes,
-						offset, userbuf);
+			tmp = encrypted ? copy_oldmem_page_encrypted(pfn,
+					    buf, nr_bytes, offset, userbuf)
+					: copy_oldmem_page(pfn, buf, nr_bytes,
+							   offset, userbuf);
+
 			if (tmp < 0)
 				return tmp;
 		}
@@ -155,7 +162,7 @@ void __weak elfcorehdr_free(unsigned long long addr)
  */
 ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
 {
-	return read_from_oldmem(buf, count, ppos, 0);
+	return read_from_oldmem(buf, count, ppos, 0, false);
 }
 
 /*
@@ -163,7 +170,7 @@ ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
  */
 ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
 {
-	return read_from_oldmem(buf, count, ppos, 0);
+	return read_from_oldmem(buf, count, ppos, 0, sme_active());
 }
 
 /*
@@ -173,6 +180,7 @@ int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
 				  unsigned long from, unsigned long pfn,
 				  unsigned long size, pgprot_t prot)
 {
+	prot = pgprot_encrypted(prot);
 	return remap_pfn_range(vma, from, pfn, size, prot);
 }
 
@@ -351,7 +359,8 @@ static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
 					    m->offset + m->size - *fpos,
 					    buflen);
 			start = m->paddr + *fpos - m->offset;
-			tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
+			tmp = read_from_oldmem(buffer, tsz, &start, userbuf,
+						sme_active());
 			if (tmp < 0)
 				return tmp;
 			buflen -= tsz;
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 3e4ba9d753c8..a7e7be8b0502 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -26,6 +26,18 @@ extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
 
 extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
 						unsigned long, int);
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+extern ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
+					   size_t csize, unsigned long offset,
+					   int userbuf);
+#else
+static inline
+ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize,
+				    unsigned long offset, int userbuf)
+{
+	return 0;
+}
+#endif
 void vmcore_cleanup(void);
 
 /* Architecture code defines this if there are other possible ELF
-- 
2.17.1

Re: [PATCH v7 RESEND 1/4] x86/ioremap: add a function ioremap_encrypted() to remap kdump old memory

[Borislav Petkov]

On Thu, Sep 27, 2018 at 03:19:51PM +0800, Lianbo Jiang wrote:
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index 6de64840dd22..f8795f9581c7 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -192,6 +192,9 @@ extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
>  #define ioremap_cache ioremap_cache
>  extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
>  #define ioremap_prot ioremap_prot
> +extern void __iomem *ioremap_encrypted(resource_size_t phys_addr,
> +					unsigned long size);

No need to break this line - see how the other externs don't.

> +#define ioremap_encrypted ioremap_encrypted
>  
>  /**
>   * ioremap     -   map bus memory into CPU space
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index c63a545ec199..e01e6c695add 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -24,6 +24,7 @@
>  #include <asm/pgalloc.h>
>  #include <asm/pat.h>
>  #include <asm/setup.h>
> +#include <linux/crash_dump.h>

What is that include for and why is it not up there with the <linux/...
includes instead here with the <asm/..> ones?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH v7 RESEND 1/4] x86/ioremap: add a function ioremap_encrypted() to remap kdump old memory

[lijiang]

在 2018年09月27日 21:17, Borislav Petkov 写道:
> On Thu, Sep 27, 2018 at 03:19:51PM +0800, Lianbo Jiang wrote:
>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>> index 6de64840dd22..f8795f9581c7 100644
>> --- a/arch/x86/include/asm/io.h
>> +++ b/arch/x86/include/asm/io.h
>> @@ -192,6 +192,9 @@ extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
>>  #define ioremap_cache ioremap_cache
>>  extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
>>  #define ioremap_prot ioremap_prot
>> +extern void __iomem *ioremap_encrypted(resource_size_t phys_addr,
>> +					unsigned long size);
> 
> No need to break this line - see how the other externs don't.
> 
Ok, i will fix it. 

If no need to break this line, it will cause a warning of exceeding 80 characters per line.

>> +#define ioremap_encrypted ioremap_encrypted
>>  
>>  /**
>>   * ioremap     -   map bus memory into CPU space
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index c63a545ec199..e01e6c695add 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -24,6 +24,7 @@
>>  #include <asm/pgalloc.h>
>>  #include <asm/pat.h>
>>  #include <asm/setup.h>
>> +#include <linux/crash_dump.h>
> 
> What is that include for and why is it not up there with the <linux/...
> includes instead here with the <asm/..> ones?
> 
Thank you for pointing out this issue, i forgot to remove this header file. 

I will get rid of this header file and post this patch again.

Regards,
Lianbo

Re: [PATCH v7 RESEND 1/4] x86/ioremap: add a function ioremap_encrypted() to remap kdump old memory

[Borislav Petkov]

On Thu, Sep 27, 2018 at 10:53:47PM +0800, lijiang wrote:
> If no need to break this line, it will cause a warning of exceeding 80 characters per line.

That's fine - we don't take the 80 cols rule blindly but apply common
sense. In this particular case the lines can stick out because they're
simply externs and are meant to be skimmed over and not really read. :)

> Thank you for pointing out this issue, i forgot to remove this header file. 
> 
> I will get rid of this header file and post this patch again.

No need - already fixed that up.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH v7 RESEND 2/4] kexec: allocate unencrypted control pages for kdump in case SME is enabled

[Borislav Petkov]

On Thu, Sep 27, 2018 at 03:19:52PM +0800, Lianbo Jiang wrote:
> When SME is enabled in the first kernel, we will allocate unencrypted pages
> for kdump in order to be able to boot the kdump kernel like kexec.

This is not what the commit does - it marks the control pages as
decrypted when SME. Why doesn't the commit message state that and why is
this being done?

> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  kernel/kexec_core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 23a83a4da38a..e7efcd1a977b 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -471,6 +471,16 @@ static struct page *kimage_alloc_crash_control_pages(struct kimage *image,
>  		}
>  	}
>  
> +	if (pages) {
> +		/*
> +		 * For kdump, we need to ensure that these pages are
> +		 * unencrypted pages if SME is enabled.

Remember to always call unencrypted pages "decrypted" - this is the
convention we agreed upon and it should keep the confusion level at
minimum to others staring at this code.

> +		 * By the way, it is unnecessary to call the arch_
> +		 * kexec_pre_free_pages(), which will make the code
> +		 * become more simple.
> +		 */

This second sentence I don't understand...

> +		arch_kexec_post_alloc_pages(page_address(pages), 1 << order, 0);
> +	}
>  	return pages;
>  }
>  
> @@ -867,6 +877,7 @@ static int kimage_load_crash_segment(struct kimage *image,
>  			result  = -ENOMEM;
>  			goto out;
>  		}
> +		arch_kexec_post_alloc_pages(page_address(page), 1, 0);
>  		ptr = kmap(page);
>  		ptr += maddr & ~PAGE_MASK;
>  		mchunk = min_t(size_t, mbytes,
> @@ -884,6 +895,7 @@ static int kimage_load_crash_segment(struct kimage *image,
>  			result = copy_from_user(ptr, buf, uchunk);
>  		kexec_flush_icache_page(page);
>  		kunmap(page);
> +		arch_kexec_pre_free_pages(page_address(page), 1);
>  		if (result) {
>  			result = -EFAULT;
>  			goto out;
> -- 
> 2.17.1
> 

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH v7 RESEND 1/4] x86/ioremap: add a function ioremap_encrypted() to remap kdump old memory

[lijiang]

在 2018年09月28日 00:10, Borislav Petkov 写道:
> On Thu, Sep 27, 2018 at 10:53:47PM +0800, lijiang wrote:
>> If no need to break this line, it will cause a warning of exceeding 80 characters per line.
> 
> That's fine - we don't take the 80 cols rule blindly but apply common
> sense. In this particular case the lines can stick out because they're
> simply externs and are meant to be skimmed over and not really read. :)
> 
Ok, i see. Thanks.

>> Thank you for pointing out this issue, i forgot to remove this header file. 
>>
>> I will get rid of this header file and post this patch again.
> 
> No need - already fixed that up.
> 
Great, thank you so much for your help.

Regards,
Lianbo
> Thx.
> 

Re: [PATCH v7 RESEND 2/4] kexec: allocate unencrypted control pages for kdump in case SME is enabled

[lijiang]

在 2018年09月28日 00:53, Borislav Petkov 写道:
> On Thu, Sep 27, 2018 at 03:19:52PM +0800, Lianbo Jiang wrote:
>> When SME is enabled in the first kernel, we will allocate unencrypted pages
>> for kdump in order to be able to boot the kdump kernel like kexec.
> 
> This is not what the commit does - it marks the control pages as
> decrypted when SME. Why doesn't the commit message state that and why is
> this being done?
> 
Ok, i will improve this patch log.

If SME is active, we need to mark the control pages as decrypted, because
when we boot to the kdump kernel, these pages won't be accessed encrypted
at the initial stage, in order to boot the kdump kernel in the same manner
as originally booted.

>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  kernel/kexec_core.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 23a83a4da38a..e7efcd1a977b 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -471,6 +471,16 @@ static struct page *kimage_alloc_crash_control_pages(struct kimage *image,
>>  		}
>>  	}
>>  
>> +	if (pages) {
>> +		/*
>> +		 * For kdump, we need to ensure that these pages are
>> +		 * unencrypted pages if SME is enabled.
> 
> Remember to always call unencrypted pages "decrypted" - this is the
> convention we agreed upon and it should keep the confusion level at
> minimum to others staring at this code.
> 
Ok, i will improve this comment.

>> +		 * By the way, it is unnecessary to call the arch_
>> +		 * kexec_pre_free_pages(), which will make the code
>> +		 * become more simple.
>> +		 */
> 
> This second sentence I don't understand...
> 

There are two functions that are usually called in pairs, they are:
arch_kexec_post_alloc_pages() and arch_kexec_pre_free_pages().

One marks the pages as decrypted, another one marks the pages as encrypted.

But for the crash control pages, no need to call arch_kexec_pre_free_pages(),
there are three reasons:
1. Crash pages are reserved in memblock, these pages are only used by kdump,
   no other people uses these pages;

2. Whenever crash pages are allocated, these pages are always marked as
   decrypted(when SME is active);

3. If we plan to call the arch_kexe_pre_free_pages(), we have to store these
pages to somewhere, which will have more code changes.

So, here it is redundant to call the arch_kexe_pre_free_pages().

Thanks.
Lianbo
>> +		arch_kexec_post_alloc_pages(page_address(pages), 1 << order, 0);
>> +	}
>>  	return pages;
>>  }
>>  
>> @@ -867,6 +877,7 @@ static int kimage_load_crash_segment(struct kimage *image,
>>  			result  = -ENOMEM;
>>  			goto out;
>>  		}
>> +		arch_kexec_post_alloc_pages(page_address(page), 1, 0);
>>  		ptr = kmap(page);
>>  		ptr += maddr & ~PAGE_MASK;
>>  		mchunk = min_t(size_t, mbytes,
>> @@ -884,6 +895,7 @@ static int kimage_load_crash_segment(struct kimage *image,
>>  			result = copy_from_user(ptr, buf, uchunk);
>>  		kexec_flush_icache_page(page);
>>  		kunmap(page);
>> +		arch_kexec_pre_free_pages(page_address(page), 1);
>>  		if (result) {
>>  			result = -EFAULT;
>>  			goto out;
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH v7 RESEND 2/4] kexec: allocate unencrypted control pages for kdump in case SME is enabled

[Borislav Petkov]

On Fri, Sep 28, 2018 at 11:52:21AM +0800, lijiang wrote:
> There are two functions that are usually called in pairs, they are:
> arch_kexec_post_alloc_pages() and arch_kexec_pre_free_pages().
> 
> One marks the pages as decrypted, another one marks the pages as encrypted.
> 
> But for the crash control pages, no need to call arch_kexec_pre_free_pages(),
> there are three reasons:
> 1. Crash pages are reserved in memblock, these pages are only used by kdump,
>    no other people uses these pages;
> 
> 2. Whenever crash pages are allocated, these pages are always marked as
>    decrypted(when SME is active);
> 
> 3. If we plan to call the arch_kexe_pre_free_pages(), we have to store these
> pages to somewhere, which will have more code changes.

I don't think any of that answers the question *why* control pages do
not need to be marked encrypted again. And I think the reason is simple:
because you don't really need to, because once the crash kernel is done,
you reboot the box.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

[Borislav Petkov]

On Thu, Sep 27, 2018 at 03:19:54PM +0800, Lianbo Jiang wrote:
> In kdump kernel, we need to dump the old memory into vmcore file,if SME
> is enabled in the first kernel, we have to remap the old memory with the
> memory encryption mask, which will be automatically decrypted when we
> read from DRAM.
> 
> For SME kdump, there are two cases that doesn't support:

... and which are simply silly to support.

> 
>  ----------------------------------------------
> | first-kernel | second-kernel | kdump support |
> |      (mem_encrypt=on|off)    |   (yes|no)    |
> |--------------+---------------+---------------|
> |     on       |     on        |     yes       |
> |     off      |     off       |     yes       |
> |     on       |     off       |     no        |
> |     off      |     on        |     no        |
> |______________|_______________|_______________|
> 
> 1. SME is enabled in the first kernel, but SME is disabled in kdump kernel
> In this case, because the old memory is encrypted, we can't decrypt the
> old memory.
> 
> 2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
> On the one hand, the old memory is unencrypted, the old memory can be dumped

s/unencrypted/decrypted/g

But I mentioned that already.

> as usual, we don't need to enable SME in kdump kernel; On the other hand, it
> will increase the complexity of the code, we will have to consider how to
> pass the SME flag from the first kernel to the kdump kernel, it is really
> too expensive to do this.
> 
> This patches are only for SME kdump, the patches don't support SEV kdump.

Please rewrite that commit message in passive voice. I.e., get rid of
that "we".

> 
> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/kernel/Makefile             |  1 +
>  arch/x86/kernel/crash_dump_encrypt.c | 53 ++++++++++++++++++++++++++++
>  fs/proc/vmcore.c                     | 21 +++++++----
>  include/linux/crash_dump.h           | 12 +++++++
>  4 files changed, 81 insertions(+), 6 deletions(-)
>  create mode 100644 arch/x86/kernel/crash_dump_encrypt.c
> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 8824d01c0c35..dfbeae0e35ce 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -97,6 +97,7 @@ obj-$(CONFIG_KEXEC_CORE)	+= machine_kexec_$(BITS).o
>  obj-$(CONFIG_KEXEC_CORE)	+= relocate_kernel_$(BITS).o crash.o
>  obj-$(CONFIG_KEXEC_FILE)	+= kexec-bzimage64.o
>  obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_$(BITS).o
> +obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= crash_dump_encrypt.o

No no.

This will build even in the CONFIG_CRASH_DUMP=n case.

Why does this need to be even a separate compilation unit? It is a file
containing a single function?!?!

I would love to know what the logic behind this was...

>  obj-y				+= kprobes/
>  obj-$(CONFIG_MODULES)		+= module.o
>  obj-$(CONFIG_DOUBLEFAULT)	+= doublefault.o
> diff --git a/arch/x86/kernel/crash_dump_encrypt.c b/arch/x86/kernel/crash_dump_encrypt.c
> new file mode 100644
> index 000000000000..e1b1a577f197
> --- /dev/null
> +++ b/arch/x86/kernel/crash_dump_encrypt.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *	Memory preserving reboot related code.
> + *
> + *	Created by: Lianbo Jiang (lijiang@redhat.com)
> + *	Copyright (C) RedHat Corporation, 2018. All rights reserved
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/crash_dump.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +
> +/**
> + * copy_oldmem_page_encrypted - copy one page from "oldmem encrypted"
> + * @pfn: page frame number to be copied
> + * @buf: target memory address for the copy; this can be in kernel address
> + *	space or user address space (see @userbuf)
> + * @csize: number of bytes to copy
> + * @offset: offset in bytes into the page (based on pfn) to begin the copy
> + * @userbuf: if set, @buf is in user address space, use copy_to_user(),
> + *	otherwise @buf is in kernel address space, use memcpy().
> + *
> + * Copy a page from "oldmem encrypted". For this page, there is no pte

What is "oldmem encrypted"? Why can't you explain that in plain english?
Note that those comments are not write-only but are meant for other
people to read in the future.

> + * mapped in the current kernel. We stitch up a pte, similar to
> + * kmap_atomic.
> + */
> +
> +ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
> +		size_t csize, unsigned long offset, int userbuf)

Align arguments on the opening brace.

> +{
> +	void  *vaddr;
> +
> +	if (!csize)
> +		return 0;
> +
> +	vaddr = (__force void *)ioremap_encrypted(pfn << PAGE_SHIFT,
> +						  PAGE_SIZE);

Let it stick out.

> +	if (!vaddr)
> +		return -ENOMEM;
> +
> +	if (userbuf) {
> +		if (copy_to_user((void __user *)buf, vaddr + offset, csize)) {
> +			iounmap((void __iomem *)vaddr);
> +			return -EFAULT;
> +		}
> +	} else
> +		memcpy(buf, vaddr + offset, csize);
> +
> +	set_iounmap_nonlazy();
> +	iounmap((void __iomem *)vaddr);
> +	return csize;
> +}
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index cbde728f8ac6..3065c8bada6a 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -25,6 +25,9 @@
>  #include <linux/pagemap.h>
>  #include <linux/uaccess.h>
>  #include <asm/io.h>
> +#include <linux/io.h>
> +#include <linux/mem_encrypt.h>
> +#include <asm/pgtable.h>

Do you not see how the order of the include files is? First linux/ then
asm/, then local headers.

>  #include "internal.h"

And you don't need that if you drop that silly crash_dump_encrypt.c thing.

>  
>  /* List representing chunks of contiguous memory areas and their offsets in
> @@ -98,7 +101,8 @@ static int pfn_is_ram(unsigned long pfn)
>  
>  /* Reads a page from the oldmem device from given offset. */
>  static ssize_t read_from_oldmem(char *buf, size_t count,
> -				u64 *ppos, int userbuf)
> +				u64 *ppos, int userbuf,
> +				bool encrypted)
>  {
>  	unsigned long pfn, offset;
>  	size_t nr_bytes;
> @@ -120,8 +124,11 @@ static ssize_t read_from_oldmem(char *buf, size_t count,
>  		if (pfn_is_ram(pfn) == 0)
>  			memset(buf, 0, nr_bytes);
>  		else {
> -			tmp = copy_oldmem_page(pfn, buf, nr_bytes,
> -						offset, userbuf);
> +			tmp = encrypted ? copy_oldmem_page_encrypted(pfn,
> +					    buf, nr_bytes, offset, userbuf)
> +					: copy_oldmem_page(pfn, buf, nr_bytes,
> +							   offset, userbuf);

Make that a simple if-else so that it can actually be readable.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH v7 RESEND 2/4] kexec: allocate unencrypted control pages for kdump in case SME is enabled

[lijiang]

在 2018年09月28日 15:57, Borislav Petkov 写道:
> On Fri, Sep 28, 2018 at 11:52:21AM +0800, lijiang wrote:
>> There are two functions that are usually called in pairs, they are:
>> arch_kexec_post_alloc_pages() and arch_kexec_pre_free_pages().
>>
>> One marks the pages as decrypted, another one marks the pages as encrypted.
>>
>> But for the crash control pages, no need to call arch_kexec_pre_free_pages(),
>> there are three reasons:
>> 1. Crash pages are reserved in memblock, these pages are only used by kdump,
>>    no other people uses these pages;
>>
>> 2. Whenever crash pages are allocated, these pages are always marked as
>>    decrypted(when SME is active);
>>
>> 3. If we plan to call the arch_kexe_pre_free_pages(), we have to store these
>> pages to somewhere, which will have more code changes.
> 
> I don't think any of that answers the question *why* control pages do
> not need to be marked encrypted again. And I think the reason is simple:
> because you don't really need to, because once the crash kernel is done,
> you reboot the box.
> 
Thanks for your comment, your explanation is very good.

But there are another cases, we might load or unload the crash kernel image and
initrafms, maybe again and again for test or debug, we don't reboot at once. For
example, repeat the following steps:

systemctl start kdump.service
...
systemctl stop kdump.service

But we always mark these pages as decrypted whenever these control pages are
allocated, because other people can't use these pages(reserved memory), which
are only used by kdump, so no need to mark these pages as encrypted again.

Regards,
Lianbo

Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

[lijiang]

在 2018年09月28日 16:38, Borislav Petkov 写道:
> On Thu, Sep 27, 2018 at 03:19:54PM +0800, Lianbo Jiang wrote:
>> In kdump kernel, we need to dump the old memory into vmcore file,if SME
>> is enabled in the first kernel, we have to remap the old memory with the
>> memory encryption mask, which will be automatically decrypted when we
>> read from DRAM.
>>
>> For SME kdump, there are two cases that doesn't support:
> 
> ... and which are simply silly to support.
> 

But the root cause is that the encryption key is not visible to any software
running on the CPU cores(AMD cpu with SME), and is randomly generated on each
system reset. That is to say, kdump kernel won't have a chance to get the
encryption key. So the encrypted memory can not be decrypted unless SME is
active. 

Thanks.
>>
>>  ----------------------------------------------
>> | first-kernel | second-kernel | kdump support |
>> |      (mem_encrypt=on|off)    |   (yes|no)    |
>> |--------------+---------------+---------------|
>> |     on       |     on        |     yes       |
>> |     off      |     off       |     yes       |
>> |     on       |     off       |     no        |
>> |     off      |     on        |     no        |
>> |______________|_______________|_______________|
>>
>> 1. SME is enabled in the first kernel, but SME is disabled in kdump kernel
>> In this case, because the old memory is encrypted, we can't decrypt the
>> old memory.
>>
>> 2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
>> On the one hand, the old memory is unencrypted, the old memory can be dumped
> 
> s/unencrypted/decrypted/g
> 
> But I mentioned that already.
> 

Thanks for your valuable advice to improve these patches.
The patch log and all code style issue will be improved.

>> as usual, we don't need to enable SME in kdump kernel; On the other hand, it
>> will increase the complexity of the code, we will have to consider how to
>> pass the SME flag from the first kernel to the kdump kernel, it is really
>> too expensive to do this.
>>
>> This patches are only for SME kdump, the patches don't support SEV kdump.
> 
> Please rewrite that commit message in passive voice. I.e., get rid of
> that "we".
> 
>>
>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  arch/x86/kernel/Makefile             |  1 +
>>  arch/x86/kernel/crash_dump_encrypt.c | 53 ++++++++++++++++++++++++++++
>>  fs/proc/vmcore.c                     | 21 +++++++----
>>  include/linux/crash_dump.h           | 12 +++++++
>>  4 files changed, 81 insertions(+), 6 deletions(-)
>>  create mode 100644 arch/x86/kernel/crash_dump_encrypt.c
>>
>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>> index 8824d01c0c35..dfbeae0e35ce 100644
>> --- a/arch/x86/kernel/Makefile
>> +++ b/arch/x86/kernel/Makefile
>> @@ -97,6 +97,7 @@ obj-$(CONFIG_KEXEC_CORE)	+= machine_kexec_$(BITS).o
>>  obj-$(CONFIG_KEXEC_CORE)	+= relocate_kernel_$(BITS).o crash.o
>>  obj-$(CONFIG_KEXEC_FILE)	+= kexec-bzimage64.o
>>  obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_$(BITS).o
>> +obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= crash_dump_encrypt.o
> 
> No no.
> 
> This will build even in the CONFIG_CRASH_DUMP=n case.
> 
> Why does this need to be even a separate compilation unit? It is a file
> containing a single function?!?!
> 
> I would love to know what the logic behind this was...
> 
Good question. It's my pleasure.

At first, i added an input parameter for read_from_oldmem() because of encryption(SME). But
for avoiding to also add the same parameter for copy_oldmem_page(), so i added a new function
copy_oldmem_page_encrypted(). Maybe you had noticed that these functions were very similar. 

Sometimes, it is hard to make a choice about this. The reasons are as follows:

A. If i add the same input parameter for copy_oldmem_page(), which will cause more code changes,
   because the interface changes will affect other architectures, such as ARM/MIPS/POWERPC/S390.

B. If not A, i have to add a new function copy_oldmem_page_encrypted(), and also put it into a
   same file(arch/x86/kernel/crash_dump_64.c).
 
   Should i merge the similar functions into one function?
   ( copy_oldmem_page() and copy_oldmem_page_encrypted() )

   a. If the answer is no, i will have two choices:
      1). put these functions into a same file(arch/x86/kernel/crash_dump_64.c). But someone
          thought that these functions were too similar, these functions should be merged or
          rebuilt. This is contradictory to the case a.
      2). add a new file for copy_oldmem_page_encrypted(), then which will avoid the case '1)'.
          That is just current solution.

   b. If the answer is yes, i have to add an input parameter for copy_oldmem_page(), this is back
      to the case A.

Could you give me any advice about this? Any advice will be appreciated.

Thanks.
Lianbo

>>  obj-y				+= kprobes/
>>  obj-$(CONFIG_MODULES)		+= module.o
>>  obj-$(CONFIG_DOUBLEFAULT)	+= doublefault.o
>> diff --git a/arch/x86/kernel/crash_dump_encrypt.c b/arch/x86/kernel/crash_dump_encrypt.c
>> new file mode 100644
>> index 000000000000..e1b1a577f197
>> --- /dev/null
>> +++ b/arch/x86/kernel/crash_dump_encrypt.c
>> @@ -0,0 +1,53 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + *	Memory preserving reboot related code.
>> + *
>> + *	Created by: Lianbo Jiang (lijiang@redhat.com)
>> + *	Copyright (C) RedHat Corporation, 2018. All rights reserved
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/crash_dump.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/io.h>
>> +
>> +/**
>> + * copy_oldmem_page_encrypted - copy one page from "oldmem encrypted"
>> + * @pfn: page frame number to be copied
>> + * @buf: target memory address for the copy; this can be in kernel address
>> + *	space or user address space (see @userbuf)
>> + * @csize: number of bytes to copy
>> + * @offset: offset in bytes into the page (based on pfn) to begin the copy
>> + * @userbuf: if set, @buf is in user address space, use copy_to_user(),
>> + *	otherwise @buf is in kernel address space, use memcpy().
>> + *
>> + * Copy a page from "oldmem encrypted". For this page, there is no pte
> 
> What is "oldmem encrypted"? Why can't you explain that in plain english?

So sorry, here "oldmem encrypted" means the "old memory is encrypted".

> Note that those comments are not write-only but are meant for other
> people to read in the future.
> 
>> + * mapped in the current kernel. We stitch up a pte, similar to
>> + * kmap_atomic.
>> + */
>> +
>> +ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
>> +		size_t csize, unsigned long offset, int userbuf)
> 
> Align arguments on the opening brace.
> 
>> +{
>> +	void  *vaddr;
>> +
>> +	if (!csize)
>> +		return 0;
>> +
>> +	vaddr = (__force void *)ioremap_encrypted(pfn << PAGE_SHIFT,
>> +						  PAGE_SIZE);
> 
> Let it stick out.
> 
>> +	if (!vaddr)
>> +		return -ENOMEM;
>> +
>> +	if (userbuf) {
>> +		if (copy_to_user((void __user *)buf, vaddr + offset, csize)) {
>> +			iounmap((void __iomem *)vaddr);
>> +			return -EFAULT;
>> +		}
>> +	} else
>> +		memcpy(buf, vaddr + offset, csize);
>> +
>> +	set_iounmap_nonlazy();
>> +	iounmap((void __iomem *)vaddr);
>> +	return csize;
>> +}
>> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
>> index cbde728f8ac6..3065c8bada6a 100644
>> --- a/fs/proc/vmcore.c
>> +++ b/fs/proc/vmcore.c
>> @@ -25,6 +25,9 @@
>>  #include <linux/pagemap.h>
>>  #include <linux/uaccess.h>
>>  #include <asm/io.h>
>> +#include <linux/io.h>
>> +#include <linux/mem_encrypt.h>
>> +#include <asm/pgtable.h>
> 
> Do you not see how the order of the include files is? First linux/ then
> asm/, then local headers.
> 
>>  #include "internal.h"
> 
> And you don't need that if you drop that silly crash_dump_encrypt.c thing.
> 
>>  
>>  /* List representing chunks of contiguous memory areas and their offsets in
>> @@ -98,7 +101,8 @@ static int pfn_is_ram(unsigned long pfn)
>>  
>>  /* Reads a page from the oldmem device from given offset. */
>>  static ssize_t read_from_oldmem(char *buf, size_t count,
>> -				u64 *ppos, int userbuf)
>> +				u64 *ppos, int userbuf,
>> +				bool encrypted)
>>  {
>>  	unsigned long pfn, offset;
>>  	size_t nr_bytes;
>> @@ -120,8 +124,11 @@ static ssize_t read_from_oldmem(char *buf, size_t count,
>>  		if (pfn_is_ram(pfn) == 0)
>>  			memset(buf, 0, nr_bytes);
>>  		else {
>> -			tmp = copy_oldmem_page(pfn, buf, nr_bytes,
>> -						offset, userbuf);
>> +			tmp = encrypted ? copy_oldmem_page_encrypted(pfn,
>> +					    buf, nr_bytes, offset, userbuf)
>> +					: copy_oldmem_page(pfn, buf, nr_bytes,
>> +							   offset, userbuf);
> 
> Make that a simple if-else so that it can actually be readable.
> 

Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

[Borislav Petkov]

On Sat, Sep 29, 2018 at 02:24:52PM +0800, lijiang wrote:
> At first, i added an input parameter for read_from_oldmem() because of encryption(SME). But
> for avoiding to also add the same parameter for copy_oldmem_page(), so i added a new function
> copy_oldmem_page_encrypted(). Maybe you had noticed that these functions were very similar.

If you have two very similar functions, you add a *static* workhorse function:

static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize, unsigned long offset,
				  int userbuf, bool encrypted)

and you define two wrappers:

copy_oldmem_page()
copy_oldmem_page_encrypted()

which both call __copy_oldmem_page() with the appropriate parameters.

But you do *not* do a separate compilation unit just because. None of
the reasons you've mentioned warrant having a separate compilation
unit while you already have *the* perfect place to put everything -
arch/x86/kernel/crash_dump_64.c

> So sorry, here "oldmem encrypted" means the "old memory is encrypted".

I know what it means - I'm trying to explain to you to write it out
in plain english and not use some strange constructs like "oldmem
encrypted".

A reader would wonder: why is this thing semi-abbreviated and in
quotation marks? Does that mean anything special?

Our comments should not be write-only. So after you've written it, try
to read it as someone who sees the code for the first time and think
hard whether she/he will understand it.

Do you catch my drift now?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH v7 RESEND 2/4] kexec: allocate unencrypted control pages for kdump in case SME is enabled

[Borislav Petkov]

On Fri, Sep 28, 2018 at 06:09:04PM +0800, lijiang wrote:
> But there are another cases, we might load or unload the crash kernel image and
> initrafms, maybe again and again for test or debug, we don't reboot at once. For

I don't think this qualifies even as a use case - this is what you do
during development.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v7 RESEND 4/4] kdump/vmcore: support encrypted old memory with SME enabled

[lijiang]

在 2018年09月29日 16:30, Borislav Petkov 写道:
> On Sat, Sep 29, 2018 at 02:24:52PM +0800, lijiang wrote:
>> At first, i added an input parameter for read_from_oldmem() because of encryption(SME). But
>> for avoiding to also add the same parameter for copy_oldmem_page(), so i added a new function
>> copy_oldmem_page_encrypted(). Maybe you had noticed that these functions were very similar.
> 
> If you have two very similar functions, you add a *static* workhorse function:
> 
> static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize, unsigned long offset,
> 				  int userbuf, bool encrypted)
> 
> and you define two wrappers:
> 
> copy_oldmem_page()
> copy_oldmem_page_encrypted()
> 
> which both call __copy_oldmem_page() with the appropriate parameters.
> 

Great! Previously i was afraid that the maintainer might disagree with
rewriting the function copy_oldmem_page().

That's really great. I will modify this patch and post the series again.

Thanks.
Lianbo
> But you do *not* do a separate compilation unit just because. None of
> the reasons you've mentioned warrant having a separate compilation
> unit while you already have *the* perfect place to put everything -
> arch/x86/kernel/crash_dump_64.c
> 
>> So sorry, here "oldmem encrypted" means the "old memory is encrypted".
> 
> I know what it means - I'm trying to explain to you to write it out
> in plain english and not use some strange constructs like "oldmem
> encrypted".
> 
> A reader would wonder: why is this thing semi-abbreviated and in
> quotation marks? Does that mean anything special?
> 
> Our comments should not be write-only. So after you've written it, try
> to read it as someone who sees the code for the first time and think
> hard whether she/he will understand it.
> 
> Do you catch my drift now?
> 
Yes, got it. Thanks for your valuable time and patience.

Regards,
Lianbo

[tip:x86/mm] x86/ioremap: Add an ioremap_encrypted() helper

[tip-bot for Lianbo Jiang]

Commit-ID:  c3a7a61c192ec350330128edb13db33a9bc0ace1
Gitweb:     https://git.kernel.org/tip/c3a7a61c192ec350330128edb13db33a9bc0ace1
Author:     Lianbo Jiang <lijiang@redhat.com>
AuthorDate: Thu, 27 Sep 2018 15:19:51 +0800
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Sat, 6 Oct 2018 11:57:51 +0200

x86/ioremap: Add an ioremap_encrypted() helper

When SME is enabled, the memory is encrypted in the first kernel. In
this case, SME also needs to be enabled in the kdump kernel, and we have
to remap the old memory with the memory encryption mask.

The case of concern here is if SME is active in the first kernel,
and it is active too in the kdump kernel. There are four cases to be
considered:

a. dump vmcore
   It is encrypted in the first kernel, and needs be read out in the
   kdump kernel.

b. crash notes
   When dumping vmcore, the people usually need to read useful
   information from notes, and the notes is also encrypted.

c. iommu device table
   It's encrypted in the first kernel, kdump kernel needs to access its
   content to analyze and get information it needs.

d. mmio of AMD iommu
   not encrypted in both kernels

Add a new bool parameter @encrypted to __ioremap_caller(). If set,
memory will be remapped with the SME mask.

Add a new function ioremap_encrypted() to explicitly pass in a true
value for @encrypted. Use ioremap_encrypted() for the above a, b, c
cases.

 [ bp: cleanup commit message, extern defs in io.h and drop forgotten
   include. ]

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Cc: kexec@lists.infradead.org
Cc: tglx@linutronix.de
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: akpm@linux-foundation.org
Cc: dan.j.williams@intel.com
Cc: bhelgaas@google.com
Cc: baiyaowei@cmss.chinamobile.com
Cc: tiwai@suse.de
Cc: brijesh.singh@amd.com
Cc: dyoung@redhat.com
Cc: bhe@redhat.com
Cc: jroedel@suse.de
Link: https://lkml.kernel.org/r/20180927071954.29615-2-lijiang@redhat.com
---
 arch/x86/include/asm/io.h |  3 ++-
 arch/x86/mm/ioremap.c     | 24 ++++++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 6de64840dd22..6df53efcecfd 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -187,11 +187,12 @@ extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size)
 #define ioremap_nocache ioremap_nocache
 extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
 #define ioremap_uc ioremap_uc
-
 extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
 #define ioremap_cache ioremap_cache
 extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
 #define ioremap_prot ioremap_prot
+extern void __iomem *ioremap_encrypted(resource_size_t phys_addr, unsigned long size);
+#define ioremap_encrypted ioremap_encrypted
 
 /**
  * ioremap     -   map bus memory into CPU space
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index c63a545ec199..24e0920a9b25 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -131,7 +131,8 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
  * caller shouldn't need to know that small detail.
  */
 static void __iomem *__ioremap_caller(resource_size_t phys_addr,
-		unsigned long size, enum page_cache_mode pcm, void *caller)
+		unsigned long size, enum page_cache_mode pcm,
+		void *caller, bool encrypted)
 {
 	unsigned long offset, vaddr;
 	resource_size_t last_addr;
@@ -199,7 +200,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	 * resulting mapping.
 	 */
 	prot = PAGE_KERNEL_IO;
-	if (sev_active() && mem_flags.desc_other)
+	if ((sev_active() && mem_flags.desc_other) || encrypted)
 		prot = pgprot_encrypted(prot);
 
 	switch (pcm) {
@@ -291,7 +292,7 @@ void __iomem *ioremap_nocache(resource_size_t phys_addr, unsigned long size)
 	enum page_cache_mode pcm = _PAGE_CACHE_MODE_UC_MINUS;
 
 	return __ioremap_caller(phys_addr, size, pcm,
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_nocache);
 
@@ -324,7 +325,7 @@ void __iomem *ioremap_uc(resource_size_t phys_addr, unsigned long size)
 	enum page_cache_mode pcm = _PAGE_CACHE_MODE_UC;
 
 	return __ioremap_caller(phys_addr, size, pcm,
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL_GPL(ioremap_uc);
 
@@ -341,7 +342,7 @@ EXPORT_SYMBOL_GPL(ioremap_uc);
 void __iomem *ioremap_wc(resource_size_t phys_addr, unsigned long size)
 {
 	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WC,
-					__builtin_return_address(0));
+					__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_wc);
 
@@ -358,14 +359,21 @@ EXPORT_SYMBOL(ioremap_wc);
 void __iomem *ioremap_wt(resource_size_t phys_addr, unsigned long size)
 {
 	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WT,
-					__builtin_return_address(0));
+					__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_wt);
 
+void __iomem *ioremap_encrypted(resource_size_t phys_addr, unsigned long size)
+{
+	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WB,
+				__builtin_return_address(0), true);
+}
+EXPORT_SYMBOL(ioremap_encrypted);
+
 void __iomem *ioremap_cache(resource_size_t phys_addr, unsigned long size)
 {
 	return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WB,
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_cache);
 
@@ -374,7 +382,7 @@ void __iomem *ioremap_prot(resource_size_t phys_addr, unsigned long size,
 {
 	return __ioremap_caller(phys_addr, size,
 				pgprot2cachemode(__pgprot(prot_val)),
-				__builtin_return_address(0));
+				__builtin_return_address(0), false);
 }
 EXPORT_SYMBOL(ioremap_prot);