Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
From: Ard Biesheuvel <ardb@kernel.org>
To: Arvind Sankar <nivedita@alum.mit.edu>
Cc: linux-efi <linux-efi@vger.kernel.org>, X86 ML <x86@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 08/10] efi/x86: Drop soft_limit for x86 initrd loading
Date: Wed, 29 Apr 2020 21:05:51 +0200
Message-ID: <CAMj1kXFe0eEHnijuB3OGM2moHw0xH5viFvCiQ6Y0cLG7s=iHfg@mail.gmail.com> (raw)
In-Reply-To: <20200429174120.1497212-10-nivedita@alum.mit.edu>

On Wed, 29 Apr 2020 at 19:41, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Currently the EFI stub attempts to load initrd(s) specified on the
> command line below hdr->initrd_addr_max (2G) and if that fails, falls
> back to allocating at an unrestricted address.
>
> The only case when loading at a low address helps is for the 32-bit
> kernel, where the initrd must be copied by the kernel into lowmem if
> it's not there already. The limit specified in hdr->initrd_addr_max is
> insufficient to ensure this in any case, since lowmem by default will
> extend to about 0.9G rather than 2G, and we don't attempt to load the
> initrd in lowmem at all for the new device-path based initrd.
>
> Simplify the code by dropping this optimization for the command line
> initrd(s) as well.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

It is not really an optimization, unfortunately. Commit 47226ad4f4cfd
has the details, but in short, loading above 4 GB broke some
platforms, so below 4 GB had to remain the default.

This was 6 years ago, and so we might be able to revisit this, but
characterising it as a mere optimization is inaccurate.


