From: Jan Beulich <jbeulich@suse.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Bertrand Marquis <bertrand.marquis@arm.com>,
wei.chen@arm.com, Ian Jackson <iwj@xenproject.org>,
Julien Grall <julien@xen.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
xen-devel@lists.xenproject.org,
Luca Fancellu <luca.fancellu@arm.com>
Subject: Re: [PATCH-4.16 v2] xen/efi: Fix Grub2 boot on arm64
Date: Mon, 8 Nov 2021 08:25:47 +0100 [thread overview]
Message-ID: <9bd58aa4-602b-4c64-e759-581513909457@suse.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2111050825240.284830@ubuntu-linux-20-04-desktop>
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@kernel.org> wrote:
>>>>> On Thu, 4 Nov 2021, Luca Fancellu wrote:
>>>>>>> On 4 Nov 2021, at 20:56, Stefano Stabellini <sstabellini@kernel.org> 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
next prev parent reply other threads:[~2021-11-08 7:26 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-04 14:12 [PATCH-4.16 v2] xen/efi: Fix Grub2 boot on arm64 Luca Fancellu
2021-11-04 14:33 ` Bertrand Marquis
2021-11-04 14:44 ` Ian Jackson
2021-11-04 16:36 ` Jan Beulich
2021-11-04 20:56 ` Stefano Stabellini
2021-11-04 21:07 ` Luca Fancellu
2021-11-04 21:35 ` Stefano Stabellini
2021-11-04 21:43 ` Luca Fancellu
2021-11-04 21:50 ` Stefano Stabellini
2021-11-05 7:35 ` Jan Beulich
2021-11-05 15:33 ` Stefano Stabellini
2021-11-08 7:25 ` Jan Beulich [this message]
2021-11-09 2:11 ` Stefano Stabellini
2021-11-09 9:23 ` Luca Fancellu
2021-11-09 11:01 ` Jan Beulich
2021-11-09 11:00 ` Jan Beulich
2021-11-09 21:52 ` Stefano Stabellini
2021-11-09 22:31 ` Julien Grall
2021-11-10 7:40 ` Jan Beulich
2021-11-10 13:05 ` Luca Fancellu
2021-11-10 13:36 ` Julien Grall
2021-11-10 14:02 ` Luca Fancellu
2021-11-15 18:57 ` Julien Grall
2021-11-15 22:00 ` Stefano Stabellini
2021-11-16 8:36 ` Luca Fancellu
2021-11-16 15:08 ` Ian Jackson
2021-11-16 16:11 ` Jan Beulich
2021-11-16 16:23 ` Julien Grall
2021-11-05 7:32 ` Jan Beulich
2021-11-05 7:27 ` Jan Beulich
2021-11-04 20:51 ` Stefano Stabellini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9bd58aa4-602b-4c64-e759-581513909457@suse.com \
--to=jbeulich@suse.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=bertrand.marquis@arm.com \
--cc=iwj@xenproject.org \
--cc=julien@xen.org \
--cc=luca.fancellu@arm.com \
--cc=sstabellini@kernel.org \
--cc=wei.chen@arm.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).