On Mon, Dec 17, 2018 at 06:41:49PM +0100, Ingo Molnar wrote:
>
>* Chao Fan <fanc.f...@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.f...@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
>
>
