[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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;

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.