[PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI

Luca Fancellu posted 3 patches 3 years, 2 months ago
There is a newer version of this series
[PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
Posted by Luca Fancellu 3 years, 2 months ago
Add support to load Dom0 boot modules from
the device tree using the uefi,binary property.

Update documentation about that.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes in v3:
- new patch
---
 docs/misc/arm/device-tree/booting.txt |  8 ++++
 docs/misc/efi.pandoc                  | 64 +++++++++++++++++++++++++--
 xen/arch/arm/efi/efi-boot.h           | 36 +++++++++++++--
 xen/common/efi/boot.c                 | 12 ++---
 4 files changed, 108 insertions(+), 12 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 354bb43fe1..e73f6476d4 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -70,6 +70,14 @@ Each node contains the following properties:
 	priority of this field vs. other mechanisms of specifying the
 	bootargs for the kernel.
 
+- uefi,binary (UEFI boot only)
+
+	String property that specifies the file name to be loaded by the UEFI
+	boot for this module. If this is specified, there is no need to specify
+	the reg property because it will be created by the UEFI stub on boot.
+	This option is needed only when UEFI boot is used, the node needs to be
+	compatible with multiboot,kernel or multiboot,ramdisk.
+
 Examples
 ========
 
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 800e67a233..4cebc47a18 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -167,6 +167,28 @@ sbsign \
 	--output xen.signed.efi \
 	xen.unified.efi
 ```
+## UEFI boot and Dom0 modules on ARM
+
+When booting using UEFI on ARM, it is possible to specify the Dom0 modules
+directly from the device tree without using the Xen configuration file, here an
+example:
+
+chosen {
+	#size-cells = <0x1>;
+	#address-cells = <0x1>;
+	xen,xen-bootargs = "[Xen boot arguments]"
+
+	module@1 {
+		compatible = "multiboot,kernel", "multiboot,module";
+		uefi,binary = "vmlinuz-3.0.31-0.4-xen";
+		bootargs = "[domain 0 command line options]";
+	};
+
+	module@2 {
+		compatible = "multiboot,ramdisk", "multiboot,module";
+		uefi,binary = "initrd-3.0.31-0.4-xen";
+	};
+}
 
 ## UEFI boot and dom0less on ARM
 
@@ -326,10 +348,10 @@ chosen {
 ### Boot Xen, Dom0 and DomU(s)
 
 This configuration is a mix of the two configuration above, to boot this one
-the configuration file must be processed so the /chosen node must have the
-"uefi,cfg-load" property.
+the configuration file can be processed or the Dom0 modules can be read from
+the device tree.
 
-Here an example:
+Here the first example:
 
 Xen configuration file:
 
@@ -369,4 +391,40 @@ chosen {
 };
 ```
 
+Here the second example:
+
+Device tree:
+
+```
+chosen {
+	#size-cells = <0x1>;
+	#address-cells = <0x1>;
+	xen,xen-bootargs = "[Xen boot arguments]"
+
+	module@1 {
+		compatible = "multiboot,kernel", "multiboot,module";
+		uefi,binary = "vmlinuz-3.0.31-0.4-xen";
+		bootargs = "[domain 0 command line options]";
+	};
+
+	module@2 {
+		compatible = "multiboot,ramdisk", "multiboot,module";
+		uefi,binary = "initrd-3.0.31-0.4-xen";
+	};
+
+	domU1 {
+		#size-cells = <0x1>;
+		#address-cells = <0x1>;
+		compatible = "xen,domain";
+		cpus = <0x1>;
+		memory = <0x0 0xc0000>;
+		vpl011;
 
+		module@1 {
+			compatible = "multiboot,kernel", "multiboot,module";
+			uefi,binary = "Image-domu1.bin";
+			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
+		};
+	};
+};
+```
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 4f7c913f86..df63387136 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -31,8 +31,10 @@ static unsigned int __initdata modules_idx;
 #define ERROR_MISSING_DT_PROPERTY   (-3)
 #define ERROR_RENAME_MODULE_NAME    (-4)
 #define ERROR_SET_REG_PROPERTY      (-5)
+#define ERROR_DOM0_ALREADY_FOUND    (-6)
 #define ERROR_DT_MODULE_DOMU        (-1)
 #define ERROR_DT_CHOSEN_NODE        (-2)
+#define ERROR_DT_MODULE_DOM0        (-3)
 
 void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
 void __flush_dcache_area(const void *vaddr, unsigned long size);
@@ -45,7 +47,8 @@ static int allocate_module_file(EFI_FILE_HANDLE dir_handle,
 static int handle_module_node(EFI_FILE_HANDLE dir_handle,
                               int module_node_offset,
                               int reg_addr_cells,
-                              int reg_size_cells);
+                              int reg_size_cells,
+                              bool is_domu_module);
 static bool is_boot_module(int dt_module_offset);
 static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
                                        int domain_node);
@@ -701,7 +704,8 @@ static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle,
 static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
                                      int module_node_offset,
                                      int reg_addr_cells,
-                                     int reg_size_cells)
+                                     int reg_size_cells,
+                                     bool is_domu_module)
 {
     const void *uefi_name_prop;
     char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
@@ -743,6 +747,24 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
         return ERROR_SET_REG_PROPERTY;
     }
 
