Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 24 Sep 2021, Luca Fancellu wrote:
> > On 23 Sep 2021, at 17:59, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> > 
> > On Thu, 23 Sep 2021, Luca Fancellu wrote:
> >>>> +/*
> >>>> + * Binaries will be translated into bootmodules, the maximum number for 
> >>>> them is
> >>>> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> >>>> + */
> >>>> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> >>>> +static struct file __initdata dom0less_file;
> >>>> +static dom0less_module_name __initdata 
> >>>> dom0less_modules[MAX_DOM0LESS_MODULES];
> >>>> +static unsigned int __initdata dom0less_modules_available =
> >>>> +                               MAX_DOM0LESS_MODULES;
> >>>> +static unsigned int __initdata dom0less_modules_idx;
> >>> 
> >>> This is a lot better!
> >>> 
> >>> We don't need both dom0less_modules_idx and dom0less_modules_available.
> >>> You can just do:
> >>> 
> >>> #define dom0less_modules_available (MAX_DOM0LESS_MODULES - 
> >>> dom0less_modules_idx)
> >>> static unsigned int __initdata dom0less_modules_idx;
> >>> 
> >>> But maybe we can even get rid of dom0less_modules_available entirely?
> >>> 
> >>> We can change the check at the beginning of allocate_dom0less_file to:
> >>> 
> >>> if ( dom0less_modules_idx == dom0less_modules_available )
> >>>   blexit
> >>> 
> >>> Would that work?
> >> 
> >> I thought about it but I think they need to stay, because 
> >> dom0less_modules_available is the
> >> upper bound for the additional dom0less modules (it is decremented each 
> >> time a dom0 module
> >> Is added), instead dom0less_modules_idx is the typical index for the array 
> >> of dom0less modules.
> > 
> > [...]
> > 
> > 
> >>>> +    /*
> >>>> +     * Check if there is any space left for a domU module, the variable
> >>>> +     * dom0less_modules_available is updated each time we use 
> >>>> read_file(...)
> >>>> +     * successfully.
> >>>> +     */
> >>>> +    if ( !dom0less_modules_available )
> >>>> +        blexit(L"No space left for domU modules");
> >>> 
> >>> This is the check that could be based on dom0less_modules_idx
> >>> 
> >> 
> >> The only way I see to have it based on dom0less_modules_idx will be to 
> >> compare it
> >> to the amount of modules still available, that is not constant because it 
> >> is dependent
> >> on how many dom0 modules are loaded, so still two variables needed.
> >> Don’t know if I’m missing something.
> > 
> > I think I understand where the confusion comes from. I am appending a
> > small patch to show what I had in mind. We are already accounting for
> > Xen and the DTB when declaring MAX_DOM0LESS_MODULES (MAX_MODULES - 2).
> > The other binaries are the Dom0 kernel and ramdisk, however, in my setup
> > they don't trigger a call to handle_dom0less_module_node because they
> > are compatible xen,linux-zimage and xen,linux-initrd.
> > 
> > However, the Dom0 kernel and ramdisk can be also compatible
> > multiboot,kernel and multiboot,ramdisk. If that is the case, then they
> > would indeed trigger a call to handle_dom0less_module_node.
> > 
> > I think that is not a good idea: a function called
> > handle_dom0less_module_node should only be called for dom0less modules
> > (domUs) and not dom0.

I can see that I misread the code yesterday: Dom0 modules don't go
through handle_dom0less_module_node thanks to the xen,domain check in
efi_arch_check_dom0less_boot.


