[PATCH v9 0/8] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory

[PATCH v9 0/8] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory

[Chao Fan]

***Background:
People reported that kaslr may randomly chooses some positions
which are located in movable memory regions. This will break memory
hotplug feature and make the movable memory chosen by KASLR can't be
removed.

***Solutions:
There should be a method to limit kaslr to choosing immovable memory
regions, so there are 2 solutions:
1) Add a kernel parameter to specify the memory regions.
2) Get the information of memory hot-remove, then kaslr will know the
   right regions.
In method 2, information about memory hot-remove is in ACPI
tables, which will be parsed after start_kernel(), kaslr can't get
the information.
In method 1, users should know the regions address and specify in
kernel parameter.

In the earliest time, I tried to dig ACPI tabls to solve this problem.
But I didn't splite the code in 'compressed/' and ACPI code, so the patch
is hard to follow so refused by community.
Somebody suggest to add a kernel parameter to specify the
immovable memory so that limit kaslr in these regions. Then I make
a new patchset. After several versions, Ingo gave a suggestion:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1634024.html
Follow Ingo's suggestion, imitate the ACPI code to parse the acpi
tables, so that the kaslr can get necessary memory information in
ACPI tables.
I think ACPI code is an independent part, so imitate the codes
and functions to 'compressed/' directory, so that kaslr won't
influence the initialization of ACPI.

PATCH 1/3 Add acpitb.c to provide functions to parse ACPI code.
PATCH 2/3 If CONFIG_MEMORY_HOTREMOVE enabled, walk all nodes and
          store the information of immovable memory regions.
PATCH 3/3 According to the immovable memory regions, filter the
          immovable regions which KASLR can choose.

v1->v2:
 -  Simplify some code.
Follow Baoquan He's suggestion:
 - Reuse the head file of acpi code.

v2->v3:
 - Test in more conditions, so remove the 'RFC' tag.
 - Change some comments.

v3->v4:
Follow Thomas Gleixner's suggetsion:
 - Put the whole efi related function into #define CONFIG_EFI and return
   false in the other stub.
 - Simplify two functions in head file.

v4->v5:
Follow Dou Liyang's suggestion:
 - Add more comments about some functions based on kernel code.
 - Change some typo in comments.
 - Clean useless variable.
 - Add check for the boundary of array.
 - Add check for 'movable_node' parameter

v5->v6:
Follow Baoquan He's suggestion:
 - Change some log.
 - Add the check for acpi_rsdp
 - Change some code logical to make code clear

v6->v7:
Follow Rafael's suggestion:
 - Add more comments and patch log.
Follow test robot's suggestion:
 - Add "static" tag for function

v7-v8:
Follow Kees Cook's suggestion:
 - Use mem_overlaps() to check memory region.
 - Use #ifdef in the definition of function.

v8-v9:
Follow Boris' suggetion:
 - Change code style.
 - Splite PATCH 1/3 to more path.
 - Introduce some new function
 - Use existing function to rework some code
Follow Masayoshi's suggetion:
 - Make code more readable

Any comments will be welcome.


Chao Fan (8):
  x86/boot: Introduce cmdline_find_option_arg()to detect if option=arg
    in cmdline
  x86/boot: Copy kstrtoull() to compressed period
  x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
  x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
  x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  x86/boot: Dig out SRAT table from RSDP and find immovable memory
  x86/boot/KASLR: Walk srat tables to filter immovable memory
  x86/boot/KASLR: Limit kaslr to choosing the immovable memory

 arch/x86/boot/compressed/Makefile  |   4 +
 arch/x86/boot/compressed/acpitb.c  | 354 +++++++++++++++++++++++++++++
 arch/x86/boot/compressed/cmdline.c |  15 ++
 arch/x86/boot/compressed/kaslr.c   |  77 ++++++-
 arch/x86/boot/compressed/misc.c    |  88 +++++++
 arch/x86/boot/compressed/misc.h    |  15 ++
 6 files changed, 542 insertions(+), 11 deletions(-)
 create mode 100644 arch/x86/boot/compressed/acpitb.c

-- 
2.17.2

[PATCH v9 1/8] x86/boot: Introduce cmdline_find_option_arg()to detect if option=arg in cmdline

[Chao Fan]

Introduce a new function cmdline_find_option_arg() to detect whether
option is in command line and the value is arg.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/cmdline.c | 15 +++++++++++++++
 arch/x86/boot/compressed/misc.h    |  1 +
 2 files changed, 16 insertions(+)

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index af6cda0b7900..61118c69feb8 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "misc.h"
+#define STATIC
+#include <linux/decompress/mm.h>
 
 #if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE || CONFIG_X86_5LEVEL
 
@@ -30,5 +32,18 @@ int cmdline_find_option_bool(const char *option)
 {
 	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
 }
+bool cmdline_find_option_arg(const char *option, const char *arg, int argsize)
+{
+	char *buffer = malloc(argsize+1);
+	bool find = false;
+	int ret;
+
+	ret = cmdline_find_option(option, buffer, argsize+1);
+	if (ret == argsize && !strncmp(buffer, arg, argsize))
+		find = true;
+
+	free(buffer);
+	return find;
+}
 
 #endif
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index a1d5918765f3..008fdc47a29c 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -67,6 +67,7 @@ static inline void debug_puthex(const char *s)
 /* cmdline.c */
 int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
+bool cmdline_find_option_arg(const char *option, const char *arg, int argsize);
 #endif
 
 
-- 
2.17.2

[PATCH v9 2/8] x86/boot: Copy kstrtoull() to compressed period

[Chao Fan]

Copy kstrtoull() to 'compressed' directory so that
we can use it to change the address in cmdline from
string to unsigned long long.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/misc.c | 88 +++++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/misc.h |  4 ++
 2 files changed, 92 insertions(+)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 8dd1d5ccae58..5b9b24949337 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -426,3 +426,91 @@ void fortify_panic(const char *name)
 {
 	error("detected buffer overflow");
 }
+
+#define KSTRTOX_OVERFLOW        (1U << 31)
+
+static inline char _tolower(const char c)
+{
+	return c | 0x20;
+}
+
+unsigned int
+_parse_integer(const char *s, unsigned int base, unsigned long long *p)
+{
+	unsigned long long res;
+	unsigned int rv;
+
+	res = 0;
+	rv = 0;
+	while (1) {
+		unsigned int c = *s;
+		unsigned int lc = c | 0x20; /* don't tolower() this line */
+		unsigned int val;
+
+		if ('0' <= c && c <= '9')
+			val = c - '0';
+		else if ('a' <= lc && lc <= 'f')
+			val = lc - 'a' + 10;
+		else
+			break;
+
+		if (val >= base)
+			break;
+		/*
+		 * Check for overflow only if we are within range of
+		 * it in the max base we support (16)
+		 */
+		if (unlikely(res & (~0ull << 60))) {
+			if (res > div_u64(ULLONG_MAX - val, base))
+				rv |= KSTRTOX_OVERFLOW;
+		}
+		res = res * base + val;
+		rv++;
+		s++;
+	}
+	*p = res;
+	return rv;
+}
+
+const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
+{
+	if (*base == 0) {
+		if (s[0] == '0') {
+			if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
+				*base = 16;
+			else
+				*base = 8;
+		} else
+			*base = 10;
+	}
+	if (*base == 16 && s[0] == '0' && _tolower(s[1]) == 'x')
+		s += 2;
+	return s;
+}
+
+static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
+{
+	unsigned long long _res;
+	unsigned int rv;
+
+	s = _parse_integer_fixup_radix(s, &base);
+	rv = _parse_integer(s, base, &_res);
+	if (rv & KSTRTOX_OVERFLOW)
+		return -ERANGE;
+	if (rv == 0)
+		return -EINVAL;
+	s += rv;
+	if (*s == '\n')
+		s++;
+	if (*s)
+		return -EINVAL;
+	*res = _res;
+	return 0;
+}
+
+int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
+{
+	if (s[0] == '+')
+		s++;
+	return _kstrtoull(s, base, res);
+}
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 008fdc47a29c..40378408d980 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -63,6 +63,10 @@ static inline void debug_puthex(const char *s)
 
 #endif
 
+#if (defined CONFIG_RANDOMIZE_BASE) && (defined CONFIG_RANDOMIZE_BASE)
+int kstrtoull(const char *s, unsigned int base, unsigned long long *res);
+#endif
+
 #if CONFIG_EARLY_PRINTK || CONFIG_RANDOMIZE_BASE
 /* cmdline.c */
 int cmdline_find_option(const char *option, char *buffer, int bufsize);
-- 
2.17.2

[PATCH v9 3/8] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table

[Chao Fan]

There is a bug that kaslr may randomly choose some positions
which are located in movable memory regions. This will break memory
hotplug feature and make the movable memory chosen by KASLR can't be
removed. So dig SRAT table from ACPI tables to get memory information.

Imitate the ACPI code of parsing ACPI tables to dig and read ACPI
tables. Since some operations are not needed here, functions are
simplified. Functions will be used to dig only SRAT tables to get
information of memory, so that KASLR can the memory in immovable node.

This function works for EFI. Dig RSDP from EFI tabler, based on efi_init().

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpitb.c | 96 +++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 arch/x86/boot/compressed/acpitb.c

diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
new file mode 100644
index 000000000000..56b54b0e0889
--- /dev/null
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0
+#define BOOT_CTYPE_H
+#include "misc.h"
+#include "error.h"
+
+#include <linux/efi.h>
+#include <asm/efi.h>
+#include <linux/numa.h>
+#include <linux/acpi.h>
+
+/* Search EFI table for RSDP table. */
+static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
+{
+#ifdef CONFIG_EFI
+	efi_system_table_t *systab;
+	bool efi_64 = false;
+	void *config_tables;
+	struct efi_info *e;
+	char *sig;
+	int size;
+	int i;
+
+	e = &boot_params->efi_info;
+	sig = (char *)&e->efi_loader_signature;
+
+	if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4))
+		efi_64 = true;
+	else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4))
+		efi_64 = false;
+	else {
+		debug_putstr("Wrong EFI loader signature.\n");
+		return;
+	}
+
+	/* Get systab from boot params. Based on efi_init(). */
+#ifdef CONFIG_X86_64
+	systab = (efi_system_table_t *)(
+			e->efi_systab | ((__u64)e->efi_systab_hi<<32));
+#else
+	if (e->efi_systab_hi || e->efi_memmap_hi) {
+		debug_putstr("Table located above 4GB. EFI should be disabled.\n");
+		return;
+	}
+	systab = (efi_system_table_t *)e->efi_systab;
+#endif
+
+	if (!systab)
+		return;
+
+	/*
+	 * Get EFI tables from systab. Based on efi_config_init() and
+	 * efi_config_parse_tables(). Only dig out the config_table.
+	 */
+	size = efi_64 ? sizeof(efi_config_table_64_t) :
+			sizeof(efi_config_table_32_t);
+
+	for (i = 0; i < systab->nr_tables; i++) {
+		efi_guid_t guid;
+		unsigned long table;
+
+		config_tables = (void *)(systab->tables + size * i);
+		if (efi_64) {
+			efi_config_table_64_t *tmp_table;
+
+			tmp_table = (efi_config_table_64_t *)config_tables;
+			guid = tmp_table->guid;
+			table = tmp_table->table;
+#ifndef CONFIG_64BIT
+			if (table >> 32) {
+				debug_putstr("Table located above 4G. EFI should be disabled.\n");
+				return;
+			}
+#endif
+		} else {
+			efi_config_table_32_t *tmp_table;
+
+			tmp_table = (efi_config_table_32_t *)config_tables;
+			guid = tmp_table->guid;
+			table = tmp_table->table;
+		}
+
+		/*
+		 * Get RSDP from EFI tables.
+		 * If ACPI20 table found, use it.
+		 * If ACPI20 table not found, but ACPI table found,
+		 * use the ACPI table.
+		 */
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID))) {
+			*rsdp_addr = (acpi_physical_address)table;
+		} else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID))) {
+			*rsdp_addr = (acpi_physical_address)table;
+			return;
+		}
+	}
+#endif
+}
-- 
2.17.2

[PATCH v9 4/8] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory

[Chao Fan]

Imitate acpi_find_root_pointer() and acpi_tb_scan_memory_for_rsdp()
to search RSDP table pointer in memory. This function only works
when RSDP not found in EFI table.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpitb.c | 106 ++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
index 56b54b0e0889..50fa65cf824d 100644
--- a/arch/x86/boot/compressed/acpitb.c
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -94,3 +94,109 @@ static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 	}
 #endif
 }
+
+static u8 compute_checksum(u8 *buffer, u32 length)
+{
+	u8 sum = 0;
+	u8 *end = buffer + length;
+
+	while (buffer < end)
+		sum = (u8)(sum + *(buffer++));
+
+	return sum;
+}
+
+/*
+ * Used to search a block of memory for the RSDP signature.
+ * Return Pointer to the RSDP if found, otherwise NULL.
+ * Based on acpi_tb_scan_memory_for_rsdp().
+ */
+static u8 *scan_mem_for_rsdp(u8 *start, u32 length)
+{
+	struct acpi_table_rsdp *rsdp;
+	u8 *end;
+	u8 *rover;
+
+	end = start + length;
+
+	/* Search from given start address for the requested length */
+	for (rover = start; rover < end; rover += ACPI_RSDP_SCAN_STEP) {
+		/*
+		 * The RSDP signature and checksum must both be correct
+		 * Note: Sometimes there exists more than one RSDP in memory;
+		 * the valid RSDP has a valid checksum, all others have an
+		 * invalid checksum.
+		 */
+		rsdp = (struct acpi_table_rsdp *)rover;
+
+		/* Nope, BAD Signature */
+		if (!ACPI_VALIDATE_RSDP_SIG(rsdp->signature))
+			continue;
+
+		/* Check the standard checksum */
+		if (compute_checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH))
+			continue;
+
+		/* Check extended checksum if table version >= 2 */
+		if ((rsdp->revision >= 2) &&
+		    (compute_checksum((u8 *) rsdp, ACPI_RSDP_XCHECKSUM_LENGTH)))
+			continue;
+
+		/* Sig and checksum valid, we have found a real RSDP */
+		return rover;
+	}
+	return NULL;
+}
+
+/*
+ * Used to search RSDP physical address.
+ * Based on acpi_find_root_pointer(). Since only use physical address
+ * in this period, so there is no need to do the memory map jobs.
+ */
+static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
+{
+	struct acpi_table_rsdp *rsdp;
+	u8 *table_ptr;
+	u8 *mem_rover;
+	u32 address;
+
+	/*
+	 * Get the location of the Extended BIOS Data Area (EBDA)
+	 * Since we use physical address directely, so
+	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
+	 * not needed here.
+	 */
+	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
+	*(u32 *)(void *)&address = *(u16 *)(void *)table_ptr;
+	address <<= 4;
+	table_ptr = (u8 *)address;
+
+	/*
+	 * Search EBDA paragraphs (EBDA is required to be a minimum of
+	 * 1K length)
+	 */
+	if (address > 0x400) {
+		mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE);
+
+		if (mem_rover) {
+			address += (u32)ACPI_PTR_DIFF(mem_rover, table_ptr);
+			*rsdp_addr = (acpi_physical_address)address;
+			return;
+		}
+	}
+
+	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
+	mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
+
+	/*
+	 * Search upper memory: 16-byte boundaries in E0000h-FFFFFh
+	 * Since we use physical address directely, so
+	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
+	 * not needed here.
+	 */
+	if (mem_rover) {
+		address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
+				ACPI_PTR_DIFF(mem_rover, table_ptr));
+		*rsdp_addr = (acpi_physical_address)address;
+	}
+}
-- 
2.17.2

[PATCH v9 5/8] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec

[Chao Fan]

If KEXEC write the RSDP pointer to cmdline, parse the cmdline
and use it.
Imitate from early_param of "acpi_rsdp".

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpitb.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
index 50fa65cf824d..37b1f4407be8 100644
--- a/arch/x86/boot/compressed/acpitb.c
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -8,6 +8,9 @@
 #include <linux/numa.h>
 #include <linux/acpi.h>
 
+#define STATIC
+#include <linux/decompress/mm.h>
+
 /* Search EFI table for RSDP table. */
 static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 {
@@ -200,3 +203,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 		*rsdp_addr = (acpi_physical_address)address;
 	}
 }
+
+static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
+{
+#ifdef CONFIG_KEXEC
+	unsigned long long res;
+	int len = 0;
+	char *val;
+
+	val = malloc(20);
+	len = cmdline_find_option("acpi_rsdp", val, 20);
+
+	if (len == -1)
+		return;
+
+	if (len > 0) {
+		val[len] = 0;
+		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 0, &res);
+	}
+#endif
+}
-- 
2.17.2

[PATCH v9 6/8] x86/boot: Dig out SRAT table from RSDP and find immovable memory

[Chao Fan]

Dig out SRAT table from RSDP, and then walk all memory to find
the immovable memory regions, and fill in the immovable_mem[].
So that we can use it to select memory for KASLR.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/Makefile |   4 +
 arch/x86/boot/compressed/acpitb.c | 129 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/misc.h   |  10 +++
 3 files changed, 143 insertions(+)

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 28764dacf018..f67674d7d2bd 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -83,6 +83,10 @@ ifdef CONFIG_X86_64
 	vmlinux-objs-y += $(obj)/pgtable_64.o
 endif
 
+#ifdef CONFIG_MEMORY_HOTREMOVE && CONFIG_RANDOMIZE_BASE
+vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
+#endif
+
 $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
 
 vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
index 37b1f4407be8..d119663c05bb 100644
--- a/arch/x86/boot/compressed/acpitb.c
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -11,6 +11,15 @@
 #define STATIC
 #include <linux/decompress/mm.h>
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+struct mem_vector {
+	unsigned long long start;
+	unsigned long long size;
+};
+/* Store the immovable memory regions */
+struct mem_vector immovable_mem[MAX_NUMNODES*2];
+#endif
+
 /* Search EFI table for RSDP table. */
 static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 {
@@ -223,3 +232,123 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
 	}
 #endif
 }
