On Wed, Jul 9, 2025 at 11:30 AM Ross Lagerwall
<ross.lagerw...@citrix.com> wrote:
>
> On Tue, Jul 8, 2025 at 9:02 PM Frediano Ziglio via Grub-devel
> <grub-devel@gnu.org> wrote:
> >
> > Allows to load modules using LoadFile2 protocol.
> > Add and use a new GUID for kernel media device.
> > This will allow Xen to pick up additional modules using
> > EFI interface instead of using multiboot2 interface (not
> > available on x86_64).
> >
> > Signed-off-by: Frediano Ziglio <frediano.zig...@cloud.com>
> > ---
> > grub-core/loader/arm64/xen_boot.c | 172 +++++++++++++++++++++++++++++-
> > include/grub/efi/api.h | 5 +
> > 2 files changed, 176 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/arm64/xen_boot.c
> > b/grub-core/loader/arm64/xen_boot.c
> > index 2975a546e..64f685de6 100644
> > --- a/grub-core/loader/arm64/xen_boot.c
> > +++ b/grub-core/loader/arm64/xen_boot.c
> > @@ -68,6 +68,56 @@ enum module_type
> > };
> > typedef enum module_type module_type_t;
> >
> > +static grub_guid_t load_file2_guid = GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID;
> > +static grub_guid_t device_path_guid = GRUB_EFI_DEVICE_PATH_GUID;
> > +
> > +static initrd_media_device_path_t initrd_lf2_device_path = {
> > + {
> > + {
> > + GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
> > + GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
> > + sizeof(grub_efi_vendor_media_device_path_t),
> > + },
> > + LINUX_EFI_INITRD_MEDIA_GUID
> > + }, {
> > + GRUB_EFI_END_DEVICE_PATH_TYPE,
> > + GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > + sizeof(grub_efi_device_path_t)
> > + }
> > +};
> > +
> > +static initrd_media_device_path_t kernel_lf2_device_path = {
> > + {
> > + {
> > + GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
> > + GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
> > + sizeof(grub_efi_vendor_media_device_path_t),
> > + },
> > + XEN_EFI_KERNEL_MEDIA_GUID
> > + }, {
> > + GRUB_EFI_END_DEVICE_PATH_TYPE,
> > + GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > + sizeof(grub_efi_device_path_t)
> > + }
> > +};
> > +
> > +typedef struct lf2
> > +{
> > + grub_efi_load_file2_t lf2;
> > + grub_efi_handle_t handle;
> > + initrd_media_device_path_t *media;
> > +} lf2_t;
> > +
> > +static grub_efi_status_t __grub_efi_api
> > +grub_efi_load_file2 (grub_efi_load_file2_t *this,
> > + grub_efi_device_path_t *device_path,
> > + grub_efi_boolean_t boot_policy,
> > + grub_efi_uintn_t *buffer_size,
> > + void *buffer);
> > +static void lf2_init(lf2_t *lf2, initrd_media_device_path_t *media);
> > +static grub_err_t lf2_start(lf2_t *lf2);
> > +static void lf2_done(lf2_t *lf2);
> > +
> > struct xen_boot_binary
> > {
> > struct xen_boot_binary *next;
> > @@ -80,6 +130,8 @@ struct xen_boot_binary
> >
> > char *cmdline;
> > int cmdline_size;
> > +
> > + lf2_t lf2;
> > };
> >
> > static grub_dl_t my_mod;
> > @@ -289,6 +341,8 @@ single_binary_unload (struct xen_boot_binary *binary)
> > binary->cmdline, binary->cmdline_size);
> > }
> >
> > + lf2_done (&binary->lf2);
> > +
> > if (!binary->is_hypervisor)
> > grub_list_remove (GRUB_AS_LIST (binary));
> >
> > @@ -393,6 +447,7 @@ grub_cmd_xen_module (grub_command_t cmd
> > __attribute__((unused)),
> > struct xen_boot_binary *module = NULL;
> > grub_file_t file = 0;
> > int nounzip = 0;
> > + initrd_media_device_path_t *media;
> >
> > if (!argc)
> > {
> > @@ -427,6 +482,19 @@ grub_cmd_xen_module (grub_command_t cmd
> > __attribute__((unused)),
> >
> > module->is_hypervisor = false;
> > module->align = 4096;
> > + switch (grub_list_length (GRUB_AS_LIST (module_head)))
> > + {
> > + case MODULE_IMAGE:
> > + media = &kernel_lf2_device_path;
> > + break;
> > + case MODULE_INITRD:
> > + media = &initrd_lf2_device_path;
> > + break;
> > + default:
> > + media = NULL;
> > + break;
>
> Shouldn't this be a hard error, at least on x86_64, rather than silently
> ignoring the extra modules?
>
Yes, probably better. In theory the third module should be the Xen
policy which is supported for x64 too, so it would be worth supporting
it.
> In general, I'm not sure I like this interface since it is neither
> scalable or flexible. i.e. adding a new module would require both Xen
> and GRUB changes and it prevents certain combinations, e.g. booting a
> combined Xen + kernel image while _only_ passing the initrd as a module.
>
The issue of skipping a module because it's embedded into xe.efi is
present for ARM too, so it would be nice if the solution would be
working on ARM too.
Maybe allowing to specify the module type with an option like
--initrd, --kernel, --policy ?
On a related subject looking at "initrd" command for "linux" it's
possible to specify multiple files to initrd allowing to put them all
together generating a final initrd. It would be nice to have, although
here it seems a bit out of scope.
> Perhaps it is the best we can do without overcomplicating the code.
>
> > + }
> > + lf2_init(&module->lf2, media);
> >
> > grub_dprintf ("xen_loader", "Init module and node info\n");
> >
> > @@ -438,7 +506,10 @@ grub_cmd_xen_module (grub_command_t cmd
> > __attribute__((unused)),
> >
> > xen_boot_binary_load (module, file, argc, argv);
> > if (grub_errno == GRUB_ERR_NONE)
> > - grub_list_push (GRUB_AS_LIST_P (&module_head), GRUB_AS_LIST (module));
> > + {
> > + grub_list_push (GRUB_AS_LIST_P (&module_head), GRUB_AS_LIST
> > (module));
> > + lf2_start (&module->lf2);
> > + }
>
> It's not directly related to this patch but isn't there a bug here where
> xen_boot_binary_load() calls single_binary_unload() on error and then it
> gets called again below causing a double free?
>
Yes, it looks so, not related to this change but it looks doable. I
think xen_boot_binary_load calling single_binary_unload is wrong.
> Ross
Frediano
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
