[PATCH 0/5 V5] Support kdump for AMD secure memory encryption(SME)

[PATCH 0/5 V5] Support kdump for AMD secure memory encryption(SME)

[Lianbo Jiang]

When sme enabled on AMD server, we also need to support kdump. Because
the memory is encrypted in the first kernel, we will remap the old memory
encrypted to the second kernel(crash kernel), and sme is also enabled in
the second kernel, otherwise the old memory encrypted can not be decrypted.
Because simply changing the value of a C-bit on a page will not
automatically encrypt the existing contents of a page, and any data in the
page prior to the C-bit modification will become unintelligible. A page of
memory that is marked encrypted will be automatically decrypted when read
from DRAM and will be automatically encrypted when written to DRAM.

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 deal with the
data(encrypted or decrypted). For example, when sme enabled, if the old
memory is encrypted, we will remap the old memory in encrypted way, which
will automatically decrypt the old memory encrypted when we read those data
from the remapping address.

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

This patch is only for SME kdump, it is not support SEV kdump.

For kdump(SME), there are two cases that doesn't support:
1. SME is enabled in the first kernel, but SME is disabled in the
second kernel
Because the old memory is encrypted, we can't decrypt the old memory
if SME is off in the second kernel.

2. SME is disabled in the first kernel, but SME is enabled in the
second kernel
Maybe it is unnecessary to support this case, because the old memory
is unencrypted, the old memory can be dumped as usual, we don't need
to enable sme in the second kernel, furthermore the requirement is
rare in actual deployment. Another, If we must support the scenario,
it will increase the complexity of the code, we will have to consider
how to transfer the sme flag from the first kernel to the second kernel,
in order to let the second kernel know that whether the old memory is
encrypted.
There are two manners to transfer the SME flag to the second kernel, the
first way is to modify the assembly code, which includes some common
code and the path is too long. The second way is to use kexec tool,
which could require the sme flag to be exported in the first kernel
by "proc" or "sysfs", kexec will read the sme flag from "proc" or
"sysfs" when we use kexec tool 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. Although we can fix this
issue, maybe it is too expensive to do this. By the way, we won't fix
the problem unless someone thinks it is necessary to do it.

Test tools:
makedumpfile[v1.6.3]: https://github.com/LianboJ/makedumpfile
commit e1de103eca8f (A draft for kdump vmcore about AMD SME)
Author: Lianbo Jiang <lijiang@redhat.com>
Date:   Mon May 14 17:02:40 2018 +0800
Note: This patch can only dump vmcore in the case of SME enabled.

crash-7.2.1: https://github.com/crash-utility/crash.git
commit 1e1bd9c4c1be (Fix for the "bpf" command display on Linux 4.17-rc1)
Author: Dave Anderson <anderson@redhat.com>
Date:   Fri May 11 15:54:32 2018 -0400

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

Linux 4.18-rc3:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
commit 021c91791a5e7e85c567452f1be3e4c2c6cb6063
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun Jul 1 16:04:53 2018 -0700

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

Some changes:
1. remove the sme_active() check in __ioremap_caller().
2. remove the '#ifdef' stuff throughout this patch.
3. put some logic into the early_memremap_pgprot_adjust() and clean the
previous unnecessary changes, for example: arch/x86/include/asm/dmi.h,
arch/x86/kernel/acpi/boot.c, drivers/acpi/tables.c.
4. add a new file and modify Makefile.
5. clean compile warning in copy_device_table() and some compile error.
6. split the original patch into five patches, it will be better for
review.
7. add some comments.