+
+/*
+ * Used to dig RSDP table from EFI table or BIOS.
+ * If RSDP table found in EFI table, use it. Or search BIOS.
+ * Based on acpi_os_get_root_pointer().
+ */
+static acpi_physical_address get_rsdp_addr(void)
+{
+	acpi_physical_address pa = 0;
+
+	get_acpi_rsdp(&pa);
+
+	if (!pa)
+		efi_get_rsdp_addr(&pa);
+
+	if (!pa)
+		bios_get_rsdp_addr(&pa);
+
+	return pa;
+}
+
+static struct acpi_table_header *get_acpi_srat_table(void)
+{
+	acpi_physical_address acpi_table;
+	acpi_physical_address root_table;
+	struct acpi_table_header *header;
+	struct acpi_table_rsdp *rsdp;
+	char *signature;
+	u8 *entry;
+	u32 count;
+	u32 size;
+	int i, j;
+	u32 len;
+
+	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
+	if (!rsdp)
+		return NULL;
+
+	/* Get RSDT or XSDT from RSDP. */
+	if (!cmdline_find_option_arg("acpi", "rsdt", 4) &&
+	    rsdp->xsdt_physical_address && rsdp->revision > 1) {
+		root_table = rsdp->xsdt_physical_address;
+		size = ACPI_XSDT_ENTRY_SIZE;
+	} else {
+		root_table = rsdp->rsdt_physical_address;
+		size = ACPI_RSDT_ENTRY_SIZE;
+	}
+
+	/* Get ACPI root table from RSDT or XSDT.*/
+	header = (struct acpi_table_header *)root_table;
+	len = header->length;
+	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
+	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
+
+	for (i = 0; i < count; i++) {
+		u64 address64;
+
+		if (size == ACPI_RSDT_ENTRY_SIZE)
+			acpi_table = ((acpi_physical_address)
+				      (*ACPI_CAST_PTR(u32, entry)));
+		else {
+			*(u64 *)(void *)&address64 = *(u64 *)(void *)entry;
+			acpi_table = (acpi_physical_address) address64;
+		}
+
+		if (acpi_table) {
+			header = (struct acpi_table_header *)acpi_table;
+			signature = header->signature;
+
+			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
+				return header;
+		}
+		entry += size;
+	}
+	return NULL;
+}
+
+/*
+ * According to ACPI table, filter the immvoable memory regions
+ * and store them in immovable_mem[].
+ */
+void get_immovable_mem(void)
+{
+	struct acpi_table_header *table_header;
+	struct acpi_subtable_header *table;
+	struct acpi_srat_mem_affinity *ma;
+	unsigned long table_end;
+	int i = 0;
+
+	if (!cmdline_find_option_bool("movable_node") ||
+	    cmdline_find_option_arg("acpi", "off", 3))
+		return;
+
+	table_header = get_acpi_srat_table();
+	if (!table_header)
+		return;
+
+	table_end = (unsigned long)table_header + table_header->length;
+
+	table = (struct acpi_subtable_header *)
+		((unsigned long)table_header + sizeof(struct acpi_table_srat));
+
+	while (((unsigned long)table) +
+		       sizeof(struct acpi_subtable_header) < table_end) {
+		if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
+			ma = (struct acpi_srat_mem_affinity *)table;
+			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
+				immovable_mem[i].start = ma->base_address;
+				immovable_mem[i].size = ma->length;
+				i++;
+			}
+
+			if (i >= MAX_NUMNODES*2)
+				break;
+		}
+		table = (struct acpi_subtable_header *)
+			((unsigned long)table + table->length);
+	}
+	num_immovable_mem = i;
+}
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 40378408d980..70c403e1444c 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -121,3 +121,13 @@ static inline void console_init(void)
 void set_sev_encryption_mask(void);
 
 #endif
+
+/* acpitb.c */
+#ifdef CONFIG_RANDOMIZE_BASE
+int num_immovable_mem;
+#ifdef CONFIG_MEMORY_HOTREMOVE
+/* Store the amount of immovable memory regions */
+#define ACPI_MAX_TABLES                128
+void get_immovable_mem(void);
+#endif
+#endif
-- 
2.17.2

[PATCH v9 7/8] x86/boot/KASLR: Walk srat tables to filter immovable memory

[Chao Fan]

If CONFIG_MEMORY_HOTREMOVE enabled, walk through the acpi srat memory
tables and store those immovable memory regions so that kaslr can get
where to choose for randomization.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 9ed9709d9947..0c3567bc231c 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -417,6 +417,11 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* Mark the memmap regions we need to avoid */
 	handle_mem_options();
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	/* Mark the immovable regions we need to choose */
+	get_immovable_mem();
+#endif
+
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 	/* Make sure video RAM can be used. */
 	add_identity_map(0, PMD_SIZE);
-- 
2.17.2

[PATCH v9 8/8] x86/boot/KASLR: Limit kaslr to choosing the immovable memory

[Chao Fan]

If CONFIG_MEMORY_HOTREMOVE enabled and the amount of immovable
memory regions is not zero. Calculate the intersection between memory
regions from e820/efi memory table and immovable memory regions.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 72 +++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 11 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 0c3567bc231c..3ebb150f61eb 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -101,6 +101,11 @@ static bool memmap_too_large;
 /* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
 static unsigned long long mem_limit = ULLONG_MAX;
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+/* Store the immovable memory regions */
+extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
+#endif
+
 
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
@@ -577,9 +582,9 @@ static unsigned long slots_fetch_random(void)
 	return 0;
 }
 
-static void process_mem_region(struct mem_vector *entry,
-			       unsigned long minimum,
-			       unsigned long image_size)
+static void slots_count(struct mem_vector *entry,
+			unsigned long minimum,
+			unsigned long image_size)
 {
 	struct mem_vector region, overlap;
 	unsigned long start_orig, end;
@@ -655,6 +660,57 @@ static void process_mem_region(struct mem_vector *entry,
 	}
 }
 
+static bool process_mem_region(struct mem_vector *region,
+			       unsigned long long minimum,
+			       unsigned long long image_size)
+{
+	int i;
+	/*
+	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
+	 * walk all the regions, so use region directely.
+	 */
+	if (num_immovable_mem == 0) {
+		slots_count(region, minimum, image_size);
+
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
+			return 1;
+		}
+		return 0;
+	}
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	/*
+	 * If immovable memory found, filter the intersection between
+	 * immovable memory and region to slots_count.
+	 * Otherwise, go on old code.
+	 */
+	for (i = 0; i < num_immovable_mem; i++) {
+		struct mem_vector entry;
+		unsigned long long start, end, entry_end, region_end;
+
+		if (!mem_overlaps(region, &immovable_mem[i]))
+			continue;
+
+		start = immovable_mem[i].start;
+		end = start + immovable_mem[i].size;
+		region_end = region->start + region->size;
+
+		entry.start = clamp(region->start, start, end);
+		entry_end = clamp(region_end, start, end);
+		entry.size = entry_end - entry.start;
+
+		slots_count(&entry, minimum, image_size);
+
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
+			return 1;
+		}
+	}
+	return 0;
+#endif
+}
+
 #ifdef CONFIG_EFI
 /*
  * Returns true if mirror region found (and must have been processed
@@ -720,11 +776,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 
 		region.start = md->phys_addr;
 		region.size = md->num_pages << EFI_PAGE_SHIFT;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+		if (process_mem_region(&region, minimum, image_size))
 			break;
-		}
 	}
 	return true;
 }
@@ -751,11 +804,8 @@ static void process_e820_entries(unsigned long minimum,
 			continue;
 		region.start = entry->addr;
 		region.size = entry->size;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+		if (process_mem_region(&region, minimum, image_size))
 			break;
-		}
 	}
 }
 
-- 
2.17.2

Re: [PATCH v9 0/8] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> In the earliest time, I tried to dig ACPI tabls to solve this problem.
> But I didn't splite the code in 'compressed/' and ACPI code, so the patch
> is hard to follow so refused by community.
> Somebody suggest to add a kernel parameter to specify the
> immovable memory so that limit kaslr in these regions. Then I make
> a new patchset. After several versions, Ingo gave a suggestion:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1634024.html
> Follow Ingo's suggestion, imitate the ACPI code to parse the acpi
> tables, so that the kaslr can get necessary memory information in
> ACPI tables.
> I think ACPI code is an independent part, so imitate the codes
> and functions to 'compressed/' directory, so that kaslr won't
> influence the initialization of ACPI.
> 
> PATCH 1/3 Add acpitb.c to provide functions to parse ACPI code.
> PATCH 2/3 If CONFIG_MEMORY_HOTREMOVE enabled, walk all nodes and
>           store the information of immovable memory regions.
> PATCH 3/3 According to the immovable memory regions, filter the
>           immovable regions which KASLR can choose.

Chao, seems you didn't update above accordingly.

> 
> v1->v2:
>  -  Simplify some code.
> Follow Baoquan He's suggestion:
>  - Reuse the head file of acpi code.
> 
> v2->v3:
>  - Test in more conditions, so remove the 'RFC' tag.
>  - Change some comments.
> 
> v3->v4:
> Follow Thomas Gleixner's suggetsion:
>  - Put the whole efi related function into #define CONFIG_EFI and return
>    false in the other stub.
>  - Simplify two functions in head file.
> 
> v4->v5:
> Follow Dou Liyang's suggestion:
>  - Add more comments about some functions based on kernel code.
>  - Change some typo in comments.
>  - Clean useless variable.
>  - Add check for the boundary of array.
>  - Add check for 'movable_node' parameter
> 
> v5->v6:
> Follow Baoquan He's suggestion:
>  - Change some log.
>  - Add the check for acpi_rsdp
>  - Change some code logical to make code clear
> 
> v6->v7:
> Follow Rafael's suggestion:
>  - Add more comments and patch log.
> Follow test robot's suggestion:
>  - Add "static" tag for function
> 
> v7-v8:
> Follow Kees Cook's suggestion:
>  - Use mem_overlaps() to check memory region.
>  - Use #ifdef in the definition of function.
> 
> v8-v9:
> Follow Boris' suggetion:
>  - Change code style.
>  - Splite PATCH 1/3 to more path.
>  - Introduce some new function
>  - Use existing function to rework some code
> Follow Masayoshi's suggetion:
>  - Make code more readable
> 
> Any comments will be welcome.
> 
> 
> Chao Fan (8):
>   x86/boot: Introduce cmdline_find_option_arg()to detect if option=arg
>     in cmdline
>   x86/boot: Copy kstrtoull() to compressed period
>   x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
>   x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
>   x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
>   x86/boot: Dig out SRAT table from RSDP and find immovable memory
>   x86/boot/KASLR: Walk srat tables to filter immovable memory
>   x86/boot/KASLR: Limit kaslr to choosing the immovable memory
> 
>  arch/x86/boot/compressed/Makefile  |   4 +
>  arch/x86/boot/compressed/acpitb.c  | 354 +++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/cmdline.c |  15 ++
>  arch/x86/boot/compressed/kaslr.c   |  77 ++++++-
>  arch/x86/boot/compressed/misc.c    |  88 +++++++
>  arch/x86/boot/compressed/misc.h    |  15 ++
>  6 files changed, 542 insertions(+), 11 deletions(-)
>  create mode 100644 arch/x86/boot/compressed/acpitb.c
> 
> -- 
> 2.17.2
> 
> 
> 

Re: [PATCH v9 1/8] x86/boot: Introduce cmdline_find_option_arg()to detect if option=arg in cmdline

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> +bool cmdline_find_option_arg(const char *option, const char *arg, int argsize)
> +{
> +	char *buffer = malloc(argsize+1);
> +	bool find = false;
> +	int ret;
> +
> +	ret = cmdline_find_option(option, buffer, argsize+1);
> +	if (ret == argsize && !strncmp(buffer, arg, argsize))
> +		find = true;

Wondering if we really need a wrapper like this. 

> +
> +	free(buffer);
> +	return find;
> +}
>  
>  #endif
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index a1d5918765f3..008fdc47a29c 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -67,6 +67,7 @@ static inline void debug_puthex(const char *s)
>  /* cmdline.c */
>  int cmdline_find_option(const char *option, char *buffer, int bufsize);
>  int cmdline_find_option_bool(const char *option);
> +bool cmdline_find_option_arg(const char *option, const char *arg, int argsize);
>  #endif
>  
>  
> -- 
> 2.17.2
> 
> 
> 

