On Tue, Sep 02, 2025 at 01:56:50PM +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 <[email protected]>
> ---
> docs/grub.texi | 33 ++++
> grub-core/Makefile.core.def | 6 +
> grub-core/commands/efi/connectefi.c | 233 ++++++++++++++++++++++++++++
> grub-core/disk/efi/efidisk.c | 13 ++
> include/grub/efi/disk.h | 2 +
> 5 files changed, 287 insertions(+)
> 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 b3f71196a..b68ac267a 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..3c1d628ef
> --- /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/efi/disk.h>
> +#include <grub/command.h>
> +#include <grub/err.h>
> +#include <grub/i18n.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +struct grub_efi_handle_list
> +{
> + struct grub_efi_handle_list *next;
> + struct grub_efi_handle_list **prev;
> + grub_efi_handle_t handle;
> +};
> +typedef struct grub_efi_handle_list grub_efi_handle_list_t;
> +
> +enum search_item_flag
> +{
> + SEARCH_NONE = 0,
> + SEARCH_LOOP = 1,
> + SEARCH_RECURSIVE = 2
> +};
> +typedef enum search_item_flag search_item_flag_t;
> +
> +struct search_item
> +{
> + grub_guid_t guid;
> + const char *name;
> + search_item_flag_t flags;
> +};
> +typedef struct search_item search_item_t;
> +
> +static 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)
This function is called only once. So, I would drop it...
> +{
> + return grub_efi_system_table->boot_services->connect_controller (
> + controller_handle, driver_image_handle,
> + remaining_device_path, recursive);
> +}
> +
> +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, *n;
> +
> + FOR_LIST_ELEMENTS_SAFE(e, n, *handles_p)
Missing space before "("...
> + grub_free (e);
> +
> + *handles_p = NULL;
> +}
> +
> +static grub_err_t
> +grub_cmd_connectefi (grub_command_t cmd __attribute__ ((unused)),
> + int argc, char **args)
> +{
> + int s;
Define s together with nitems...
> + search_item_t pciroot_items[] =
> + {
> + { GRUB_EFI_PCI_ROOT_IO_GUID, "PCI root", SEARCH_RECURSIVE }
> + };
> + search_item_t disk_items[] =
> + {
> + { GRUB_EFI_PCI_ROOT_IO_GUID, "PCI root", SEARCH_NONE },
> + { GRUB_EFI_PCI_IO_GUID, "PCI", SEARCH_LOOP },
> + { GRUB_EFI_SCSI_IO_PROTOCOL_GUID, "SCSI I/O", SEARCH_RECURSIVE },
> + { GRUB_EFI_DISK_IO_GUID, "DISK I/O", SEARCH_RECURSIVE }
> + };
> + search_item_t *items = NULL;
> + int nitems = 0;
> + grub_err_t grub_err = GRUB_ERR_NONE;
s/grub_err/err/
> + bool connected_devices = false;
> + grub_efi_handle_list_t *already_handled = NULL;
> + grub_efi_handle_t *handles = 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++)
After reading code again I still think it would be better to
use i and j instead of s. More below...
> + {
> + grub_efi_uintn_t num_handles, i;
> + int loop = 0;
> + bool connected = false;
> +
> + loop:
This is not really needed. Please look below...
> + loop++;
> + if (items != NULL)
> + {
> + grub_dprintf ("efi", "step '%s' loop %d:\n", items[s].name, loop);
Drop this from here...
> + handles = grub_efi_locate_handle (GRUB_EFI_BY_PROTOCOL,
> + &items[s].guid, 0, &num_handles);
s/0/NULL/
> + }
> + else
> + handles = grub_efi_locate_handle (GRUB_EFI_ALL_HANDLES,
> + NULL, NULL, &num_handles);
Then add:
grub_dprintf ("efi", "item: %s step: %d\n", (items != NULL) ? items[s].name :
"ALL", step);
So, I think your "step" should be an "item" and your "loop" should
be a "step". Then "i" (though it is rather an index but...) instead
of "s" makes more sense...
Additionally, probably I would set an artificial limit on number of
loops/steps. 100? I would define it as a constant...
> + if (handles == NULL)
> + continue;
> +
> + for (i = 0; i < num_handles; i++)
Probably s/i/j/...
> + {
> + grub_efi_handle_t handle = handles[i];
> + grub_efi_status_t status;
> + grub_efi_handle_list_t *item;
> +
> + /* 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 &
> SEARCH_RECURSIVE ? 1 : 0);
(items == NULL || (items[s].flags & SEARCH_RECURSIVE)) ? 1 : 0
> + if (status == GRUB_EFI_SUCCESS)
> + {
> + connected = true;
s/connected/continue_search/
> + connected_devices = true;
s/connected_devices/reenumerate_disks/
> + grub_dprintf ("efi", " handle %p: connected\n", handle);
> + }
> + else
> + grub_dprintf ("efi", " handle %p: failed to connect (%"
> + PRIuGRUB_EFI_UINTN_T ")\n", handle, status);
s/PRIuGRUB_EFI_UINTN_T/PRIxGRUB_EFI_UINTN_T/...
Note "u" change to "x"... Hex is more sensible then decimal here...
> + item = grub_malloc (sizeof (*item));
> + if (item == NULL)
> + goto fail; /* fatal */
> + grub_list_push (GRUB_AS_LIST_P (&already_handled), GRUB_AS_LIST
> (item));
> + item->handle = handle;
> + }
> +
> + grub_free (handles);
> + handles = NULL;
> +
> + /*
> + * If devices got connected, new devices may be visible now, so loop to
> + * connect those
> + */
> + if (items != NULL && (items[s].flags & SEARCH_LOOP) && connected ==
> true)
> + {
> + connected = false;
> + goto loop;
do
{
continue_search = false;
...
if (status == GRUB_EFI_SUCCESS)
{
...
if (items != NULL && (items[s].flags & SEARCH_LOOP))
continue_search = true;
...
}
...
}
while (continue_search == true);
Then you do not need goto...
> + }
> +
> + /* Otherwise we are done with this step, proceed to next step */
s/step/item/
> + free_handle_list (&already_handled);
> + }
> +
> +fail:
Missing space before label...
> + grub_free (handles);
> + free_handle_list (&already_handled);
> +
> + if (connected_devices)
if (reenumerate_disks == true)
> + grub_efidisk_reenumerate_disks ();
> +
> + 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/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 3b5ed5691..4cba5b43a 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -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/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);
You can drop EXPORT_FUNC(). It is needed only when you export symbols
from the kernel...
> void grub_efidisk_init (void);
> void grub_efidisk_fini (void);
... and this two functions declarations prove that...
Hmmm... I would move grub_efidisk_reenumerate_disks() behind
grub_efidisk_fini() declaration.
Daniel
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
