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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.11.2021 21:56, Stefano Stabellini wrote:
> On Thu, 4 Nov 2021, Jan Beulich wrote:
>> On 04.11.2021 15:12, Luca Fancellu wrote:
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -449,6 +449,15 @@ static EFI_FILE_HANDLE __init 
>>> get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>>>      CHAR16 *pathend, *ptr;
>>>      EFI_STATUS ret;
>>>  
>>> +    /*
>>> +     * Grub2 running on top of EDK2 has been observed to supply a NULL
>>> +     * DeviceHandle. We can't use that to gain access to the filesystem.
>>> +     * However the system can still boot if it doesn’t require access to 
>>> the
>>> +     * filesystem.
>>> +     */
>>> +    if ( !loaded_image->DeviceHandle )
>>> +        return NULL;
>>> +
>>>      do {
>>>          EFI_FILE_IO_INTERFACE *fio;
>>>  
>>> @@ -581,6 +590,8 @@ static bool __init read_file(EFI_FILE_HANDLE 
>>> dir_handle, CHAR16 *name,
>>>      EFI_STATUS ret;
>>>      const CHAR16 *what = NULL;
>>>  
>>> +    if ( !dir_handle )
>>> +        blexit(L"Error: No access to the filesystem");
>>>      if ( !name )
>>>          PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
>>>      ret = dir_handle->Open(dir_handle, &FileHandle, name,
>>> @@ -1333,8 +1344,18 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>>> *SystemTable)
>>>              EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
>>>                                                         &file_name);
>>>  
>>> -            handle->Close(handle);
>>> -            *argv = file_name;
>>> +            if ( !handle )
>>> +            {
>>> +                PrintErr(L"Error retrieving image name: no filesystem 
>>> access."
>>> +                         L" Setting default to xen.efi");
>>> +                PrintErr(newline);
>>> +                *argv = L"xen.efi";
>>> +            }
>>> +            else
>>> +            {
>>> +                handle->Close(handle);
>>> +                *argv = file_name;
>>> +            }
>>>          }
>>>  
>>>          name.s = get_value(&cfg, section.s, "options");
>>> @@ -1369,7 +1390,8 @@ 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(dir_handle);
>>>  
>>> -    dir_handle->Close(dir_handle);
>>> +    if ( dir_handle )
>>> +        dir_handle->Close(dir_handle);
>>>  
>>>      if ( dt_modules_found < 0 )
>>>          /* efi_check_dt_boot throws some error */
>>>
>>
>> I'm sorry, but I think we need to take a step back here and revisit
>> the earlier change. If that hadn't moved obtaining dir_handle out by
>> one level of scope, nothing bad would have happened to the case that
>> you're now trying to fix, I understand? So perhaps that part wants
>> undoing, with efi_check_dt_boot() instead getting passed loaded_image.
>> That way, down the call tree the needed handle can be obtained via
>> another call to get_parent_handle(), and quite likely in the scenario
>> you're trying to fix here execution wouldn't even make it there. This
>> then wouldn't be much different to the image name retrieval calling
>> get_parent_handle() a 2nd time, rather than trying to re-use
>> dir_handle.
>>
>> Net effect being that I think get_parent_handle() would then again
>> only be called when the returned handle is actually needed, and hence
>> when failure of HandleProtocol() (for DeviceHandle being NULL just
>> like for any other reason) is indeed an error that needs reporting.
> 
> In my opinion the current version is good enough. Regardless, I looked
> at your suggestion into details. As it took me some time to understand
> it, I thought I would share the code changes that I think correspond to
> what you wrote. Does everything check out?

Well, first of all I understand that's an incremental change on top of
Luca's, not a replacement. And then there are a couple of things to be
done slightly differently (imo), to match the present model:

> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -8,6 +8,8 @@
>  #include <asm/setup.h>
>  #include <asm/smp.h>
>  
> +extern EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE 
> *loaded_image,
> +                                                CHAR16 **leaf);

This should remain static, but will need forward-declaring (for the
time being, I have a post-4.16 patch eliminating a fair part of
those forward decls).

> @@ -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);
>  
>      /* Check for the chosen node in the current DTB */
>      chosen = setup_chosen_node(fdt, &addr_len, &size_len);
> @@ -895,6 +901,8 @@ static int __init efi_check_dt_boot(EFI_FILE_HANDLE 
> dir_handle)
>          efi_bs->FreePool(modules[i].name);
>      }
>  
> +    dir_handle->Close(dir_handle);
> +
>      return modules_found;
>  }

Imo obtaining of the handle wants pushing further down the call tree.
Placing it here will, afaict, still trip the problem Luca is trying
to resolve. Plus of course the handle wants closing also on error
paths (if any in the function this really wants to be put into).

> @@ -1236,9 +1236,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  
>      efi_arch_relocate_image(0);
>  
> -    /* Get the file system interface. */
> -    dir_handle = get_parent_handle(loaded_image, &file_name);
> -
>      if ( use_cfg_file )
>      {
>          UINTN depth, cols, rows, size;
> @@ -1251,6 +1248,9 @@ 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);

Along with this the declaration of dir_handle also wants to move back
into the more narrow scope.

Jan