[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 9 Nov 2021, at 02:11, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote: > > On Mon, 8 Nov 2021, Jan Beulich wrote: >> On 05.11.2021 16:33, Stefano Stabellini wrote: >>> On Fri, 5 Nov 2021, Jan Beulich wrote: >>>> On 04.11.2021 22:50, Stefano Stabellini wrote: >>>>> On Thu, 4 Nov 2021, Luca Fancellu wrote: >>>>>>> On 4 Nov 2021, at 21:35, Stefano Stabellini <sstabellini@xxxxxxxxxx> >>>>>>> wrote: >>>>>>> On Thu, 4 Nov 2021, Luca Fancellu wrote: >>>>>>>>> On 4 Nov 2021, at 20:56, Stefano Stabellini <sstabellini@xxxxxxxxxx> >>>>>>>>> wrote: >>>>>>>>> @@ -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? >>>>>> >>>>>> Oh yes sorry my bad Stefano! Having this version of the patch, it will >>>>>> work. >>>>>> >>>>>> My understanding was instead that the Jan suggestion is to revert the >>>>>> place >>>>>> of call of get_parent_handle (like in your code diff), but also to >>>>>> remove the >>>>>> changes to get_parent_handle and read_file. >>>>>> I guess Jan will specify his preference, but if he meant the last one, >>>>>> then >>>>>> the only way I see... >>>>> >>>>> I think we should keep the changes to get_parent_handle and read_file, >>>>> otherwise it will make it awkward, and those changes are good in their >>>>> own right anyway. >>>> >>>> As a maintainer of this code I'm afraid I have to say that I disagree. >>>> These changes were actually part of the reason why I went and looked >>>> how things could (and imo ought to be) done differently. >>> >>> You know this code and EFI booting better than me -- aren't you >>> concerned about Xen calling get_parent_handle / dir_handle->Close so >>> many times (by allocate_module_file)? >> >> I'm not overly concerned there; my primary concern is for it to get called >> without need in the first place. > > Exactly my thinking! Except that now it gets called 10x times with > dom0less boot :-( > > >>> My main concern is performance and resource utilization. With v3 of the >>> patch get_parent_handle will get called for every module to be loaded. >>> With dom0less, it could easily get called 10 times or more. Taking a >>> look at get_parent_handle, the Xen side doesn't seem small and I have >>> no idea how the EDK2 side looks. I am just worried that it would >>> actually have an impact on boot times (also depending on the bootloader >>> implementation). >> >> The biggest part of the function deals with determining the "residual" of >> the file name. That part looks to be of no interest at all to >> allocate_module_file() (whether that's actually correct I can't tell). I >> don't see why this couldn't be made conditional (e.g. by passing in NULL >> for "leaf"). > > I understand the idea of passing NULL instead of "leaf", but I tried > having a look and I can't tell what we would be able to skip in > get_parent_handle. > Hi Stefano, Jan, > Should we have a global variable to keep the dir_handle open during > dom0less module loading? I thought about a solution for that, here the changes, please not that I’ve just built them, not tested, Would they be something acceptable to have loaded_image global? diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 458cfbbed4..b4d86e9f7c 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -44,17 +44,17 @@ void __flush_dcache_area(const void *vaddr, unsigned long size); static int get_module_file_index(const char *name, unsigned int name_len); static void PrintMessage(const CHAR16 *s); -static int allocate_module_file(EFI_LOADED_IMAGE *loaded_image, +static int allocate_module_file(EFI_FILE_HANDLE *dir_handle, const char *name, unsigned int name_len); -static int handle_module_node(EFI_LOADED_IMAGE *loaded_image, +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); -static int handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, +static int handle_dom0less_domain_node(EFI_FILE_HANDLE *dir_handle, int domain_node); -static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image); +static int efi_check_dt_boot(void); #define DEVICE_TREE_GUID \ {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}} @@ -647,11 +647,10 @@ static void __init PrintMessage(const CHAR16 *s) * This function allocates a binary and keeps track of its name, it returns the * index of the file in the modules array or a negative number on error. */ -static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, +static int __init allocate_module_file(EFI_FILE_HANDLE *dir_handle, const char *name, unsigned int name_len) { - EFI_FILE_HANDLE dir_handle; module_name *file_name; CHAR16 *fname; union string module_name; @@ -686,12 +685,11 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, file_name->name_len = name_len; /* Get the file system interface. */ - dir_handle = get_parent_handle(loaded_image, &fname); + if ( !*dir_handle ) + *dir_handle = get_parent_handle(&fname); /* Load the binary in memory */ - read_file(dir_handle, s2w(&module_name), &module_binary, NULL); - - dir_handle->Close(dir_handle); + read_file(*dir_handle, s2w(&module_name), &module_binary, NULL); /* Save address and size */ file_name->addr = module_binary.addr; @@ -711,7 +709,7 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, * 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_LOADED_IMAGE *loaded_image, +static int __init handle_module_node(EFI_FILE_HANDLE *dir_handle, int module_node_offset, int reg_addr_cells, int reg_size_cells, @@ -744,7 +742,7 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, file_idx = get_module_file_index(uefi_name_prop, uefi_name_len); if ( file_idx < 0 ) { - file_idx = allocate_module_file(loaded_image, uefi_name_prop, + file_idx = allocate_module_file(dir_handle, uefi_name_prop, uefi_name_len); if ( file_idx < 0 ) return file_idx; @@ -811,7 +809,7 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, * in the DT. * Returns number of multiboot,module found or negative number on error. */ -static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, +static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE *dir_handle, int domain_node) { int module_node, addr_cells, size_cells, len; @@ -842,7 +840,7 @@ static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, module_node > 0; module_node = fdt_next_subnode(fdt, module_node) ) { - int ret = handle_module_node(loaded_image, module_node, addr_cells, + int ret = handle_module_node(dir_handle, module_node, addr_cells, size_cells, true); if ( ret < 0 ) return ret; @@ -858,10 +856,11 @@ static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, * 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_LOADED_IMAGE *loaded_image) +static int __init efi_check_dt_boot(void) { int chosen, node, addr_len, size_len; unsigned int i = 0, modules_found = 0; + EFI_FILE_HANDLE *dir_handle = NULL; /* Check for the chosen node in the current DTB */ chosen = setup_chosen_node(fdt, &addr_len, &size_len); @@ -881,13 +880,13 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) if ( !fdt_node_check_compatible(fdt, node, "xen,domain") ) { /* Found a node with compatible xen,domain; handle this node. */ - ret = handle_dom0less_domain_node(loaded_image, node); + ret = handle_dom0less_domain_node(dir_handle, node); if ( ret < 0 ) return ERROR_DT_MODULE_DOMU; } else { - ret = handle_module_node(loaded_image, node, addr_len, size_len, + ret = handle_module_node(dir_handle, node, addr_len, size_len, false); if ( ret < 0 ) return ERROR_DT_MODULE_DOM0; @@ -895,6 +894,9 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) modules_found += ret; } + if ( *dir_handle ) + (*dir_handle)->Close(*dir_handle); + /* Free boot modules file names if any */ for ( ; i < modules_idx; i++ ) { diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 8fd5e2d078..1a91920e8a 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -121,8 +121,7 @@ static char *get_value(const struct file *cfg, const char *section, static char *split_string(char *s); static CHAR16 *s2w(union string *str); static char *w2s(const union string *str); -static EFI_FILE_HANDLE get_parent_handle(EFI_LOADED_IMAGE *loaded_image, - CHAR16 **leaf); +static EFI_FILE_HANDLE get_parent_handle(CHAR16 **leaf); static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file, const char *options); static bool read_section(const EFI_LOADED_IMAGE *image, const CHAR16 *name, @@ -145,6 +144,7 @@ static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) static const EFI_BOOT_SERVICES *__initdata efi_bs; static UINT32 __initdata efi_bs_revision; static EFI_HANDLE __initdata efi_ih; +static EFI_LOADED_IMAGE *__initdata loaded_image; static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut; static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr; @@ -169,7 +169,7 @@ static void __init PrintErr(const CHAR16 *s) } #ifndef CONFIG_HAS_DEVICE_TREE -static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) +static int __init efi_check_dt_boot(void) { return 0; } @@ -441,8 +441,7 @@ 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) +static EFI_FILE_HANDLE __init get_parent_handle(CHAR16 **leaf) { static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL; static CHAR16 __initdata buffer[512]; @@ -451,6 +450,8 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image, CHAR16 *pathend, *ptr; EFI_STATUS ret; + BUG_ON(!loaded_image); + do { EFI_FILE_IO_INTERFACE *fio; @@ -1134,7 +1135,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL; static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; - EFI_LOADED_IMAGE *loaded_image; EFI_STATUS status; unsigned int i, argc; CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; @@ -1240,7 +1240,7 @@ 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); + dir_handle = get_parent_handle(&file_name); /* Read and parse the config file. */ if ( read_section(loaded_image, L"config", &cfg, NULL) ) @@ -1332,8 +1332,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) */ if ( argc && !*argv ) { - EFI_FILE_HANDLE handle = get_parent_handle(loaded_image, - &file_name); + EFI_FILE_HANDLE handle = get_parent_handle(&file_name); handle->Close(handle); *argv = file_name; @@ -1371,7 +1370,7 @@ 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(loaded_image); + dt_modules_found = efi_check_dt_boot(); if ( dt_modules_found < 0 ) /* efi_check_dt_boot throws some error */ Cheers, Luca
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |