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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> 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