On Fri, 24 Sep 2021, Luca Fancellu wrote: > > On 23 Sep 2021, at 17:59, Stefano Stabellini 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.