Thank you for explaining. I would suggest adding some maximum number of
iterations to prevent an infinite loop if perhaps the EFI is misbehaving
inside the "loop:" section. Not sure the right limit, but there must be
some maximum allowed assuming worst case conditions or maximum allow based
on protocols.
Thanks,
Andrew
On Sat, May 31, 2025 at 4:32 AM Renaud Métrich <rmetr...@redhat.com> wrote:
> Hi Andrew,
>
> Sorry I missed your email.
>
> IMHO the "goto" is useful here to be able to loop again at same "s"
> level: we actually want to redo the operation without going one step
> further.
>
> Let me explain with an example.
>
> We want to connect PCIROOT, then PCI level, then SCSI I/O level.
>
> There is only one PCIROOT, then you can have multiple levels of PCI.
>
> Whenever you enable a PCI handle, you may have new PCI handles showing
> up, so you want to enable them through listing all the handles again and
> processing them.
>
> Only once we didn't enable some new PCI handle in the current loop, we
> are ready to enable the next level, which is SCSI I/O devices.
>
> For these devices, however, all we do is let the UEFI function enable
> them recursively.
>
> If we were letting the UEFI function enable recursively the PCI devices,
> then almost everything would be enabled and this could potentially take
> a long time.
>
> Renaud.
>
> Le 3/20/25 à 1:28 AM, Andrew Hamilton a écrit :
> > Hello,
> >
> > I just had one question about the use of the 'goto' and the 'loop'
> > label in the below code... see my question below the 'loop' label.
> > Let me know if I just misunderstand or am wrong.
> >
> > Thank you!
> > Andrew
> >
> > On Wed, Mar 19, 2025 at 7:53 AM Renaud Métrich via Grub-devel
> > <grub-devel@gnu.org> 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>
> >> ---
> >> NEWS | 5 +-
> >> grub-core/Makefile.core.def | 6 +
> >> grub-core/commands/efi/connectefi.c | 212 ++++++++++++++++++++++++++++
> >> grub-core/commands/efi/lsefi.c | 3 +-
> >> grub-core/disk/efi/efidisk.c | 13 ++
> >> grub-core/kern/efi/efi.c | 13 ++
> >> include/grub/efi/api.h | 2 +-
> >> include/grub/efi/disk.h | 2 +
> >> include/grub/efi/efi.h | 5 +
> >> 9 files changed, 258 insertions(+), 3 deletions(-)
> >> create mode 100644 grub-core/commands/efi/connectefi.c
> >>
> >> diff --git a/NEWS b/NEWS
> >> index 310130962..48d9dd4dd 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -32,6 +32,9 @@ New in 2.06:
> >> * BootHole and BootHole2 fixes.
> >> * ...and tons of other fixes and cleanups...
> >>
> >> +* New/improved platform support:
> >> + * New `connectefi' command on EFI platforms.
> >> +
> >> New in 2.04:
> >>
> >> * GCC 8 and 9 support.
> >> @@ -118,7 +121,7 @@ New in 2.02:
> >> * Prefer pmtimer for TSC calibration.
> >>
> >> * New/improved platform support:
> >> - * New `efifwsetup' and `lsefi' commands on EFI platforms.
> >> + * New `efifwsetup', `lsefi' commands on EFI platforms.
> >> * New `cmosdump' and `cmosset' commands on platforms with CMOS
> support.
> >> * New command `pcidump' for PCI platforms.
> >> * Improve opcode parsing in ACPI halt implementation.
> >> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> >> index f70e02e69..55e43ea88 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..5049c2abe
> >> --- /dev/null
> >> +++ b/grub-core/commands/efi/connectefi.c
> >> @@ -0,0 +1,212 @@
> >> +/*
> >> + * GRUB -- GRand Unified Bootloader
> >> + * Copyright (C) 2022 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+");
> >> +
> >> +struct grub_efi_already_handled
> >> +{
> >> + struct grub_efi_already_handled *next;
> >> + struct grub_efi_already_handled **prev;
> >> + grub_efi_handle_t handle;
> >> +};
> >> +
> >> +static struct grub_efi_already_handled *already_handled;
> >> +
> >> +static struct grub_efi_already_handled *
> >> +is_in_list (grub_efi_handle_t handle)
> >> +{
> >> + struct grub_efi_already_handled *e;
> >> +
> >> + FOR_LIST_ELEMENTS (e, already_handled)
> >> + if (e->handle == handle)
> >> + return e;
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +static void
> >> +free_handle_list (void)
> >> +{
> >> + struct grub_efi_already_handled *e;
> >> + while ((e = already_handled) != NULL)
> >> + {
> >> + already_handled = already_handled->next;
> >> + grub_free (e);
> >> + }
> >> +}
> >> +
> >> +typedef enum searched_item_flag
> >> +{
> >> + SEARCHED_ITEM_FLAG_LOOP = 1,
> >> + SEARCHED_ITEM_FLAG_RECURSIVE = 2
> >> +} searched_item_flags;
> >> +
> >> +typedef struct searched_item
> >> +{
> >> + grub_efi_guid_t guid;
> >> + const char *name;
> >> + searched_item_flags flags;
> >> +} searched_items;
> >> +
> >> +static grub_err_t
> >> +grub_cmd_connectefi (grub_command_t cmd __attribute__ ((unused)),
> >> + int argc, char **args)
> >> +{
> >> + unsigned s;
> >> + searched_items pciroot_items[] =
> >> + {
> >> + { GRUB_EFI_PCI_ROOT_IO_GUID, "PCI root",
> SEARCHED_ITEM_FLAG_RECURSIVE }
> >> + };
> >> + searched_items 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_items *items = NULL;
> >> + unsigned nitems = 0;
> >> + grub_err_t grub_err = GRUB_ERR_NONE;
> >> + unsigned total_connected = 0;
> >> +
> >> + 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], N_("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;
> >> + unsigned i, connected = 0, loop = 0;
> >> +
> >> +loop:
> > In the section later that uses 'goto' to get back here, is there a
> > potential this code will enter an infinite loop since I assume the
> > increment of 's' is being skipped by the 'goto'? Any reason the
> > 'goto' later in the code can't use a 'continue'?
> >
> >> + loop++;
> >> + 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
> >> + handles = grub_efi_locate_handle (GRUB_EFI_ALL_HANDLES,
> >> + NULL, NULL, &num_handles);
> >> +
> >> + if (!handles)
> >> + continue;
> >> +
> >> + for (i = 0; i < num_handles; i++)
> >> + {
> >> + grub_efi_handle_t handle = handles[i];
> >> + grub_efi_status_t status;
> >> + unsigned j;
> >> +
> >> + /* Skip already handled handles */
> >> + if (is_in_list (handle) != NULL)
> >> + {
> >> + 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);
> >> + if (status == GRUB_EFI_SUCCESS)
> >> + {
> >> + connected++;
> >> + total_connected++;
> >> + grub_dprintf ("efi", " handle %p: connected\n", handle);
> >> + }
> >> + else
> >> + grub_dprintf ("efi", " handle %p: failed to connect
> (%d)\n",
> >> + handle, (grub_efi_int8_t) status);
> >> +
> >> + struct grub_efi_already_handled *item = grub_malloc(sizeof
> (*item));
> >> + if (!item)
> >> + break; /* fatal */
> >> + 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)
> >> + {
> >> + connected = 0;
> >> + goto loop;
> >> + }
> >> +
> >> + free_handle_list ();
> >> + }
> >> +
> >> + free_handle_list ();
> >> +
> >> + if (total_connected)
> >> + grub_efidisk_reenumerate_disks ();
> >> +
> >> + return grub_err;
> >> +}
> >> +
> >> +static grub_command_t cmd;
> >> +
> >> +GRUB_MOD_INIT(connectefi)
> >> +{
> >> + cmd = grub_register_command ("connectefi", grub_cmd_connectefi,
> >> + /* TRANSLATORS: only translate 'all'
> keyword */
> >> + N_("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..8a82ababf 100644
> >> --- a/grub-core/commands/efi/lsefi.c
> >> +++ b/grub-core/commands/efi/lsefi.c
> >> @@ -19,6 +19,7 @@
> >> #include <grub/mm.h>
> >> #include <grub/misc.h>
> >> #include <grub/efi/api.h>
> >> +#include <grub/efi/disk.h>
> >> #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" },
> >> { 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..525d59838 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 = 0;
> >> + hd_devices = 0;
> >> + cd_devices = 0;
> >> +
> >> + 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..b7c861b1e 100644
> >> --- a/grub-core/kern/efi/efi.c
> >> +++ b/grub-core/kern/efi/efi.c
> >> @@ -96,6 +96,19 @@ grub_efi_locate_handle
> (grub_efi_locate_search_type_t search_type,
> >> return buffer;
> >> }
> >>
> >> +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);
> >> +}
> >> +
> >> void *
> >> grub_efi_open_protocol (grub_efi_handle_t handle,
> >> grub_guid_t *protocol,
> >> 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 \
> >> { 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);
> >> +
> >> void grub_efidisk_init (void);
> >> void grub_efidisk_fini (void);
> >>
> >> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> >> index a5cd99e5a..60d0b2bf8 100644
> >> --- a/include/grub/efi/efi.h
> >> +++ b/include/grub/efi/efi.h
> >> @@ -44,6 +44,11 @@ EXPORT_FUNC(grub_efi_locate_handle)
> (grub_efi_locate_search_type_t search_type,
> >> grub_guid_t *protocol,
> >> void *search_key,
> >> grub_efi_uintn_t *num_handles);
> >> +grub_efi_status_t
> >> +EXPORT_FUNC(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);
> >> void *EXPORT_FUNC(grub_efi_open_protocol) (grub_efi_handle_t handle,
> >> grub_guid_t *protocol,
> >> grub_efi_uint32_t
> attributes);
> >> --
> >> 2.48.1
> >>
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> Grub-devel@gnu.org
> >> https://lists.gnu.org/mailman/listinfo/grub-devel
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