Some known issues:
1. about SME
Upstream kernel doesn't work when we use kexec in the follow command. The
system will hang on 'HP ProLiant DL385Gen10 AMD EPYC 7251'. But it can't
reproduce on speedway.
(This issue doesn't matter with the kdump patch.)

Reproduce steps:
 # kexec -l /boot/vmlinuz-4.18.0-rc3+ --initrd=/boot/initramfs-4.18.0-rc3+.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)

The system will hang:
[ 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]

2. about SEV
Upstream kernel(Host OS) doesn't work in host side, some drivers about
SEV always go wrong in host side. We can't boot SEV Guest OS to test
kdump patch. Maybe it is more reasonable to improve SEV in another
patch. When some drivers can work in host side and it can also boot
Virtual Machine(SEV Guest OS), it will be suitable to fix SEV for kdump.

[  369.426131] INFO: task systemd-udevd:865 blocked for more than 120 seconds.
[  369.433177]       Not tainted 4.17.0-rc5+ #60
[  369.437585] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  369.445783] systemd-udevd   D    0   865    813 0x80000004
[  369.451323] Call Trace:
[  369.453815]  ? __schedule+0x290/0x870
[  369.457523]  schedule+0x32/0x80
[  369.460714]  __sev_do_cmd_locked+0x1f6/0x2a0 [ccp]
[  369.465556]  ? cleanup_uevent_env+0x10/0x10
[  369.470084]  ? remove_wait_queue+0x60/0x60
[  369.474219]  ? 0xffffffffc0247000
[  369.477572]  __sev_platform_init_locked+0x2b/0x70 [ccp]
[  369.482843]  sev_platform_init+0x1d/0x30 [ccp]
[  369.487333]  psp_pci_init+0x40/0xe0 [ccp]
[  369.491380]  ? 0xffffffffc0247000
[  369.494936]  sp_mod_init+0x18/0x1000 [ccp]
[  369.499071]  do_one_initcall+0x4e/0x1d4
[  369.502944]  ? _cond_resched+0x15/0x30
[  369.506728]  ? kmem_cache_alloc_trace+0xae/0x1d0
[  369.511386]  ? do_init_module+0x22/0x220
[  369.515345]  do_init_module+0x5a/0x220
[  369.519444]  load_module+0x21cb/0x2a50
[  369.523227]  ? m_show+0x1c0/0x1c0
[  369.526571]  ? security_capable+0x3f/0x60
[  369.530611]  __do_sys_finit_module+0x94/0xe0
[  369.534915]  do_syscall_64+0x5b/0x180
[  369.538607]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  369.543698] RIP: 0033:0x7f708e6311b9
[  369.547536] RSP: 002b:00007ffff9d32aa8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[  369.555162] RAX: ffffffffffffffda RBX: 000055602a04c2d0 RCX: 00007f708e6311b9
[  369.562346] RDX: 0000000000000000 RSI: 00007f708ef52039 RDI: 0000000000000008
[  369.569801] RBP: 00007f708ef52039 R08: 0000000000000000 R09: 000055602a048b20
[  369.576988] R10: 0000000000000008 R11: 0000000000000246 R12: 0000000000000000
[  369.584177] R13: 000055602a075260 R14: 0000000000020000 R15: 0000000000000000

Lianbo Jiang (5):
  Add a function(ioremap_encrypted) for kdump when AMD sme enabled
  Allocate pages for kdump without encryption when SME is enabled
  Remap the device table of IOMMU in encrypted manner for kdump
  Adjust some permanent mappings in unencrypted ways for kdump when SME
    is enabled.
  Help to dump the old memory encrypted into vmcore file

 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                | 36 ++++++++++++++++++------
 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, 135 insertions(+), 17 deletions(-)
 create mode 100644 arch/x86/kernel/crash_dump_encrypt.c

-- 
2.9.5

[PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Lianbo Jiang]

It is convenient to remap the old memory encrypted to the second
kernel by calling ioremap_encrypted().

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
Some changes:
1. remove the sme_active() check in __ioremap_caller().
2. revert some logic in the early_memremap_pgprot_adjust() for
early memremap and make it separate a new patch.

 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 6de6484..f8795f9 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 c63a545..e01e6c6 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.9.5

[PATCH 2/5 V5] Allocate pages for kdump without encryption when SME is enabled

[Lianbo Jiang]

When SME is enabled in the first kernel, we will allocate pages
for kdump without encryption in order to be able to boot the
second kernel in the same manner as kexec, which helps to keep
the same code style.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
Some changes:
1. remove some redundant codes for crash control pages.
2. add some comments.

 kernel/kexec_core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 23a83a4..e7efcd1 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.9.5

[PATCH 3/5 V5] Remap the device table of IOMMU in encrypted manner for kdump

[Lianbo Jiang]

In kdump mode, 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 must remap it in encrypted manner in order to be
automatically decrypted when we read.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
Some changes:
1. add some comments
2. clean compile warning.
3. remove unnecessary code when we clear sme mask bit.

 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 904c575..4cebb00 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -888,12 +888,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 mode.
+	 */
+	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.9.5

[PATCH 4/5 V5] Adjust some permanent mappings in unencrypted ways for kdump when SME is enabled.

[Lianbo Jiang]

For kdump, the acpi table and dmi table will need to be remapped in
unencrypted ways during early init, they have just a simple wrapper
around early_memremap(), but the early_memremap() remaps the memory
in encrypted ways by default when SME is enabled, so we put some logic
into the early_memremap_pgprot_adjust(), which will have an opportunity
to adjust it.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
 arch/x86/mm/ioremap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index e01e6c6..3c1c8c4 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -689,8 +689,17 @@ pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
 	encrypted_prot = true;
 
 	if (sme_active()) {
+		/*
+		 * In kdump mode, the acpi table and dmi table will need to
+		 * be remapped in unencrypted ways during early init when
+		 * SME is enabled. They have just a simple wrapper around
+		 * early_memremap(), but the early_memremap() remaps the
+		 * memory in encrypted ways by default when SME is enabled,
+		 * so we must adjust it.
+		 */
 		if (early_memremap_is_setup_data(phys_addr, size) ||
-		    memremap_is_efi_data(phys_addr, size))
+		    memremap_is_efi_data(phys_addr, size) ||
+		    is_kdump_kernel())
 			encrypted_prot = false;
 	}
 