+    if ( !is_domu_module &&
+         (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;
+    }
+
     return 0;
 }
 
@@ -799,7 +821,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
         if ( is_boot_module(module_node) )
         {
             int ret = handle_module_node(dir_handle, module_node, addr_cells,
-                                         size_cells);
+                                         size_cells, true);
             if ( ret < 0 )
                 return ret;
         }
@@ -809,7 +831,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_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
@@ -836,6 +858,12 @@ static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
             if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
                 return ERROR_DT_MODULE_DOMU;
         }
+        else if ( is_boot_module(node) )
+        {
+            if ( handle_module_node(dir_handle, node, addr_len, size_len,
+                                    false) < 0 )
+                return ERROR_DT_MODULE_DOM0;
+        }
     }
 
     /* Free dom0less file names if any */
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index c8c57fbb54..b221494a06 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         {
             read_file(dir_handle, s2w(&name), &kernel, option_str);
             efi_bs->FreePool(name.w);
-
-            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
-                            (void **)&shim_lock)) &&
-                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
-                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
         }
 
         if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
@@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if (dt_module_found < 0)
         /* efi_arch_check_dt_boot throws some error */
         blexit(L"Error processing boot modules on DT.");
+
+    /* If Dom0 is specified, verify it */
+    if ( kernel.ptr &&
+         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
+                                           (void **)&shim_lock)) &&
+        (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
+        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
     /*
      * Check if a proper configuration is provided to start Xen:
      *  - Dom0 specified (minimum required)
-- 
2.17.1


Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
Posted by Stefano Stabellini 3 years, 2 months ago
On Tue, 28 Sep 2021, Luca Fancellu wrote:
> Add support to load Dom0 boot modules from
> the device tree using the uefi,binary property.
> 
> Update documentation about that.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

It is great how simple this patch is!

The patch looks all correct. Only one question: do we need a check to
make sure the dom0 ramdisk is not loaded twice? Once via uefi,binary and
another time via the config file? In other words...

> ---
> Changes in v3:
> - new patch
> ---
>  docs/misc/arm/device-tree/booting.txt |  8 ++++
>  docs/misc/efi.pandoc                  | 64 +++++++++++++++++++++++++--
>  xen/arch/arm/efi/efi-boot.h           | 36 +++++++++++++--
>  xen/common/efi/boot.c                 | 12 ++---
>  4 files changed, 108 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 354bb43fe1..e73f6476d4 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -70,6 +70,14 @@ Each node contains the following properties:
>  	priority of this field vs. other mechanisms of specifying the
>  	bootargs for the kernel.
>  
> +- uefi,binary (UEFI boot only)
> +
> +	String property that specifies the file name to be loaded by the UEFI
> +	boot for this module. If this is specified, there is no need to specify
> +	the reg property because it will be created by the UEFI stub on boot.
> +	This option is needed only when UEFI boot is used, the node needs to be
> +	compatible with multiboot,kernel or multiboot,ramdisk.
> +
>  Examples
>  ========
>  
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index 800e67a233..4cebc47a18 100644
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -167,6 +167,28 @@ sbsign \
>  	--output xen.signed.efi \
>  	xen.unified.efi
>  ```
> +## UEFI boot and Dom0 modules on ARM
> +
> +When booting using UEFI on ARM, it is possible to specify the Dom0 modules
> +directly from the device tree without using the Xen configuration file, here an
> +example:
> +
> +chosen {
> +	#size-cells = <0x1>;
> +	#address-cells = <0x1>;
> +	xen,xen-bootargs = "[Xen boot arguments]"
> +
> +	module@1 {
> +		compatible = "multiboot,kernel", "multiboot,module";
> +		uefi,binary = "vmlinuz-3.0.31-0.4-xen";
> +		bootargs = "[domain 0 command line options]";
> +	};
> +
> +	module@2 {
> +		compatible = "multiboot,ramdisk", "multiboot,module";
> +		uefi,binary = "initrd-3.0.31-0.4-xen";
> +	};
> +}
>  
>  ## UEFI boot and dom0less on ARM
>  
> @@ -326,10 +348,10 @@ chosen {
>  ### Boot Xen, Dom0 and DomU(s)
>  
>  This configuration is a mix of the two configuration above, to boot this one
> -the configuration file must be processed so the /chosen node must have the
> -"uefi,cfg-load" property.
> +the configuration file can be processed or the Dom0 modules can be read from
> +the device tree.
>  
> -Here an example:
> +Here the first example:
>  
>  Xen configuration file:
>  
> @@ -369,4 +391,40 @@ chosen {
>  };
>  ```
>  
> +Here the second example:
> +
> +Device tree:
> +
> +```
> +chosen {
> +	#size-cells = <0x1>;
> +	#address-cells = <0x1>;
> +	xen,xen-bootargs = "[Xen boot arguments]"
> +
> +	module@1 {
> +		compatible = "multiboot,kernel", "multiboot,module";
> +		uefi,binary = "vmlinuz-3.0.31-0.4-xen";
> +		bootargs = "[domain 0 command line options]";
> +	};
> +
> +	module@2 {
> +		compatible = "multiboot,ramdisk", "multiboot,module";
> +		uefi,binary = "initrd-3.0.31-0.4-xen";
> +	};
> +
> +	domU1 {
> +		#size-cells = <0x1>;
> +		#address-cells = <0x1>;
> +		compatible = "xen,domain";
> +		cpus = <0x1>;
> +		memory = <0x0 0xc0000>;
> +		vpl011;
>  
> +		module@1 {
> +			compatible = "multiboot,kernel", "multiboot,module";
> +			uefi,binary = "Image-domu1.bin";
> +			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
> +		};
> +	};
> +};
> +```
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 4f7c913f86..df63387136 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -31,8 +31,10 @@ static unsigned int __initdata modules_idx;
>  #define ERROR_MISSING_DT_PROPERTY   (-3)
>  #define ERROR_RENAME_MODULE_NAME    (-4)
>  #define ERROR_SET_REG_PROPERTY      (-5)
> +#define ERROR_DOM0_ALREADY_FOUND    (-6)
>  #define ERROR_DT_MODULE_DOMU        (-1)
>  #define ERROR_DT_CHOSEN_NODE        (-2)
> +#define ERROR_DT_MODULE_DOM0        (-3)
>  
>  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>  void __flush_dcache_area(const void *vaddr, unsigned long size);
> @@ -45,7 +47,8 @@ static int allocate_module_file(EFI_FILE_HANDLE dir_handle,
>  static int handle_module_node(EFI_FILE_HANDLE dir_handle,
>                                int module_node_offset,
>                                int reg_addr_cells,
> -                              int reg_size_cells);
> +                              int reg_size_cells,
> +                              bool is_domu_module);
>  static bool is_boot_module(int dt_module_offset);
>  static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>                                         int domain_node);
> @@ -701,7 +704,8 @@ static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle,
>  static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
>                                       int module_node_offset,
>                                       int reg_addr_cells,
> -                                     int reg_size_cells)
> +                                     int reg_size_cells,
> +                                     bool is_domu_module)
>  {
>      const void *uefi_name_prop;
>      char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
> @@ -743,6 +747,24 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
>          return ERROR_SET_REG_PROPERTY;
>      }
>  
> +    if ( !is_domu_module &&
> +         (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;
> +    }

... is it necessary to update ramdisk as well or check if it is already
set? As far as I can tell it is the only other potential conflict that
we could have.


>      return 0;
>  }
>  
> @@ -799,7 +821,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>          if ( is_boot_module(module_node) )
>          {
>              int ret = handle_module_node(dir_handle, module_node, addr_cells,
> -                                         size_cells);
> +                                         size_cells, true);
>              if ( ret < 0 )
>                  return ret;
>          }
> @@ -809,7 +831,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_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> @@ -836,6 +858,12 @@ static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
>              if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
>                  return ERROR_DT_MODULE_DOMU;
>          }
> +        else if ( is_boot_module(node) )
> +        {
> +            if ( handle_module_node(dir_handle, node, addr_len, size_len,
> +                                    false) < 0 )
> +                return ERROR_DT_MODULE_DOM0;
> +        }
>      }
>  
>      /* Free dom0less file names if any */
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index c8c57fbb54..b221494a06 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          {
>              read_file(dir_handle, s2w(&name), &kernel, option_str);
>              efi_bs->FreePool(name.w);
> -
> -            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> -                            (void **)&shim_lock)) &&
> -                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> -                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>          }
>  
>          if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      if (dt_module_found < 0)
>          /* efi_arch_check_dt_boot throws some error */
>          blexit(L"Error processing boot modules on DT.");
> +
> +    /* If Dom0 is specified, verify it */
> +    if ( kernel.ptr &&
> +         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> +                                           (void **)&shim_lock)) &&
> +        (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> +        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>      /*
>       * Check if a proper configuration is provided to start Xen:
>       *  - Dom0 specified (minimum required)
> -- 
> 2.17.1
> 

Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
Posted by Jan Beulich 3 years, 2 months ago
On 28.09.2021 18:32, Luca Fancellu wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          {
>              read_file(dir_handle, s2w(&name), &kernel, option_str);
>              efi_bs->FreePool(name.w);
> -
> -            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> -                            (void **)&shim_lock)) &&
> -                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> -                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>          }
>  
>          if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      if (dt_module_found < 0)
>          /* efi_arch_check_dt_boot throws some error */
>          blexit(L"Error processing boot modules on DT.");
> +
> +    /* If Dom0 is specified, verify it */
> +    if ( kernel.ptr &&
> +         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> +                                           (void **)&shim_lock)) &&
> +        (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> +        PrintErrMesg(L"Dom0 kernel image could not be verified", status);

Why does this code need moving in the first place? That's (to me at least)
not obvious from looking just at the common (i.e. non-Arm-specific) part.
Hence I would wish for the comment to be slightly extended to indicate
that the kernel image may also have been loaded by <whichever function>.

Also, two nits: You've broken indentation here (you've lost a space too
much compared to the original code) and ...

>      /*
>       * Check if a proper configuration is provided to start Xen:
>       *  - Dom0 specified (minimum required)
> 

... you will want to insert a blank line not only before, but also after
your insertion (or, imo less desirable, neither of the two blank lines).

Further I wonder whether your addition wouldn't better be after the code
following this comment.

And finally I wonder (looking back at the earlier patch) why you use
kernel.addr there when kernel.ptr is being used here. Unless there's a
reason for the difference, being consistent would be better.

Jan


Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
Posted by Luca Fancellu 3 years, 2 months ago

> On 29 Sep 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 28.09.2021 18:32, Luca Fancellu wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>         {
>>             read_file(dir_handle, s2w(&name), &kernel, option_str);
>>             efi_bs->FreePool(name.w);
>> -
>> -            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
>> -                            (void **)&shim_lock)) &&
>> -                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
>> -                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>>         }
>> 
>>         if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
>> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>     if (dt_module_found < 0)
>>         /* efi_arch_check_dt_boot throws some error */
>>         blexit(L"Error processing boot modules on DT.");
>> +
>> +    /* If Dom0 is specified, verify it */
>> +    if ( kernel.ptr &&
>> +         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
>> +                                           (void **)&shim_lock)) &&
>> +        (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
>> +        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> 
> Why does this code need moving in the first place? That's (to me at least)
> not obvious from looking just at the common (i.e. non-Arm-specific) part.
> Hence I would wish for the comment to be slightly extended to indicate
> that the kernel image may also have been loaded by <whichever function>.
> 

Sure I will improve the comment to explain that.

> Also, two nits: You've broken indentation here (you've lost a space too
> much compared to the original code) and ...

Yes sorry for that, we didn’t see it, I will fix it.

> 
>>     /*
>>      * Check if a proper configuration is provided to start Xen:
>>      *  - Dom0 specified (minimum required)
>> 
> 
> ... you will want to insert a blank line not only before, but also after
> your insertion (or, imo less desirable, neither of the two blank lines).
> 
> Further I wonder whether your addition wouldn't better be after the code
> following this comment.
> 
> And finally I wonder (looking back at the earlier patch) why you use
> kernel.addr there when kernel.ptr is being used here. Unless there's a
> reason for the difference, being consistent would be better.

I will do all of the above for the v4.

Thanks for all the feedbacks.

Cheers,
Luca

> 
> Jan
>