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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

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

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

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.

Cheers,
Luca