-- 
2.9.5

[PATCH 5/5 V5] Help to dump the old memory encrypted into vmcore file

[Lianbo Jiang]

In kdump mode, we need to dump the old memory into vmcore file,
if SME is enabled in the first kernel, we must remap the old
memory in encrypted manner, which will be automatically decrypted
when we read from DRAM. It helps to parse the vmcore for some tools.

Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
Some changes:
1. add a new file and modify Makefile.
2. revert some code about previously using sev_active().

 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 02d6f5c..afb5bad 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -96,6 +96,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 0000000..e1b1a57
--- /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 cfb6674..07c1934 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);
 }
 
@@ -349,7 +357,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 3e4ba9d..a7e7be8 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.9.5

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Borislav Petkov]

On Mon, Jul 02, 2018 at 03:26:35PM +0800, Lianbo Jiang wrote:
> @@ -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)

So instead of sprinkling that @encrypted argument everywhere and then
setting it based on sme_active() ...

>  {
>  	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)

... why can't you simply do your checks:

	sme_active() && is_kdump_kernel()

here so that __ioremap_caller() can automatically choose the proper
pgprot value when ioremapping the memory in the kdump kernel?

And this way the callers don't even have to care whether the memory is
encrypted or not?

>  		prot = pgprot_encrypted(prot);
>  
>  	switch (pcm) {

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月02日 18:14, Borislav Petkov 写道:
> On Mon, Jul 02, 2018 at 03:26:35PM +0800, Lianbo Jiang wrote:
>> @@ -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)
> 
> So instead of sprinkling that @encrypted argument everywhere and then
> setting it based on sme_active() ...
> 
>>  {
>>  	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)
> 
> ... why can't you simply do your checks:
> 
> 	sme_active() && is_kdump_kernel()
> 
> here so that __ioremap_caller() can automatically choose the proper
> pgprot value when ioremapping the memory in the kdump kernel?
> 
> And this way the callers don't even have to care whether the memory is
> encrypted or not?
> 
Thank you, Boris. I'm very glad to read your comments. That's a good idea, but it has some
unencrypted memory in kdump mode, for example, the elfcorehdr. In fact, the elfcorehdr and
notes call the same function(read_from_oldmem->ioremap_cache), in this case, it is very
difficult to properly remap the memory if the caller don't care whether the memory is encrypted.

Regards,
Lianbo
>>  		prot = pgprot_encrypted(prot);
>>  
>>  	switch (pcm) {
> 

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Borislav Petkov]

On Tue, Jul 03, 2018 at 10:17:19AM +0800, lijiang wrote:
> for example, the elfcorehdr. In fact, the elfcorehdr and notes

You mean this?

 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());

That looks encrypted to me.

> call the same function(read_from_oldmem->ioremap_cache), in this case,
> it is very difficult to properly remap the memory if the caller don't
> care whether the memory is encrypted.

So beef up the logic in __ioremap_caller() to figure out based on the
address whether to access the memory encrypted or not. You can find out
the elfcorehdr address in the capture kernel.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月03日 10:17, lijiang 写道:
> 在 2018年07月02日 18:14, Borislav Petkov 写道:
>> On Mon, Jul 02, 2018 at 03:26:35PM +0800, Lianbo Jiang wrote:
>>> @@ -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)
>>
>> So instead of sprinkling that @encrypted argument everywhere and then
>> setting it based on sme_active() ...
>>
>>>  {
>>>  	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)
>>
>> ... why can't you simply do your checks:
>>
>> 	sme_active() && is_kdump_kernel()
>>
>> here so that __ioremap_caller() can automatically choose the proper
>> pgprot value when ioremapping the memory in the kdump kernel?
>>
>> And this way the callers don't even have to care whether the memory is
>> encrypted or not?
>>
> Thank you, Boris. I'm very glad to read your comments. That's a good idea, but it has some
> unencrypted memory in kdump mode, for example, the elfcorehdr. In fact, the elfcorehdr and
> notes call the same function(read_from_oldmem->ioremap_cache), in this case, it is very
> difficult to properly remap the memory if the caller don't care whether the memory is encrypted.
> 
Hi, Boris,
About this, maybe I should describe the more details.
For kdump, the elf header finally use the crash kernel reserved memory, it is not an old memory.
When we load the crash kernel image, kexec tools will copy the elf header from encrypted memory
region to unencrypted memory region, we know that a page of memory that is marked encrypted will
be automatically decrypted when read from DRAM if SME is enabled. This operation make us get the
plaintext, that is to say, it becomes unencrypted data, so we need to remap the elfcorehdr in un-
encrypted manners in kdump kernel, but elf notes use an old memory, it is encrypted. That is the
real reason why we need to use the different functions(ioremap_cache/ioremap_encrypted).
If the elf core header is set to be encrypted, we could need to know which part of memory is
allocated for the elfcorehdr in kernel space and change the page attribute, maybe which will have
to modify another common codes and kexec-tools.

