Re: [PATCH-4.16] arm/efi: Improve performance requesting filesystem handle

[Julien Grall <julien@xen.org>]

Hi,

On 16/11/2021 20:38, Stefano Stabellini wrote:
> On Tue, 16 Nov 2021, Luca Fancellu wrote:
> So I think we can queue it in the Xen on ARM temporary for-next branch.

I have pushed it to for-next/4.17 on both my repo and gitlab (hopefully 
this triggered a CI run).

Cheers,

> 
> 
>> 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 | 33 +++++++++++++++++++++------------
>>   1 file changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index 458cfbbed4..c4ed412845 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -45,14 +45,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,
>> +                                EFI_FILE_HANDLE *dir_handle,
>>                                   const char *name,
>>                                   unsigned int name_len);
>>   static int handle_module_node(EFI_LOADED_IMAGE *loaded_image,
>> +                              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,
>> +                                       EFI_FILE_HANDLE *dir_handle,
>>                                          int domain_node);
>>   static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image);
>>   
>> @@ -648,10 +651,10 @@ static void __init PrintMessage(const CHAR16 *s)
>>    * 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,
>> +                                       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 +689,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(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(*dir_handle, s2w(&module_name), &module_binary, NULL);
>>   
>>       /* Save address and size */
>>       file_name->addr = module_binary.addr;
>> @@ -712,6 +714,7 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image,
>>    * Returns 1 if module is multiboot,module, 0 if not, < 0 on error
>>    */
>>   static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image,
>> +                                     EFI_FILE_HANDLE *dir_handle,
>>                                        int module_node_offset,
>>                                        int reg_addr_cells,
>>                                        int reg_size_cells,
>> @@ -744,8 +747,8 @@ 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,
>> -                                        uefi_name_len);
>> +        file_idx = allocate_module_file(loaded_image, dir_handle,
>> +                                        uefi_name_prop, uefi_name_len);
>>           if ( file_idx < 0 )
>>               return file_idx;
>>       }
>> @@ -812,6 +815,7 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image,
>>    * Returns number of multiboot,module found or negative number on error.
>>    */
>>   static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image,
>> +                                              EFI_FILE_HANDLE *dir_handle,
>>                                                 int domain_node)
>>   {
>>       int module_node, addr_cells, size_cells, len;
>> @@ -842,8 +846,8 @@ 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,
>> -                                     size_cells, true);
>> +        int ret = handle_module_node(loaded_image, dir_handle, module_node,
>> +                                     addr_cells, size_cells, true);
>>           if ( ret < 0 )
>>               return ret;
>>   
>> @@ -862,6 +866,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;
>> +    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,20 +886,24 @@ 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(loaded_image, &dir_handle, node);
>>               if ( ret < 0 )
>>                   return ERROR_DT_MODULE_DOMU;
>>           }
>>           else
>>           {
>> -            ret = handle_module_node(loaded_image, node, addr_len, size_len,
>> -                                     false);
>> +            ret = handle_module_node(loaded_image, &dir_handle, node, addr_len,
>> +                                     size_len, false);
>>               if ( ret < 0 )
>>                    return ERROR_DT_MODULE_DOM0;
>>           }
>>           modules_found += ret;
>>       }
>>   
>> +    /* dir_handle can be allocated in allocate_module_file, free it if exists */
>> +    if ( dir_handle )
>> +        dir_handle->Close(dir_handle);
>> +
>>       /* Free boot modules file names if any */
>>       for ( ; i < modules_idx; i++ )
>>       {
>> -- 
>> 2.17.1
>>

-- 
Julien Grall