Re: [PATCH v9 2/8] x86/boot: Copy kstrtoull() to compressed period

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> Copy kstrtoull() to 'compressed' directory so that
> we can use it to change the address in cmdline from
> string to unsigned long long.

So you don't like simple_strtoull() in arch/x86/boot/string.c which has
been used in boot/compressed/kaslr.c . Why? Are you going to clean up
simple_strtoull()?

Re: [PATCH v9 8/8] x86/boot/KASLR: Limit kaslr to choosing the immovable memory

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> If CONFIG_MEMORY_HOTREMOVE enabled and the amount of immovable
> memory regions is not zero. Calculate the intersection between memory

This if conditional adverbial clauses is not an complete sentence.

> regions from e820/efi memory table and immovable memory regions.
					^ get?
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 72 +++++++++++++++++++++++++++-----
>  1 file changed, 61 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 0c3567bc231c..3ebb150f61eb 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -101,6 +101,11 @@ static bool memmap_too_large;
>  /* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
>  static unsigned long long mem_limit = ULLONG_MAX;
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the immovable memory regions */
> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];

Sorry, Chao. I may not follow your old patch change, why the length of
immovable_mem is MAX_NUMNODES*2, is there any reason or basis?

> +#endif
> +
>  
>  enum mem_avoid_index {
>  	MEM_AVOID_ZO_RANGE = 0,
> @@ -577,9 +582,9 @@ static unsigned long slots_fetch_random(void)
>  	return 0;
>  }
>  
> -static void process_mem_region(struct mem_vector *entry,
> -			       unsigned long minimum,
> -			       unsigned long image_size)
> +static void slots_count(struct mem_vector *entry,
> +			unsigned long minimum,
> +			unsigned long image_size)
>  {
>  	struct mem_vector region, overlap;
>  	unsigned long start_orig, end;
> @@ -655,6 +660,57 @@ static void process_mem_region(struct mem_vector *entry,
>  	}
>  }
>  
> +static bool process_mem_region(struct mem_vector *region,
> +			       unsigned long long minimum,
> +			       unsigned long long image_size)
> +{
> +	int i;
> +	/*
> +	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
> +	 * walk all the regions, so use region directely.
> +	 */
> +	if (num_immovable_mem == 0) {
> +		slots_count(region, minimum, image_size);
> +
> +		if (slot_area_index == MAX_SLOT_AREA) {
> +			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
> +			return 1;
> +		}
> +		return 0;
> +	}
> +
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +	/*
> +	 * If immovable memory found, filter the intersection between
> +	 * immovable memory and region to slots_count.
> +	 * Otherwise, go on old code.

Could you explain more about what is the old code in otherwise case you
want to go on?

> +	 */
> +	for (i = 0; i < num_immovable_mem; i++) {
> +		struct mem_vector entry;
> +		unsigned long long start, end, entry_end, region_end;
> +
> +		if (!mem_overlaps(region, &immovable_mem[i]))
> +			continue;
> +
> +		start = immovable_mem[i].start;
> +		end = start + immovable_mem[i].size;
> +		region_end = region->start + region->size;
> +
> +		entry.start = clamp(region->start, start, end);
> +		entry_end = clamp(region_end, start, end);
> +		entry.size = entry_end - entry.start;
> +
> +		slots_count(&entry, minimum, image_size);

Obviously, your patch log only covers this place of code. About renaming
process_mem_region() to  slot_count(), and add another level of wrapper 
process_mem_region(), may also need be mentioned in patch log.

> +
> +		if (slot_area_index == MAX_SLOT_AREA) {
> +			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
> +			return 1;
> +		}
> +	}
> +	return 0;
> +#endif
> +}
> +
>  #ifdef CONFIG_EFI
>  /*
>   * Returns true if mirror region found (and must have been processed
> @@ -720,11 +776,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>  
>  		region.start = md->phys_addr;
>  		region.size = md->num_pages << EFI_PAGE_SHIFT;
> -		process_mem_region(&region, minimum, image_size);
> -		if (slot_area_index == MAX_SLOT_AREA) {
> -			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
> +		if (process_mem_region(&region, minimum, image_size))
>  			break;
> -		}
>  	}
>  	return true;
>  }
> @@ -751,11 +804,8 @@ static void process_e820_entries(unsigned long minimum,
>  			continue;
>  		region.start = entry->addr;
>  		region.size = entry->size;
> -		process_mem_region(&region, minimum, image_size);
> -		if (slot_area_index == MAX_SLOT_AREA) {
> -			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
> +		if (process_mem_region(&region, minimum, image_size))
>  			break;
> -		}
>  	}
>  }
>  
> -- 
> 2.17.2
> 
> 
> 

Re: [PATCH v9 7/8] x86/boot/KASLR: Walk srat tables to filter immovable memory

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> If CONFIG_MEMORY_HOTREMOVE enabled, walk through the acpi srat memory
> tables and store those immovable memory regions so that kaslr can get
> where to choose for randomization.

This patch only adds invocation of get_immovable_mem() inside
mem_avoid_init(). It's not doing what you said in patch log. Can it be
merged with other patch, e.g patch 6 or 8?

> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 9ed9709d9947..0c3567bc231c 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -417,6 +417,11 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  	/* Mark the memmap regions we need to avoid */
>  	handle_mem_options();
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +	/* Mark the immovable regions we need to choose */
> +	get_immovable_mem();
> +#endif
> +
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>  	/* Make sure video RAM can be used. */
>  	add_identity_map(0, PMD_SIZE);
> -- 
> 2.17.2
> 
> 
> 

Re: [PATCH v9 3/8] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> There is a bug that kaslr may randomly choose some positions
> which are located in movable memory regions. This will break memory
> hotplug feature and make the movable memory chosen by KASLR can't be
> removed. So dig SRAT table from ACPI tables to get memory information.

This patch is only adding a function efi_get_rsdp_addr() which will be
used in later patch. Do we need to describe bug here?

Can we focus on what is this function, and why it's written like that,
and why it's here?
> 
> Imitate the ACPI code of parsing ACPI tables to dig and read ACPI
> tables. Since some operations are not needed here, functions are
> simplified. Functions will be used to dig only SRAT tables to get
> information of memory, so that KASLR can the memory in immovable node.
> 

Re: [PATCH v9 0/8] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory

[Chao Fan]

On Thu, Oct 18, 2018 at 11:59:58AM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> In the earliest time, I tried to dig ACPI tabls to solve this problem.
>> But I didn't splite the code in 'compressed/' and ACPI code, so the patch
>> is hard to follow so refused by community.
>> Somebody suggest to add a kernel parameter to specify the
>> immovable memory so that limit kaslr in these regions. Then I make
>> a new patchset. After several versions, Ingo gave a suggestion:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1634024.html
>> Follow Ingo's suggestion, imitate the ACPI code to parse the acpi
>> tables, so that the kaslr can get necessary memory information in
>> ACPI tables.
>> I think ACPI code is an independent part, so imitate the codes
>> and functions to 'compressed/' directory, so that kaslr won't
>> influence the initialization of ACPI.
>> 
>> PATCH 1/3 Add acpitb.c to provide functions to parse ACPI code.
>> PATCH 2/3 If CONFIG_MEMORY_HOTREMOVE enabled, walk all nodes and
>>           store the information of immovable memory regions.
>> PATCH 3/3 According to the immovable memory regions, filter the
>>           immovable regions which KASLR can choose.
>
>Chao, seems you didn't update above accordingly.

oops, sorry for that.

Thanks,
Chao Fan

