|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/efi: Fix Grub2 boot on arm64
On Tue, 2 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.
>
> The problem comes from the function get_parent_handle(...) that inside
> uses the HandleProtocol on loaded_image->DeviceHandle, but the last
> is NULL, making Xen stop the UEFI boot.
>
> 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.
>
> Fixes: a1743fc3a9 ("arm/efi: Use dom0less configuration when using EFI boot")
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> 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
>
> ---
> xen/arch/arm/efi/efi-boot.h | 28 ++++++++++++++++++----------
> xen/common/efi/boot.c | 15 ++++++++++++++-
> 2 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 8b88dd26a5..e714b2b44c 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -51,9 +51,11 @@ static int handle_module_node(EFI_FILE_HANDLE dir_handle,
> int module_node_offset,
> int reg_addr_cells,
> int reg_size_cells,
> - bool is_domu_module);
> + bool is_domu_module,
> + unsigned int *modules_found);
> static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> - int domain_node);
> + int domain_node,
> + unsigned int *modules_found);
> static int efi_check_dt_boot(EFI_FILE_HANDLE dir_handle);
>
> #define DEVICE_TREE_GUID \
> @@ -707,7 +709,8 @@ static int __init handle_module_node(EFI_FILE_HANDLE
> dir_handle,
> int module_node_offset,
> int reg_addr_cells,
> int reg_size_cells,
> - bool is_domu_module)
> + bool is_domu_module,
> + unsigned int *modules_found)
> {
> const void *uefi_name_prop;
> char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
> @@ -725,6 +728,9 @@ static int __init handle_module_node(EFI_FILE_HANDLE
> dir_handle,
> /* Module is not a multiboot,module */
> return 0;
>
> + /* Count the multiboot module as found */
> + (*modules_found)++;
> +
> /* Read xen,uefi-binary property to get the file name. */
> uefi_name_prop = fdt_getprop(fdt, module_node_offset, "xen,uefi-binary",
> &uefi_name_len);
> @@ -804,7 +810,8 @@ static int __init handle_module_node(EFI_FILE_HANDLE
> dir_handle,
> * Returns 0 on success, negative number on error.
> */
> static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> - int domain_node)
> + int domain_node,
> + unsigned int *modules_found)
> {
> int module_node, addr_cells, size_cells, len;
> const struct fdt_property *prop;
> @@ -834,7 +841,7 @@ static int __init
> handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> module_node = fdt_next_subnode(fdt, module_node) )
> {
> int ret = handle_module_node(dir_handle, module_node, addr_cells,
> - size_cells, true);
> + size_cells, true, modules_found);
> if ( ret < 0 )
> return ret;
> }
> @@ -845,12 +852,12 @@ static int __init
> handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> /*
> * 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);
> @@ -868,11 +875,12 @@ static int __init efi_check_dt_boot(EFI_FILE_HANDLE
> dir_handle)
> 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 )
> + if ( handle_dom0less_domain_node(dir_handle, node,
> + &modules_found) < 0 )
> return ERROR_DT_MODULE_DOMU;
> }
> else if ( handle_module_node(dir_handle, node, addr_len, size_len,
> - false) < 0 )
> + false, &modules_found) < 0 )
> return ERROR_DT_MODULE_DOM0;
I think there is no need to add modules_found to the parameters of
handle_dom0less_domain_node and handle_module_node. You could just
increment modules_found here for every iteration of the loop where
there is no error. You would have to change a couple of returns in
handle_module_node, just to give you the idea:
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index e714b2b44c..7739789c41 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -726,7 +726,7 @@ static int __init handle_module_node(EFI_FILE_HANDLE
dir_handle,
if ( module_compat != 0 )
/* Module is not a multiboot,module */
- return 0;
+ return 1;
/* Count the multiboot module as found */
(*modules_found)++;
@@ -737,7 +737,7 @@ static int __init handle_module_node(EFI_FILE_HANDLE
dir_handle,
if ( !uefi_name_prop )
/* Property not found */
- return 0;
+ return 1;
file_idx = get_module_file_index(uefi_name_prop, uefi_name_len);
if ( file_idx < 0 )
> }
>
> @@ -883,7 +891,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..495e7a4096 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -449,6 +449,13 @@ static EFI_FILE_HANDLE __init
> get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> CHAR16 *pathend, *ptr;
> EFI_STATUS ret;
>
> + /*
> + * If DeviceHandle is NULL, we can't use the SIMPLE_FILE_SYSTEM_PROTOCOL
> + * to have access to the filesystem.
> + */
> + if ( !loaded_image->DeviceHandle )
> + return NULL;
> +
> do {
> EFI_FILE_IO_INTERFACE *fio;
>
> @@ -581,6 +588,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,6 +1342,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> *SystemTable)
> EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
> &file_name);
>
> + if ( !handle )
> + blexit(L"Error retrieving image name: no filesystem access");
I think it would be nice to have an other explicit check like this one
at the beginning of if ( use_cfg_file ) to make sure dir_handle is not
null in that case. If I am not mistaken, if we take the use_cfg_file
path, dir_handle has to be valid, right?
> handle->Close(handle);
> *argv = file_name;
> }
> @@ -1369,7 +1381,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
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |