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

Re: [PATCH v6 2/2] arm/efi: load dom0 modules from DT using UEFI



On Mon, 11 Oct 2021, Stefano Stabellini wrote:
> On Mon, 11 Oct 2021, Luca Fancellu wrote:
> > > On 11 Oct 2021, at 20:53, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> > > wrote:
> > > 
> > > On Mon, 11 Oct 2021, Luca Fancellu wrote:
> > >> Add support to load Dom0 boot modules from
> > >> the device tree using the xen,uefi-binary property.
> > >> 
> > >> Update documentation about that.
> > >> 
> > >> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > > 
> > 
> > Hi Stefano,
> > 
> > > Unfortunately, due to the change to the previous patch, this patch now
> > > has one issue, see below.
> > > 
> > > 
> > >> @@ -754,6 +760,41 @@ static int __init 
> > >> handle_module_node(EFI_FILE_HANDLE dir_handle,
> > >>         return ERROR_SET_REG_PROPERTY;
> > >>     }
> > >> 
> > >> +    if ( !is_domu_module )
> > >> +    {
> > >> +        if ( (fdt_node_check_compatible(fdt, module_node_offset,
> > >> +                                    "multiboot,kernel") == 0) )
> > >> +        {
> > >> +            /*
> > >> +            * This is the Dom0 kernel, wire it to the kernel variable 
> > >> because it
> > >> +            * will be verified by the shim lock protocol later in the 
> > >> common
> > >> +            * code.
> > >> +            */
> > >> +            if ( kernel.addr )
> > >> +            {
> > >> +                PrintMessage(L"Dom0 kernel already found in cfg file.");
> > >> +                return ERROR_DOM0_ALREADY_FOUND;
> > >> +            }
> > >> +            kernel.need_to_free = false; /* Freed using the module 
> > >> array */
> > >> +            kernel.addr = file->addr;
> > >> +            kernel.size = file->size;
> > >> +        }
> > >> +        else if ( ramdisk.addr &&
> > >> +                  (fdt_node_check_compatible(fdt, module_node_offset,
> > >> +                                             "multiboot,ramdisk") == 0) 
> > >> )
> > >> +        {
> > >> +            PrintMessage(L"Dom0 ramdisk already found in cfg file.");
> > >> +            return ERROR_DOM0_RAMDISK_FOUND;
> > >> +        }
> > >> +        else if ( xsm.addr &&
> > >> +                  (fdt_node_check_compatible(fdt, module_node_offset,
> > >> +                                             "xen,xsm-policy") == 0) )
> > >> +        {
> > >> +            PrintMessage(L"XSM policy already found in cfg file.");
> > >> +            return ERROR_XSM_ALREADY_FOUND;
> > >> +        }
> > >> +    }
> > >> +
> > >>     return 0;
> > >> }
> > >> 
> > >> @@ -793,7 +834,7 @@ static int __init 
> > >> handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> > >>           module_node = fdt_next_subnode(fdt, module_node) )
> > >>     {
> > >>         int ret = handle_module_node(dir_handle, module_node, addr_cells,
> > >> -                                        size_cells);
> > >> +                                     size_cells, true);
> > >>         if ( ret < 0 )
> > >>             return ret;
> > >>     }
> > >> @@ -803,7 +844,7 @@ static int __init 
> > >> handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> > >> 
> > >> /*
> > >>  * This function checks for xen domain nodes under the /chosen node for 
> > >> possible
> > >> - * domU guests to be loaded.
> > >> + * dom0 and domU guests to be loaded.
> > >>  * Returns the number of modules loaded or a negative number for error.
> > >>  */
> > >> static int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> > >> @@ -830,6 +871,9 @@ static int __init efi_check_dt_boot(EFI_FILE_HANDLE 
> > >> dir_handle)
> > >>             if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
> > >>                 return ERROR_DT_MODULE_DOMU;
> > >>         }
> > >> +        else if ( handle_module_node(dir_handle, node, addr_len, 
> > >> size_len,
> > >> +                                     false) < 0 )
> > >> +                 return ERROR_DT_MODULE_DOM0;
> > >>     }
> > > 
> > > handle_module_node comes with a "multiboot,module" compatible check now,
> > > which is fine for dom0less DomU modules, but it is not OK for dom0
> > > modules.
> > > 
> > > That is because it is also possible to write the dom0 modules (kernel
> > > and ramdisk) with the following compabile strings:
> > > 
> > > /chosen/dom0 compatible "xen,linux-zimage" "xen,multiboot-module"
> > > /chosen/dom0-ramdisk compatible "xen,linux-initrd" "xen,multiboot-module"
> > 
> > Oh ok I’m surprised because I think even before I was not managing any 
> > module
> > with “xen,multiboot-module”, just any multiboot,{kernel,ramdisk,device-tree}
> 
> At least by looking at the code it seemed to work before, although we
> weren't explicitly checking for this case 
> 
>  
> > > They are legacy but we are not meant to break support for them. Also
> > > third party tools might still use them -- I checked and even
> > > ImageBuilder still uses them.
> > > 
> > > One way to solve the problem is to make the "multiboot,module"
> > > compatible check at the beginning of handle_module_node conditional on
> > > !is_domu_module.
> > > 
> > > Or maybe just ignore compabible errors if !is_domu_module. Something like:
> > > 
> > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> > > index 840728d6c0..cbfcd55449 100644
> > > --- a/xen/arch/arm/efi/efi-boot.h
> > > +++ b/xen/arch/arm/efi/efi-boot.h
> > > @@ -721,7 +721,7 @@ static int __init handle_module_node(EFI_FILE_HANDLE 
> > > dir_handle,
> > >         /* Error while checking the compatible string */
> > >         return ERROR_CHECK_MODULE_COMPAT;
> > > 
> > > -    if ( module_compat != 0 )
> > > +    if ( is_domu_module && module_compat != 0 )
> > >         /* Module is not a multiboot,module */
> > >         return 0;
> > > 
> > 
> > I can be ok with this change but it means that any node under chosen that 
> > is not a “xen,domain”
> > will be handled as it is a Dom0 boot module (if it has xen,uefi-binary), is 
> > it always true?
>  
> Good point. I don't think it is a safe assumption.
> 
> 
> > Otherwise I can do these changes:
> > 
> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -721,10 +721,19 @@ static int __init handle_module_node(EFI_FILE_HANDLE 
> > dir_handle,
> >          /* Error while checking the compatible string */
> >          return ERROR_CHECK_MODULE_COMPAT;
> >  
> > -    if ( module_compat != 0 )
> > +    if ( is_domu_module && (module_compat != 0) )
> >          /* Module is not a multiboot,module */
> >          return 0;
> >  
> > +    /*
> > +     * For Dom0 boot modules can be specified also using the legacy 
> > compatible
> > +     * xen,multiboot-module
> > +     */
> > +    if ( !is_domu_module && module_compat &&
> > +         (fdt_node_check_compatible(fdt, module_node_offset,
> > +                                    "xen,multiboot-module") != 0) )
> > +         return 0;
> > +
> >      /* Read xen,uefi-binary property to get the file name. */
> >      uefi_name_prop = fdt_getprop(fdt, module_node_offset, 
> > "xen,uefi-binary",
> >                                   &uefi_name_len);
> > @@ -763,7 +772,9 @@ static int __init handle_module_node(EFI_FILE_HANDLE 
> > dir_handle,
> >      if ( !is_domu_module )
> >      {
> >          if ( (fdt_node_check_compatible(fdt, module_node_offset,
> > -                                    "multiboot,kernel") == 0) )
> > +                                        "multiboot,kernel") == 0) ||
> > +             (fdt_node_check_compatible(fdt, module_node_offset,
> > +                                        "xen,linux-zimage") == 0) )
> >          {
> >              /*
> >              * This is the Dom0 kernel, wire it to the kernel variable 
> > because it
> > @@ -780,8 +791,10 @@ static int __init handle_module_node(EFI_FILE_HANDLE 
> > dir_handle,
> >              kernel.size = file->size;
> >          }
> >          else if ( ramdisk.addr &&
> > -                  (fdt_node_check_compatible(fdt, module_node_offset,
> > -                                             "multiboot,ramdisk") == 0) )
> > +                  ((fdt_node_check_compatible(fdt, module_node_offset,
> > +                                              "multiboot,ramdisk") == 0) ||
> > +                   (fdt_node_check_compatible(fdt, module_node_offset,
> > +                                              "xen,linux-initrd") == 0)) )
> >          {
> >              PrintMessage(L"Dom0 ramdisk already found in cfg file.");
> >              return ERROR_DOM0_RAMDISK_FOUND;
> > 
> > 
> > I would need to check for “xen,linux-zimage” and “xen,linux-initrd” however
> > to be sure the user is not specifying the kernel and ramdisk twice.
> > 
> > Please let me know if you agree or if there is some issue with them.
> 
> I have another idea: I don't think we need to actually check for
> "xen,linux-zimage" or "xen,linux-initrd" because I am pretty sure they
> were always used in conjunction with "xen,multiboot-module".
> 
> So maybe it is enough to check for:
> 
> - for dom0: "xen,multiboot-module"
> - domU: "multiboot,module"
> 
> 
> E.g.:
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 840728d6c0..076b827bdd 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -713,10 +713,12 @@ static int __init handle_module_node(EFI_FILE_HANDLE 
> dir_handle,
>      char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
>      int uefi_name_len, file_idx, module_compat;
>      module_name *file;
> +    const char *compat_string = is_domu_module ? "multiboot,module" :
> +                                "xen,multiboot-module";
>  
>      /* Check if the node is a multiboot,module otherwise return */
>      module_compat = fdt_node_check_compatible(fdt, module_node_offset,
> -                                              "multiboot,module");
> +                                              compat_string);
>      if ( module_compat < 0 )
>          /* Error while checking the compatible string */
>          return ERROR_CHECK_MODULE_COMPAT;


Well... not exactly like this because this would stop a normal
"multiboot,module" dom0 kernel from being recognized.

So we need for domU: only "multiboot,module"
For Dom0, either "multiboot,module" or "xen,multiboot-module"

 


Rackspace

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