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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 9 Nov 2021, at 21:52, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> 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;
> }
> 

Hi Stefano,

I thought having the EFI_FILE_HANDLE global in efi-boot.h was a “no go”, but if 
it’s not then instead of
calling get_parent_handle in efi_check_dt_boot (that is the main issue with 
EDK2+Grub2), we can do
something like this:

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 458cfbbed4..169bbfc215 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 fs_dir_handle;
 
 #define ERROR_BINARY_FILE_NOT_FOUND (-1)
 #define ERROR_ALLOC_MODULE_NO_SPACE (-1)
@@ -651,7 +652,6 @@ 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;
@@ -686,12 +686,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 ( !fs_dir_handle )
+        fs_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);
+    read_file(fs_dir_handle, s2w(&module_name), &module_binary, NULL);
 
     /* Save address and size */
     file_name->addr = module_binary.addr;
@@ -895,6 +894,10 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE 
*loaded_image)
         modules_found += ret;
     }
 
+    /* allocate_module_file could have allocated the handle, if so, close it */
+    if ( fs_dir_handle )
+        fs_dir_handle->Close(fs_dir_handle);
+
     /* Free boot modules file names if any */
     for ( ; i < modules_idx; i++ )
     {


I’ve not tested these changes, but I’ve built them for arm/x86 and they build.

What everyone think about that?

Cheers,
Luca