From: Stefano Stabellini <sstabellini@kernel.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
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: Fri, 5 Nov 2021 08:33:14 -0700 (PDT) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2111050825240.284830@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <e4b2e1be-0e41-0e6e-5ea8-3c12b4593724@suse.com>
[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]
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)?
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).
next prev parent reply other threads:[~2021-11-05 15:33 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 [this message]
2021-11-08 7:25 ` Jan Beulich
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=alpine.DEB.2.22.394.2111050825240.284830@ubuntu-linux-20-04-desktop \
--to=sstabellini@kernel.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=bertrand.marquis@arm.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=luca.fancellu@arm.com \
--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).