> > But from the memory consumption point of view, it would be better
> > actually to catch dom0 modules too as you intended. In that case we need to:
> > 
> > - add a check for xen,linux-zimage and xen,linux-initrd in
> >  handle_dom0less_module_node also
> > 
> > - rename handle_dom0less_domain_node, handle_dom0less_module_node,
> >  dom0less_file, dom0less_modules, dom0less_modules_idx to something
> >  else more generic
> > 
> > 
> > For instance they could be called:
> > 
> > handle_domain_node
> > handle_module_node
> > module_file
> > modules
> > modules_idx
> > 
> > 
> > 
> > 
> > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> > index e2b007ece0..812d0bd607 100644
> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -22,8 +22,6 @@ typedef struct {
> > #define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> > static struct file __initdata dom0less_file;
> > static dom0less_module_name __initdata 
> > dom0less_modules[MAX_DOM0LESS_MODULES];
> > -static unsigned int __initdata dom0less_modules_available =
> > -                               MAX_DOM0LESS_MODULES;
> > static unsigned int __initdata dom0less_modules_idx;
> > 
> > #define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
> > @@ -592,14 +590,6 @@ static void __init efi_arch_handle_module(const struct 
> > file *file,
> >          * stop here.
> >          */
> >         blexit(L"Unknown module type");
> > -
> > -    /*
> > -     * dom0less_modules_available is decremented here because for each dom0
> > -     * file added, there will be an additional bootmodule, so the number
> > -     * of dom0less module files will be decremented because there is
> > -     * a maximum amount of bootmodules that can be loaded.
> > -     */
> > -    dom0less_modules_available--;
> > }
> > 
> > /*
> > @@ -643,7 +633,7 @@ static unsigned int __init 
> > allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> >      * dom0less_modules_available is updated each time we use read_file(...)
> >      * successfully.
> >      */
> > -    if ( !dom0less_modules_available )
> > +    if ( dom0less_modules_idx == MAX_DOM0LESS_MODULES )
> >         blexit(L"No space left for domU modules");
> > 
> >     module_name.s = (char*) name;
> 
> Hi Stefano,
> 
> Yes I understand what you mean, unfortunately it can’t be done, I will 
> explain why in details
> because the UEFI code is very tricky in the way it was written.
> 
> Dom0 modules and xsm-policy are handled in boot.c by read_section(…) or 
> read_file(…) and
> both call handle_file_info(…) that inside calls efi_arch_handle_module(…).
> Dom0less modules (xen,domain child modules) are handled in efi-boot.h by 
> handle_dom0less_module_node(…)
> that may call allocate_dom0less_file(…) and (to reuse code) calls 
> read_file(…).
> 
> So there can be maximum (MAX_MODULES-2) boot modules because at start in 
> start_xen(…)
> we add Xen and the DTB as boot modules.
> 
> This amount is to be shared among dom0 modules (kernel, ramdisk), xsm-policy 
> and domU
> modules (kernel, ramdisk, dtb).
> 
> The thing is that we don’t know how many dom0 modules will be specified and 
> also for the xsm-policy.
> 
> In your code example, let’s say I have MAX_DOM0LESS_MODULES=(32-2) so 30 
> modules available,
> If I declare a Dom0 kernel and 30 DomU kernels, it will pass the check, but 
> on boot I think it will ignore
> the exceeding modules.
> 
> For that reason we need to keep track of how many “slots” are available, so 
> in my code every time the
> read_file(…)/read_section(…) is called, the dom0less_modules_available is 
> decremented.
> 
> If we want to get rid of one variable, we can just assume the dom0 modules 
> and xsm-policy will be always
> loaded and we can set MAX_DOM0LESS_MODULES=(MAX_MODULES - 2 - 3) where 3 is 
> dom0 kernel,
> dom0 ramdisk and xsm-policy. If the user doesn’t load any of them, we just 
> lost 3 slots.
> 
> Another solution can be keeping just the dom0less_modules_available and every 
> time loop backward in the array
> starting from that position and stopping when we have a 
> dom0less_module_name.name == NULL so we
> know we have an available slot or we reach below zero and we know there is no 
> space. But I think it’s not
> worthy.
> 
> So if you really want to have only one variable, I will remove space from 
> MAX_DOM0LESS_MODULES and
> I will push it in v3.
 
You are right, let's just keep dom0less_modules_available, thanks for
the explanation.

One suggestion for future improvement (this patch series is OK as is)
would be to extend get_dom0less_file_index to also be able to search for
the dom0 kernel and ramdisk modules so that if a dom0less kernel or
ramdisk has the same filename as the dom0 kernel or ramdisk, the file
doesn't need to be loaded twice. It could be a follow-up patch after
this series gets committed.