> On 2 Nov 2021, at 23:17, Stefano Stabellini <sstabell...@kernel.org> wrote:
>
> 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.fance...@arm.com>
>> ---
>> 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:
Yes we could do that but when we handle a xen,domain node we will count
only one module and that defeats the aim to count every multiboot,module.
If we want to continue with your proposal let me know and I will implement it.
>
>
> 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?
Dir_handle could be invalid and we would be able to boot successfully when we
take everywhere
the path using read_section, for that reason I didn’t stop the boot earlier.
Given Jan suggestion that check could be also modified to be something like “if
there is no handle, *argv=“xen.efi” "
so the boot can continue without problem if we don’t need to read anything from
the filesystem.
>
>
>> 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
