On Thu, 4 Nov 2021, Luca Fancellu wrote: > The code introduced by commit a1743fc3a9fe9b68c265c45264dddf214fd9b882 > ("arm/efi: Use dom0less configuration when using EFI boot") is > introducing a problem to boot Xen using Grub2 on ARM machine using EDK2. > > Despite UEFI specification, EDK2+Grub2 is returning a NULL DeviceHandle > inside the interface given by the LOADED_IMAGE_PROTOCOL service, this > handle is used later by efi_bs->HandleProtocol(...) inside > get_parent_handle(...) when requesting the SIMPLE_FILE_SYSTEM_PROTOCOL > interface, causing Xen to stop the boot because of an EFI_INVALID_PARAMETER > error. > > Before the commit above, the function was never called because the > logic was skipping the call when there were multiboot modules in the > DT because the filesystem was never used and the bootloader had > put in place all the right modules in memory and the addresses > in the DT. > > To fix the problem we allow the get_parent_handle(...) function to > return a NULL handle on error and we check the usage of the function > to handle the new use case. The function in fact should not prevent > the boot even if the filesystem can't be used, because the DT and > the modules could be put in place by the bootloader before running > Xen and if xen,uefi-binary property is not used, there is no need > for the filesystem. > > Another problem is found when the UEFI stub tries to check if Dom0 > image or DomUs are present. > The logic doesn't work when the UEFI stub is not responsible to load > any modules, so the efi_check_dt_boot(...) return value is modified > to return the number of multiboot module found and not only the number > of module loaded by the stub. > Taking the occasion to update the comment in handle_module_node(...) > to explain why we return success even if xen,uefi-binary is not found. > > Fixes: a1743fc3a9 ("arm/efi: Use dom0less configuration when using EFI boot") > Signed-off-by: Luca Fancellu For the ARM part (xen/arch/arm/efi/efi-boot.h): Reviewed-by: Stefano Stabellini To be honest the changes to xen/common/efi/boot.c look OK too, but I'll reply to Jan's email about his suggestion > --- > Justification for integration in 4.16: > Upside: allow booting xen from grub on arm64 when the stub doesn't load > any module. > Downside: It's affecting the EFI boot path. > Risk: It's not affecting x86 arch that works the same way as before. > If something is wrong it creates a problem on early boot and not at > runtime, so risk is low. > > Tested in this configurations: > - Bootloader loads modules and specify them as multiboot modules in DT: > * combination of Dom0, DomUs, Dom0 and DomUs > - DT specifies multiboot modules in DT using xen,uefi-binary property: > * combination of Dom0, DomUs, Dom0 and DomUs > - Bootloader loads a Dom0 module and appends it as multiboot module in DT, > other multiboot modules are listed for DomUs using xen,uefi-binary > - No multiboot modules in DT and no kernel entry in cfg file: > * proper error thrown > > Changes in v2: > - Changed comment on DeviceHandle NULL (Jan) > - Removed fatal condition on handle NULL (Jan) > - Add more info about the EDK2+Grub2 issue to the commit msg (Jan) > - Removed modules_found from function signature and pass everything > on return (Stefano) > - Improved comment in handle_module_node > > --- > xen/arch/arm/efi/efi-boot.h | 34 +++++++++++++++++++++++----------- > xen/common/efi/boot.c | 28 +++++++++++++++++++++++++--- > 2 files changed, 48 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index 8b88dd26a5..c3ae9751ab 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -702,6 +702,7 @@ static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle, > * This function checks for the presence of the xen,uefi-binary property in the > * module, if found it loads the binary as module and sets the right address > * for the reg property into the module DT node. > + * Returns 1 if module is multiboot,module, 0 if not, < 0 on error > */ > static int __init handle_module_node(EFI_FILE_HANDLE dir_handle, > int module_node_offset, > @@ -730,8 +731,8 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle, > &uefi_name_len); > > if ( !uefi_name_prop ) > - /* Property not found */ > - return 0; > + /* Property not found, but signal this is a multiboot,module */ > + return 1; > > file_idx = get_module_file_index(uefi_name_prop, uefi_name_len); > if ( file_idx < 0 ) > @@ -795,19 +796,20 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle, > } > } > > - return 0; > + return 1; > } > > /* > * This function checks for boot modules under the domU guest domain node > * in the DT. > - * Returns 0 on success, negative number on error. > + * Returns number of multiboot,module found or negative number on error. > */ > static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle, > int domain_node) > { > int module_node, addr_cells, size_cells, len; > const struct fdt_property *prop; > + unsigned int mb_modules_found = 0; > > /* Get #address-cells and #size-cells from domain node */ > prop = fdt_get_property(fdt, domain_node, "#address-cells", &len); > @@ -837,20 +839,22 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle, > size_cells, true); > if ( ret < 0 ) > return ret; > + > + mb_modules_found += ret; > } > > - return 0; > + return mb_modules_found; > } > > /* > * This function checks for xen domain nodes under the /chosen node for possible > * dom0 and domU guests to be loaded. > - * Returns the number of modules loaded or a negative number for error. > + * 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) > { > int chosen, node, addr_len, size_len; > - unsigned int i = 0; > + unsigned int i = 0, modules_found = 0; > > /* Check for the chosen node in the current DTB */ > chosen = setup_chosen_node(fdt, &addr_len, &size_len); > @@ -865,15 +869,23 @@ static int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle) > node > 0; > node = fdt_next_subnode(fdt, node) ) > { > + int ret; > + > if ( !fdt_node_check_compatible(fdt, node, "xen,domain") ) > { > /* Found a node with compatible xen,domain; handle this node. */ > - if ( handle_dom0less_domain_node(dir_handle, node) < 0 ) > + ret = handle_dom0less_domain_node(dir_handle, node); > + if ( ret < 0 ) > return ERROR_DT_MODULE_DOMU; > } > - else if ( handle_module_node(dir_handle, node, addr_len, size_len, > - false) < 0 ) > + else > + { > + ret = handle_module_node(dir_handle, node, addr_len, size_len, > + false); > + if ( ret < 0 ) > return ERROR_DT_MODULE_DOM0; > + } > + modules_found += ret; > } > > /* Free boot modules file names if any */ > @@ -883,7 +895,7 @@ static int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle) > efi_bs->FreePool(modules[i].name); > } > > - return modules_idx; > + return modules_found; > } > > static void __init efi_arch_cpu(void) > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 392ff3ac9b..112b7e7571 100644 > --- 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 */ > -- > 2.17.1 >