>
>> 
>> v1->v2:
>>  -  Simplify some code.
>> Follow Baoquan He's suggestion:
>>  - Reuse the head file of acpi code.
>> 
>> v2->v3:
>>  - Test in more conditions, so remove the 'RFC' tag.
>>  - Change some comments.
>> 
>> v3->v4:
>> Follow Thomas Gleixner's suggetsion:
>>  - Put the whole efi related function into #define CONFIG_EFI and return
>>    false in the other stub.
>>  - Simplify two functions in head file.
>> 
>> v4->v5:
>> Follow Dou Liyang's suggestion:
>>  - Add more comments about some functions based on kernel code.
>>  - Change some typo in comments.
>>  - Clean useless variable.
>>  - Add check for the boundary of array.
>>  - Add check for 'movable_node' parameter
>> 
>> v5->v6:
>> Follow Baoquan He's suggestion:
>>  - Change some log.
>>  - Add the check for acpi_rsdp
>>  - Change some code logical to make code clear
>> 
>> v6->v7:
>> Follow Rafael's suggestion:
>>  - Add more comments and patch log.
>> Follow test robot's suggestion:
>>  - Add "static" tag for function
>> 
>> v7-v8:
>> Follow Kees Cook's suggestion:
>>  - Use mem_overlaps() to check memory region.
>>  - Use #ifdef in the definition of function.
>> 
>> v8-v9:
>> Follow Boris' suggetion:
>>  - Change code style.
>>  - Splite PATCH 1/3 to more path.
>>  - Introduce some new function
>>  - Use existing function to rework some code
>> Follow Masayoshi's suggetion:
>>  - Make code more readable
>> 
>> Any comments will be welcome.
>> 
>> 
>> Chao Fan (8):
>>   x86/boot: Introduce cmdline_find_option_arg()to detect if option=arg
>>     in cmdline
>>   x86/boot: Copy kstrtoull() to compressed period
>>   x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
>>   x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
>>   x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
>>   x86/boot: Dig out SRAT table from RSDP and find immovable memory
>>   x86/boot/KASLR: Walk srat tables to filter immovable memory
>>   x86/boot/KASLR: Limit kaslr to choosing the immovable memory
>> 
>>  arch/x86/boot/compressed/Makefile  |   4 +
>>  arch/x86/boot/compressed/acpitb.c  | 354 +++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/cmdline.c |  15 ++
>>  arch/x86/boot/compressed/kaslr.c   |  77 ++++++-
>>  arch/x86/boot/compressed/misc.c    |  88 +++++++
>>  arch/x86/boot/compressed/misc.h    |  15 ++
>>  6 files changed, 542 insertions(+), 11 deletions(-)
>>  create mode 100644 arch/x86/boot/compressed/acpitb.c
>> 
>> -- 
>> 2.17.2
>> 
>> 
>> 
>
>

Re: [PATCH v9 2/8] x86/boot: Copy kstrtoull() to compressed period

[Chao Fan]

On Thu, Oct 18, 2018 at 12:03:38PM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> Copy kstrtoull() to 'compressed' directory so that
>> we can use it to change the address in cmdline from
>> string to unsigned long long.
>
>So you don't like simple_strtoull() in arch/x86/boot/string.c which has
>been used in boot/compressed/kaslr.c . Why? Are you going to clean up
>simple_strtoull()?

Boris told me use the new function in old version's review.
Both work well for me.

Thanks,
Chao Fan

>
>

Re: [PATCH v9 3/8] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table

[Chao Fan]

On Thu, Oct 18, 2018 at 12:35:39PM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> There is a bug that kaslr may randomly choose some positions
>> which are located in movable memory regions. This will break memory
>> hotplug feature and make the movable memory chosen by KASLR can't be
>> removed. So dig SRAT table from ACPI tables to get memory information.
>
>This patch is only adding a function efi_get_rsdp_addr() which will be
>used in later patch. Do we need to describe bug here?

Yes, it's the first mail of this serial, so I add more log.
>
>Can we focus on what is this function, and why it's written like that,
>and why it's here?

Sure.
I am just afraid lack of log.

Thanks,
Chao Fan

>> 
>> Imitate the ACPI code of parsing ACPI tables to dig and read ACPI
>> tables. Since some operations are not needed here, functions are
>> simplified. Functions will be used to dig only SRAT tables to get
>> information of memory, so that KASLR can the memory in immovable node.
>> 
>
>

Re: [PATCH v9 7/8] x86/boot/KASLR: Walk srat tables to filter immovable memory

[Chao Fan]

On Thu, Oct 18, 2018 at 12:23:20PM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> If CONFIG_MEMORY_HOTREMOVE enabled, walk through the acpi srat memory
>> tables and store those immovable memory regions so that kaslr can get
>> where to choose for randomization.
>
>This patch only adds invocation of get_immovable_mem() inside
>mem_avoid_init(). It's not doing what you said in patch log. Can it be
>merged with other patch, e.g patch 6 or 8?

Yes, in old version, this patch will do more job.
Later I move more functions to acpitb.c, just this left.
So I can move it to 8/8.

Thanks,
Chao Fan

>
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 9ed9709d9947..0c3567bc231c 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -417,6 +417,11 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>  	/* Mark the memmap regions we need to avoid */
>>  	handle_mem_options();
>>  
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +	/* Mark the immovable regions we need to choose */
>> +	get_immovable_mem();
>> +#endif
>> +
>>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>>  	/* Make sure video RAM can be used. */
>>  	add_identity_map(0, PMD_SIZE);
>> -- 
>> 2.17.2
>> 
>> 
>> 
>
>

Re: [PATCH v9 3/8] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table

[Baoquan He]

On 10/18/18 at 01:54pm, Chao Fan wrote:
> On Thu, Oct 18, 2018 at 12:35:39PM +0800, Baoquan He wrote:
> >On 10/17/18 at 06:20pm, Chao Fan wrote:
> >> There is a bug that kaslr may randomly choose some positions
> >> which are located in movable memory regions. This will break memory
> >> hotplug feature and make the movable memory chosen by KASLR can't be
> >> removed. So dig SRAT table from ACPI tables to get memory information.
> >
> >This patch is only adding a function efi_get_rsdp_addr() which will be
> >used in later patch. Do we need to describe bug here?
> 
> Yes, it's the first mail of this serial, so I add more log.

patch 3/8?

Re: [PATCH v9 2/8] x86/boot: Copy kstrtoull() to compressed period

[Baoquan He]

On 10/18/18 at 01:51pm, Chao Fan wrote:
> On Thu, Oct 18, 2018 at 12:03:38PM +0800, Baoquan He wrote:
> >On 10/17/18 at 06:20pm, Chao Fan wrote:
> >> Copy kstrtoull() to 'compressed' directory so that
> >> we can use it to change the address in cmdline from
> >> string to unsigned long long.
> >
> >So you don't like simple_strtoull() in arch/x86/boot/string.c which has
> >been used in boot/compressed/kaslr.c . Why? Are you going to clean up
> >simple_strtoull()?
> 
> Boris told me use the new function in old version's review.
> Both work well for me.

OK, then maybe you can clear simple_strtoull() out in a separate patch
since you have introduced a new one. Or after this patchset.

Re: [PATCH v9 2/8] x86/boot: Copy kstrtoull() to compressed period

[Chao Fan]

On Thu, Oct 18, 2018 at 02:01:28PM +0800, Baoquan He wrote:
>On 10/18/18 at 01:51pm, Chao Fan wrote:
>> On Thu, Oct 18, 2018 at 12:03:38PM +0800, Baoquan He wrote:
>> >On 10/17/18 at 06:20pm, Chao Fan wrote:
>> >> Copy kstrtoull() to 'compressed' directory so that
>> >> we can use it to change the address in cmdline from
>> >> string to unsigned long long.
>> >
>> >So you don't like simple_strtoull() in arch/x86/boot/string.c which has
>> >been used in boot/compressed/kaslr.c . Why? Are you going to clean up
>> >simple_strtoull()?
>> 
>> Boris told me use the new function in old version's review.
>> Both work well for me.
>
>OK, then maybe you can clear simple_strtoull() out in a separate patch
>since you have introduced a new one. Or after this patchset.

Yes, after this patchset, some cleanup will be needed.

Thanks,
Chao Fan
>
>

Re: [PATCH v9 3/8] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table

[Chao Fan]

On Thu, Oct 18, 2018 at 01:56:44PM +0800, Baoquan He wrote:
>On 10/18/18 at 01:54pm, Chao Fan wrote:
>> On Thu, Oct 18, 2018 at 12:35:39PM +0800, Baoquan He wrote:
>> >On 10/17/18 at 06:20pm, Chao Fan wrote:
>> >> There is a bug that kaslr may randomly choose some positions
>> >> which are located in movable memory regions. This will break memory
>> >> hotplug feature and make the movable memory chosen by KASLR can't be
>> >> removed. So dig SRAT table from ACPI tables to get memory information.
>> >
>> >This patch is only adding a function efi_get_rsdp_addr() which will be
>> >used in later patch. Do we need to describe bug here?
>> 
>> Yes, it's the first mail of this serial, so I add more log.
>
>patch 3/8?

Since 1/8 and 2/8 are two functions introduced.
I will consider more about the log.

Thanks,
Chao Fan

>
>
>

Re: [PATCH v9 6/8] x86/boot: Dig out SRAT table from RSDP and find immovable memory

[Chao Fan]

On Wed, Oct 17, 2018 at 06:20:10PM +0800, Chao Fan wrote:
>Dig out SRAT table from RSDP, and then walk all memory to find
>the immovable memory regions, and fill in the immovable_mem[].
>So that we can use it to select memory for KASLR.
>
>Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>---
> arch/x86/boot/compressed/Makefile |   4 +
> arch/x86/boot/compressed/acpitb.c | 129 ++++++++++++++++++++++++++++++
> arch/x86/boot/compressed/misc.h   |  10 +++
> 3 files changed, 143 insertions(+)
>
>diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>index 28764dacf018..f67674d7d2bd 100644
>--- a/arch/x86/boot/compressed/Makefile
>+++ b/arch/x86/boot/compressed/Makefile
>@@ -83,6 +83,10 @@ ifdef CONFIG_X86_64
> 	vmlinux-objs-y += $(obj)/pgtable_64.o
> endif
> 
>+#ifdef CONFIG_MEMORY_HOTREMOVE && CONFIG_RANDOMIZE_BASE

Sorry for this, should be
#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)

I have change it in another file but forget here.

Thanks,
Chao Fan

