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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 8 Nov 2021, Jan Beulich wrote:
> 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.

Exactly my thinking! Except that now it gets called 10x times with
dom0less boot :-(


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

I understand the idea of passing NULL instead of "leaf", but I tried
having a look and I can't tell what we would be able to skip in
get_parent_handle.

Should we have a global variable to keep the dir_handle open during
dom0less module loading?