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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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;