Re: [PATCH-4.16 v2] xen/efi: Fix Grub2 boot on arm64

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 9 Nov 2021, Jan Beulich wrote:
> On 09.11.2021 03:11, Stefano Stabellini wrote:
> > On Mon, 8 Nov 2021, Jan Beulich wrote:
> >> On 05.11.2021 16:33, Stefano Stabellini wrote:
> >>> 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.
> 
> My bad - I did overlook that dir_handle gets updated even past the
> initial loop.
> 
> > Should we have a global variable to keep the dir_handle open during
> > dom0less module loading?
> 
> If that's contained within Arm-specific code, I (obviously) don't mind.
> Otherwise I remain to be convinced.

I think we can do something decent entirely within
xen/arch/arm/efi/efi-boot.h.

Luca, see below as reference; it is untested and incomplete but should
explain the idea better than words. With the below, we only open/close
the handle once for the all dom0less modules.


diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 458cfbbed4..b5218d5228 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -24,6 +24,7 @@ static struct file __initdata module_binary;
 static module_name __initdata modules[MAX_UEFI_MODULES];
 static unsigned int __initdata modules_available = MAX_UEFI_MODULES;
 static unsigned int __initdata modules_idx;
+static EFI_FILE_HANDLE __initdata dir_handle;
 
 #define ERROR_BINARY_FILE_NOT_FOUND (-1)
 #define ERROR_ALLOC_MODULE_NO_SPACE (-1)
@@ -651,9 +652,7 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE 
*loaded_image,
                                        const char *name,
                                        unsigned int name_len)
 {
-    EFI_FILE_HANDLE dir_handle;
     module_name *file_name;
-    CHAR16 *fname;
     union string module_name;
     int ret;
 
@@ -685,14 +684,9 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE 
*loaded_image,
     strlcpy(file_name->name, name, name_len + 1);
     file_name->name_len = name_len;
 
-    /* Get the file system interface. */
-    dir_handle = get_parent_handle(loaded_image, &fname);
-
     /* Load the binary in memory */
     read_file(dir_handle, s2w(&module_name), &module_binary, NULL);
 
-    dir_handle->Close(dir_handle);
-
     /* Save address and size */
     file_name->addr = module_binary.addr;
     file_name->size = module_binary.size;
@@ -862,6 +856,7 @@ 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;
+    CHAR16 *fname;
 
     /* Check for the chosen node in the current DTB */
     chosen = setup_chosen_node(fdt, &addr_len, &size_len);
@@ -871,6 +866,8 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE 
*loaded_image)
         return ERROR_DT_CHOSEN_NODE;
     }
 
+    dir_handle = get_parent_handle(loaded_image, &fname);
+
     /* Check for nodes compatible with xen,domain under the chosen node */
     for ( node = fdt_first_subnode(fdt, chosen);
           node > 0;
@@ -902,6 +899,8 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE 
*loaded_image)
         efi_bs->FreePool(modules[i].name);
     }
 
+    if ( dir_handle )
+        dir_handle->Close(dir_handle);
     return modules_found;
 }