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:
> 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>

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"

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;