On Thu, Jun 01, 2023 at 09:25:38PM +0300, Kirill A. Shutemov wrote:
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 454757fbdfe5..749f0fe7e446 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -672,6 +672,28 @@ static bool process_mem_region(struct mem_vector *region,
> }
>
> #ifdef CONFIG_EFI
> +
> +/*
> + * Only EFI_CONVENTIONAL_MEMORY and EFI_UNACCEPTED_MEMORY (if supported) are
> + * guaranteed to be free.
> + *
> + * It is more conservative in picking free memory than the EFI spec allows:
"Pick free memory more conservatively than the EFI spec allows:
EFI_BOOT_SERVICES_ ..."
> + *
> + * According to the spec, EFI_BOOT_SERVICES_{CODE|DATA} are also free memory
> + * and thus available to place the kernel image into, but in practice there's
> + * firmware where using that memory leads to crashes.
... because that firmware still scratches into that memory or why?
> + */
> +static inline bool memory_type_is_free(efi_memory_desc_t *md)
> +{
> + if (md->type == EFI_CONVENTIONAL_MEMORY)
> + return true;
> +
> + if (md->type == EFI_UNACCEPTED_MEMORY)
> + return IS_ENABLED(CONFIG_UNACCEPTED_MEMORY);
Make it plan and simple:
if (IS_ENABLED(CONFIG_UNACCEPTED_MEMORY) &&
md->type == EFI_UNACCEPTED_MEMORY)
return true;
> +
> + return false;
> +}
> +
> /*
> * Returns true if we processed the EFI memmap, which we prefer over the E820
> * table if it is available.
> @@ -716,18 +738,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
> for (i = 0; i < nr_desc; i++) {
> md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
>
> - /*
> - * Here we are more conservative in picking free memory than
> - * the EFI spec allows:
> - *
> - * According to the spec, EFI_BOOT_SERVICES_{CODE|DATA} are also
> - * free memory and thus available to place the kernel image into,
> - * but in practice there's firmware where using that memory leads
> - * to crashes.
> - *
> - * Only EFI_CONVENTIONAL_MEMORY is guaranteed to be free.
> - */
> - if (md->type != EFI_CONVENTIONAL_MEMORY)
> + if (!memory_type_is_free(md))
> continue;
>
> if (efi_soft_reserve_enabled() &&
> diff --git a/arch/x86/boot/compressed/mem.c b/arch/x86/boot/compressed/mem.c
> index 67594fcb11d9..4ecf26576a77 100644
> --- a/arch/x86/boot/compressed/mem.c
> +++ b/arch/x86/boot/compressed/mem.c
> @@ -1,9 +1,40 @@
> // SPDX-License-Identifier: GPL-2.0-only
>
> #include "error.h"
> +#include "misc.h"
>
> void arch_accept_memory(phys_addr_t start, phys_addr_t end)
> {
> /* Platform-specific memory-acceptance call goes here */
> error("Cannot accept memory");
> }
> +
> +void init_unaccepted_memory(void)
> +{
> + guid_t guid = LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID;
An additional space after the "=".
> + struct efi_unaccepted_memory *unaccepted_table;
> + unsigned long cfg_table_pa;
> + unsigned int cfg_table_len;
> + enum efi_type et;
> + int ret;
> +
> + et = efi_get_type(boot_params);
> + if (et == EFI_TYPE_NONE)
> + return;
> +
> + ret = efi_get_conf_table(boot_params, &cfg_table_pa, &cfg_table_len);
> + if (ret)
> + error("EFI config table not found.");
What's the point in erroring out here?
> + unaccepted_table = (void *)efi_find_vendor_table(boot_params,
> + cfg_table_pa,
> + cfg_table_len,
> + guid);
> + if (!unaccepted_table)
> + return;
> +
> + if (unaccepted_table->version != 1)
> + error("Unknown version of unaccepted memory table\n");
> +
> + set_unaccepted_table(unaccepted_table);
Why is this a function at all and not a simple assignment?
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 014ff222bf4b..36535a3753f5 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -455,6 +455,13 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> #endif
>
> debug_putstr("\nDecompressing Linux... ");
> +
> + if (IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) {
> + debug_putstr("Accepting memory... ");
This needs to happen...
> + init_unaccepted_memory();
... after the init, after the init has parsed the config table and has
found unaccepted memory.
If not, you don't need to issue anything as that would be wrong.
> + accept_memory(__pa(output), __pa(output) + needed_size);
> + }
> +
> __decompress(input_data, input_len, NULL, NULL, output, output_len,
> NULL, error);
> entry_offset = parse_elf(output);
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