>+vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
>+#endif
>+
> $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
> 
> vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
>diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>index 37b1f4407be8..d119663c05bb 100644
>--- a/arch/x86/boot/compressed/acpitb.c
>+++ b/arch/x86/boot/compressed/acpitb.c
>@@ -11,6 +11,15 @@
> #define STATIC
> #include <linux/decompress/mm.h>
> 
>+#ifdef CONFIG_MEMORY_HOTREMOVE
>+struct mem_vector {
>+	unsigned long long start;
>+	unsigned long long size;
>+};
>+/* Store the immovable memory regions */
>+struct mem_vector immovable_mem[MAX_NUMNODES*2];
>+#endif
>+
> /* Search EFI table for RSDP table. */
> static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
> {
>@@ -223,3 +232,123 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> 	}
> #endif
> }
>+
>+/*
>+ * Used to dig RSDP table from EFI table or BIOS.
>+ * If RSDP table found in EFI table, use it. Or search BIOS.
>+ * Based on acpi_os_get_root_pointer().
>+ */
>+static acpi_physical_address get_rsdp_addr(void)
>+{
>+	acpi_physical_address pa = 0;
>+
>+	get_acpi_rsdp(&pa);
>+
>+	if (!pa)
>+		efi_get_rsdp_addr(&pa);
>+
>+	if (!pa)
>+		bios_get_rsdp_addr(&pa);
>+
>+	return pa;
>+}
>+
>+static struct acpi_table_header *get_acpi_srat_table(void)
>+{
>+	acpi_physical_address acpi_table;
>+	acpi_physical_address root_table;
>+	struct acpi_table_header *header;
>+	struct acpi_table_rsdp *rsdp;
>+	char *signature;
>+	u8 *entry;
>+	u32 count;
>+	u32 size;
>+	int i, j;
>+	u32 len;
>+
>+	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
>+	if (!rsdp)
>+		return NULL;
>+
>+	/* Get RSDT or XSDT from RSDP. */
>+	if (!cmdline_find_option_arg("acpi", "rsdt", 4) &&
>+	    rsdp->xsdt_physical_address && rsdp->revision > 1) {
>+		root_table = rsdp->xsdt_physical_address;
>+		size = ACPI_XSDT_ENTRY_SIZE;
>+	} else {
>+		root_table = rsdp->rsdt_physical_address;
>+		size = ACPI_RSDT_ENTRY_SIZE;
>+	}
>+
>+	/* Get ACPI root table from RSDT or XSDT.*/
>+	header = (struct acpi_table_header *)root_table;
>+	len = header->length;
>+	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
>+	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
>+
>+	for (i = 0; i < count; i++) {
>+		u64 address64;
>+
>+		if (size == ACPI_RSDT_ENTRY_SIZE)
>+			acpi_table = ((acpi_physical_address)
>+				      (*ACPI_CAST_PTR(u32, entry)));
>+		else {
>+			*(u64 *)(void *)&address64 = *(u64 *)(void *)entry;
>+			acpi_table = (acpi_physical_address) address64;
>+		}
>+
>+		if (acpi_table) {
>+			header = (struct acpi_table_header *)acpi_table;
>+			signature = header->signature;
>+
>+			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
>+				return header;
>+		}
>+		entry += size;
>+	}
>+	return NULL;
>+}
>+
>+/*
>+ * According to ACPI table, filter the immvoable memory regions
>+ * and store them in immovable_mem[].
>+ */
>+void get_immovable_mem(void)
>+{
>+	struct acpi_table_header *table_header;
>+	struct acpi_subtable_header *table;
>+	struct acpi_srat_mem_affinity *ma;
>+	unsigned long table_end;
>+	int i = 0;
>+
>+	if (!cmdline_find_option_bool("movable_node") ||
>+	    cmdline_find_option_arg("acpi", "off", 3))
>+		return;
>+
>+	table_header = get_acpi_srat_table();
>+	if (!table_header)
>+		return;
>+
>+	table_end = (unsigned long)table_header + table_header->length;
>+
>+	table = (struct acpi_subtable_header *)
>+		((unsigned long)table_header + sizeof(struct acpi_table_srat));
>+
>+	while (((unsigned long)table) +
>+		       sizeof(struct acpi_subtable_header) < table_end) {
>+		if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
>+			ma = (struct acpi_srat_mem_affinity *)table;
>+			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
>+				immovable_mem[i].start = ma->base_address;
>+				immovable_mem[i].size = ma->length;
>+				i++;
>+			}
>+
>+			if (i >= MAX_NUMNODES*2)
>+				break;
>+		}
>+		table = (struct acpi_subtable_header *)
>+			((unsigned long)table + table->length);
>+	}
>+	num_immovable_mem = i;
>+}
>diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>index 40378408d980..70c403e1444c 100644
>--- a/arch/x86/boot/compressed/misc.h
>+++ b/arch/x86/boot/compressed/misc.h
>@@ -121,3 +121,13 @@ static inline void console_init(void)
> void set_sev_encryption_mask(void);
> 
> #endif
>+
>+/* acpitb.c */
>+#ifdef CONFIG_RANDOMIZE_BASE
>+int num_immovable_mem;
>+#ifdef CONFIG_MEMORY_HOTREMOVE
>+/* Store the amount of immovable memory regions */
>+#define ACPI_MAX_TABLES                128
>+void get_immovable_mem(void);
>+#endif
>+#endif
>-- 
>2.17.2
>

Re: [PATCH v9 1/8] x86/boot: Introduce cmdline_find_option_arg()to detect if option=arg in cmdline

[Chao Fan]

On Thu, Oct 18, 2018 at 12:01:20PM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> +bool cmdline_find_option_arg(const char *option, const char *arg, int argsize)
>> +{
>> +	char *buffer = malloc(argsize+1);
>> +	bool find = false;
>> +	int ret;
>> +
>> +	ret = cmdline_find_option(option, buffer, argsize+1);
>> +	if (ret == argsize && !strncmp(buffer, arg, argsize))
>> +		find = true;
>
>Wondering if we really need a wrapper like this. 

Well, my opinion is there are many code in kaslr.c can replaced by
cmdline_find_option_bool() and cmdline_find_option() and this new
cmdline_find_option_arg().
If this function introduced, we can clean up more code.

Thanks,
Chao Fan

>
>> +
>> +	free(buffer);
>> +	return find;
>> +}
>>  
>>  #endif
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index a1d5918765f3..008fdc47a29c 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -67,6 +67,7 @@ static inline void debug_puthex(const char *s)
>>  /* cmdline.c */
>>  int cmdline_find_option(const char *option, char *buffer, int bufsize);
>>  int cmdline_find_option_bool(const char *option);
>> +bool cmdline_find_option_arg(const char *option, const char *arg, int argsize);
>>  #endif
>>  
>>  
>> -- 
>> 2.17.2
>> 
>> 
>> 
>
>

Re: [PATCH v9 5/8] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> If KEXEC write the RSDP pointer to cmdline, parse the cmdline
> and use it.
> Imitate from early_param of "acpi_rsdp".
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpitb.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 50fa65cf824d..37b1f4407be8 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -8,6 +8,9 @@
>  #include <linux/numa.h>
>  #include <linux/acpi.h>
>  
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -200,3 +203,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  		*rsdp_addr = (acpi_physical_address)address;
>  	}
>  }
> +
> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> +{
> +#ifdef CONFIG_KEXEC
> +	unsigned long long res;
> +	int len = 0;
> +	char *val;
> +
> +	val = malloc(20);

Why is the length 20? Defined a macro?

> +	len = cmdline_find_option("acpi_rsdp", val, 20);
> +
> +	if (len == -1)
> +		return;
> +
> +	if (len > 0) {
> +		val[len] = 0;
> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 0, &res);
> +	}
> +#endif
> +}
> -- 
> 2.17.2
> 
> 
> 

Re: [PATCH v9 6/8] x86/boot: Dig out SRAT table from RSDP and find immovable memory

[Baoquan He]

On 10/17/18 at 06:20pm, Chao Fan wrote:
> Dig out SRAT table from RSDP, and then walk all memory to find
> the immovable memory regions, and fill in the immovable_mem[].
> So that we can use it to select memory for KASLR.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/Makefile |   4 +
>  arch/x86/boot/compressed/acpitb.c | 129 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/misc.h   |  10 +++
>  3 files changed, 143 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 28764dacf018..f67674d7d2bd 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -83,6 +83,10 @@ ifdef CONFIG_X86_64
>  	vmlinux-objs-y += $(obj)/pgtable_64.o
>  endif
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE && CONFIG_RANDOMIZE_BASE
> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
> +#endif
> +
>  $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
>  
>  vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 37b1f4407be8..d119663c05bb 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -11,6 +11,15 @@
>  #define STATIC
>  #include <linux/decompress/mm.h>
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +struct mem_vector {
> +	unsigned long long start;
> +	unsigned long long size;
> +};

Why not moving this definition to misc.h? There has been a mem_vector in
boot/compressed/kaslr.c.

