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