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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.11.2021 18:12, Luca Fancellu wrote:
>> On 2 Nov 2021, at 14:45, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 02.11.2021 15:05, Luca Fancellu wrote:
>>> The code introduced by commit a1743fc3a9fe9b68c265c45264dddf214fd9b882
>>> ("arm/efi: Use dom0less configuration when using EFI boot") is
>>> introducing a problem to boot Xen using Grub2 on ARM machine using EDK2.
>>>
>>> The problem comes from the function get_parent_handle(...) that inside
>>> uses the HandleProtocol on loaded_image->DeviceHandle, but the last
>>> is NULL, making Xen stop the UEFI boot.
>>
>> According to my reading the UEFI spec doesn't (explicitly) allow for
>> this to be NULL. Could you clarify why this is the case? What other
>> information may end up being invalid / absent? Is e.g. read_section()
>> safe to use?
> 
> My test on an arm machine running Grub2 on top of EDK2 showed that
> when Xen is started, the get_parent_handle(…) call was failing and stopping
> the boot because the efi_bs->HandleProtocol(…) was called with the
> loaded_image->DeviceHandle argument NULL and the call was returning
> a EFI_INVALID_PARAMETER.
> So the parent handle can’t be requested and the filesystem can’t be used,
> but any other code that doesn’t use the handle provided by 
> get_parent_handle(…)
> can be used without problem like read_section(...).

I understand this. My question was for the reason of ->DeviceHandle
being NULL. IOW I'm wondering whether we're actually talking about a
firmware or GrUB bug, in which case your change is a workaround for
that rather than (primarily) a fix for the earlier Xen change.

>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -449,6 +449,13 @@ static EFI_FILE_HANDLE __init 
>>> get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>>>     CHAR16 *pathend, *ptr;
>>>     EFI_STATUS ret;
>>>
>>> +    /*
>>> +     * If DeviceHandle is NULL, we can't use the 
>>> SIMPLE_FILE_SYSTEM_PROTOCOL
>>> +     * to have access to the filesystem.
>>> +     */
>>> +    if ( !loaded_image->DeviceHandle )
>>> +        return NULL;
>>
>> I couldn't find anything in the spec saying that NULL (a pointer with
>> the numeric value zero) could actually not be a valid handle. Could
>> you point me to text saying so?
> 
> I am reading UEFI spec 2.8 A, section 7.3 Protocol Handler Services, when it 
> talks about
> EFI_BOOT_SERVICES.HandleProtocol() there is a table of “Status Code Returned” 
> listing
> the EFI_INVALID_PARAMETER when the Handle is NULL.

Oh, okay. I guess I didn't search very well.

>>> @@ -1333,6 +1342,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>>> *SystemTable)
>>>             EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
>>>                                                        &file_name);
>>>
>>> +            if ( !handle )
>>> +                blexit(L"Error retrieving image name: no filesystem 
>>> access");
>>
>> I don't think this should be fatal - see the comment ahead of the
>> enclosing if().
> 
> I’m not sure I get it, I put the fatal condition in part because the handle 
> was dereferenced by
> handle->Close(handle), but also because file_name would have not being 
> modified by the call
> and we have then *argv = file_name.

Instead of you making boot fail I was trying to suggest that you insert
a made-up name ("xen" or "xen.efi"?) alongside issuing just a warning
message.

Jan