Re: [PATCH v13 2/6] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Borislav Petkov]

On Wed, Dec 12, 2018 at 11:10:49AM +0800, Chao Fan 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.
> 
> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
> 
> Signed-off-by: Chao Fan <fanc.fnst@xxxxxxxxxxxxxx>
> ---
>  arch/x86/Kconfig                | 10 ++++++++++
>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/misc.h |  6 ++++++
>  3 files changed, 46 insertions(+)
>  create mode 100644 arch/x86/boot/compressed/acpi.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ba7e3464ee92..455da382fa9e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2149,6 +2149,16 @@ config X86_NEED_RELOCS
>  	def_bool y
>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>  
> +config EARLY_PARSE_RSDP

That should be EARLY_SRAT_PARSE or so

> +	bool "Parse RSDP pointer on compressed period for KASLR"

and that should be

	bool "Early SRAT table parsing"

> +	def_bool y
> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
> +	help
> +	  This option parses RSDP in compressed period. Works

do s/compressed period/compressed stage/g

I think "compressed stage" or "compressed boot stage" is more precise.

> +	  for KASLR to get memory information from SRAT table and choose
> +	  immovable memory to extract kernel.
> +	  Say Y if you want to use both KASLR and memory-hotremove.

This help text needs to explain what the functionality is for. "This
option parses RSDP in compressed period" doesn't tell a whole lot to the
normal user wondering whether she should enable this or not. You need to
explain whether people would need this or not so should start along the
lines of:

"This option enables early SRAT parsing so that memory hot remove ranges do not
overlap with KASLR chosen ranges..."

Basically, the "***Background" in your 0th message.

> +
>  config PHYSICAL_ALIGN
>  	hex "Alignment value to which kernel should be aligned"
>  	default "0x200000"
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> new file mode 100644
> index 000000000000..cad15686f82c
> --- /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>
> +
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#include "../string.h"
> +
> +static acpi_physical_address get_acpi_rsdp(void)
> +{
> +#ifdef CONFIG_KEXEC
> +	unsigned long long res;
> +	int len = 0;
> +	char val[MAX_ADDRESS_LENGTH+1];

Please sort function local variables declaration in a reverse christmas
tree order:

	<type A> longest_variable_name;
	<type B> shorter_var_name;
	<type C> even_shorter;
	<type D> i;

> +
> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);

That MAX_ADDRESS_LENGTH define is used only here, right? So put it at
the top in this file - no need to have it in a header.

> +	if (len > 0) {
> +		val[len] = 0;
> +		return (acpi_physical_address)kstrtoull(val, 16, &res);
> +	}
> +	return 0;
> +#endif

Those two lines need to be the other way around:

#endif
	return 0;

because in the !CONFIG_KEXEC case that function won't return anything.
gcc is smart enough to optimize that function away so that there's no
build error but still.

Ditto for efi_get_rsdp_addr() in your next patch.

> +}
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index a1d5918765f3..72fcfbfec3c6 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -116,3 +116,9 @@ static inline void console_init(void)
>  void set_sev_encryption_mask(void);
>  
>  #endif
> +
> +/* acpi.c */
> +#ifdef CONFIG_EARLY_PARSE_RSDP
> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
> +#define MAX_ADDRESS_LENGTH 18
> +#endif
> -- 

Yeah, just put that define in acpi.c.

-- 
Regards/Gruss,
    Boris.

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