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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

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

Cheers,
Luca