[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, 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); /* 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 |