[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH-4.16 v2] xen/efi: Fix Grub2 boot on arm64
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |