On Wed, Mar 19, 2025 at 01:47:57PM +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.
> 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)
This has to be documented in the GRUB docs.
> 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
Please drop NEWS file change.
> 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.
s/2022/2025/
> + *
> + * 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/>.
> + */
Please add an empty line here.
> +#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;
This probably does not need to be global.
> +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;
Please add an empty line here.
> + while ((e = already_handled) != NULL)
> + {
> + already_handled = already_handled->next;
> + grub_free (e);
> + }
> +}
> +
> +typedef enum searched_item_flag
enum searched_item_flag
> +{
> + SEARCHED_ITEM_FLAG_LOOP = 1,
> + SEARCHED_ITEM_FLAG_RECURSIVE = 2
> +} searched_item_flags;
typedef enum searched_item_flag searched_item_flag_t;
Please fix similar issues everywhere.
> +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;
s/unsigned/unsigned int/
And below please...
> + 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;
Probably an int is good enough here.
> + grub_err_t grub_err = GRUB_ERR_NONE;
> + unsigned total_connected = 0;
Ditto.
> + 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)
Please drop N_()...
> + {
> + items = NULL;
> + nitems = 1;
> + }
> + else
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("unexpected argument `%s'"), args[0]);
unexpected argument: %s
> +
> + for (s = 0; s < nitems; s++)
> + {
> + grub_efi_handle_t *handles;
> + grub_efi_uintn_t num_handles;
> + unsigned i, connected = 0, loop = 0;
> +
> +loop:
> + 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,
Wrong coding style. Missing space before "(".
I can see similar issues in other places too.
> + !items || items[s].flags & SEARCHED_ITEM_FLAG_RECURSIVE
> ? 1 : 0);
> + if (status == GRUB_EFI_SUCCESS)
> + {
> + connected++;
> + total_connected++;
What is a difference between connected and total_connected?
I think both could be bool. And then probably one could be dropped...
> + grub_dprintf ("efi", " handle %p: connected\n", handle);
> + }
> + else
> + grub_dprintf ("efi", " handle %p: failed to connect (%d)\n",
Please use PRIuGRUB_EFI_UINTN_T instead of %d and...
> + handle, (grub_efi_int8_t) status);
... drop grub_efi_int8_t cast...
> + struct grub_efi_already_handled *item = grub_malloc(sizeof (*item));
Please do not mix code with variable definitions.
> + if (!item)
if (item == NULL)
Please check for NULL explicitly.
> + 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;
I think it is possible to not use goto for the loop here.
> + }
> +
> + 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 */
Do not translate arguments for commands. It will only make confusion...
> + 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" },
Why? This change begs for separate patch with good explanation why the
change is needed.
> { 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;
Please use NULL instead of 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);
> +}
Please make this function part of the connectefi module.
> 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 \
This change together with lsefi changes should go to separate patch...
But first of all, are they really needed?
> { 0xce345171, 0xba0b, 0x11d2, \
> { 0x8e, 0x4f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b } \
> }
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
