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

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


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 8 Nov 2021 08:25:47 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=HEzHDHgvA+riJnyqQwRa7+/5OHq7PCoSB7kENDfplXs=; b=A7DKvUnCrpWegTok/uHQhtifOQYd/1er8N6fRRH/GgEGPavsXEkdjSSNbQw31vlSuhU+m/U6kpWs0OjKMnZFB1jupw8oPLvSnfQL0PosjWyvp5WzdSNCMZj1+cdxFc507cSQvkgDwxTqBy3Whr0XOiUKi74/5Yl6GOhTmCLvkua8DJNf7u+T9Wfz+MjAmknadnRK2xgO6hP8vBimZWTa7P7pDUCVmdjB1heASgYvP8Yt3WPqqRMGY3m5IB+M4L9j8C09ppJv5QZfhGau/fjKeGeBa6p9VzCHo6hw70dwcsgCqFYCE7/ItckXUbU1IYqtrIo4HmnZOvn1SIKAvMyN4w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fHIvCvWjIjVI0DJ+CpseDN2uZwbaIAEiisqEi8DL5FKR5nz9Fg8JKUF51oQYgZAj9f6UMVuO+lU6JVfrox3U1y9kXzo5CKZlgvbmyx/qtosTu1Tu7z2NVBNG9MoLeKjpUL4kpBMqQYhfUFLgqUIu1v9tHI55a/y6V3tqYaMoSpJTD4IHOYI4UiOQphT4XKJZJAj/xBWewckPxWOlperuXimR+InuB9T6nULhBIpgFTyaD5tq19AqQeoH4HQ7mQuCnt9W9BD4MycaF+4ObGRFa8XWOhTXiPdl0HDcCzybajph7XGcH/7qNNbxLIKrZni55mdUUp6wdPG8jham5TpedQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Bertrand Marquis <bertrand.marquis@xxxxxxx>, wei.chen@xxxxxxx, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Luca Fancellu <luca.fancellu@xxxxxxx>
  • Delivery-date: Mon, 08 Nov 2021 07:26:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 05.11.2021 16:33, Stefano Stabellini wrote:
> On Fri, 5 Nov 2021, Jan Beulich wrote:
>> On 04.11.2021 22:50, Stefano Stabellini wrote:
>>> On Thu, 4 Nov 2021, 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:
>>>>>>> @@ -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.
>>>> I guess Jan will specify his preference, but if he meant the last one, then
>>>> the only way I see...
>>>
>>> I think we should keep the changes to get_parent_handle and read_file,
>>> otherwise it will make it awkward, and those changes are good in their
>>> own right anyway.
>>
>> As a maintainer of this code I'm afraid I have to say that I disagree.
>> These changes were actually part of the reason why I went and looked
>> how things could (and imo ought to be) done differently.
> 
> You know this code and EFI booting better than me -- aren't you
> concerned about Xen calling get_parent_handle / dir_handle->Close so
> many times (by allocate_module_file)?

I'm not overly concerned there; my primary concern is for it to get called
without need in the first place.

> My main concern is performance and resource utilization. With v3 of the
> patch get_parent_handle will get called for every module to be loaded.
> With dom0less, it could easily get called 10 times or more. Taking a
> look at get_parent_handle, the Xen side doesn't seem small and I have
> no idea how the EDK2 side looks. I am just worried that it would
> actually have an impact on boot times (also depending on the bootloader
> implementation).

The biggest part of the function deals with determining the "residual" of
the file name. That part looks to be of no interest at all to
allocate_module_file() (whether that's actually correct I can't tell). I
don't see why this couldn't be made conditional (e.g. by passing in NULL
for "leaf").

Jan




 


Rackspace

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