[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Luca Fancellu <luca.fancellu@xxxxxxx>
  • Date: Tue, 2 Nov 2021 17:12:35 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=uIuUAZWnzYjAYxu8FBARlgStez5hR787SRhucLPk6os=; b=dMl0JoUO5aapliwP1ZXE03iO6w5VTMhMSXZIks4vyM3WBJMwAXNjTQ2gvsWQ9IxZEggFHFUY82sHjkR/nV5kUDWKbSIhi4f5IvXuNUT7kEhD0Ah4wSyDhuhUv0LNma5gF1GZ07vW3bjpTnST0h27N9uf3ns009cSnVPaFxbODq9K/etCLjwGRVH446PH0ZYCFWwqhIcv28UtwVvmyzMg+LPJiHdYbkB5w1JtrTIVUgd0rDdrJkQI8UPwCJVs7Jqj6wRo7wfEqhcE4emF5VuJaWvuGBrfCc4bUNvOLm9tUptt+lfRMJook28k4QzHXgyR06Rs7caldkphoDe5euIdNQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ASfRC3PYefv/IqMSusxjoQZMyxigA9DHZCrUjtutaDx8wJH6RagrEl4ThEDQ7X7vdeE4UDkfqxcz4BT0UBl79u1b/xIwVV0lML+fN+58uWw9e4dPwiLC3ximxhDqOxr1N0uNPRUg556Yb6gg6ext+XUdSN/IqhVhK6E4pFOdsfN+aMBGU+ZjdZ9BuR0wA53xmxphKHbLLKpbOIu/NKbWIMRRag7mdPtaMJphMI4ySTW8ZMMpqQ3YUz310dGOSxD9ObpW/OrmtHBZvSjI97x4dVs+fzbwwzAguPrESEJXqcwMX7Sm2eqrRwX/Qvfbn3PGRDPAuinC72194NHGW6Hy6g==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Bertrand Marquis <bertrand.marquis@xxxxxxx>, wei.chen@xxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • Delivery-date: Tue, 02 Nov 2021 17:13:30 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;

+ Ian Jackson for 4.16 release

> 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(...).

> 
>> --- 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.

> 
>> @@ -581,6 +588,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");
> 
> dir_handle also gets passed to efi_arch_cfg_file_{early,late}() -
> those don't need any adjustment only because they merely pass the
> parameter on to read_file()?

Yes, the handling is done in read_file(...)

> 
>> @@ -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.

Cheers,
Luca

> 
> Jan
> 




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.