> +/* Store the immovable memory regions */
> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -223,3 +232,123 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>  	}
>  #endif
>  }
> +
> +/*
> + * Used to dig RSDP table from EFI table or BIOS.
> + * If RSDP table found in EFI table, use it. Or search BIOS.
> + * Based on acpi_os_get_root_pointer().
> + */
> +static acpi_physical_address get_rsdp_addr(void)
> +{
> +	acpi_physical_address pa = 0;
> +
> +	get_acpi_rsdp(&pa);
> +
> +	if (!pa)
> +		efi_get_rsdp_addr(&pa);
> +
> +	if (!pa)
> +		bios_get_rsdp_addr(&pa);
> +
> +	return pa;
> +}
> +
> +static struct acpi_table_header *get_acpi_srat_table(void)
> +{
> +	acpi_physical_address acpi_table;
> +	acpi_physical_address root_table;
> +	struct acpi_table_header *header;
> +	struct acpi_table_rsdp *rsdp;
> +	char *signature;
> +	u8 *entry;
> +	u32 count;
> +	u32 size;
> +	int i, j;
> +	u32 len;
> +
> +	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
> +	if (!rsdp)
> +		return NULL;
> +
> +	/* Get RSDT or XSDT from RSDP. */
> +	if (!cmdline_find_option_arg("acpi", "rsdt", 4) &&
> +	    rsdp->xsdt_physical_address && rsdp->revision > 1) {
> +		root_table = rsdp->xsdt_physical_address;
> +		size = ACPI_XSDT_ENTRY_SIZE;
> +	} else {
> +		root_table = rsdp->rsdt_physical_address;
> +		size = ACPI_RSDT_ENTRY_SIZE;
> +	}
> +
> +	/* Get ACPI root table from RSDT or XSDT.*/
> +	header = (struct acpi_table_header *)root_table;
> +	len = header->length;
> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
> +
> +	for (i = 0; i < count; i++) {
> +		u64 address64;
> +
> +		if (size == ACPI_RSDT_ENTRY_SIZE)
> +			acpi_table = ((acpi_physical_address)
> +				      (*ACPI_CAST_PTR(u32, entry)));
> +		else {
> +			*(u64 *)(void *)&address64 = *(u64 *)(void *)entry;
> +			acpi_table = (acpi_physical_address) address64;
> +		}
> +
> +		if (acpi_table) {
> +			header = (struct acpi_table_header *)acpi_table;
> +			signature = header->signature;
> +
> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
> +				return header;
> +		}
> +		entry += size;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * According to ACPI table, filter the immvoable memory regions
> + * and store them in immovable_mem[].
> + */
> +void get_immovable_mem(void)
> +{
> +	struct acpi_table_header *table_header;
> +	struct acpi_subtable_header *table;
> +	struct acpi_srat_mem_affinity *ma;
> +	unsigned long table_end;
> +	int i = 0;
> +
> +	if (!cmdline_find_option_bool("movable_node") ||
> +	    cmdline_find_option_arg("acpi", "off", 3))
> +		return;
> +
> +	table_header = get_acpi_srat_table();
> +	if (!table_header)
> +		return;
> +
> +	table_end = (unsigned long)table_header + table_header->length;
> +
> +	table = (struct acpi_subtable_header *)
> +		((unsigned long)table_header + sizeof(struct acpi_table_srat));
> +
> +	while (((unsigned long)table) +
> +		       sizeof(struct acpi_subtable_header) < table_end) {
> +		if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
> +			ma = (struct acpi_srat_mem_affinity *)table;
> +			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
> +				immovable_mem[i].start = ma->base_address;
> +				immovable_mem[i].size = ma->length;
> +				i++;
> +			}
> +
> +			if (i >= MAX_NUMNODES*2)
		No warning message printed in this case?
> +				break;
> +		}
> +		table = (struct acpi_subtable_header *)
> +			((unsigned long)table + table->length);
> +	}
> +	num_immovable_mem = i;
> +}
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 40378408d980..70c403e1444c 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -121,3 +121,13 @@ static inline void console_init(void)
>  void set_sev_encryption_mask(void);
>  
>  #endif
> +
> +/* acpitb.c */
> +#ifdef CONFIG_RANDOMIZE_BASE
> +int num_immovable_mem;
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the amount of immovable memory regions */
> +#define ACPI_MAX_TABLES                128
> +void get_immovable_mem(void);
> +#endif
> +#endif
> -- 
> 2.17.2
> 
> 
> 

Re: [PATCH v9 6/8] x86/boot: Dig out SRAT table from RSDP and find immovable memory

[Fan, Chao]

On Sun, Oct 21, 2018 at 10:34:58AM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> +	if (!cmdline_find_option_bool("movable_node") ||
>> +	    cmdline_find_option_arg("acpi", "off", 3))
>> +		return;
>> +
>> +	table_header = get_acpi_srat_table();
>> +	if (!table_header)
>> +		return;
>> +
>> +	table_end = (unsigned long)table_header + table_header->length;
>> +
>> +	table = (struct acpi_subtable_header *)
>> +		((unsigned long)table_header + sizeof(struct acpi_table_srat));
>> +
>> +	while (((unsigned long)table) +
>> +		       sizeof(struct acpi_subtable_header) < table_end) {
>> +		if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
>> +			ma = (struct acpi_srat_mem_affinity *)table;
>> +			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
>> +				immovable_mem[i].start = ma->base_address;
>> +				immovable_mem[i].size = ma->length;
>> +				i++;
>> +			}
>> +
>> +			if (i >= MAX_NUMNODES*2)
>		No warning message printed in this case?

I will add. BTW, what message is appropriate?
I can't figure out in what condition, i >= MAX_NUMNODES*2.

Thanks,
Chao Fan

>> +				break;
>> +		}
>> +		table = (struct acpi_subtable_header *)
>> +			((unsigned long)table + table->length);
>> +	}
>> +	num_immovable_mem = i;
>> +}
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index 40378408d980..70c403e1444c 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -121,3 +121,13 @@ static inline void console_init(void)
>>  void set_sev_encryption_mask(void);
>>  
>>  #endif
>> +
>> +/* acpitb.c */
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +int num_immovable_mem;
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the amount of immovable memory regions */
>> +#define ACPI_MAX_TABLES                128
>> +void get_immovable_mem(void);
>> +#endif
>> +#endif
>> -- 
>> 2.17.2
>> 
>> 
>> 
>
>

Re: [PATCH v9 5/8] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec

[Fan, Chao]

On Sun, Oct 21, 2018 at 10:26:50AM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> If KEXEC write the RSDP pointer to cmdline, parse the cmdline
>> and use it.
>> Imitate from early_param of "acpi_rsdp".
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpitb.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 50fa65cf824d..37b1f4407be8 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -8,6 +8,9 @@
>>  #include <linux/numa.h>
>>  #include <linux/acpi.h>
>>  
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -200,3 +203,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  		*rsdp_addr = (acpi_physical_address)address;
>>  	}
>>  }
>> +
>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> +#ifdef CONFIG_KEXEC
>> +	unsigned long long res;
>> +	int len = 0;
>> +	char *val;
>> +
>> +	val = malloc(20);
>
>Why is the length 20? Defined a macro?
>

Not a calculation, if it's enough to store the address, that will be OK.

Thanks,
Chao Fan

>> +	len = cmdline_find_option("acpi_rsdp", val, 20);
>> +
>> +	if (len == -1)
>> +		return;
>> +
>> +	if (len > 0) {
>> +		val[len] = 0;
>> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 0, &res);
>> +	}
>> +#endif
>> +}
>> -- 
>> 2.17.2
>> 
>> 
>> 
>
>

Re: [PATCH v9 5/8] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec

[Baoquan He]

