From: Stefano Stabellini <sstabellini@kernel.org>
To: Luca Fancellu <luca.fancellu@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Jan Beulich <jbeulich@suse.com>,
Bertrand Marquis <bertrand.marquis@arm.com>,
wei.chen@arm.com, Ian Jackson <iwj@xenproject.org>,
Julien Grall <julien@xen.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH-4.16 v2] xen/efi: Fix Grub2 boot on arm64
Date: Thu, 4 Nov 2021 14:50:34 -0700 (PDT) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2111041449180.284830@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <9E52FA33-422B-4B1C-A6AF-601CDF565700@arm.com>
[-- Attachment #1: Type: text/plain, Size: 11485 bytes --]
On Thu, 4 Nov 2021, Luca Fancellu wrote:
> > On 4 Nov 2021, at 21:35, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >
> > On Thu, 4 Nov 2021, Luca Fancellu wrote:
> >>> On 4 Nov 2021, at 20:56, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >>>
> >>> On Thu, 4 Nov 2021, Jan Beulich wrote:
> >>>> On 04.11.2021 15:12, Luca Fancellu wrote:
> >>>>> --- a/xen/common/efi/boot.c
> >>>>> +++ b/xen/common/efi/boot.c
> >>>>> @@ -449,6 +449,15 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> >>>>> CHAR16 *pathend, *ptr;
> >>>>> EFI_STATUS ret;
> >>>>>
> >>>>> + /*
> >>>>> + * Grub2 running on top of EDK2 has been observed to supply a NULL
> >>>>> + * DeviceHandle. We can't use that to gain access to the filesystem.
> >>>>> + * However the system can still boot if it doesn’t require access to the
> >>>>> + * filesystem.
> >>>>> + */
> >>>>> + if ( !loaded_image->DeviceHandle )
> >>>>> + return NULL;
> >>>>> +
> >>>>> do {
> >>>>> EFI_FILE_IO_INTERFACE *fio;
> >>>>>
> >>>>> @@ -581,6 +590,8 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> >>>>> EFI_STATUS ret;
> >>>>> const CHAR16 *what = NULL;
> >>>>>
> >>>>> + if ( !dir_handle )
> >>>>> + blexit(L"Error: No access to the filesystem");
> >>>>> if ( !name )
> >>>>> PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
> >>>>> ret = dir_handle->Open(dir_handle, &FileHandle, name,
> >>>>> @@ -1333,8 +1344,18 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> >>>>> EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
> >>>>> &file_name);
> >>>>>
> >>>>> - handle->Close(handle);
> >>>>> - *argv = file_name;
> >>>>> + if ( !handle )
> >>>>> + {
> >>>>> + PrintErr(L"Error retrieving image name: no filesystem access."
> >>>>> + L" Setting default to xen.efi");
> >>>>> + PrintErr(newline);
> >>>>> + *argv = L"xen.efi";
> >>>>> + }
> >>>>> + else
> >>>>> + {
> >>>>> + handle->Close(handle);
> >>>>> + *argv = file_name;
> >>>>> + }
> >>>>> }
> >>>>>
> >>>>> name.s = get_value(&cfg, section.s, "options");
> >>>>> @@ -1369,7 +1390,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> >>>>> /* Get the number of boot modules specified on the DT or an error (<0) */
> >>>>> dt_modules_found = efi_check_dt_boot(dir_handle);
> >>>>>
> >>>>> - dir_handle->Close(dir_handle);
> >>>>> + if ( dir_handle )
> >>>>> + dir_handle->Close(dir_handle);
> >>>>>
> >>>>> if ( dt_modules_found < 0 )
> >>>>> /* efi_check_dt_boot throws some error */
> >>>>>
> >>>>
> >>>> I'm sorry, but I think we need to take a step back here and revisit
> >>>> the earlier change. If that hadn't moved obtaining dir_handle out by
> >>>> one level of scope, nothing bad would have happened to the case that
> >>>> you're now trying to fix, I understand? So perhaps that part wants
> >>>> undoing, with efi_check_dt_boot() instead getting passed loaded_image.
> >>>> That way, down the call tree the needed handle can be obtained via
> >>>> another call to get_parent_handle(), and quite likely in the scenario
> >>>> you're trying to fix here execution wouldn't even make it there. This
> >>>> then wouldn't be much different to the image name retrieval calling
> >>>> get_parent_handle() a 2nd time, rather than trying to re-use
> >>>> dir_handle.
> >>>>
> >>>> Net effect being that I think get_parent_handle() would then again
> >>>> only be called when the returned handle is actually needed, and hence
> >>>> when failure of HandleProtocol() (for DeviceHandle being NULL just
> >>>> like for any other reason) is indeed an error that needs reporting.
> >>>
> >>> In my opinion the current version is good enough. Regardless, I looked
> >>> at your suggestion into details. As it took me some time to understand
> >>> it, I thought I would share the code changes that I think correspond to
> >>> what you wrote. Does everything check out?
> >>>
> >>> If so, I think it looks fine, maybe a bit better than the current
> >>> version. I'll leave that to you and Luca.
> >>>
> >>>
> >>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> >>> index c3ae9751ab..9dcd8547cd 100644
> >>> --- a/xen/arch/arm/efi/efi-boot.h
> >>> +++ b/xen/arch/arm/efi/efi-boot.h
> >>> @@ -8,6 +8,8 @@
> >>> #include <asm/setup.h>
> >>> #include <asm/smp.h>
> >>>
> >>> +extern EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> >>> + CHAR16 **leaf);
> >>> typedef struct {
> >>> char *name;
> >>> unsigned int name_len;
> >>> @@ -54,7 +56,7 @@ static int handle_module_node(EFI_FILE_HANDLE dir_handle,
> >>> bool is_domu_module);
> >>> static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> >>> int domain_node);
> >>> -static int efi_check_dt_boot(EFI_FILE_HANDLE dir_handle);
> >>> +static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image);
> >>>
> >>> #define DEVICE_TREE_GUID \
> >>> {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
> >>> @@ -851,10 +853,14 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> >>> * dom0 and domU guests to be loaded.
> >>> * Returns the number of multiboot modules found or a negative number for error.
> >>> */
> >>> -static int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> >>> +static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image)
> >>> {
> >>> int chosen, node, addr_len, size_len;
> >>> unsigned int i = 0, modules_found = 0;
> >>> + EFI_FILE_HANDLE dir_handle;
> >>> + CHAR16 *file_name;
> >>> +
> >>> + dir_handle = get_parent_handle(loaded_image, &file_name);
> >>
> >> We can’t use get_parent_handle here because we will end up with the same problem,
> >> we would need to use the filesystem if and only if we need to use it,
> >
> > Understood, but it would work the same way as this version of the patch:
> > if we end up calling read_file with dir_handle == NULL, then read_file
> > would do:
> >
> > blexit(L"Error: No access to the filesystem");
> >
> > If we don't end up calling read_file, then everything works even if
> > dir_handle == NULL. Right?
>
> Oh yes sorry my bad Stefano! Having this version of the patch, it will work.
>
> My understanding was instead that the Jan suggestion is to revert the place
> of call of get_parent_handle (like in your code diff), but also to remove the
> changes to get_parent_handle and read_file.
> I guess Jan will specify his preference, but if he meant the last one, then
> the only way I see...
I think we should keep the changes to get_parent_handle and read_file,
otherwise it will make it awkward, and those changes are good in their
own right anyway.
> >> so the way I see
> >> is to pass loaded_image down to the stack until allocate_module_file(…), in this
> >> function we can use get_parent_handle(…) because the user wants us to do that.
> >> The downside is that we must close the handle there, so for each loaded file we will
> >> request and close the handle. Is this something we don’t bother too much?
> >
> > Yeah, that doesn't seem ideal.
>
> … is this one.
>
> >
> >
> >>>
> >>> /* Check for the chosen node in the current DTB */
> >>> chosen = setup_chosen_node(fdt, &addr_len, &size_len);
> >>> @@ -895,6 +901,8 @@ static int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> >>> efi_bs->FreePool(modules[i].name);
> >>> }
> >>>
> >>> + dir_handle->Close(dir_handle);
> >>> +
> >>> return modules_found;
> >>> }
> >>>
> >>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> >>> index 112b7e7571..2407671a7d 100644
> >>> --- a/xen/common/efi/boot.c
> >>> +++ b/xen/common/efi/boot.c
> >>> @@ -167,7 +167,7 @@ static void __init PrintErr(const CHAR16 *s)
> >>> }
> >>>
> >>> #ifndef CONFIG_HAS_DEVICE_TREE
> >>> -static int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> >>> +static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image)
> >>> {
> >>> return 0;
> >>> }
> >>> @@ -439,8 +439,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>> return argc;
> >>> }
> >>>
> >>> -static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> >>> - CHAR16 **leaf)
> >>> +EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> >>> + CHAR16 **leaf)
> >>> {
> >>> static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
> >>> static CHAR16 __initdata buffer[512];
> >>> @@ -1236,9 +1236,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> >>>
> >>> efi_arch_relocate_image(0);
> >>>
> >>> - /* Get the file system interface. */
> >>> - dir_handle = get_parent_handle(loaded_image, &file_name);
> >>> -
> >>> if ( use_cfg_file )
> >>> {
> >>> UINTN depth, cols, rows, size;
> >>> @@ -1251,6 +1248,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> >>>
> >>> gop = efi_get_gop();
> >>>
> >>> + /* Get the file system interface. */
> >>> + dir_handle = get_parent_handle(loaded_image, &file_name);
> >>> +
> >>> /* Read and parse the config file. */
> >>> if ( read_section(loaded_image, L"config", &cfg, NULL) )
> >>> PrintStr(L"Using builtin config file\r\n");
> >>> @@ -1344,18 +1344,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> >>> EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
> >>> &file_name);
> >>>
> >>> - if ( !handle )
> >>> - {
> >>> - PrintErr(L"Error retrieving image name: no filesystem access."
> >>> - L" Setting default to xen.efi");
> >>> - PrintErr(newline);
> >>> - *argv = L"xen.efi";
> >>> - }
> >>> - else
> >>> - {
> >>> - handle->Close(handle);
> >>> - *argv = file_name;
> >>> - }
> >>> + handle->Close(handle);
> >>> + *argv = file_name;
> >>> }
> >>>
> >>> name.s = get_value(&cfg, section.s, "options");
> >>> @@ -1383,15 +1373,14 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> >>> efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> >>> cfg.addr = 0;
> >>>
> >>> + dir_handle->Close(dir_handle);
> >>> +
> >>> if ( gop && !base_video )
> >>> gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
> >>> }
> >>>
> >>> /* Get the number of boot modules specified on the DT or an error (<0) */
> >>> - dt_modules_found = efi_check_dt_boot(dir_handle);
> >>> -
> >>> - if ( dir_handle )
> >>> - dir_handle->Close(dir_handle);
> >>> + dt_modules_found = efi_check_dt_boot(loaded_image);
> >>>
> >>> if ( dt_modules_found < 0 )
> >>> /* efi_check_dt_boot throws some error */
>
next prev parent reply other threads:[~2021-11-04 21:50 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-04 14:12 [PATCH-4.16 v2] xen/efi: Fix Grub2 boot on arm64 Luca Fancellu
2021-11-04 14:33 ` Bertrand Marquis
2021-11-04 14:44 ` Ian Jackson
2021-11-04 16:36 ` Jan Beulich
2021-11-04 20:56 ` Stefano Stabellini
2021-11-04 21:07 ` Luca Fancellu
2021-11-04 21:35 ` Stefano Stabellini
2021-11-04 21:43 ` Luca Fancellu
2021-11-04 21:50 ` Stefano Stabellini [this message]
2021-11-05 7:35 ` Jan Beulich
2021-11-05 15:33 ` Stefano Stabellini
2021-11-08 7:25 ` Jan Beulich
2021-11-09 2:11 ` Stefano Stabellini
2021-11-09 9:23 ` Luca Fancellu
2021-11-09 11:01 ` Jan Beulich
2021-11-09 11:00 ` Jan Beulich
2021-11-09 21:52 ` Stefano Stabellini
2021-11-09 22:31 ` Julien Grall
2021-11-10 7:40 ` Jan Beulich
2021-11-10 13:05 ` Luca Fancellu
2021-11-10 13:36 ` Julien Grall
2021-11-10 14:02 ` Luca Fancellu
2021-11-15 18:57 ` Julien Grall
2021-11-15 22:00 ` Stefano Stabellini
2021-11-16 8:36 ` Luca Fancellu
2021-11-16 15:08 ` Ian Jackson
2021-11-16 16:11 ` Jan Beulich
2021-11-16 16:23 ` Julien Grall
2021-11-05 7:32 ` Jan Beulich
2021-11-05 7:27 ` Jan Beulich
2021-11-04 20:51 ` Stefano Stabellini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.22.394.2111041449180.284830@ubuntu-linux-20-04-desktop \
--to=sstabellini@kernel.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=bertrand.marquis@arm.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=luca.fancellu@arm.com \
--cc=wei.chen@arm.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).