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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.11.2021 22:43, 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:
>>>>
>>>> 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?
>>>>
>>>> If so, I think it looks fine, maybe a bit better than the current
>>>> version. I'll leave that to you and Luca.
>>>>
>>>>
>>>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>>>> index c3ae9751ab..9dcd8547cd 100644
>>>> --- 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);
>>>> typedef struct {
>>>>    char *name;
>>>>    unsigned int name_len;
>>>> @@ -54,7 +56,7 @@ static int handle_module_node(EFI_FILE_HANDLE dir_handle,
>>>>                              bool is_domu_module);
>>>> static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>>>>                                       int domain_node);
>>>> -static int efi_check_dt_boot(EFI_FILE_HANDLE dir_handle);
>>>> +static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image);
>>>>
>>>> #define DEVICE_TREE_GUID \
>>>> {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 
>>>> 0xe0}}
>>>> @@ -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.

This is indeed a correct understanding of yours. (Hence why an incremental
change on top of yours wasn't the most expressive way to outline Stefano's
thoughts.)

Jan