Sorry for late reply... I was taking care of the release... On Wed, Nov 26, 2025 at 03:34:58PM +0100, 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.
Could you explain why file is listed if the 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". s/by"chainloader"/by "chainloader" command/ > 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 Could you copy description from the GRUB documentation changes below? I like it more then the one here. > - 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) Ditto and below... > - if 'all' is specified, recursion is performed on all handles (not > recommended since it may heavily impact Grub performances) s/Grub/GRUB/ The project name is "GRUB". Please fix it everywhere... > 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 | 227 ++++++++++++++++++++++++++++ > grub-core/disk/efi/efidisk.c | 13 ++ > include/grub/efi/disk.h | 2 + > 5 files changed, 281 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 s/chainloading to/chainloading from/ > +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, s/network booting/booting from network/ > +it may be required to connect the EFI devices using @command{connectefi} > +command (@pxref{connectefi}) prior to chainloading to the disk, otherwise no s/chainloading to the disk/chainloading from the disk/ > +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..61a3d0efb > --- /dev/null > +++ b/grub-core/commands/efi/connectefi.c > @@ -0,0 +1,227 @@ > +/* > + * GRUB -- GRand Unified Bootloader > + * Copyright (C) 2025 Free Software Foundation, Inc. s/2025/2025,2026/ > + * 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 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) > + grub_free (e); > + > + *handles_p = NULL; > +} > + > +static grub_err_t > +grub_cmd_connectefi (grub_command_t cmd __attribute__ ((unused)), > + int argc, char **args) > +{ > + 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 i, nitems = 0; > + grub_err_t err = GRUB_ERR_NONE; > + bool reenumerate_disks = 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 (i = 0; i < nitems; i++) > + { > + grub_efi_uintn_t num_handles, j; > + int loop = 0; > + const int max_loop_depth = 100; Please define MAX_SEARCH_LOOPS after GRUB_MOD_LICENSE() instead of max_loop_depth here. > + bool continue_search; > + > + do > + { > + continue_search = false; I would add an empty line here. > + grub_dprintf ("efi", "item: %s loop: %d\n", (items != NULL) ? > items[i].name: "ALL", loop); Ditto... > + if (items != NULL) > + { > + if (++loop >= max_loop_depth) > + { > + err = grub_error (GRUB_ERR_RECURSION_DEPTH, > + "connectefi: Maximum recursion depth exceeded for item > '%s'", > + items[i].name); Even if it is correct to some extent I would use "Maximum loops exceeded..." instead to align the error message with the code. And s/GRUB_ERR_RECURSION_DEPTH/GRUB_ERR_OUT_OF_RANGE/... > + goto fail; Did you consider not failing but continuing with partial set of devices? Or maybe we should have an option to ignore such failures? > + } > + handles = grub_efi_locate_handle (GRUB_EFI_BY_PROTOCOL, > + &items[i].guid, NULL, > &num_handles); > + } > + else > + handles = grub_efi_locate_handle (GRUB_EFI_ALL_HANDLES, > + NULL, NULL, &num_handles); > + > + if (handles == NULL) > + break; /* proceed to next item */ s/proceed to next item/Proceed to next item./ Please fix similar things in he other places too. > + for (j = 0; j < num_handles; j++) > + { > + grub_efi_handle_t handle = handles[j]; > + 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); I am not sure why you make an indention before the "handle %p:...". I would drop it from here and there... > + continue; > + } > + > + status = grub_efi_system_table->boot_services->connect_controller > ( > + handle, NULL, NULL, > + (items == NULL) || items[i].flags & SEARCH_RECURSIVE ? > 1 : 0); > + if (status == GRUB_EFI_SUCCESS) > + { > + if (items != NULL && (items[i].flags & SEARCH_LOOP)) > + continue_search = true; > + reenumerate_disks = true; > + grub_dprintf ("efi", " handle %p: connected\n", handle); Ditto and below... > + } > + else > + grub_dprintf ("efi", " handle %p: failed to connect (%" > + PRIxGRUB_EFI_UINTN_T ")\n", handle, status); > + > + item = grub_malloc (sizeof (*item)); > + if (item == NULL) > + { > + err = grub_errno; > + goto fail; /* fatal */ > + } > + grub_list_push (GRUB_AS_LIST_P (&already_handled), GRUB_AS_LIST > (item)); > + item->handle = handle; > + } > + > + grub_free (handles); > + handles = NULL; > + } > + while (continue_search == true); > + > + /* We are done with this item, proceed to next one */ > + free_handle_list (&already_handled); > + } > + > + fail: > + grub_free (handles); > + free_handle_list (&already_handled); > + > + if (reenumerate_disks) if (reenumerate_disks == true) > + grub_efidisk_reenumerate_disks (); > + > + return err; > +} Now code looks much better. Thank you for taking my previous comments into the account. I am looking forward for next version of the patch. Please rebase it on latest master... Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
