|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH-4.16 v2] xen/efi: Fix Grub2 boot on arm64
On Thu, 4 Nov 2021, Luca Fancellu wrote:
> > On 4 Nov 2021, at 20:56, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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?
> 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.
> >
> > /* 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 */
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |