> On 23 Sep 2021, at 17:59, Stefano Stabellini <sstabell...@kernel.org> 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