If you agree with my explanation, i will add them to patch log and also post it again.
Welcome to review my patches again.

Thanks.
Lianbo

> Regards,
> Lianbo>>>  		prot = pgprot_encrypted(prot);
>>>  
>>>  	switch (pcm) {
>>

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Borislav Petkov]

On Tue, Jul 03, 2018 at 06:58:14PM +0800, lijiang wrote:
> For kdump, the elf header finally use the crash kernel reserved memory, it is not an old memory.

Lamme repeat my suggestion:

So beef up the logic in __ioremap_caller() to figure out based on the
address whether to access the memory encrypted or not. In general, you
can deduce, based on the region you're mapping, whether you need to map
in encrypted or decrypted.

For example:

        addr = elfcorehdr_addr;

        /* Read Elf header */
        rc = elfcorehdr_read((char *)&ehdr, sizeof(Elf64_Ehdr), &addr);
        if (rc < 0)
                return rc;

elfcorehdr_addr has that elfcorehdr address. So you can check which address
you're mapping and do:

__ioremap_caller:

...
	prot = __ioremap_compute_prot(...);

and that __ioremap_compute_prot() function which you will add will have
all that logic to determine encrypted or not by comparing addresses etc.

Does that make more sense?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月03日 17:39, Borislav Petkov 写道:
> On Tue, Jul 03, 2018 at 10:17:19AM +0800, lijiang wrote:
>> for example, the elfcorehdr. In fact, the elfcorehdr and notes
> 
> You mean this?
> 
>  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());
> 
> That looks encrypted to me.
> 
The elf notes is an old memory, it is encrypted.

But the elf header is a crash kernel reserved memory, which is unencrypted.
 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);
 }

They call the same function(read_from_oldmem->ioremap_cache), so it is hard to
properly remap the memory if we don't use the parameter to distinguish. 

Regards,
Lianbo
>> call the same function(read_from_oldmem->ioremap_cache), in this case,
>> it is very difficult to properly remap the memory if the caller don't
>> care whether the memory is encrypted.
> 
> So beef up the logic in __ioremap_caller() to figure out based on the
> address whether to access the memory encrypted or not. You can find out
> the elfcorehdr address in the capture kernel.
> 

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月03日 19:14, Borislav Petkov 写道:
> On Tue, Jul 03, 2018 at 06:58:14PM +0800, lijiang wrote:
>> For kdump, the elf header finally use the crash kernel reserved memory, it is not an old memory.
> 
> Lamme repeat my suggestion:
> 
> So beef up the logic in __ioremap_caller() to figure out based on the
> address whether to access the memory encrypted or not. In general, you
> can deduce, based on the region you're mapping, whether you need to map
> in encrypted or decrypted.
> 
> For example:
> 
>         addr = elfcorehdr_addr;
> 
>         /* Read Elf header */
>         rc = elfcorehdr_read((char *)&ehdr, sizeof(Elf64_Ehdr), &addr);
>         if (rc < 0)
>                 return rc;
> 
> elfcorehdr_addr has that elfcorehdr address. So you can check which address
> you're mapping and do:
> 
> __ioremap_caller:
> 
> ...
> 	prot = __ioremap_compute_prot(...);
> 
> and that __ioremap_compute_prot() function which you will add will have
> all that logic to determine encrypted or not by comparing addresses etc.
> 
> Does that make more sense?
> 
Thank you, Boris. Good idea, I will rethink about this issue and post it again.

Regards,
Lianbo

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月03日 19:44, lijiang 写道:
> 在 2018年07月03日 19:14, Borislav Petkov 写道:
>> On Tue, Jul 03, 2018 at 06:58:14PM +0800, lijiang wrote:
>>> For kdump, the elf header finally use the crash kernel reserved memory, it is not an old memory.
>>
>> Lamme repeat my suggestion:
>>
>> So beef up the logic in __ioremap_caller() to figure out based on the
>> address whether to access the memory encrypted or not. In general, you
>> can deduce, based on the region you're mapping, whether you need to map
>> in encrypted or decrypted.
>>
>> For example:
>>
>>         addr = elfcorehdr_addr;
>>
>>         /* Read Elf header */
>>         rc = elfcorehdr_read((char *)&ehdr, sizeof(Elf64_Ehdr), &addr);
>>         if (rc < 0)
>>                 return rc;
>>
>> elfcorehdr_addr has that elfcorehdr address. So you can check which address
>> you're mapping and do:
>>
>> __ioremap_caller:
>>
>> ...
>> 	prot = __ioremap_compute_prot(...);
>>
>> and that __ioremap_compute_prot() function which you will add will have
>> all that logic to determine encrypted or not by comparing addresses etc.
>>
>> Does that make more sense?
>>
> Thank you, Boris. Good idea, I will rethink about this issue and post it again.
> 
Hi, Boris
Last week, I had tried many ways to do this work, but it looks like that the ways of
deducing address is not suitable to another scenarios, such as mapping some devices
mmio space, which are unencrypted, and the device mmio space is outside kdump kernel
like the old memory, but the old memory is encrypted, we can't find the general rules
to decide when to encrypt or not. So it should be the best way that the caller care about
encryption or not. What do you think?