On 10/22/18 at 05:30am, Fan, Chao wrote:
> >> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> >> +{
> >> +#ifdef CONFIG_KEXEC
> >> +	unsigned long long res;
> >> +	int len = 0;
> >> +	char *val;
> >> +
> >> +	val = malloc(20);
> >
> >Why is the length 20? Defined a macro?
> >
> 
> Not a calculation, if it's enough to store the address, that will be OK.

Sorry, I didn't catch. It's 16 in setup_acpi_rsdp() of
drivers/acpi/osl.c . What does 'that' mean?

Wondering why not making it 200, it's also enough to store the address.

Re: [PATCH v9 6/8] x86/boot: Dig out SRAT table from RSDP and find immovable memory

[Baoquan He]

On 10/22/18 at 05:29am, Fan, Chao wrote:
> On Sun, Oct 21, 2018 at 10:34:58AM +0800, Baoquan He wrote:
> >On 10/17/18 at 06:20pm, Chao Fan wrote:
> >> +	if (!cmdline_find_option_bool("movable_node") ||
> >> +	    cmdline_find_option_arg("acpi", "off", 3))
> >> +		return;
> >> +
> >> +	table_header = get_acpi_srat_table();
> >> +	if (!table_header)
> >> +		return;
> >> +
> >> +	table_end = (unsigned long)table_header + table_header->length;
> >> +
> >> +	table = (struct acpi_subtable_header *)
> >> +		((unsigned long)table_header + sizeof(struct acpi_table_srat));
> >> +
> >> +	while (((unsigned long)table) +
> >> +		       sizeof(struct acpi_subtable_header) < table_end) {
> >> +		if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
> >> +			ma = (struct acpi_srat_mem_affinity *)table;
> >> +			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
> >> +				immovable_mem[i].start = ma->base_address;
> >> +				immovable_mem[i].size = ma->length;
> >> +				i++;
> >> +			}
> >> +
> >> +			if (i >= MAX_NUMNODES*2)
> >		No warning message printed in this case?
> 
> I will add. BTW, what message is appropriate?
> I can't figure out in what condition, i >= MAX_NUMNODES*2.

Do you mean it's impossible to happen? 

Re: [PATCH v9 5/8] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec

[Chao Fan]

On Mon, Oct 22, 2018 at 02:06:13PM +0800, Baoquan He wrote:
>On 10/22/18 at 05:30am, Fan, Chao wrote:
>> >> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> >> +{
>> >> +#ifdef CONFIG_KEXEC
>> >> +	unsigned long long res;
>> >> +	int len = 0;
>> >> +	char *val;
>> >> +
>> >> +	val = malloc(20);
>> >
>> >Why is the length 20? Defined a macro?
>> >
>> 
>> Not a calculation, if it's enough to store the address, that will be OK.
>
>Sorry, I didn't catch. It's 16 in setup_acpi_rsdp() of
>drivers/acpi/osl.c . What does 'that' mean?

The second parameter of kstrtoull(), the 16 you mentioned means
hexadecimal, not the length.
I checked my host and guest, the value are ACPI20=0xbfbfa014, ACPI20=0xdb807000.
The length of memory is 8. Well the max memory address is 16, add
"0x" and '\0' is 19. So I set it as 20.
I am not sure whether 8 is enough for the address, if OK, 11 will
be enough, or 19 is OK.
If my understanding is wrong, please tell me.

Thanks,
Chao Fan

>
>Wondering why not making it 200, it's also enough to store the address.
>
>

Re: [PATCH v9 5/8] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec

[Baoquan He]

On 10/22/18 at 03:30pm, Chao Fan wrote:
> On Mon, Oct 22, 2018 at 02:06:13PM +0800, Baoquan He wrote:
> >On 10/22/18 at 05:30am, Fan, Chao wrote:
> >> >> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> >> >> +{
> >> >> +#ifdef CONFIG_KEXEC
> >> >> +	unsigned long long res;
> >> >> +	int len = 0;
> >> >> +	char *val;
> >> >> +
> >> >> +	val = malloc(20);
> >> >
> >> >Why is the length 20? Defined a macro?
> >> >
> >> 
> >> Not a calculation, if it's enough to store the address, that will be OK.
> >
> >Sorry, I didn't catch. It's 16 in setup_acpi_rsdp() of
> >drivers/acpi/osl.c . What does 'that' mean?
> 
> The second parameter of kstrtoull(), the 16 you mentioned means
> hexadecimal, not the length.

Yes, you are right.

> I checked my host and guest, the value are ACPI20=0xbfbfa014, ACPI20=0xdb807000.
> The length of memory is 8. Well the max memory address is 16, add
> "0x" and '\0' is 19. So I set it as 20.
> I am not sure whether 8 is enough for the address, if OK, 11 will
> be enough, or 19 is OK.

I am fine with 20. Thanks.

Re: [PATCH v9 8/8] x86/boot/KASLR: Limit kaslr to choosing the immovable memory

[Chao Fan]

On Thu, Oct 18, 2018 at 12:21:23PM +0800, Baoquan He wrote:
>On 10/17/18 at 06:20pm, Chao Fan wrote:
>> If CONFIG_MEMORY_HOTREMOVE enabled and the amount of immovable
>> memory regions is not zero. Calculate the intersection between memory
>
>This if conditional adverbial clauses is not an complete sentence.
>
>> regions from e820/efi memory table and immovable memory regions.
>					^ get?
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 72 +++++++++++++++++++++++++++-----
>>  1 file changed, 61 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 0c3567bc231c..3ebb150f61eb 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -101,6 +101,11 @@ static bool memmap_too_large;
>>  /* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
>>  static unsigned long long mem_limit = ULLONG_MAX;
>>  
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the immovable memory regions */
>> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
>
>Sorry, Chao. I may not follow your old patch change, why the length of
>immovable_mem is MAX_NUMNODES*2, is there any reason or basis?
>
>> +#endif
>> +
>>  
>>  enum mem_avoid_index {
>>  	MEM_AVOID_ZO_RANGE = 0,
>> @@ -577,9 +582,9 @@ static unsigned long slots_fetch_random(void)
>>  	return 0;
>>  }
>>  
>> -static void process_mem_region(struct mem_vector *entry,
>> -			       unsigned long minimum,
>> -			       unsigned long image_size)
>> +static void slots_count(struct mem_vector *entry,
>> +			unsigned long minimum,
>> +			unsigned long image_size)
>>  {
>>  	struct mem_vector region, overlap;
>>  	unsigned long start_orig, end;
>> @@ -655,6 +660,57 @@ static void process_mem_region(struct mem_vector *entry,
>>  	}
>>  }
>>  
>> +static bool process_mem_region(struct mem_vector *region,
>> +			       unsigned long long minimum,
>> +			       unsigned long long image_size)
>> +{
>> +	int i;
>> +	/*
>> +	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
>> +	 * walk all the regions, so use region directely.
>> +	 */
>> +	if (num_immovable_mem == 0) {
>> +		slots_count(region, minimum, image_size);
>> +
>> +		if (slot_area_index == MAX_SLOT_AREA) {
>> +			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
>> +			return 1;
>> +		}
>> +		return 0;
>> +	}
>> +
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +	/*
>> +	 * If immovable memory found, filter the intersection between
>> +	 * immovable memory and region to slots_count.
>> +	 * Otherwise, go on old code.
>
>Could you explain more about what is the old code in otherwise case you
>want to go on?

Sure,
1. 'movable_node' not specified in cmdline.
2. CONFIG_HOT_REMOVE not difned.
3. Just one node in this machine.
>
>> +	 */
>> +	for (i = 0; i < num_immovable_mem; i++) {
>> +		struct mem_vector entry;
>> +		unsigned long long start, end, entry_end, region_end;
>> +
>> +		if (!mem_overlaps(region, &immovable_mem[i]))
>> +			continue;
>> +
>> +		start = immovable_mem[i].start;
>> +		end = start + immovable_mem[i].size;
>> +		region_end = region->start + region->size;
>> +
>> +		entry.start = clamp(region->start, start, end);
>> +		entry_end = clamp(region_end, start, end);
>> +		entry.size = entry_end - entry.start;
>> +
>> +		slots_count(&entry, minimum, image_size);
>
>Obviously, your patch log only covers this place of code. About renaming
>process_mem_region() to  slot_count(), and add another level of wrapper 
>process_mem_region(), may also need be mentioned in patch log.
>
Sorry for missing the comment.
Rename process_mem_region to slots_count to match slots_fetch_random,
and name new function as process_mem_region.

Thanks,
Chao Fan

>> +
>> +		if (slot_area_index == MAX_SLOT_AREA) {
>> +			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
>> +			return 1;
>> +		}
>> +	}
>> +	return 0;
>> +#endif
>> +}
>> +
>>  #ifdef CONFIG_EFI
>>  /*
>>   * Returns true if mirror region found (and must have been processed
>> @@ -720,11 +776,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>>  
>>  		region.start = md->phys_addr;
>>  		region.size = md->num_pages << EFI_PAGE_SHIFT;
>> -		process_mem_region(&region, minimum, image_size);
>> -		if (slot_area_index == MAX_SLOT_AREA) {
>> -			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
>> +		if (process_mem_region(&region, minimum, image_size))
>>  			break;
>> -		}
>>  	}
>>  	return true;
>>  }
>> @@ -751,11 +804,8 @@ static void process_e820_entries(unsigned long minimum,
>>  			continue;
>>  		region.start = entry->addr;
>>  		region.size = entry->size;
>> -		process_mem_region(&region, minimum, image_size);
>> -		if (slot_area_index == MAX_SLOT_AREA) {
>> -			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
>> +		if (process_mem_region(&region, minimum, image_size))
>>  			break;
>> -		}
>>  	}
>>  }
>>  
>> -- 
>> 2.17.2
>> 
>> 
>> 
>
>

Re: [PATCH v9 8/8] x86/boot/KASLR: Limit kaslr to choosing the immovable memory

[Baoquan He]

On 10/22/18 at 06:13pm, Chao Fan wrote:
> >> +static bool process_mem_region(struct mem_vector *region,
> >> +			       unsigned long long minimum,
> >> +			       unsigned long long image_size)
> >> +{
> >> +	int i;
> >> +	/*
> >> +	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
> >> +	 * walk all the regions, so use region directely.
> >> +	 */
> >> +	if (num_immovable_mem == 0) {
> >> +		slots_count(region, minimum, image_size);
> >> +
> >> +		if (slot_area_index == MAX_SLOT_AREA) {
> >> +			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
> >> +			return 1;
> >> +		}
> >> +		return 0;
> >> +	}
> >> +
> >> +#ifdef CONFIG_MEMORY_HOTREMOVE
> >> +	/*
> >> +	 * If immovable memory found, filter the intersection between
> >> +	 * immovable memory and region to slots_count.
> >> +	 * Otherwise, go on old code.
> >
> >Could you explain more about what is the old code in otherwise case you
> >want to go on?
> 
> Sure,
> 1. 'movable_node' not specified in cmdline.
> 2. CONFIG_HOT_REMOVE not difned.
> 3. Just one node in this machine.

So these cases are not covered by 'if (num_immovable_mem == 0)' code?
In thise ifdef code block, where do you handle above three cases?

Thanks
Baoquan

Re: [PATCH v9 8/8] x86/boot/KASLR: Limit kaslr to choosing the immovable memory

[Chao Fan]

On Mon, Oct 22, 2018 at 06:24:55PM +0800, Baoquan He wrote:
>On 10/22/18 at 06:13pm, Chao Fan wrote:
>> >> +static bool process_mem_region(struct mem_vector *region,
>> >> +			       unsigned long long minimum,
>> >> +			       unsigned long long image_size)
>> >> +{
>> >> +	int i;
>> >> +	/*
>> >> +	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
>> >> +	 * walk all the regions, so use region directely.
>> >> +	 */
>> >> +	if (num_immovable_mem == 0) {
>> >> +		slots_count(region, minimum, image_size);
>> >> +
>> >> +		if (slot_area_index == MAX_SLOT_AREA) {
>> >> +			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
>> >> +			return 1;
>> >> +		}
>> >> +		return 0;
>> >> +	}
>> >> +
>> >> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> >> +	/*
>> >> +	 * If immovable memory found, filter the intersection between
>> >> +	 * immovable memory and region to slots_count.
>> >> +	 * Otherwise, go on old code.
>> >
>> >Could you explain more about what is the old code in otherwise case you
>> >want to go on?
>> 
>> Sure,
>> 1. 'movable_node' not specified in cmdline.
>> 2. CONFIG_HOT_REMOVE not difned.
>> 3. Just one node in this machine.
>
>So these cases are not covered by 'if (num_immovable_mem == 0)' code?
They are covered by 'if (num_immovable_mem == 0)' code.
>In thise ifdef code block, where do you handle above three cases?
"go on old code." means above:
+ if (num_immovable_mem == 0) {
+         slots_count(region, minimum, image_size);
+
+         if (slot_area_index == MAX_SLOT_AREA) {
+                 debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
+                 return 1;
+         }
+         return 0;
+ }

Thanks,
Chao Fan

>
>Thanks
>Baoquan
>
>