On Tue, Jun 10, 2025 at 11:54:25AM +0200, Renaud Métrich via Grub-devel wrote:
> When network booting is used, trying to chainload to the local disk
> (which is used in deployment tools such as Red Hat Satellite) may fail
> when searching for the boot loader, e.g. /EFI/redhat/shimx64.efi:
> the boot loader file is listed, but not readable, because UEFI DISK I/O
> and/or SCSI DISK I/O devices are not connected.
> Sometimes devices are connected but not the partition of the disk
> matching $prefix, causing partition to not be found by"chainloader".
>
> This issue was seen already on:
> - VMWare systems with efi.quickboot enabled (which is the default)
> - QEMU/KVM when network booting and not enabling any disk
> - Physical hardware such as Cisco and HPE systems
>
> Typical chainload configuration used:
> -------- 8< ---------------- 8< ---------------- 8< --------
> unset prefix
> search --file --set=prefix /EFI/redhat/shimx64.efi
> if [ -n "$prefix" ]; then
>   chainloader ($prefix)/EFI/redhat/shimx64.efi
> ...
> -------- 8< ---------------- 8< ---------------- 8< --------
>
> This patch introduces a new "connectefi pciroot|disk|all" command which
> recursively connects all EFI devices starting from a given controller
> type:
> - if 'pciroot' is specified, recursion is performed for all PCI root
>   handles
> - if 'disk' is specified, recursion is performed for all SCSI and DISK
>   I/O handles (recommended usage to avoid connecting unwanted handles
>   which may impact Grub performances)
> - if 'all' is specified, recursion is performed on all handles (not
>   recommended since it may heavily impact Grub performances)
>
> Typical grub.cfg snippet would then be:
> -------- 8< ---------------- 8< ---------------- 8< --------
> connectefi disk
> unset prefix
> search --file --set=prefix /EFI/redhat/shimx64.efi
> if [ -n "$prefix" ]; then
>   chainloader ($prefix)/EFI/redhat/shimx64.efi
> ...
> -------- 8< ---------------- 8< ---------------- 8< --------
>
> The code is easily extensible to handle other arguments in the future if
> needed.
>
> Signed-off-by: Renaud Métrich <rmetr...@redhat.com>
> ---
>  docs/grub.texi                      |  33 ++++
>  grub-core/Makefile.core.def         |   6 +
>  grub-core/commands/efi/connectefi.c | 233 ++++++++++++++++++++++++++++
>  grub-core/commands/efi/lsefi.c      |   5 +-
>  grub-core/disk/efi/efidisk.c        |  15 +-
>  grub-core/kern/efi/efi.c            |   2 +-
>  include/grub/efi/api.h              |   2 +-
>  include/grub/efi/disk.h             |   2 +
>  8 files changed, 293 insertions(+), 5 deletions(-)
>  create mode 100644 grub-core/commands/efi/connectefi.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 34b3484dc..3b8b05fca 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3918,6 +3918,7 @@ Modules can be loaded via the @command{insmod} 
> (@pxref{insmod}) command.
>  * cmp_module::
>  * cmp_test_module::
>  * configfile_module::
> +* connectefi_module::
>  * cpio_module::
>  * cpio_be_module::
>  * cpuid_module::
> @@ -4413,6 +4414,14 @@ This module provides support for the commands: 
> @command{configfile},
>  @command{extract_entries_configfile}, @command{.} (dot command).
>  @xref{configfile} / @pxref{source}.
>
> +@node connectefi_module
> +@section connectefi
> +This module provides support for the @command{connectefi} command. This 
> command
> +can be used on EFI platforms to connect SCSI I/O and DISK I/O EFI handles,
> +which may be required when chainloading to the disk after booting from the
> +network, or from one disk to another for example.
> +@pxref{chainloader} / @pxref{connectefi} for more information.
> +
>  @node cpio_module
>  @section cpio
>  This module provides support for the CPIO archive file format. This module is
> @@ -6147,6 +6156,11 @@ image.
>  If you specify the option @option{--force}, then load @var{file} forcibly,
>  whether it has a correct signature or not. This is required when you want to
>  load a defective boot loader, such as SCO UnixWare 7.1.
> +
> +On EFI platforms, when initially network booting, or booting from another 
> disk,
> +it may be required to connect the EFI devices using @command{connectefi}
> +command (@pxref{connectefi}) prior to chainloading to the disk, otherwise no
> +disk may show up or the EFI partition on the disk may not be readable.
>  @end deffn
>
>
> @@ -6715,6 +6729,25 @@ after @command{configfile} returns.
>  @end deffn
>
>
> +@node connectefi
> +@subsection connectefi
> +
> +@deffn Command connectefi pciroot|disk|all
> +When used with @samp{pciroot} parameter, all PCI devices are recursively
> +connected, which may be a slow operation, depending on the hardware.
> +
> +When used with @samp{disk} parameter, only SCSI I/O and DISK I/O devices are
> +connected; this is the recommended usage when chainloading to internal disk,
> +unless some disks are not detected anyway, in such case @samp{all} may be 
> used
> +and a bug be reported to enhance the implementation.
> +
> +When used with @samp{all} parameter, all EFI devices are recursively 
> connected,
> +which may be an extremely slow operation, depending on the hardware.
> +
> +Note: This command is only available on EFI platforms.
> +@end deffn
> +
> +
>  @node cpuid
>  @subsection cpuid
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 24e8c8437..dfbd6c221 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -840,6 +840,12 @@ module = {
>    enable = efi;
>  };
>
> +module = {
> +  name = connectefi;
> +  efi = commands/efi/connectefi.c;
> +  enable = efi;
> +};
> +
>  module = {
>    name = blocklist;
>    common = commands/blocklist.c;
> diff --git a/grub-core/commands/efi/connectefi.c 
> b/grub-core/commands/efi/connectefi.c
> new file mode 100644
> index 000000000..78c58194b
> --- /dev/null
> +++ b/grub-core/commands/efi/connectefi.c
> @@ -0,0 +1,233 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2025  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/types.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +#include <grub/efi/api.h>
> +#include <grub/efi/pci.h>
> +#include <grub/efi/efi.h>
> +#include <grub/command.h>
> +#include <grub/err.h>
> +#include <grub/i18n.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +grub_efi_status_t
> +grub_efi_connect_controller (grub_efi_handle_t controller_handle,
> +                          grub_efi_handle_t *driver_image_handle,
> +                          grub_efi_device_path_protocol_t 
> *remaining_device_path,
> +                          grub_efi_boolean_t recursive)
> +{
> +  grub_efi_boot_services_t *b;
> +
> +  b = grub_efi_system_table->boot_services;
> +  return efi_call_4 (b->connect_controller, controller_handle,
> +                  driver_image_handle, remaining_device_path, recursive);
> +}
> +
> +struct grub_efi_handle_list
> +{
> +  struct grub_efi_handle_list *next;
> +  struct grub_efi_handle_list **prev;
> +  grub_efi_handle_t handle;
> +};
> +

Please drop this empty line.

> +typedef struct grub_efi_handle_list grub_efi_handle_list_t;

Please define all constants, types, global variables, etc. at the
beginning of the file.

> +static bool
> +is_in_list (grub_efi_handle_t handle, grub_efi_handle_list_t *handles)
> +{
> +  grub_efi_handle_list_t *e;
> +
> +  FOR_LIST_ELEMENTS (e, handles)
> +    if (e->handle == handle)
> +      return true;
> +
> +  return false;
> +}
> +
> +static void
> +free_handle_list (grub_efi_handle_list_t **handles_p)
> +{
> +  grub_efi_handle_list_t *e;
> +
> +  while ((e = *handles_p) != NULL)

Could not you use FOR_LIST_ELEMENTS() here? If not please add a comment
explaining why it is not possible.

> +    {
> +      *handles_p = e->next;
> +      grub_free (e);
> +    }
> +}
> +
> +enum searched_item_flag

s/searched_item_flag/search_item_flag/

In general I would s/searched/search/ in all types names.

> +{
> +  SEARCHED_ITEM_FLAG_LOOP = 1,
> +  SEARCHED_ITEM_FLAG_RECURSIVE = 2

s/SEARCHED/SEARCH/ for both...

SEARCH_NONE = 0,
SEARCH_LOOP = 1,
SEARCH_RECURSIVE

> +};
> +

Please drop this empty line...

> +typedef enum searched_item_flag searched_item_flag_t;

... and move this enum/type definition to the beginning of the file.

> +struct searched_item

s/searched_item/search_item/ and similar changes below please...

> +{
> +  grub_efi_guid_t guid;
> +  const char *name;
> +  searched_item_flag_t flags;
> +};
> +
> +typedef struct searched_item searched_item_t;

Ditto.

> +static grub_err_t
> +grub_cmd_connectefi (grub_command_t cmd __attribute__ ((unused)),
> +                  int argc, char **args)
> +{
> +  int s;
> +  searched_item_t pciroot_items[] =
> +    {
> +      { GRUB_EFI_PCI_ROOT_IO_GUID, "PCI root", SEARCHED_ITEM_FLAG_RECURSIVE }
> +    };
> +  searched_item_t disk_items[] =
> +    {
> +      { GRUB_EFI_PCI_ROOT_IO_GUID, "PCI root", 0 },
> +      { GRUB_EFI_PCI_IO_GUID, "PCI", SEARCHED_ITEM_FLAG_LOOP },
> +      { GRUB_EFI_SCSI_IO_PROTOCOL_GUID, "SCSI I/O", 
> SEARCHED_ITEM_FLAG_RECURSIVE },
> +      { GRUB_EFI_DISK_IO_PROTOCOL_GUID, "DISK I/O", 
> SEARCHED_ITEM_FLAG_RECURSIVE }
> +    };
> +  searched_item_t *items = NULL;
> +  int nitems = 0;

If you add SEARCH_NONE as above and add last element with empty name and
SEARCH_NONE flag then you probably do not need nitems (guid can be
anything but it would be nice to find one which can be used as NONE).

> +  grub_err_t grub_err = GRUB_ERR_NONE;
> +  bool connected_devices = false;

s/connected_devices/reenumerate_disks/

> +  grub_efi_handle_list_t *already_handled = NULL;
> +
> +  if (argc != 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
> +
> +  if (grub_strcmp (args[0], "pciroot") == 0)
> +    {
> +      items = pciroot_items;
> +      nitems = ARRAY_SIZE (pciroot_items);
> +    }
> +  else if (grub_strcmp (args[0], "disk") == 0)
> +    {
> +      items = disk_items;
> +      nitems = ARRAY_SIZE (disk_items);
> +    }
> +  else if (grub_strcmp (args[0], "all") == 0)
> +    {
> +      items = NULL;
> +      nitems = 1;
> +    }
> +  else
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("unexpected argument: `%s'"), args[0]);
> +
> +  for (s = 0; s < nitems; s++)
> +    {
> +      grub_efi_handle_t *handles;
> +      grub_efi_uintn_t num_handles;
> +      int i, loop = 0;

Could you use "i" and "j" instead of "i" and "s"?
If yes define both at the beginning of the function.

And for what do you use "loop" variable?

> +      bool connected = false;
> +
> +loop:
> +      loop++;

Even if it works it does not parse well...

> +      if (items != NULL)
> +     {
> +       grub_dprintf ("efi", "step '%s' loop %d:\n", items[s].name, loop);
> +       handles = grub_efi_locate_handle (GRUB_EFI_BY_PROTOCOL,
> +                                         &items[s].guid, 0, &num_handles);
> +     }
> +      else

If you have grub_dprintf() above I would add one here too.

> +     handles = grub_efi_locate_handle (GRUB_EFI_ALL_HANDLES,
> +                                       NULL, NULL, &num_handles);
> +
> +      if (!handles)

if (handles == NULL)

> +     continue;
> +
> +      for (i = 0; i < num_handles; i++)
> +     {
> +       grub_efi_handle_t handle = handles[i];
> +       grub_efi_status_t status;
> +       int j;
> +
> +       /* Skip already handled handles  */
> +       if (is_in_list (handle, already_handled))
> +         {
> +           grub_dprintf ("efi", "  handle %p: already processed\n",
> +                                handle);
> +           continue;
> +         }
> +
> +       status = grub_efi_connect_controller (handle, NULL, NULL,
> +                     !items || items[s].flags & SEARCHED_ITEM_FLAG_RECURSIVE 
> ? 1 : 0);

(items == NULL || items[s].flags & SEARCHED_ITEM_FLAG_RECURSIVE) ? 1 : 0

Do not we have a constant for 1 and 0 defined for
grub_efi_connect_controller() call somewhere?

> +       if (status == GRUB_EFI_SUCCESS)
> +         {
> +           connected = true;
> +           connected_devices = true;
> +           grub_dprintf ("efi", "  handle %p: connected\n", handle);
> +         }
> +       else
> +         grub_dprintf ("efi", "  handle %p: failed to connect ("
> +                       PRIuGRUB_EFI_UINTN_T ")\n", handle, status);
> +
> +       grub_efi_handle_list_t *item;

Please move this to the beginning of the block...

> +       item = grub_malloc (sizeof (*item));
> +       if (item == NULL)
> +         break; /* fatal  */

goto fail;

> +       grub_list_push (GRUB_AS_LIST_P (&already_handled), GRUB_AS_LIST 
> (item));
> +       item->handle = handle;
> +     }
> +
> +      grub_free (handles);
> +      if (grub_err != GRUB_ERR_NONE)
> +     break; /* fatal  */
> +
> +      if (items && items[s].flags & SEARCHED_ITEM_FLAG_LOOP && connected)

This if begs for comment. What happens here? And...

  if (items != NULL && (items[s].flags & SEARCHED_ITEM_FLAG_LOOP) && connected 
== true)

> +     {
> +       connected = false;
> +       goto loop;
> +     }
> +
> +      free_handle_list (&already_handled);

This probably can be dropped. Then "goto loop" too...

> +    }
> +
> +  free_handle_list (&already_handled);
> +
> +  if (connected_devices)

if (connected_devices == true)

> +    grub_efidisk_reenumerate_disks ();

Note space before "fail" label...

 fail:
   grub_free (handles);
   free_handle_list (&already_handled);

And then probably your code would be much simpler. However, you have to
initialize "handles" to NULL at the beginning of the function.

> +  return grub_err;
> +}
> +
> +static grub_command_t cmd;
> +
> +GRUB_MOD_INIT(connectefi)
> +{
> +  cmd = grub_register_command ("connectefi", grub_cmd_connectefi,
> +                            "pciroot|disk|all",
> +                            N_("Connect EFI handles."
> +                               " If 'pciroot' is specified, connect PCI"
> +                               " root EFI handles recursively."
> +                               " If 'disk' is specified, connect SCSI"
> +                               " and DISK I/O EFI handles recursively."
> +                               " If 'all' is specified, connect all"
> +                               " EFI handles recursively."));
> +}
> +
> +GRUB_MOD_FINI(connectefi)
> +{
> +  grub_unregister_command (cmd);
> +}
> diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
> index bda25a3a9..ec845b7fd 100644
> --- a/grub-core/commands/efi/lsefi.c
> +++ b/grub-core/commands/efi/lsefi.c
> @@ -1,6 +1,6 @@
>  /*
>   *  GRUB  --  GRand Unified Bootloader
> - *  Copyright (C) 2012  Free Software Foundation, Inc.
> + *  Copyright (C) 2025  Free Software Foundation, Inc.

Please do not overwrite years in copyright. Add them if you really need
that. Though I think you do not need update them in this case.

>   *  GRUB is free software: you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by
> @@ -19,6 +19,7 @@
>  #include <grub/mm.h>
>  #include <grub/misc.h>
>  #include <grub/efi/api.h>
> +#include <grub/efi/disk.h>

Is it really needed?

>  #include <grub/efi/edid.h>
>  #include <grub/efi/pci.h>
>  #include <grub/efi/efi.h>
> @@ -35,7 +36,7 @@ static struct known_protocol
>    const char *name;
>  } known_protocols[] =
>    {
> -    { GRUB_EFI_DISK_IO_GUID, "disk" },
> +    { GRUB_EFI_DISK_IO_PROTOCOL_GUID, "disk" },

Again, I am OK with this change but it has to be in separate patch.

>      { GRUB_EFI_BLOCK_IO_GUID, "block" },
>      { GRUB_EFI_SERIAL_IO_GUID, "serial" },
>      { GRUB_EFI_SIMPLE_NETWORK_GUID, "network" },
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 3b5ed5691..c166fb01e 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -1,6 +1,6 @@
>  /*
>   *  GRUB  --  GRand Unified Bootloader
> - *  Copyright (C) 2006,2007,2008  Free Software Foundation, Inc.
> + *  Copyright (C) 2025  Free Software Foundation, Inc.

Please take a look at my comment WRT copyright above.

>   *  GRUB is free software: you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by
> @@ -396,6 +396,19 @@ enumerate_disks (void)
>    free_devices (devices);
>  }
>
> +void
> +grub_efidisk_reenumerate_disks (void)
> +{
> +  free_devices (fd_devices);
> +  free_devices (hd_devices);
> +  free_devices (cd_devices);
> +  fd_devices = NULL;
> +  hd_devices = NULL;
> +  cd_devices = NULL;
> +
> +  enumerate_disks ();
> +}
> +
>  static int
>  grub_efidisk_iterate (grub_disk_dev_iterate_hook_t hook, void *hook_data,
>                     grub_disk_pull_t pull)
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index b93ae3aba..faf293da7 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -1,7 +1,7 @@
>  /* efi.c - generic EFI support */
>  /*
>   *  GRUB  --  GRand Unified Bootloader
> - *  Copyright (C) 2006,2007,2008,2009,2010  Free Software Foundation, Inc.
> + *  Copyright (C) 2025  Free Software Foundation, Inc.

Ditto.

>   *
>   *  GRUB is free software: you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index b686e8afe..d73a10c28 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -79,7 +79,7 @@
>      { 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b } \
>    }
>
> -#define GRUB_EFI_DISK_IO_GUID        \
> +#define GRUB_EFI_DISK_IO_PROTOCOL_GUID       \

Again, separate patch for this please...

>    { 0xce345171, 0xba0b, 0x11d2, \
>      { 0x8e, 0x4f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b } \
>    }
> diff --git a/include/grub/efi/disk.h b/include/grub/efi/disk.h
> index 254475c84..6845c2f1f 100644
> --- a/include/grub/efi/disk.h
> +++ b/include/grub/efi/disk.h
> @@ -27,6 +27,8 @@ grub_efi_handle_t
>  EXPORT_FUNC(grub_efidisk_get_device_handle) (grub_disk_t disk);
>  char *EXPORT_FUNC(grub_efidisk_get_device_name) (grub_efi_handle_t *handle);
>
> +void EXPORT_FUNC(grub_efidisk_reenumerate_disks) (void);

Please drop EXPORT_FUNC from here.

>  void grub_efidisk_init (void);
>  void grub_efidisk_fini (void);

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Reply via email to