> Regards,
> Lianbo
> 

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Borislav Petkov]

On Mon, Jul 09, 2018 at 02:28:11PM +0800, lijiang wrote:
> Last week, I had tried many ways to do this work, but it looks
> like that the ways of deducing address is not suitable to another
> scenarios, such as mapping some devices mmio space, which are
> unencrypted, and the device mmio space is outside kdump kernel like
> the old memory, but the old memory is encrypted, we can't find the
> general rules to decide when to encrypt or not.

If we can't find the "general rules" how is the caller supposed to know
how to access the memory?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月09日 17:29, Borislav Petkov 写道:
> On Mon, Jul 09, 2018 at 02:28:11PM +0800, lijiang wrote:
>> Last week, I had tried many ways to do this work, but it looks
>> like that the ways of deducing address is not suitable to another
>> scenarios, such as mapping some devices mmio space, which are
>> unencrypted, and the device mmio space is outside kdump kernel like
>> the old memory, but the old memory is encrypted, we can't find the
>> general rules to decide when to encrypt or not.
> 
> If we can't find the "general rules" how is the caller supposed to know
> how to access the memory?
> 
About this issue, i want to use an example to describe it.
/* drivers/iommu/amd_iommu_init.c */
static u8 __iomem * __init iommu_map_mmio_space(u64 address, u64 end)
{
        if (!request_mem_region(address, end, "amd_iommu")) {
                pr_err("AMD-Vi: Can not reserve memory region %llx-%llx for mmio\n",
                        address, end);
                pr_err("AMD-Vi: This is a BIOS bug. Please contact your hardware vendor\n");
                return NULL;
        }

        return (u8 __iomem *)ioremap_nocache(address, end);
}

[root@hp-dl385g10-03 linux]# cat /proc/iomem |grep -i crash
  7fa000000-879ffffff : Crash kernel
[root@hp-dl385g10-03 linux]# cat /proc/iomem |grep amd_iommu
  9c800000-9c87ffff : amd_iommu
  a9800000-a987ffff : amd_iommu
  b6800000-b687ffff : amd_iommu
  c3800000-c387ffff : amd_iommu
  d0800000-d087ffff : amd_iommu
  dd800000-dd87ffff : amd_iommu
  ea800000-ea87ffff : amd_iommu
  fd800000-fd87ffff : amd_iommu

Obviously, the iommu mmio space is not encrypted, and the device mmio space is outside kdump
kernel. We know that the old memory is encrypted, and the old memory is also outside kdump
kernel. For the current case, e820__get_entry_type() and walk_iomem_res_desc() can't get the
desired result, so we can't also decide whether encryption or not according to this result(rules).
If we want to know whether encryption or not by deducing the address, we will need to read
the content of memory and have a reference value for comparison, then what's a reference value?
Sometimes we don't know that.

Regards,
Lianbo

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Borislav Petkov]

On Mon, Jul 09, 2018 at 09:55:35PM +0800, lijiang wrote:
> About this issue, i want to use an example to describe it.
> /* drivers/iommu/amd_iommu_init.c */
> static u8 __iomem * __init iommu_map_mmio_space(u64 address, u64 end)

Those addresses come from the IVHD header which is an ACPI table. So the
dump kernel can find that out too.

> Obviously, the iommu mmio space is not encrypted, and the device
> mmio space is outside kdump kernel. We know that the old memory is
> encrypted, and the old memory is also outside kdump kernel. For the
> current case, e820__get_entry_type() and walk_iomem_res_desc() can't
> get the desired result, so we can't also decide whether encryption
> or not according to this result(rules). If we want to know whether
> encryption or not by deducing the address, we will need to read the
> content of memory and have a reference value for comparison, then
> what's a reference value? Sometimes we don't know that.

Again, if we don't know that how is the *caller* supposed to know
whether the memory is encrypted or not? Because

"we" == "caller"

in the kdump kernel.

And the more important question is, why are we dumping MMIO space of the
previous kernel *at* *all*? That doesn't make any sense to me.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月14日 01:08, Borislav Petkov 写道:
> On Mon, Jul 09, 2018 at 09:55:35PM +0800, lijiang wrote:
>> About this issue, i want to use an example to describe it.
>> /* drivers/iommu/amd_iommu_init.c */
>> static u8 __iomem * __init iommu_map_mmio_space(u64 address, u64 end)
> 
> Those addresses come from the IVHD header which is an ACPI table. So the
> dump kernel can find that out too.
> Sure. I might understand your means, that will have to find all address out in
order to cover any cases in kdump kernel, those address  might include MMIO
space, HPET, ACPI device table, ERST, and so on... 

