Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Ingo Molnar]

* Chao Fan <fanc.fnst@xxxxxxxxxxxxxx> wrote:

> Memory information in SRAT is necessary to fix the conflict between
> KASLR and memory-hotremove.
> 
> ACPI SRAT (System/Static Resource Affinity Table) shows the details
> about memory ranges, including ranges of memory provided by hot-added
> memory devices. SRAT is introduced by Root System Description
> Pointer(RSDP), so RSDP should be found firstly.
> 
> When booting form KEXEC/EFI/BIOS, the methods to find RSDP
> are different. When booting from KEXEC, 'acpi_rsdp' may have been
> added to cmdline, so parse cmdline to find RSDP.

What is the nature of the basic conflict? Should the kernel only allow 
hot-removing memory ranges that are explicitly marked as such in the 
SRAT? I presume that's already so, correct? Or should the kernel avoiding 
KASLR-randomizing into hot-removable memory areas?

Some minor nits:

> Signed-off-by: Chao Fan <fanc.fnst@xxxxxxxxxxxxxx>
> ---
>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 arch/x86/boot/compressed/acpi.c
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> new file mode 100644
> index 000000000000..44f19546c169
> --- /dev/null
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define BOOT_CTYPE_H
> +#include "misc.h"
> +#include "error.h"
> +
> +#include <linux/efi.h>
> +#include <linux/numa.h>
> +#include <linux/acpi.h>
> +#include <asm/efi.h>
> +
> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digits */
> +#define MAX_ADDRESS_LENGTH 18

I'd suggest adding the +1 for the string termination \0 as well, and use 
that below.

Also, 'max address length' is way too generic (and hence pretty 
meaningless) - how about MAX_HEX_ADDRESS_STRING_LEN or so?

> +
> +static acpi_physical_address get_acpi_rsdp(void)
> +{
> +#ifdef CONFIG_KEXEC
> +	char val[MAX_ADDRESS_LENGTH+1];
> +	unsigned long long res;
> +	int len;
> +
> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
> +	if (len > 0) {
> +		char *e;
> +
> +		val[len] = '\0';
> +		return (acpi_physical_address)simple_strtoull(val, &e, 16);

'return' is not a function - no need for the parenthesis.

Thanks,

	Ingo