> ---
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 14 +++++---------
>  drivers/firmware/efi/libstub/efi-stub.c        |  3 +--
>  drivers/firmware/efi/libstub/efistub.h         |  8 +++-----
>  drivers/firmware/efi/libstub/file.c            | 13 ++-----------
>  drivers/firmware/efi/libstub/x86-stub.c        |  3 +--
>  5 files changed, 12 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 2c0c2c34b4cc..32768fa04b32 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -378,8 +378,7 @@ static
>  efi_status_t efi_load_initrd_cmdline(efi_loaded_image_t *image,
>                                      unsigned long *load_addr,
>                                      unsigned long *load_size,
> -                                    unsigned long soft_limit,
> -                                    unsigned long hard_limit)
> +                                    unsigned long max)
>  {
>         if (!IS_ENABLED(CONFIG_EFI_GENERIC_STUB_INITRD_CMDLINE_LOADER) ||
>             (IS_ENABLED(CONFIG_X86) && (!efi_is_native() || image == NULL))) {
> @@ -388,27 +387,24 @@ efi_status_t efi_load_initrd_cmdline(efi_loaded_image_t *image,
>         }
>
>         return handle_cmdline_files(image, L"initrd=", sizeof(L"initrd=") - 2,
> -                                   soft_limit, hard_limit,
> -                                   load_addr, load_size);
> +                                   max, load_addr, load_size);
>  }
>
>  efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>                              unsigned long *load_addr,
>                              unsigned long *load_size,
> -                            unsigned long soft_limit,
> -                            unsigned long hard_limit)
> +                            unsigned long max)
>  {
>         efi_status_t status;
>
>         if (!load_addr || !load_size)
>                 return EFI_INVALID_PARAMETER;
>
> -       status = efi_load_initrd_dev_path(load_addr, load_size, hard_limit);
> +       status = efi_load_initrd_dev_path(load_addr, load_size, max);
>         if (status == EFI_SUCCESS) {
>                 pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
>         } else if (status == EFI_NOT_FOUND) {
> -               status = efi_load_initrd_cmdline(image, load_addr, load_size,
> -                                                soft_limit, hard_limit);
> +               status = efi_load_initrd_cmdline(image, load_addr, load_size, max);
>                 if (status == EFI_SUCCESS && *load_size > 0)
>                         pr_efi("Loaded initrd from command line option\n");
>         }
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> index d8f24f5c91bd..930302d9415a 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -265,8 +265,7 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
>
>         if (!efi_noinitrd) {
>                 max_addr = efi_get_max_initrd_addr(dram_base, image_addr);
> -               status = efi_load_initrd(image, &initrd_addr, &initrd_size,
> -                                        ULONG_MAX, max_addr);
> +               status = efi_load_initrd(image, &initrd_addr, &initrd_size, max_addr);
>                 if (status != EFI_SUCCESS)
>                         pr_efi_err("Failed to load initrd!\n");
>         }
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index dfdd7954bf58..1ba0887818d9 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -663,8 +663,7 @@ efi_status_t efi_setup_gop(struct screen_info *si, efi_guid_t *proto,
>  efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
>                                   const efi_char16_t *optstr,
>                                   int optstr_size,
> -                                 unsigned long soft_limit,
> -                                 unsigned long hard_limit,
> +                                 unsigned long max,
>                                   unsigned long *load_addr,
>                                   unsigned long *load_size);
>
> @@ -674,13 +673,12 @@ static inline efi_status_t efi_load_dtb(efi_loaded_image_t *image,
>                                         unsigned long *load_size)
>  {
>         return handle_cmdline_files(image, L"dtb=", sizeof(L"dtb=") - 2,
> -                                   ULONG_MAX, ULONG_MAX, load_addr, load_size);
> +                                   ULONG_MAX, load_addr, load_size);
>  }
>
>  efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>                              unsigned long *load_addr,
>                              unsigned long *load_size,
> -                            unsigned long soft_limit,
> -                            unsigned long hard_limit);
> +                            unsigned long max);
>
>  #endif
> diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
> index 50aaf15f9ad5..7dee3c5d81fb 100644
> --- a/drivers/firmware/efi/libstub/file.c
> +++ b/drivers/firmware/efi/libstub/file.c
> @@ -124,8 +124,7 @@ static int find_file_option(const efi_char16_t *cmdline, int cmdline_len,
>  efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
>                                   const efi_char16_t *optstr,
>                                   int optstr_size,
> -                                 unsigned long soft_limit,
> -                                 unsigned long hard_limit,
> +                                 unsigned long max,
>                                   unsigned long *load_addr,
>                                   unsigned long *load_size)
>  {
> @@ -181,15 +180,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
>                     round_up(alloc_size, EFI_ALLOC_ALIGN)) {
>                         unsigned long old_addr = alloc_addr;
>
> -                       status = EFI_OUT_OF_RESOURCES;
> -                       if (soft_limit < hard_limit)
> -                               status = efi_allocate_pages(alloc_size + size,
> -                                                           &alloc_addr,
> -                                                           soft_limit);
> -                       if (status == EFI_OUT_OF_RESOURCES)
> -                               status = efi_allocate_pages(alloc_size + size,
> -                                                           &alloc_addr,
> -                                                           hard_limit);
> +                       status = efi_allocate_pages(alloc_size + size, &alloc_addr, max);
>                         if (status != EFI_SUCCESS) {
>                                 pr_efi_err("Failed to allocate memory for files\n");
>                                 goto err_close_file;
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 1d3f94f1dafa..85a924fecc87 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -755,8 +755,7 @@ unsigned long efi_main(efi_handle_t handle,
>         if (!efi_noinitrd) {
>                 unsigned long addr, size;
>
> -               status = efi_load_initrd(image, &addr, &size,
> -                                        hdr->initrd_addr_max, ULONG_MAX);
> +               status = efi_load_initrd(image, &addr, &size, ULONG_MAX);
>
>                 if (status != EFI_SUCCESS) {
>                         pr_efi_err("Failed to load initrd!\n");
> --
> 2.26.2
>

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-29 17:41 [PATCH 00/10] efi: some cleanups/refactoring for efi/next Arvind Sankar
2020-04-29 17:41 ` [PATCH 01/10] efi/x86: Use correct size for boot_params Arvind Sankar
2020-04-29 17:41 ` [PATCH 02/10] efi: Add a helper function to split 64-bit values Arvind Sankar
2020-04-29 23:51   ` kbuild test robot
2020-04-29 17:41 ` [PATCH 02/10] efi/libstub: " Arvind Sankar
2020-04-29 17:41 ` [PATCH 03/10] efi/x86: Use pr_efi_err for error messages Arvind Sankar
2020-04-29 18:47   ` Joe Perches
2020-04-29 18:49     ` Ard Biesheuvel
2020-04-29 18:57       ` Joe Perches
2020-04-29 18:59         ` Ard Biesheuvel
2020-04-29 19:47           ` Joe Perches
2020-04-29 19:48             ` Ard Biesheuvel
2020-04-29 21:43       ` Arvind Sankar
2020-04-29 21:45         ` Ard Biesheuvel
2020-04-29 21:51           ` Arvind Sankar
2020-04-29 21:53         ` Joe Perches
2020-04-29 21:55           ` Ard Biesheuvel
2020-04-29 22:20             ` Arvind Sankar
2020-04-30 15:14               ` Ard Biesheuvel
2020-04-29 17:41 ` [PATCH 04/10] efi/gop: " Arvind Sankar
2020-04-29 17:41 ` [PATCH 05/10] efi/tpm: " Arvind Sankar
2020-04-29 17:41 ` [PATCH 06/10] efi/x86: Move command-line initrd loading to efi_main Arvind Sankar
2020-04-29 17:41 ` [PATCH 07/10] efi/libstub: Unify initrd loading across architectures Arvind Sankar
2020-04-29 17:41 ` [PATCH 08/10] efi/x86: Drop soft_limit for x86 initrd loading Arvind Sankar
2020-04-29 19:05   ` Ard Biesheuvel [this message]
2020-04-29 21:33     ` Arvind Sankar
2020-04-29 17:41 ` [PATCH 09/10] efi/x86: Support builtin command line Arvind Sankar
2020-04-29 19:07   ` Ard Biesheuvel
2020-04-29 21:39     ` Arvind Sankar
2020-04-29 21:40       ` Ard Biesheuvel
2020-04-29 21:48         ` Arvind Sankar
2020-04-29 21:51           ` Ard Biesheuvel
2020-04-29 17:41 ` [PATCH 10/10] efi/libstub: Check return value of efi_parse_options Arvind Sankar
2020-04-30  4:31   ` kbuild test robot
2020-04-30 18:28 ` [PATCH v2 00/11] efi: some cleanups/refactoring for efi/next Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 01/11] efi/x86: Use correct size for boot_params Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 02/11] efi/libstub: Add a helper function to split 64-bit values Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 03/11] efi/libstub: Move pr_efi/pr_efi_err into efi namespace Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 04/11] efi/x86: Use efi_err for error messages Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 05/11] efi/gop: " Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 06/11] efi/tpm: " Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 07/11] efi/libstub: Upgrade ignored dtb= argument message to error Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 08/11] efi/x86: Move command-line initrd loading to efi_main Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 09/11] efi/libstub: Unify initrd loading across architectures Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 10/11] efi/x86: Support builtin command line Arvind Sankar
2020-04-30 18:28   ` [PATCH v2 11/11] efi/libstub: Check return value of efi_parse_options Arvind Sankar
2020-04-30 19:12   ` [PATCH 1/2] efi/libstub: efi_info/efi_err message neatening Joe Perches
2020-04-30 19:12     ` [PATCH 2/2] efi/libstub: Correct comment typos Joe Perches
2020-04-30 19:30       ` Ard Biesheuvel
2020-05-04 18:29         ` [trivial PATCH] efi/libstub: Reduce efi_printk object size Joe Perches
2020-05-05  7:50           ` Ard Biesheuvel
2020-05-05  8:01             ` Joe Perches
2020-04-30 19:29     ` [PATCH 1/2] efi/libstub: efi_info/efi_err message neatening Ard Biesheuvel
2020-04-30 19:38       ` Joe Perches
2020-04-30 20:40       ` Arvind Sankar
2020-04-30 20:42         ` Ard Biesheuvel
2020-05-04  8:17   ` [PATCH v2 00/11] efi: some cleanups/refactoring for efi/next Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMj1kXFe0eEHnijuB3OGM2moHw0xH5viFvCiQ6Y0cLG7s=iHfg@mail.gmail.com' \
    --to=ardb@kernel.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nivedita@alum.mit.edu \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org
	public-inbox-index linux-efi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-efi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git