>> Obviously, the iommu mmio space is not encrypted, and the device
>> mmio space is outside kdump kernel. We know that the old memory is
>> encrypted, and the old memory is also outside kdump kernel. For the
>> current case, e820__get_entry_type() and walk_iomem_res_desc() can't
>> get the desired result, so we can't also decide whether encryption
>> or not according to this result(rules). If we want to know whether
>> encryption or not by deducing the address, we will need to read the
>> content of memory and have a reference value for comparison, then
>> what's a reference value? Sometimes we don't know that.
> 
> Again, if we don't know that how is the *caller* supposed to know
> whether the memory is encrypted or not? Because
> 
> "we" == "caller"
> 
> in the kdump kernel.
> 
> And the more important question is, why are we dumping MMIO space of the
> previous kernel *at* *all*? That doesn't make any sense to me.
> 
Sorry for my late reply.
Here, it doesn't need to dump MMIO space of the previous kernel, when the
kdump kernel boot, the MMIO address will be remapped in decryption manners,
but the MMIO address don't belong to the range of the crash reserved memory,
for the kdump kernel, the MMIO space(address) and IOMMU device table(address)
are outside address, whereas, the IOMMU device table is encrypted in the first
kernel, the kdump kernel will need to copy the content of IOMMU device table
from the first kernel when the kdump kernel boot, so the IOMMU device table will
be remapped in encryption manners.
So some of them require to be remapped in encryption manners, and some(address)
require to be remapped in decryption manners.

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Dave Young]

On 07/20/18 at 01:06pm, lijiang wrote:
> 在 2018年07月14日 01:08, Borislav Petkov 写道:
> > On Mon, Jul 09, 2018 at 09:55:35PM +0800, lijiang wrote:
> >> About this issue, i want to use an example to describe it.
> >> /* drivers/iommu/amd_iommu_init.c */
> >> static u8 __iomem * __init iommu_map_mmio_space(u64 address, u64 end)
> > 
> > Those addresses come from the IVHD header which is an ACPI table. So the
> > dump kernel can find that out too.
> > Sure. I might understand your means, that will have to find all address out in
> order to cover any cases in kdump kernel, those address  might include MMIO
> space, HPET, ACPI device table, ERST, and so on... 
> 
> >> Obviously, the iommu mmio space is not encrypted, and the device
> >> mmio space is outside kdump kernel. We know that the old memory is
> >> encrypted, and the old memory is also outside kdump kernel. For the
> >> current case, e820__get_entry_type() and walk_iomem_res_desc() can't
> >> get the desired result, so we can't also decide whether encryption
> >> or not according to this result(rules). If we want to know whether
> >> encryption or not by deducing the address, we will need to read the
> >> content of memory and have a reference value for comparison, then
> >> what's a reference value? Sometimes we don't know that.
> > 
> > Again, if we don't know that how is the *caller* supposed to know
> > whether the memory is encrypted or not? Because
> > 
> > "we" == "caller"
> > 
> > in the kdump kernel.
> > 
> > And the more important question is, why are we dumping MMIO space of the
> > previous kernel *at* *all*? That doesn't make any sense to me.
> > 
> Sorry for my late reply.
> Here, it doesn't need to dump MMIO space of the previous kernel, when the
> kdump kernel boot, the MMIO address will be remapped in decryption manners,
> but the MMIO address don't belong to the range of the crash reserved memory,
> for the kdump kernel, the MMIO space(address) and IOMMU device table(address)
> are outside address, whereas, the IOMMU device table is encrypted in the first
> kernel, the kdump kernel will need to copy the content of IOMMU device table
> from the first kernel when the kdump kernel boot, so the IOMMU device table will
> be remapped in encryption manners.
> So some of them require to be remapped in encryption manners, and some(address)
> require to be remapped in decryption manners.
> 

There could be some misunderstanding here.  From the code
copy_device_table in amd_iommu_init.c,  iommu table entry is retrieved
by read mmio address, then use memremap to map the entry address for
copying the device table, so the thing related to your patch is the dev
table entry address not the mmio address.

As for why need copy the old dev table, I think it is for addressing
on-flight DMA issue,  just like the git log of below commit although the
commit is for Intel IOMMU but I think AMD IOMMU solution is similar:

commit 091d42e43d21b6ca7ec39bf5f9e17bc0bd8d4312
Author: Joerg Roedel <jroedel@suse.de>
Date:   Fri Jun 12 11:56:10 2015 +0200

    iommu/vt-d: Copy translation tables from old kernel
    
    If we are in a kdump kernel and find translation enabled in
    the iommu, try to copy the translation tables from the old
    kernel to preserve the mappings until the device driver
    takes over.

Baoquan knows more about this I think he can correct if I'm wrong.

Thanks
Dave

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Borislav Petkov]

On Fri, Jul 20, 2018 at 01:23:04PM +0800, Dave Young wrote:
> > Here, it doesn't need to dump MMIO space of the previous kernel, when the
> > kdump kernel boot, the MMIO address will be remapped in decryption manners,
> > but the MMIO address don't belong to the range of the crash reserved memory,
> > for the kdump kernel, the MMIO space(address) and IOMMU device table(address)
> > are outside address, whereas, the IOMMU device table is encrypted in the first
> > kernel, the kdump kernel will need to copy the content of IOMMU device table
> > from the first kernel when the kdump kernel boot, so the IOMMU device table will
> > be remapped in encryption manners.

-ENOFCKINGPARSE

I believe you're the only one who understands that humongous sentence.
How about using a fullstop from time to time. And WTF is "encryption
manners"?

> > So some of them require to be remapped in encryption manners, and some(address)
> > require to be remapped in decryption manners.

> There could be some misunderstanding here.

Hell yeah there's a misunderstanding!

Can you folks first relax, sit down and explain the whole problem in
*plain* English using *simple* sentences. *Not* like the unparseable
mess above. Use simple, declaratory sentences and don't even try to
sound fancy:

"The first kernel boots. It's memory is encrypted... Now, the second
kernel boots. It must do A because of B. In order to do A, it needs to
do C. Because D..."

And so on. Explain what the problem is first. Then explain the proposed
solution. Explain *why* it needs to be done this way.

When you've written your explanation, try to read it as someone who
doesn't know kdump and *think* hard whether your explanation makes
sense. If it doesn't, fix it and read it again. Rinse and repeat. Until
it is clear to unenlightened readers too.

It is about time this hurried throwing of half-baked patches at
maintainers and seeing what sticks, stops!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月20日 15:32, Borislav Petkov 写道:
> On Fri, Jul 20, 2018 at 01:23:04PM +0800, Dave Young wrote:
>>> Here, it doesn't need to dump MMIO space of the previous kernel, when the
>>> kdump kernel boot, the MMIO address will be remapped in decryption manners,
>>> but the MMIO address don't belong to the range of the crash reserved memory,
>>> for the kdump kernel, the MMIO space(address) and IOMMU device table(address)
>>> are outside address, whereas, the IOMMU device table is encrypted in the first
>>> kernel, the kdump kernel will need to copy the content of IOMMU device table
>>> from the first kernel when the kdump kernel boot, so the IOMMU device table will
>>> be remapped in encryption manners.
> 
> -ENOFCKINGPARSE
> 
> I believe you're the only one who understands that humongous sentence.Sorry for this.

> How about using a fullstop from time to time. And WTF is "encryption
> manners"?
> 
>>> So some of them require to be remapped in encryption manners, and some(address)
>>> require to be remapped in decryption manners.
> 
>> There could be some misunderstanding here.
> 
> Hell yeah there's a misunderstanding!
> 
> Can you folks first relax, sit down and explain the whole problem in
> *plain* English using *simple* sentences. *Not* like the unparseable
> mess above. Use simple, declaratory sentences and don't even try to
> sound fancy:
> 
> "The first kernel boots. It's memory is encrypted... Now, the second
> kernel boots. It must do A because of B. In order to do A, it needs to
> do C. Because D..."
> 
> And so on. Explain what the problem is first. Then explain the proposed
> solution. Explain *why* it needs to be done this way.
> 
> When you've written your explanation, try to read it as someone who
> doesn't know kdump and *think* hard whether your explanation makes
> sense. If it doesn't, fix it and read it again. Rinse and repeat. Until
> it is clear to unenlightened readers too.
> 
Thanks for your advice, I will rewrite the log and send them again.

> It is about time this hurried throwing of half-baked patches at
> maintainers and seeing what sticks, stops!
> 

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Boris Petkov]

On July 20, 2018 12:55:04 PM GMT+03:00, lijiang <lijiang@redhat.com> wrote:>
>Thanks for your advice, I will rewrite the log and send them again.

Do not send them again - explain the problem properly first!

-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[lijiang]

在 2018年07月20日 18:08, Boris Petkov 写道:
> On July 20, 2018 12:55:04 PM GMT+03:00, lijiang <lijiang@redhat.com> wrote:>
>> Thanks for your advice, I will rewrite the log and send them again.
> 
> Do not send them again - explain the problem properly first!
> 
I have compared two solutions to handle the encrypted memory, the solution A is mine, the solution B is Boris's.
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. For solution A and solution B, these four cases are same.
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.

Solution A:
   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()  /    

Solution B(Boris suggested):
   1. no need to add a new function(ioremap_encrypted), check whether the old memory is encrypted by comparing the address.
   2. add new functions to check whether the old memory is encrypted for all cases.
      a. dump vmcore
         bool addr_is_old_memory(unsignd long addr)
      b. crash notes
         bool addr_is_crash_notes(unsigned long addr)
      c. iommu device table
         bool addr_is_iommu_device_table(unsigned long addr)
   3. when remapping the memory, it will call all new functions, and check whether the memory is encrypted in __ioremap_caller().
      __ioremap_caller()->__ioremap_compute_prot()-> /-> addr_is_crash_notes()
                                                     |-> addr_is_iommu_device_table()
                                                     |-> addr_is_old_memory()
                                                     \ ......
   For solution B, i made draft patch for demonstration, just pasted them at bottom.


Conclusion:

  Solution A:
      advantages:
                  1). It's very simple and very clean, less code change;
                  2). It's easier to understand.
      disadvantages:
                  1). Need change the interface of __ioremap_caller() and adjust its wrapper functions;
                  2). Need call ioremap_encrypted() explicitly for vmcore/crash notes/dev table reading.

  Solution B:
      advantages:
                 1). No need to touch interface;
                 2). Automatically detect and do inside __ioremap_caller().
      disadvantages:
                 1). Need add each function for each kind of encrypted old memory reading;
                 2). In the future, need add these kinds of functions too for intel MKTME;
                 3). It's more complicated and more code changes.

I personally prefer solution A, which is presented in posted patches.
What do you think, Boris? And other reviewers?

Thanks,
Lianbo




The solution B will be described by pseudo-code, for example:

   modified file: drivers/iommu/amd_iommu_init.c
   inline bool addr_is_iommu_device_table(unsigned long addr) {
        struct amd_iommu *iommu;

        /* search the addr */
        for_each_iommu(iommu) {
            lo = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET);
            hi = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET + 4);
            entry = (((u64) hi) << 32) + lo;
            /* check whether it is an iommu device table address */
            if (addr == entry) {
                return true;
            }
        }
        return false;
   }

   modified file: fs/proc/vmcore.c
   inline bool addr_is_crash_notes(unsigned long addr) {
       Elf64_Ehdr ehdr;
       
       /* code */

       rc = elfcorehdr_read((char*)&ehdr, sizeof(ELF64_Ehdr), &elfcorehdr_addr);
       elfcorebuf_sz_orig = sizeof(Elf64_Ehdr) + ehdr.e_phnum*sizeof(Elf64_Phdr);
       rc = elfcorehdr_read(elfcorebuf, elfcorebuf_sz_orig, &elfcorehdr_addr);
       ehdr_ptr = (Elf64_Ehdr*)(elfcorebuf + 1);

       /* search the addr */
       for (i = 0; i < ehdr_ptr->e_phnum; i++, phdr_ptr++) {
           offset = phdr_ptr->p_offset;
           /* if found it, break the loop */
           if (offset == addr)
               return true;
       }
       return false;
   }

   modified file: fs/proc/vmcore.c
   inline bool addr_is_old_memory(unsignd long addr) {
      struct vmcore *m;

      /* code */

      /*search the addr*/
      list_for_each_entry(m, &vmcore_list, list) {
          /* code */
          
          sz = min_t(unsigned long long, m->offset + m->size - *pos, size);
          start = m->paddr + *pos - m->offset;
          
          do {
             /* if found it, break the loop */ 
             if (addr == start)
                  return true;

              offset = (unsigned long)(*start % PAGE_SIZE);
              bytes = PAGE_SIZE - offset;
              start += bytes;
              sz -= bytes;

              /* code */

          } while (sz);
      }
      return false;
   }

   modified file: arch/x86/mm/ioremap.c
   static void __ioremap_compute_prot(resource_size_t addr, unsigned long size,
                                   pgprot_t *prot)
   {
        bool encrypted_prot = false;

        /* code */

        if (!is_kdump_kernel())
            return;

        if (addr_is_iommu_device_table(addr))
            encrypted_prot = true;

        if (addr_is_crash_notes(addr))
            encrypted_prot = true;

        if (addr_is_old_memory(addr))
            encrypted_prot = true;

        /* code */
        
        *prot = encrypted_prot ? pgprot_encrypted(*prot)
                               : pgprot_decrypted(*prot);
   }

   modified file: arch/x86/mm/ioremap.c
   static void __iomem *__ioremap_caller(resource_size_t phys_addr,
                unsigned long size, enum page_cache_mode pcm, void *caller)
   {
        /* code */

         prot = PAGE_KERNEL_IO;
         if (sev_active() && mem_flags.desc_other)
                prot = pgprot_encrypted(prot);
       
        /* check whether the memory is encrypted */  
        __ioremap_compute_prot(phys_addr, size, &prot);

        switch (pcm) {
        case _PAGE_CACHE_MODE_UC:
        default:

        /* code */
   }

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

[Borislav Petkov]

On Thu, Aug 16, 2018 at 01:35:28PM +0800, lijiang wrote:
> I personally prefer solution A, which is presented in posted patches.
> What do you think, Boris? And other reviewers?

Ok, thanks for taking the time and effort in explaning this in detail.

Solution B really turns out to be too complex after all. So as long as
that @encrypted argument does not need to propagate any further than a
couple of helpers - as it does now - I guess we can do solution A.

BUT! Wait until next week before sending your patches again because the
merge window isn't over yet.

Thx.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--