Re: [PATCH v14 4/5] x86/boot: Parse SRAT address from RSDP and store immovable memory

[Chao Fan <fanc.fnst@cn.fujitsu.com>]

On Mon, Dec 17, 2018 at 06:41:49PM +0100, Ingo Molnar wrote:
>
>* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
>> SRAT should be parsed by RSDP to fix the conflict between KASLR
>> and memory-hotremove, then find the immovable memory regions and store
>> them in an array called immovable_mem[]. With immovable_mem[], KASLR
>> can avoid to extract kernel to specific regions.
>> 
>> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
>> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/Kconfig                  |  12 +++
>>  arch/x86/boot/compressed/Makefile |   2 +
>>  arch/x86/boot/compressed/acpi.c   | 128 ++++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/kaslr.c  |   4 -
>>  arch/x86/boot/compressed/misc.h   |  19 +++++
>>  5 files changed, 161 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index ba7e3464ee92..333c383478b7 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2149,6 +2149,18 @@ config X86_NEED_RELOCS
>>  	def_bool y
>>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>>  
>> +config EARLY_SRAT_PARSE
>> +	bool "Early SRAT table parsing"
>> +	def_bool y
>> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
>> +	help
>> +	  This option enables early SRAT parsing in compressed boot stage
>> +	  so that memory hot-remove ranges do not overlap with KASLR
>> +	  chosen ranges. Kernel won't be extracted in hot-removable
>> +	  memory, so that make sure memory-hotremove works well with
>> +	  KASLR enabled.
>> +	  Say Y if you want to use both KASLR and memory-hotremove.
>
>So why would we want to make this a config option, instead of enabling it 
>unconditionally?

Well, it can make ifdeffery more clear, and Boris suggested to do that.
If there is no KASLR enabled or MEMORY-HOTREMOVE enabled, the code is
not needed, so it's not good to enable it unconditionally.

>
>How reliable are the hot-removable memory markings in various firmware 
>versions?

But we can only get the information from firmware, I can't figure out
other information. And ACPI also gets the table from here.

>
>> +/* Compute SRAT table from RSDP. */
>> +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;
>> +	u32 num_entries;
>> +	char arg[10];
>
>The '10' is just a magic number attached to a meaningless local variable 
>name. Please explain the limit in the code, and the role of the variable 
>if it's non-obvious from the name. Or better, try to find a more obvious 
>name?
>

Yes, the '10' is magic number, the meaning is detail. Here, the '10'
is used to store the result of 'acpi=' in cmdline. Well, see
Documentation/admin-guide/kernel-parameters.txt:
        acpi=           [HW,ACPI,X86,ARM64]
                        Advanced Configuration and Power Interface
                        Format: { force | on | off | strict | noirq | rsdt |
                                  copy_dsdt }
'copy_dsdt' is the longest result, its size is '9', plus '\0' is '10'.
So I set it as '10'.

And I see the usage like this is:
        char arg[5];
in pti_check_boottime_disable() of arch/x86/mm/pti.c, since the longest
result of 'pti=' in cmdline is 'auto', so it's '5' here.

And:
        char arg[32];
in fpu__init_parse_early_param() of arch/x86/kernel/fpu/init.c.

And so on. All usage of cmdline_find_option() needs a string, they are
often named as arg[] and with a number which can cover the longest result.

Thanks,
Chao Fan

>Thanks,
>
>	Ingo
>
>