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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.11.2021 15:09, Luca Fancellu wrote:
>> On 3 Nov 2021, at 11:28, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 03.11.2021 11:20, Luca Fancellu wrote:
>>>> On 3 Nov 2021, at 08:20, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> 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.
>>>
>>> The issue was found only when using EDK2+Grub2, no issue when booting
>>> directly from EDK2.
>>> This is a fix for the regression, because without the EFI changes, Grub2 was
>>> booting successfully Xen. Using grub2 to boot Xen on arm is a very common
>>> solution so not supporting this anymore could lead to lots of people having
>>> issues if they update to Xen 4.16.
>>
>> I'm not objecting to addressing the issue. But the description needs
>> to make clear where the origin of the problem lies, and afaict that's
>> not the earlier Xen change. That one merely uncovered what, according
>> to your reply, might then be a GrUB bug. Unless, as said earlier, I
>> merely haven't been able to spot provisions in the spec for the field
>> in question to be NULL.
> 
> Maybe I can rephrase to be more specific from:
> 
> 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.
> 
> To:
> 
> Despite UEFI specification, EDK2+Grub2 is returning a NULL DeviceHandle,
> that is used by efi_bs->HandleProtocol(…) inside get_parent_handle(…),
> causing Xen to stop the boot getting an EFI_INVALID_PARAMETER error.
> 
> Do you think it can be ok like this?

Much better, yes, but I wonder what "returning" refers to. You want to
describe the origin of the NULL handle as precisely as possible. And
considering this turns out as a workaround, in a suitable place you
will also want to add a code comment, such that a later reader won't
decide this is all dead code and can be done in a simpler way.

Jan