Re: [tip: x86/urgent] x86/setup: Call early_reserve_memory() earlier

[Dan Williams <dan.j.williams@intel.com>]

On Thu, Nov 4, 2021 at 4:40 AM Borislav Petkov <bp@alien8.de> wrote:
>
> + Mike.
>
> On Thu, Nov 04, 2021 at 05:38:54AM +0000, Williams, Dan J wrote:
> > By inspection, this commit looks like the source of the problem because
> > early_reserve_memory() now runs before parse_early_param().
>
> Yah, I see it too:
>
> early_reserve_memory
> |-> efi_memblock_x86_reserve_range
>     |-> efi_fake_memmap_early
>
> which does
>
>         if (!efi_soft_reserve_enabled())
>                 return;
>
> and that would have set EFI_MEM_NO_SOFT_RESERVE after having parsed
> "nosoftreserve".
>
> And parse_early_param() happens later.
>
> Uuuf, that early memory reservation dance is not going to be over,
> ever...
>
> Well, I guess we can do something like this below. The intent being to
> carve out the command line preparation into a separate function which
> does the early param parsing too. So that it all goes together.
>
> And then call that function before early_reserve_memory() so that the
> params have been parsed by then.
>
> Looking at the changed flow, I think we should be ok but I've said that
> a bunch of times already regarding this memory reservation early and
> something in our house of cards called early boot order always broke.
>
> Damn.

Thanks Boris!

You can add:

Tested-by: Anjaneya Chagam <anjaneya.chagam@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>

>
> ---
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 40ed44ead063..05f69e7d84dc 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -742,6 +742,28 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
>         return 0;
>  }
>
> +static char *prepare_command_line(void)
> +{
> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_OVERRIDE
> +       strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +#else
> +       if (builtin_cmdline[0]) {
> +               /* append boot loader cmdline to builtin */
> +               strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +               strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +               strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +       }
> +#endif
> +#endif
> +
> +       strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> +
> +       parse_early_param();
> +
> +       return command_line;
> +}
> +
>  /*
>   * Determine if we were loaded by an EFI loader.  If so, then we have also been
>   * passed the efi memmap, systab, etc., so we should use these data structures
> @@ -830,6 +852,23 @@ void __init setup_arch(char **cmdline_p)
>
>         x86_init.oem.arch_setup();
>
> +       /*
> +        * x86_configure_nx() is called before parse_early_param() (called by
> +        * prepare_command_line()) to detect whether hardware doesn't support
> +        * NX (so that the early EHCI debug console setup can safely call
> +        * set_fixmap()). It may then be called again from within noexec_setup()
> +        * during parsing early parameters to honor the respective command line
> +        * option.
> +        */
> +       x86_configure_nx();
> +
> +       /*
> +        * This parses early params and it needs to run before
> +        * early_reserve_memory() because latter relies on such settings
> +        * supplies as early params.
> +        */
> +       *cmdline_p = prepare_command_line();
> +
>         /*
>          * Do some memory reservations *before* memory is added to memblock, so
>          * memblock allocations won't overwrite it.
> @@ -863,33 +902,6 @@ void __init setup_arch(char **cmdline_p)
>         bss_resource.start = __pa_symbol(__bss_start);
>         bss_resource.end = __pa_symbol(__bss_stop)-1;
>
> -#ifdef CONFIG_CMDLINE_BOOL
> -#ifdef CONFIG_CMDLINE_OVERRIDE
> -       strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> -#else
> -       if (builtin_cmdline[0]) {
> -               /* append boot loader cmdline to builtin */
> -               strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> -               strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> -               strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> -       }
> -#endif
> -#endif
> -
> -       strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> -       *cmdline_p = command_line;
> -
> -       /*
> -        * x86_configure_nx() is called before parse_early_param() to detect
> -        * whether hardware doesn't support NX (so that the early EHCI debug
> -        * console setup can safely call set_fixmap()). It may then be called
> -        * again from within noexec_setup() during parsing early parameters
> -        * to honor the respective command line option.
> -        */
> -       x86_configure_nx();
> -
> -       parse_early_param();
> -
>  #ifdef CONFIG_MEMORY_HOTPLUG
>         /*
>          * Memory used by the kernel cannot be hot-removed because Linux
>
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette