[PATCH v2 0/2] arm/efi: Add dom0less support to UEFI boot

[PATCH v2 0/2] arm/efi: Add dom0less support to UEFI boot

[Luca Fancellu]

This serie introduces a way to start a dom0less setup when Xen is booting as EFI
application.
Using the device tree it's now possible to fetch from the disk and load in
memory all the modules needed to start any domU defined in the DT.
Dom0less for now is supported only by the arm architecture.

Luca Fancellu (2):
  arm/efi: Introduce uefi,cfg-load DT property
  arm/efi: Use dom0less configuration when using EFI boot

 docs/misc/arm/device-tree/booting.txt |  21 ++
 docs/misc/efi.pandoc                  | 205 ++++++++++++++++++
 xen/arch/arm/efi/efi-boot.h           | 285 +++++++++++++++++++++++++-
 xen/arch/x86/efi/efi-boot.h           |   6 +
 xen/common/efi/boot.c                 |  36 ++--
 5 files changed, 533 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] arm/efi: Introduce uefi,cfg-load DT property

[Luca Fancellu]

Introduce the uefi,cfg-load DT property of /chosen
node for ARM whose presence decide whether to force
the load of the UEFI Xen configuration file.

The logic is that if any multiboot,module is found in
the DT, then the uefi,cfg-load property is used to see
if the UEFI Xen configuration file is needed.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
- Introduced uefi,cfg-load property
- Add documentation about the property
---
 docs/misc/efi.pandoc        |  2 ++
 xen/arch/arm/efi/efi-boot.h | 28 +++++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index ac3cd58cae..e289c5e7ba 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -14,6 +14,8 @@ loaded the modules and describes them in the device tree provided to Xen.  If a
 bootloader provides a device tree containing modules then any configuration
 files are ignored, and the bootloader is responsible for populating all
 relevant device tree nodes.
+The property "uefi,cfg-load" can be specified in the /chosen node to force Xen
+to load the configuration file even if multiboot modules are found.
 
 Once built, `make install-xen` will place the resulting binary directly into
 the EFI boot partition, provided `EFI_VENDOR` is set in the environment (and
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index cf9c37153f..8ceeba4ad1 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -581,22 +581,40 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
 
 static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
 {
+    bool skip_cfg_file = false;
     /*
      * For arm, we may get a device tree from GRUB (or other bootloader)
      * that contains modules that have already been loaded into memory.  In
-     * this case, we do not use a configuration file, and rely on the
-     * bootloader to have loaded all required modules and appropriate
-     * options.
+     * this case, we search for the property uefi,cfg-load in the /chosen node
+     * to decide whether to skip the UEFI Xen configuration file or not.
      */
 
     fdt = lookup_fdt_config_table(SystemTable);
     dtbfile.ptr = fdt;
     dtbfile.need_to_free = false; /* Config table memory can't be freed. */
-    if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 )
+
+    if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 )
+    {
+        /* Locate chosen node */
+        int node = fdt_subnode_offset(fdt, 0, "chosen");
+        const void *cfg_load_prop;
+        int cfg_load_len;
+
+        if ( node > 0 )
+        {
+            /* Check if uefi,cfg-load property exists */
+            cfg_load_prop = fdt_getprop(fdt, node, "uefi,cfg-load",
+                                        &cfg_load_len);
+            if ( !cfg_load_prop )
+                skip_cfg_file = true;
+        }
+    }
+
+    if ( !fdt || !skip_cfg_file )
     {
         /*
          * We either have no FDT, or one without modules, so we must have a
-         * Xen EFI configuration file to specify modules.  (dom0 required)
+         * Xen EFI configuration file to specify modules.
          */
         return true;
     }
-- 
2.17.1

[PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Luca Fancellu]

This patch introduces the support for dom0less configuration
when using UEFI boot on ARM, it permits the EFI boot to
continue if no dom0 kernel is specified but at least one domU
is found.

Introduce the new property "uefi,binary" for device tree boot
module nodes that are subnode of "xen,domain" compatible nodes.
The property holds a string containing the file name of the
binary that shall be loaded by the uefi loader from the filesystem.

Update efi documentation about how to start a dom0less
setup using UEFI

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes in v2:
- remove array of struct file
- fixed some int types
- Made the code use filesystem even when configuration
file is skipped.
- add documentation of uefi,binary in booting.txt
- add documentation on how to boot all configuration
for Xen using UEFI in efi.pandoc
---
 docs/misc/arm/device-tree/booting.txt |  21 +++
 docs/misc/efi.pandoc                  | 203 ++++++++++++++++++++
 xen/arch/arm/efi/efi-boot.h           | 257 +++++++++++++++++++++++++-
 xen/arch/x86/efi/efi-boot.h           |   6 +
 xen/common/efi/boot.c                 |  36 ++--
 5 files changed, 508 insertions(+), 15 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 44cd9e1a9a..bc0f8913db 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -182,6 +182,13 @@ The kernel sub-node has the following properties:
 
     Command line parameters for the guest kernel.
 
+- efi,binary (UEFI boot only)
+
+    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 ramdisk sub-node has the following properties:
 
 - compatible
@@ -193,6 +200,13 @@ The ramdisk sub-node has the following properties:
     Specifies the physical address of the ramdisk in RAM and its
     length.
 
+- efi,binary (UEFI boot only)
+
+    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.
+
 
 Example
 =======
@@ -257,6 +271,13 @@ The dtb sub-node should have the following properties:
     Specifies the physical address of the device tree binary fragment
     RAM and its length.
 
+- efi,binary (UEFI boot only)
+
+    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.
+
 As an example:
 
         module@0xc000000 {
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index e289c5e7ba..698196e129 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -167,3 +167,206 @@ sbsign \
 	--output xen.signed.efi \
 	xen.unified.efi
 ```
+
+## UEFI boot and dom0less on ARM
+
+Dom0less feature is supported by ARM and it is possible to use it when Xen is
+started as an EFI application.
+The way to specify the domU domains is by Device Tree as specified in the
+[dom0less](dom0less.html) documentation page under the "Device Tree
+configuration" section, but instead of declaring the reg property in the boot
+module, the user must specify the "uefi,binary" property containing the name
+of the binary file that has to be loaded in memory.
+The UEFI stub will load the binary in memory and it will add the reg property
+accordingly.
+
+An example here:
+
+domU1 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	compatible = "xen,domain";
+	memory = <0 0x20000>;
+	cpus = <1>;
+	vpl011;
+
+	module@1 {
+		compatible = "multiboot,kernel", "multiboot,module";
+		uefi,binary = "vmlinuz-3.0.31-0.4-xen";
+		bootargs = "console=ttyAMA0";
+	};
+	module@2 {
+		compatible = "multiboot,ramdisk", "multiboot,module";
+		uefi,binary = "initrd-3.0.31-0.4-xen";
+	};
+	module@3 {
+		compatible = "multiboot,ramdisk", "multiboot,module";
+		uefi,binary = "passthrough.dtb";
+	};
+};
+
+## How to boot different Xen setup using UEFI
+
+Here the supported user cases for Xen when UEFI boot is used:
+
+ - Boot Xen and Dom0 (minimum required)
+ - Boot Xen and DomU(s) (true dom0less, only on ARM)
+ - Boot Xen, Dom0 and DomU(s) (only on ARM)
+
+### Boot Xen and Dom0
+
+This configuration can be started using the Xen configuration file in the
+example above.
+
+### Boot Xen and DomU(s)
+
+This configuration needs the domU domain(s) specified in the /chosen node,
+examples of how to do that are provided by the documentation about dom0less
+and the example above shows how to use the "uefi,binary" property to use the
+UEFI stub for module loading.
+Providing the multiboot modules in the device tree, make Xen skip its UEFI
+configuration file, if it is needed for some reason, specify the "uefi,cfg-load"
+property in the /chosen node.
+
+Example 1 of how to boot a true dom0less configuration:
+
+Xen configuration file: skipped.
+
+Device tree:
+
+```
+chosen {
+	#size-cells = <0x1>;
+	#address-cells = <0x1>;
+	xen,xen-bootargs = "<Xen command line>"
+
+	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";
+		};
+	};
+	domU2 {
+		#size-cells = <0x1>;
+		#address-cells = <0x1>;
+		compatible = "xen,domain";
+		cpus = <0x1>;
+		memory = <0x0 0x100000>;
+		vpl011;
+
+		module@2 {
+			compatible = "multiboot,kernel", "multiboot,module";
+			uefi,binary = "Image-domu2.bin";
+			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
+		};
+	};
+};
+```
+
+Example 2 of how to boot a true dom0less configuration:
+
+Xen configuration file:
+
+```
+[global]
+default=xen
+
+[xen]
+options=<Xen command line>
+dtb=<optional DTB>
+```
+
+Device tree:
+
+```
+chosen {
+	#size-cells = <0x1>;
+	#address-cells = <0x1>;
+	uefi,cfg-load;
+
+	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";
+		};
+	};
+	domU2 {
+		#size-cells = <0x1>;
+		#address-cells = <0x1>;
+		compatible = "xen,domain";
+		cpus = <0x1>;
+		memory = <0x0 0x100000>;
+		vpl011;
+
+		module@2 {
+			compatible = "multiboot,kernel", "multiboot,module";
+			uefi,binary = "Image-domu2.bin";
+			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
+		};
+	};
+};
+```
+
+### 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.
+
+Here an example:
+
+Xen configuration file:
+
+```
+[global]
+default=xen
+
+[xen]
+options=<Xen command line>
+kernel=vmlinuz-3.0.31-0.4-xen [domain 0 command line options]
+ramdisk=initrd-3.0.31-0.4-xen
+dtb=<optional DTB>
+```
+
+Device tree:
+
+```
+chosen {
+	#size-cells = <0x1>;
+	#address-cells = <0x1>;
+	uefi,cfg-load;
+
+	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 8ceeba4ad1..e2b007ece0 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -8,9 +8,43 @@
 #include <asm/setup.h>
 #include <asm/smp.h>
 
+typedef struct {
+    char *name;
+    unsigned int name_len;
+    EFI_PHYSICAL_ADDRESS addr;
+    UINTN size;
+} dom0less_module_name;
+
+/*
+ * Binaries will be translated into bootmodules, the maximum number for them is
+ * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
+ */
+#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
+static struct file __initdata dom0less_file;
+static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
+static unsigned int __initdata dom0less_modules_available =
+                               MAX_DOM0LESS_MODULES;
+static unsigned int __initdata dom0less_modules_idx;
+
+#define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
+
 void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
 void __flush_dcache_area(const void *vaddr, unsigned long size);
 
+static int get_dom0less_file_index(const char *name, unsigned int name_len);
+static unsigned int allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
+                                           const char *name,
+                                           unsigned int name_len);
+static void handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
+                                        int module_node_offset,
+                                        int reg_addr_cells,
+                                        int reg_size_cells);
+static void handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
+                                        int domain_node,
+                                        int addr_cells,
+                                        int size_cells);
+static bool efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle);
+
 #define DEVICE_TREE_GUID \
 {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
 
@@ -552,8 +586,218 @@ static void __init efi_arch_handle_module(const struct file *file,
                          kernel.size) < 0 )
             blexit(L"Unable to set reg property.");
     }
-    else
+    else if ( file != &dom0less_file )
+        /*
+         * If file is not a dom0 module file and it's not a domU module,
+         * stop here.
+         */
         blexit(L"Unknown module type");
+
+    /*
+     * dom0less_modules_available is decremented here because for each dom0
+     * file added, there will be an additional bootmodule, so the number
+     * of dom0less module files will be decremented because there is
+     * a maximum amount of bootmodules that can be loaded.
+     */
+    dom0less_modules_available--;
+}
+
+/*
+ * This function checks for a binary previously loaded with a give name, it
+ * returns the index of the file in the dom0less_files array or a negative
+ * number if no file with that name is found.
+ */
+static int __init get_dom0less_file_index(const char *name,
+                                          unsigned int name_len)
+{
+    unsigned int i;
+    int ret = ERROR_DOM0LESS_FILE_NOT_FOUND;
+
+    for (i = 0; i < dom0less_modules_idx; i++)
+    {
+        dom0less_module_name *mod = &dom0less_modules[i];
+        if ( (mod->name_len == name_len) &&
+             (strncmp(mod->name, name, name_len) == 0) )
+        {
+            ret = i;
+            break;
+        }
+    }
+    return ret;
+}
+
+/*
+ * This function allocates a binary and keeps track of its name, it
+ * returns the index of the file in the dom0less_files array.
+ */
+static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
+                                                  const char *name,
+                                                  unsigned int name_len)
+{
+    dom0less_module_name* file_name;
+    union string module_name;
+    unsigned int ret_idx;
+
+    /*
+     * Check if there is any space left for a domU module, the variable
+     * dom0less_modules_available is updated each time we use read_file(...)
+     * successfully.
+     */
+    if ( !dom0less_modules_available )
+        blexit(L"No space left for domU modules");
+
+    module_name.s = (char*) name;
+    ret_idx = dom0less_modules_idx;
+
+    /* Save at this index the name of this binary */
+    file_name = &dom0less_modules[ret_idx];
+
+    if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
+                              (void**)&file_name->name) != EFI_SUCCESS )
+        blexit(L"Error allocating memory for dom0less binary name");
+
+    /* Save name and length of the binary in the data structure */
+    strlcpy(file_name->name, name, name_len);
+    file_name->name_len = name_len;
+
+    /* Load the binary in memory */
+    read_file(dir_handle, s2w(&module_name), &dom0less_file, NULL);
+
+    /* Save address and size */
+    file_name->addr = dom0less_file.addr;
+    file_name->size = dom0less_file.size;
+
+    /* s2w(...) allocates some memory, free it */
+    efi_bs->FreePool(module_name.w);
+
+    dom0less_modules_idx++;
+
+    return ret_idx;
+}
+
+/*
+ * This function checks for the presence of the uefi,binary property in the
+ * module, if found it loads the binary as dom0less module and sets the right
+ * address for the reg property into the module DT node.
+ */
+static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
+                                          int module_node_offset,
+                                          int reg_addr_cells,
+                                          int reg_size_cells)
+{
+    const void *uefi_name_prop;
+    char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
+    int uefi_name_len, file_idx;
+    dom0less_module_name *file;
+
+    /* Read uefi,binary property to get the file name. */
+    uefi_name_prop = fdt_getprop(fdt, module_node_offset, "uefi,binary",
+                                 &uefi_name_len);
+
+    if ( !uefi_name_prop )
+        /* Property not found */
+        return;
+
+    file_idx = get_dom0less_file_index(uefi_name_prop, uefi_name_len);
+    if (file_idx < 0)
+        file_idx = allocate_dom0less_file(dir_handle, uefi_name_prop,
+                                          uefi_name_len);
+
+    file = &dom0less_modules[file_idx];
+
+    snprintf(mod_string, sizeof(mod_string), "module@%"PRIx64, file->addr);
+
+    /* Rename the module to be module@{address} */
+    if ( fdt_set_name(fdt, module_node_offset, mod_string) < 0 )
+        blexit(L"Unable to add domU ramdisk FDT node.");
+
+    if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
+                     file->addr, file->size) < 0 )
+        blexit(L"Unable to set reg property.");
+}
+
+/*
+ * This function checks for boot modules under the domU guest domain node
+ * in the DT.
+ */
+static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
+                                               int domain_node,
+                                               int addr_cells,
+                                               int size_cells)
+{
+    int module_node;
+    /*
+     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
+     * inside this node
+     */
+    for ( module_node = fdt_first_subnode(fdt, domain_node);
+          module_node > 0;
+          module_node = fdt_next_subnode(fdt, module_node) )
+        if ( (fdt_node_check_compatible(fdt, module_node,
+                                        "multiboot,kernel") == 0) ||
+             (fdt_node_check_compatible(fdt, module_node,
+                                        "multiboot,ramdisk") == 0) ||
+             (fdt_node_check_compatible(fdt, module_node,
+                                        "multiboot,device-tree") == 0) )
+            /* The compatible is one of the strings above, check the module */
+            handle_dom0less_module_node(dir_handle, module_node, addr_cells,
+                                        size_cells);
+}
+
+/*
+ * This function checks for xen domain nodes under the /chosen node for possible
+ * domU guests to be loaded.
+ */
+static bool __init efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle)
+{
+    int chosen;
+    int addr_len, size_len;
+    unsigned int i = 0;
+
+    /* Check for the chosen node in the current DTB */
+    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
+    if ( chosen < 0 )
+        blexit(L"Unable to setup chosen node");
+
+    /* Check for nodes compatible with xen,domain under the chosen node */
+    for ( int node = fdt_first_subnode(fdt, chosen);
+          node > 0;
+          node = fdt_next_subnode(fdt, node) )
+    {
+        int addr_cells, size_cells, len;
+        const struct fdt_property *prop;
+
+        if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
+            continue;
+
+        /* Get or set #address-cells and #size-cells */
+        prop = fdt_get_property(fdt, node, "#address-cells", &len);
+        if ( !prop )
+            blexit(L"#address-cells not found in domain node.");
+
+        addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+        prop = fdt_get_property(fdt, node, "#size-cells", &len);
+        if ( !prop )
+            blexit(L"#size-cells not found in domain node.");
+
+        size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+        /* Found a node with compatible xen,domain; handle this node. */
+        handle_dom0less_domain_node(dir_handle, node, addr_cells, size_cells);
+    }
+
+    /* Free dom0less file names if any */
+    for ( ; i < dom0less_modules_idx; i++ )
+    {
+        /* Free dom0less binary names */
+        efi_bs->FreePool(dom0less_modules[i].name);
+    }
+
+    if ( dom0less_modules_idx > 0 )
+        return true;
+
+    return false;
 }
 
 static void __init efi_arch_cpu(void)
@@ -562,8 +806,19 @@ static void __init efi_arch_cpu(void)
 
 static void __init efi_arch_blexit(void)
 {
+    unsigned int i = 0;
+
     if ( dtbfile.need_to_free )
         efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
+    /* Free dom0less files if any */
+    for ( ; i < dom0less_modules_idx; i++ )
+    {
+        /* Free dom0less binary names */
+        efi_bs->FreePool(dom0less_modules[i].name);
+        /* Free dom0less binaries */
+        efi_bs->FreePages(dom0less_modules[i].addr,
+                          PFN_UP(dom0less_modules[i].size));
+    }
     if ( memmap )
         efi_bs->FreePool(memmap);
 }
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 9b0cc29aae..950fdf16b7 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -678,6 +678,12 @@ static void __init efi_arch_handle_module(const struct file *file,
     efi_bs->FreePool(ptr);
 }
 
+static bool __init efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle)
+{
+    /* x86 doesn't support dom0less */
+    return false;
+}
+
 static void __init efi_arch_cpu(void)
 {
     uint32_t eax = cpuid_eax(0x80000000);
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 758f9d74d2..7d8734199e 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1127,15 +1127,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
-    unsigned int i, argc;
-    CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
+    unsigned int i, argc = 0;
+    CHAR16 **argv, *file_name = NULL, *cfg_file_name = NULL, *options = NULL;
     UINTN gop_mode = ~0;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
     union string section = { NULL }, name;
     bool base_video = false;
-    const char *option_str;
+    const char *option_str = NULL;
     bool use_cfg_file;
+    EFI_FILE_HANDLE dir_handle;
 
     __set_bit(EFI_BOOT, &efi_flags);
     __set_bit(EFI_LOADER, &efi_flags);
@@ -1216,9 +1217,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     efi_arch_relocate_image(0);
 
+    /* Get the file system interface. */
+    dir_handle = get_parent_handle(loaded_image, &file_name);
+
     if ( use_cfg_file )
     {
-        EFI_FILE_HANDLE dir_handle;
         UINTN depth, cols, rows, size;
 
         size = cols = rows = depth = 0;
@@ -1229,9 +1232,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
         gop = efi_get_gop();
 
-        /* Get the file system interface. */
-        dir_handle = get_parent_handle(loaded_image, &file_name);
-
         /* Read and parse the config file. */
         if ( read_section(loaded_image, L"config", &cfg, NULL) )
             PrintStr(L"Using builtin config file\r\n");
@@ -1285,14 +1285,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
             efi_bs->FreePool(name.w);
         }
 
-        if ( !name.s )
-            blexit(L"No Dom0 kernel image specified.");
-
         efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
 
-        option_str = split_string(name.s);
+        if ( name.s )
+            option_str = split_string(name.s);
 
-        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) )
+        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) &&
+             name.s )
         {
             read_file(dir_handle, s2w(&name), &kernel, option_str);
             efi_bs->FreePool(name.w);
@@ -1361,12 +1360,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
         cfg.addr = 0;
 
-        dir_handle->Close(dir_handle);
-
         if ( gop && !base_video )
             gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
     }
 
+    /*
+     * Check if a proper configuration is provided to start Xen:
+     *  - Dom0 specified (minimum required)
+     *  - Dom0 and DomU(s) specified
+     *  - DomU(s) specified
+     */
+    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
+        blexit(L"No Dom0 kernel image specified.");
+
+    dir_handle->Close(dir_handle);
+
     efi_arch_edd();
 
     /* XXX Collect EDID info. */
-- 
2.17.1

Re: [PATCH v2 1/2] arm/efi: Introduce uefi,cfg-load DT property

[Stefano Stabellini]

On Wed, 22 Sep 2021, Luca Fancellu wrote:
> Introduce the uefi,cfg-load DT property of /chosen
> node for ARM whose presence decide whether to force
> the load of the UEFI Xen configuration file.
> 
> The logic is that if any multiboot,module is found in
> the DT, then the uefi,cfg-load property is used to see
> if the UEFI Xen configuration file is needed.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

The patch looks OK, just a couple of minor comments below.


> ---
> v2 changes:
> - Introduced uefi,cfg-load property
> - Add documentation about the property
> ---
>  docs/misc/efi.pandoc        |  2 ++
>  xen/arch/arm/efi/efi-boot.h | 28 +++++++++++++++++++++++-----
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index ac3cd58cae..e289c5e7ba 100644
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -14,6 +14,8 @@ loaded the modules and describes them in the device tree provided to Xen.  If a
>  bootloader provides a device tree containing modules then any configuration
>  files are ignored, and the bootloader is responsible for populating all
>  relevant device tree nodes.
> +The property "uefi,cfg-load" can be specified in the /chosen node to force Xen
> +to load the configuration file even if multiboot modules are found.

I think that, in addition to this, we also need to add the property to
docs/misc/arm/device-tree/booting.txt where our "official" device tree
bindings are maintained. I would add it below "Command lines" and before
"Creating Multiple Domains directly from Xen" maybe as a new chapter.
It could be called "Other Options" but other ideas could be valid too.

You can say that uefi,cfg-load is a boolean.


>  Once built, `make install-xen` will place the resulting binary directly into
>  the EFI boot partition, provided `EFI_VENDOR` is set in the environment (and
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index cf9c37153f..8ceeba4ad1 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -581,22 +581,40 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>  
>  static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>  {
> +    bool skip_cfg_file = false;
>      /*
>       * For arm, we may get a device tree from GRUB (or other bootloader)
>       * that contains modules that have already been loaded into memory.  In
> -     * this case, we do not use a configuration file, and rely on the
> -     * bootloader to have loaded all required modules and appropriate
> -     * options.
> +     * this case, we search for the property uefi,cfg-load in the /chosen node
> +     * to decide whether to skip the UEFI Xen configuration file or not.
>       */
>  
>      fdt = lookup_fdt_config_table(SystemTable);
>      dtbfile.ptr = fdt;
>      dtbfile.need_to_free = false; /* Config table memory can't be freed. */
> -    if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 )
> +
> +    if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 )
> +    {
> +        /* Locate chosen node */
> +        int node = fdt_subnode_offset(fdt, 0, "chosen");
> +        const void *cfg_load_prop;
> +        int cfg_load_len;
> +
> +        if ( node > 0 )
> +        {
> +            /* Check if uefi,cfg-load property exists */
> +            cfg_load_prop = fdt_getprop(fdt, node, "uefi,cfg-load",
> +                                        &cfg_load_len);
> +            if ( !cfg_load_prop )
> +                skip_cfg_file = true;
> +        }
> +    }
> +
> +    if ( !fdt || !skip_cfg_file )

Just a suggestion, but rather than the double negative, wouldn't it be
simpler to define it as

    bool load_cfg_file = true;

?


>      {
>          /*
>           * We either have no FDT, or one without modules, so we must have a
> -         * Xen EFI configuration file to specify modules.  (dom0 required)
> +         * Xen EFI configuration file to specify modules.
>           */

Also mention in the commit message that you are taking the opportunity
to update this comment do remove "dom0 required".


>          return true;
>      }
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Stefano Stabellini]

On Wed, 22 Sep 2021, Luca Fancellu wrote:
> This patch introduces the support for dom0less configuration
> when using UEFI boot on ARM, it permits the EFI boot to
> continue if no dom0 kernel is specified but at least one domU
> is found.
> 
> Introduce the new property "uefi,binary" for device tree boot
> module nodes that are subnode of "xen,domain" compatible nodes.
> The property holds a string containing the file name of the
> binary that shall be loaded by the uefi loader from the filesystem.
> 
> Update efi documentation about how to start a dom0less
> setup using UEFI
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Overall the patch is pretty good. I don't have any major concerns (only
one about the documentation). Some minor comments below.


> ---
> Changes in v2:
> - remove array of struct file
> - fixed some int types
> - Made the code use filesystem even when configuration
> file is skipped.
> - add documentation of uefi,binary in booting.txt
> - add documentation on how to boot all configuration
> for Xen using UEFI in efi.pandoc
> ---
>  docs/misc/arm/device-tree/booting.txt |  21 +++
>  docs/misc/efi.pandoc                  | 203 ++++++++++++++++++++
>  xen/arch/arm/efi/efi-boot.h           | 257 +++++++++++++++++++++++++-
>  xen/arch/x86/efi/efi-boot.h           |   6 +
>  xen/common/efi/boot.c                 |  36 ++--
>  5 files changed, 508 insertions(+), 15 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 44cd9e1a9a..bc0f8913db 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -182,6 +182,13 @@ The kernel sub-node has the following properties:
>  
>      Command line parameters for the guest kernel.
>  
> +- efi,binary (UEFI boot only)

Below it is actually uefi,binary, not efi,binary. I would use
uefi,binary consistently.


> +    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.

We need to specify that this parameter is a string.


>  The ramdisk sub-node has the following properties:
>  
>  - compatible
> @@ -193,6 +200,13 @@ The ramdisk sub-node has the following properties:
>      Specifies the physical address of the ramdisk in RAM and its
>      length.
>  
> +- efi,binary (UEFI boot only)
> +
> +    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.

same here

>  
>  Example
>  =======
> @@ -257,6 +271,13 @@ The dtb sub-node should have the following properties:
>      Specifies the physical address of the device tree binary fragment
>      RAM and its length.
>  
> +- efi,binary (UEFI boot only)
> +
> +    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.

same here


>  As an example:
>  
>          module@0xc000000 {
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index e289c5e7ba..698196e129 100644
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -167,3 +167,206 @@ sbsign \
>  	--output xen.signed.efi \
>  	xen.unified.efi
>  ```
> +
> +## UEFI boot and dom0less on ARM
> +
> +Dom0less feature is supported by ARM and it is possible to use it when Xen is
> +started as an EFI application.
> +The way to specify the domU domains is by Device Tree as specified in the
> +[dom0less](dom0less.html) documentation page under the "Device Tree
> +configuration" section, but instead of declaring the reg property in the boot
> +module, the user must specify the "uefi,binary" property containing the name
> +of the binary file that has to be loaded in memory.
> +The UEFI stub will load the binary in memory and it will add the reg property
> +accordingly.
> +
> +An example here:
> +
> +domU1 {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	compatible = "xen,domain";
> +	memory = <0 0x20000>;
> +	cpus = <1>;
> +	vpl011;
> +
> +	module@1 {
> +		compatible = "multiboot,kernel", "multiboot,module";
> +		uefi,binary = "vmlinuz-3.0.31-0.4-xen";
> +		bootargs = "console=ttyAMA0";
> +	};
> +	module@2 {
> +		compatible = "multiboot,ramdisk", "multiboot,module";
> +		uefi,binary = "initrd-3.0.31-0.4-xen";
> +	};
> +	module@3 {
> +		compatible = "multiboot,ramdisk", "multiboot,module";
> +		uefi,binary = "passthrough.dtb";
> +	};
> +};
> +
> +## How to boot different Xen setup using UEFI
> +
> +Here the supported user cases for Xen when UEFI boot is used:

"Supported" means different things to different people, so I would say
instead:

These are the different ways to boot a Xen system from UEFI:


> +
> + - Boot Xen and Dom0 (minimum required)
> + - Boot Xen and DomU(s) (true dom0less, only on ARM)
> + - Boot Xen, Dom0 and DomU(s) (only on ARM)
> +
> +### Boot Xen and Dom0
> +
> +This configuration can be started using the Xen configuration file in the
> +example above.
> +
> +### Boot Xen and DomU(s)
> +
> +This configuration needs the domU domain(s) specified in the /chosen node,
> +examples of how to do that are provided by the documentation about dom0less
> +and the example above shows how to use the "uefi,binary" property to use the
> +UEFI stub for module loading.
> +Providing the multiboot modules in the device tree, make Xen skip its UEFI
> +configuration file, if it is needed for some reason, specify the "uefi,cfg-load"
> +property in the /chosen node.

This last sentence is not very clear and it looks like one needs to
specify "uefi,cfg-load" to skip the Xen config file when actually it is
the opposite. I would suggest instead:

When adding DomU modules to device tree, also add the property
uefi,cfg-load under chosen to have Xen load the Xen config file.
Otherwise, Xen will skip the config file and rely on device tree alone.


> +Example 1 of how to boot a true dom0less configuration:
> +
> +Xen configuration file: skipped.
> +
> +Device tree:
> +
> +```
> +chosen {
> +	#size-cells = <0x1>;
> +	#address-cells = <0x1>;
> +	xen,xen-bootargs = "<Xen command line>"
> +
> +	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";
> +		};
> +	};
> +	domU2 {
> +		#size-cells = <0x1>;
> +		#address-cells = <0x1>;
> +		compatible = "xen,domain";
> +		cpus = <0x1>;
> +		memory = <0x0 0x100000>;
> +		vpl011;
> +
> +		module@2 {
> +			compatible = "multiboot,kernel", "multiboot,module";
> +			uefi,binary = "Image-domu2.bin";
> +			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
> +		};
> +	};
> +};
> +```
> +
> +Example 2 of how to boot a true dom0less configuration:
> +
> +Xen configuration file:
> +
> +```
> +[global]
> +default=xen
> +
> +[xen]
> +options=<Xen command line>
> +dtb=<optional DTB>
> +```
> +
> +Device tree:
> +
> +```
> +chosen {
> +	#size-cells = <0x1>;
> +	#address-cells = <0x1>;
> +	uefi,cfg-load;
> +
> +	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";
> +		};
> +	};
> +	domU2 {
> +		#size-cells = <0x1>;
> +		#address-cells = <0x1>;
> +		compatible = "xen,domain";
> +		cpus = <0x1>;
> +		memory = <0x0 0x100000>;
> +		vpl011;
> +
> +		module@2 {
> +			compatible = "multiboot,kernel", "multiboot,module";
> +			uefi,binary = "Image-domu2.bin";
> +			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
> +		};
> +	};
> +};
> +```
> +
> +### 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.
> +
> +Here an example:
> +
> +Xen configuration file:
> +
> +```
> +[global]
> +default=xen
> +
> +[xen]
> +options=<Xen command line>
> +kernel=vmlinuz-3.0.31-0.4-xen [domain 0 command line options]
> +ramdisk=initrd-3.0.31-0.4-xen
> +dtb=<optional DTB>
> +```
> +
> +Device tree:
> +
> +```
> +chosen {
> +	#size-cells = <0x1>;
> +	#address-cells = <0x1>;
> +	uefi,cfg-load;
> +
> +	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 8ceeba4ad1..e2b007ece0 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -8,9 +8,43 @@
>  #include <asm/setup.h>
>  #include <asm/smp.h>
>  
> +typedef struct {
> +    char *name;
> +    unsigned int name_len;
> +    EFI_PHYSICAL_ADDRESS addr;
> +    UINTN size;
> +} dom0less_module_name;
> +
> +/*
> + * Binaries will be translated into bootmodules, the maximum number for them is
> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> + */
> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> +static struct file __initdata dom0less_file;
> +static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
> +static unsigned int __initdata dom0less_modules_available =
> +                               MAX_DOM0LESS_MODULES;
> +static unsigned int __initdata dom0less_modules_idx;

This is a lot better!

We don't need both dom0less_modules_idx and dom0less_modules_available.
You can just do:

#define dom0less_modules_available (MAX_DOM0LESS_MODULES - dom0less_modules_idx)
static unsigned int __initdata dom0less_modules_idx;

But maybe we can even get rid of dom0less_modules_available entirely?

We can change the check at the beginning of allocate_dom0less_file to:

if ( dom0less_modules_idx == dom0less_modules_available )
    blexit

Would that work?


> +#define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
> +
>  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>  void __flush_dcache_area(const void *vaddr, unsigned long size);
>  
> +static int get_dom0less_file_index(const char *name, unsigned int name_len);
> +static unsigned int allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                           const char *name,
> +                                           unsigned int name_len);
> +static void handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
> +                                        int module_node_offset,
> +                                        int reg_addr_cells,
> +                                        int reg_size_cells);
> +static void handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> +                                        int domain_node,
> +                                        int addr_cells,
> +                                        int size_cells);
> +static bool efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle);
>
>  #define DEVICE_TREE_GUID \
>  {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
>  
> @@ -552,8 +586,218 @@ static void __init efi_arch_handle_module(const struct file *file,
>                           kernel.size) < 0 )
>              blexit(L"Unable to set reg property.");
>      }
> -    else
> +    else if ( file != &dom0less_file )
> +        /*
> +         * If file is not a dom0 module file and it's not a domU module,
> +         * stop here.
> +         */
>          blexit(L"Unknown module type");
> +    /*
> +     * dom0less_modules_available is decremented here because for each dom0
> +     * file added, there will be an additional bootmodule, so the number
> +     * of dom0less module files will be decremented because there is
> +     * a maximum amount of bootmodules that can be loaded.
> +     */
> +    dom0less_modules_available--;
> +}
> +
> +/*
> + * This function checks for a binary previously loaded with a give name, it
> + * returns the index of the file in the dom0less_files array or a negative
> + * number if no file with that name is found.
> + */
> +static int __init get_dom0less_file_index(const char *name,
> +                                          unsigned int name_len)
> +{
> +    unsigned int i;
> +    int ret = ERROR_DOM0LESS_FILE_NOT_FOUND;
> +
> +    for (i = 0; i < dom0less_modules_idx; i++)
> +    {
> +        dom0less_module_name *mod = &dom0less_modules[i];
> +        if ( (mod->name_len == name_len) &&
> +             (strncmp(mod->name, name, name_len) == 0) )
> +        {
> +            ret = i;
> +            break;
> +        }
> +    }
> +    return ret;
> +}
> +
> +/*
> + * This function allocates a binary and keeps track of its name, it
> + * returns the index of the file in the dom0less_files array.
> + */
> +static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                                  const char *name,
> +                                                  unsigned int name_len)
> +{
> +    dom0less_module_name* file_name;
> +    union string module_name;
> +    unsigned int ret_idx;
> +
> +    /*
> +     * Check if there is any space left for a domU module, the variable
> +     * dom0less_modules_available is updated each time we use read_file(...)
> +     * successfully.
> +     */
> +    if ( !dom0less_modules_available )
> +        blexit(L"No space left for domU modules");

This is the check that could be based on dom0less_modules_idx


> +    module_name.s = (char*) name;
> +    ret_idx = dom0less_modules_idx;
> +
> +    /* Save at this index the name of this binary */
> +    file_name = &dom0less_modules[ret_idx];
> +
> +    if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
> +                              (void**)&file_name->name) != EFI_SUCCESS )
> +        blexit(L"Error allocating memory for dom0less binary name");
> +
> +    /* Save name and length of the binary in the data structure */
> +    strlcpy(file_name->name, name, name_len);
> +    file_name->name_len = name_len;

Sorry for the silly question but do we need to set the '\0' at the end
of the string or can we assumed the memory returned by AllocatePool is
already zeroed? Or should we call strlcpy with name_len+1 ?


> +
> +    /* Load the binary in memory */
> +    read_file(dir_handle, s2w(&module_name), &dom0less_file, NULL);
> +
> +    /* Save address and size */
> +    file_name->addr = dom0less_file.addr;
> +    file_name->size = dom0less_file.size;
> +
> +    /* s2w(...) allocates some memory, free it */
> +    efi_bs->FreePool(module_name.w);
> +
> +    dom0less_modules_idx++;
> +
> +    return ret_idx;
> +}
> +
> +/*
> + * This function checks for the presence of the uefi,binary property in the
> + * module, if found it loads the binary as dom0less module and sets the right
> + * address for the reg property into the module DT node.
> + */
> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
> +                                          int module_node_offset,
> +                                          int reg_addr_cells,
> +                                          int reg_size_cells)
> +{
> +    const void *uefi_name_prop;
> +    char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
> +    int uefi_name_len, file_idx;
> +    dom0less_module_name *file;
> +
> +    /* Read uefi,binary property to get the file name. */
> +    uefi_name_prop = fdt_getprop(fdt, module_node_offset, "uefi,binary",
> +                                 &uefi_name_len);
> +
> +    if ( !uefi_name_prop )
> +        /* Property not found */
> +        return;
> +
> +    file_idx = get_dom0less_file_index(uefi_name_prop, uefi_name_len);
> +    if (file_idx < 0)
> +        file_idx = allocate_dom0less_file(dir_handle, uefi_name_prop,
> +                                          uefi_name_len);
> +
> +    file = &dom0less_modules[file_idx];
> +
> +    snprintf(mod_string, sizeof(mod_string), "module@%"PRIx64, file->addr);
> +
> +    /* Rename the module to be module@{address} */
> +    if ( fdt_set_name(fdt, module_node_offset, mod_string) < 0 )
> +        blexit(L"Unable to add domU ramdisk FDT node.");
> +
> +    if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
> +                     file->addr, file->size) < 0 )
> +        blexit(L"Unable to set reg property.");
> +}
> +
> +/*
> + * This function checks for boot modules under the domU guest domain node
> + * in the DT.
> + */
> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> +                                               int domain_node,
> +                                               int addr_cells,
> +                                               int size_cells)
> +{
> +    int module_node;
> +    /*
> +     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
> +     * inside this node
> +     */
> +    for ( module_node = fdt_first_subnode(fdt, domain_node);
> +          module_node > 0;
> +          module_node = fdt_next_subnode(fdt, module_node) )
> +        if ( (fdt_node_check_compatible(fdt, module_node,
> +                                        "multiboot,kernel") == 0) ||
> +             (fdt_node_check_compatible(fdt, module_node,
> +                                        "multiboot,ramdisk") == 0) ||
> +             (fdt_node_check_compatible(fdt, module_node,
> +                                        "multiboot,device-tree") == 0) )
> +            /* The compatible is one of the strings above, check the module */
> +            handle_dom0less_module_node(dir_handle, module_node, addr_cells,
> +                                        size_cells);
> +}
> +
> +/*
> + * This function checks for xen domain nodes under the /chosen node for possible
> + * domU guests to be loaded.
> + */
> +static bool __init efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle)
> +{
> +    int chosen;
> +    int addr_len, size_len;
> +    unsigned int i = 0;
> +
> +    /* Check for the chosen node in the current DTB */
> +    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
> +    if ( chosen < 0 )
> +        blexit(L"Unable to setup chosen node");
> +
> +    /* Check for nodes compatible with xen,domain under the chosen node */
> +    for ( int node = fdt_first_subnode(fdt, chosen);
> +          node > 0;
> +          node = fdt_next_subnode(fdt, node) )
> +    {
> +        int addr_cells, size_cells, len;
> +        const struct fdt_property *prop;
> +
> +        if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
> +            continue;
> +
> +        /* Get or set #address-cells and #size-cells */
> +        prop = fdt_get_property(fdt, node, "#address-cells", &len);
> +        if ( !prop )
> +            blexit(L"#address-cells not found in domain node.");
> +
> +        addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
> +
> +        prop = fdt_get_property(fdt, node, "#size-cells", &len);
> +        if ( !prop )
> +            blexit(L"#size-cells not found in domain node.");
> +
> +        size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
> +
> +        /* Found a node with compatible xen,domain; handle this node. */
> +        handle_dom0less_domain_node(dir_handle, node, addr_cells, size_cells);
> +    }
> +
> +    /* Free dom0less file names if any */
> +    for ( ; i < dom0less_modules_idx; i++ )
> +    {
> +        /* Free dom0less binary names */
> +        efi_bs->FreePool(dom0less_modules[i].name);
> +    }
> +
> +    if ( dom0less_modules_idx > 0 )
> +        return true;
> +
> +    return false;
>  }
>  
>  static void __init efi_arch_cpu(void)
> @@ -562,8 +806,19 @@ static void __init efi_arch_cpu(void)
>  
>  static void __init efi_arch_blexit(void)
>  {
> +    unsigned int i = 0;
> +
>      if ( dtbfile.need_to_free )
>          efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
> +    /* Free dom0less files if any */
> +    for ( ; i < dom0less_modules_idx; i++ )
> +    {
> +        /* Free dom0less binary names */
> +        efi_bs->FreePool(dom0less_modules[i].name);
> +        /* Free dom0less binaries */
> +        efi_bs->FreePages(dom0less_modules[i].addr,
> +                          PFN_UP(dom0less_modules[i].size));
> +    }
>      if ( memmap )
>          efi_bs->FreePool(memmap);
>  }
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 9b0cc29aae..950fdf16b7 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -678,6 +678,12 @@ static void __init efi_arch_handle_module(const struct file *file,
>      efi_bs->FreePool(ptr);
>  }
>  
> +static bool __init efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle)
> +{
> +    /* x86 doesn't support dom0less */
> +    return false;
> +}
> +
>  static void __init efi_arch_cpu(void)
>  {
>      uint32_t eax = cpuid_eax(0x80000000);
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 758f9d74d2..7d8734199e 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1127,15 +1127,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
>      EFI_LOADED_IMAGE *loaded_image;
>      EFI_STATUS status;
> -    unsigned int i, argc;
> -    CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
> +    unsigned int i, argc = 0;
> +    CHAR16 **argv, *file_name = NULL, *cfg_file_name = NULL, *options = NULL;
>      UINTN gop_mode = ~0;
>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>      union string section = { NULL }, name;
>      bool base_video = false;
> -    const char *option_str;
> +    const char *option_str = NULL;
>      bool use_cfg_file;
> +    EFI_FILE_HANDLE dir_handle;
>  
>      __set_bit(EFI_BOOT, &efi_flags);
>      __set_bit(EFI_LOADER, &efi_flags);
> @@ -1216,9 +1217,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>  
>      efi_arch_relocate_image(0);
>  
> +    /* Get the file system interface. */
> +    dir_handle = get_parent_handle(loaded_image, &file_name);
> +
>      if ( use_cfg_file )
>      {
> -        EFI_FILE_HANDLE dir_handle;
>          UINTN depth, cols, rows, size;
>  
>          size = cols = rows = depth = 0;
> @@ -1229,9 +1232,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>  
>          gop = efi_get_gop();
>  
> -        /* Get the file system interface. */
> -        dir_handle = get_parent_handle(loaded_image, &file_name);
> -
>          /* Read and parse the config file. */
>          if ( read_section(loaded_image, L"config", &cfg, NULL) )
>              PrintStr(L"Using builtin config file\r\n");
> @@ -1285,14 +1285,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>              efi_bs->FreePool(name.w);
>          }
>  
> -        if ( !name.s )
> -            blexit(L"No Dom0 kernel image specified.");
> -
>          efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
>  
> -        option_str = split_string(name.s);
> +        if ( name.s )
> +            option_str = split_string(name.s);
>  
> -        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) )
> +        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) &&
> +             name.s )

I'd probably indent this and put it under the previous if, instead of
adding "&& name.s". But I'll leave it up to you.


>          {
>              read_file(dir_handle, s2w(&name), &kernel, option_str);
>              efi_bs->FreePool(name.w);
> @@ -1361,12 +1360,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>          cfg.addr = 0;
>  
> -        dir_handle->Close(dir_handle);
> -
>          if ( gop && !base_video )
>              gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>      }
>  
> +    /*
> +     * Check if a proper configuration is provided to start Xen:
> +     *  - Dom0 specified (minimum required)
> +     *  - Dom0 and DomU(s) specified
> +     *  - DomU(s) specified
> +     */
> +    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
> +        blexit(L"No Dom0 kernel image specified.");
> +
> +    dir_handle->Close(dir_handle);
> +
>      efi_arch_edd();
>  
>      /* XXX Collect EDID info. */
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/2] arm/efi: Introduce uefi,cfg-load DT property

[Luca Fancellu]

> On 22 Sep 2021, at 22:19, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 22 Sep 2021, Luca Fancellu wrote:
>> Introduce the uefi,cfg-load DT property of /chosen
>> node for ARM whose presence decide whether to force
>> the load of the UEFI Xen configuration file.
>> 
>> The logic is that if any multiboot,module is found in
>> the DT, then the uefi,cfg-load property is used to see
>> if the UEFI Xen configuration file is needed.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> The patch looks OK, just a couple of minor comments below.
> 
> 
>> ---
>> v2 changes:
>> - Introduced uefi,cfg-load property
>> - Add documentation about the property
>> ---
>> docs/misc/efi.pandoc        |  2 ++
>> xen/arch/arm/efi/efi-boot.h | 28 +++++++++++++++++++++++-----
>> 2 files changed, 25 insertions(+), 5 deletions(-)
>> 
>> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
>> index ac3cd58cae..e289c5e7ba 100644
>> --- a/docs/misc/efi.pandoc
>> +++ b/docs/misc/efi.pandoc
>> @@ -14,6 +14,8 @@ loaded the modules and describes them in the device tree provided to Xen.  If a
>> bootloader provides a device tree containing modules then any configuration
>> files are ignored, and the bootloader is responsible for populating all
>> relevant device tree nodes.
>> +The property "uefi,cfg-load" can be specified in the /chosen node to force Xen
>> +to load the configuration file even if multiboot modules are found.
> 

Hi Stefano,

> I think that, in addition to this, we also need to add the property to
> docs/misc/arm/device-tree/booting.txt where our "official" device tree
> bindings are maintained. I would add it below "Command lines" and before
> "Creating Multiple Domains directly from Xen" maybe as a new chapter.
> It could be called "Other Options" but other ideas could be valid too.
> 
> You can say that uefi,cfg-load is a boolean.

Sure, I will add in v3, what about this:

UEFI boot and DT
==============

When Xen is booted using UEFI, it doesn't read the configuration file if any
multiboot module is specified. To force Xen to load the configuration file, the
boolean property uefi,cfg-load must be declared in the /chosen node.

> 
>> Once built, `make install-xen` will place the resulting binary directly into
>> the EFI boot partition, provided `EFI_VENDOR` is set in the environment (and
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index cf9c37153f..8ceeba4ad1 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -581,22 +581,40 @@ static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>> 
>> static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>> {
>> +    bool skip_cfg_file = false;
>>     /*
>>      * For arm, we may get a device tree from GRUB (or other bootloader)
>>      * that contains modules that have already been loaded into memory.  In
>> -     * this case, we do not use a configuration file, and rely on the
>> -     * bootloader to have loaded all required modules and appropriate
>> -     * options.
>> +     * this case, we search for the property uefi,cfg-load in the /chosen node
>> +     * to decide whether to skip the UEFI Xen configuration file or not.
>>      */
>> 
>>     fdt = lookup_fdt_config_table(SystemTable);
>>     dtbfile.ptr = fdt;
>>     dtbfile.need_to_free = false; /* Config table memory can't be freed. */
>> -    if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 )
>> +
>> +    if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 )
>> +    {
>> +        /* Locate chosen node */
>> +        int node = fdt_subnode_offset(fdt, 0, "chosen");
>> +        const void *cfg_load_prop;
>> +        int cfg_load_len;
>> +
>> +        if ( node > 0 )
>> +        {
>> +            /* Check if uefi,cfg-load property exists */
>> +            cfg_load_prop = fdt_getprop(fdt, node, "uefi,cfg-load",
>> +                                        &cfg_load_len);
>> +            if ( !cfg_load_prop )
>> +                skip_cfg_file = true;
>> +        }
>> +    }
>> +
>> +    if ( !fdt || !skip_cfg_file )
> 
> Just a suggestion, but rather than the double negative, wouldn't it be
> simpler to define it as
> 
>    bool load_cfg_file = true;
> 
> ?

Sure I will modify it.

> 
> 
>>     {
>>         /*
>>          * We either have no FDT, or one without modules, so we must have a
>> -         * Xen EFI configuration file to specify modules.  (dom0 required)
>> +         * Xen EFI configuration file to specify modules.
>>          */
> 
> Also mention in the commit message that you are taking the opportunity
> to update this comment do remove "dom0 required".

Yes I will add it in the commit message

Cheers,
Luca

> 
> 
>>         return true;
>>     }
>> -- 
>> 2.17.1

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Luca Fancellu]

> On 22 Sep 2021, at 22:51, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 22 Sep 2021, Luca Fancellu wrote:
>> This patch introduces the support for dom0less configuration
>> when using UEFI boot on ARM, it permits the EFI boot to
>> continue if no dom0 kernel is specified but at least one domU
>> is found.
>> 
>> Introduce the new property "uefi,binary" for device tree boot
>> module nodes that are subnode of "xen,domain" compatible nodes.
>> The property holds a string containing the file name of the
>> binary that shall be loaded by the uefi loader from the filesystem.
>> 
>> Update efi documentation about how to start a dom0less
>> setup using UEFI
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Overall the patch is pretty good. I don't have any major concerns (only
> one about the documentation). Some minor comments below.
> 
> 
>> ---
>> Changes in v2:
>> - remove array of struct file
>> - fixed some int types
>> - Made the code use filesystem even when configuration
>> file is skipped.
>> - add documentation of uefi,binary in booting.txt
>> - add documentation on how to boot all configuration
>> for Xen using UEFI in efi.pandoc
>> ---
>> docs/misc/arm/device-tree/booting.txt |  21 +++
>> docs/misc/efi.pandoc                  | 203 ++++++++++++++++++++
>> xen/arch/arm/efi/efi-boot.h           | 257 +++++++++++++++++++++++++-
>> xen/arch/x86/efi/efi-boot.h           |   6 +
>> xen/common/efi/boot.c                 |  36 ++--
>> 5 files changed, 508 insertions(+), 15 deletions(-)
>> 
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 44cd9e1a9a..bc0f8913db 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -182,6 +182,13 @@ The kernel sub-node has the following properties:
>> 
>>     Command line parameters for the guest kernel.
>> 
>> +- efi,binary (UEFI boot only)
> 
> Below it is actually uefi,binary, not efi,binary. I would use
> uefi,binary consistently.

Yes that was a mistake, thanks for catching it

> 
> 
>> +    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.
> 
> We need to specify that this parameter is a string.

ok

> 
> 
>> The ramdisk sub-node has the following properties:
>> 
>> - compatible
>> @@ -193,6 +200,13 @@ The ramdisk sub-node has the following properties:
>>     Specifies the physical address of the ramdisk in RAM and its
>>     length.
>> 
>> +- efi,binary (UEFI boot only)
>> +
>> +    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.
> 
> same here

Ok

> 
>> 
>> Example
>> =======
>> @@ -257,6 +271,13 @@ The dtb sub-node should have the following properties:
>>     Specifies the physical address of the device tree binary fragment
>>     RAM and its length.
>> 
>> +- efi,binary (UEFI boot only)
>> +
>> +    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.
> 
> same here

Ok

> 
> 
>> As an example:
>> 
>>         module@0xc000000 {
>> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
>> index e289c5e7ba..698196e129 100644
>> --- a/docs/misc/efi.pandoc
>> +++ b/docs/misc/efi.pandoc
>> @@ -167,3 +167,206 @@ sbsign \
>> 	--output xen.signed.efi \
>> 	xen.unified.efi
>> ```
>> +
>> +## UEFI boot and dom0less on ARM
>> +
>> +Dom0less feature is supported by ARM and it is possible to use it when Xen is
>> +started as an EFI application.
>> +The way to specify the domU domains is by Device Tree as specified in the
>> +[dom0less](dom0less.html) documentation page under the "Device Tree
>> +configuration" section, but instead of declaring the reg property in the boot
>> +module, the user must specify the "uefi,binary" property containing the name
>> +of the binary file that has to be loaded in memory.
>> +The UEFI stub will load the binary in memory and it will add the reg property
>> +accordingly.
>> +
>> +An example here:
>> +
>> +domU1 {
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	compatible = "xen,domain";
>> +	memory = <0 0x20000>;
>> +	cpus = <1>;
>> +	vpl011;
>> +
>> +	module@1 {
>> +		compatible = "multiboot,kernel", "multiboot,module";
>> +		uefi,binary = "vmlinuz-3.0.31-0.4-xen";
>> +		bootargs = "console=ttyAMA0";
>> +	};
>> +	module@2 {
>> +		compatible = "multiboot,ramdisk", "multiboot,module";
>> +		uefi,binary = "initrd-3.0.31-0.4-xen";
>> +	};
>> +	module@3 {
>> +		compatible = "multiboot,ramdisk", "multiboot,module";
>> +		uefi,binary = "passthrough.dtb";
>> +	};
>> +};
>> +
>> +## How to boot different Xen setup using UEFI
>> +
>> +Here the supported user cases for Xen when UEFI boot is used:
> 
> "Supported" means different things to different people, so I would say
> instead:
> 
> These are the different ways to boot a Xen system from UEFI:

Ok

> 
> 
>> +
>> + - Boot Xen and Dom0 (minimum required)
>> + - Boot Xen and DomU(s) (true dom0less, only on ARM)
>> + - Boot Xen, Dom0 and DomU(s) (only on ARM)
>> +
>> +### Boot Xen and Dom0
>> +
>> +This configuration can be started using the Xen configuration file in the
>> +example above.
>> +
>> +### Boot Xen and DomU(s)
>> +
>> +This configuration needs the domU domain(s) specified in the /chosen node,
>> +examples of how to do that are provided by the documentation about dom0less
>> +and the example above shows how to use the "uefi,binary" property to use the
>> +UEFI stub for module loading.
>> +Providing the multiboot modules in the device tree, make Xen skip its UEFI
>> +configuration file, if it is needed for some reason, specify the "uefi,cfg-load"
>> +property in the /chosen node.
> 
> This last sentence is not very clear and it looks like one needs to
> specify "uefi,cfg-load" to skip the Xen config file when actually it is
> the opposite. I would suggest instead:
> 
> When adding DomU modules to device tree, also add the property
> uefi,cfg-load under chosen to have Xen load the Xen config file.
> Otherwise, Xen will skip the config file and rely on device tree alone.

Ok

> 
> 
>> +Example 1 of how to boot a true dom0less configuration:
>> +
>> +Xen configuration file: skipped.
>> +
>> +Device tree:
>> +
>> +```
>> +chosen {
>> +	#size-cells = <0x1>;
>> +	#address-cells = <0x1>;
>> +	xen,xen-bootargs = "<Xen command line>"
>> +
>> +	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";
>> +		};
>> +	};
>> +	domU2 {
>> +		#size-cells = <0x1>;
>> +		#address-cells = <0x1>;
>> +		compatible = "xen,domain";
>> +		cpus = <0x1>;
>> +		memory = <0x0 0x100000>;
>> +		vpl011;
>> +
>> +		module@2 {
>> +			compatible = "multiboot,kernel", "multiboot,module";
>> +			uefi,binary = "Image-domu2.bin";
>> +			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
>> +		};
>> +	};
>> +};
>> +```
>> +
>> +Example 2 of how to boot a true dom0less configuration:
>> +
>> +Xen configuration file:
>> +
>> +```
>> +[global]
>> +default=xen
>> +
>> +[xen]
>> +options=<Xen command line>
>> +dtb=<optional DTB>
>> +```
>> +
>> +Device tree:
>> +
>> +```
>> +chosen {
>> +	#size-cells = <0x1>;
>> +	#address-cells = <0x1>;
>> +	uefi,cfg-load;
>> +
>> +	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";
>> +		};
>> +	};
>> +	domU2 {
>> +		#size-cells = <0x1>;
>> +		#address-cells = <0x1>;
>> +		compatible = "xen,domain";
>> +		cpus = <0x1>;
>> +		memory = <0x0 0x100000>;
>> +		vpl011;
>> +
>> +		module@2 {
>> +			compatible = "multiboot,kernel", "multiboot,module";
>> +			uefi,binary = "Image-domu2.bin";
>> +			bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
>> +		};
>> +	};
>> +};
>> +```
>> +
>> +### 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.
>> +
>> +Here an example:
>> +
>> +Xen configuration file:
>> +
>> +```
>> +[global]
>> +default=xen
>> +
>> +[xen]
>> +options=<Xen command line>
>> +kernel=vmlinuz-3.0.31-0.4-xen [domain 0 command line options]
>> +ramdisk=initrd-3.0.31-0.4-xen
>> +dtb=<optional DTB>
>> +```
>> +
>> +Device tree:
>> +
>> +```
>> +chosen {
>> +	#size-cells = <0x1>;
>> +	#address-cells = <0x1>;
>> +	uefi,cfg-load;
>> +
>> +	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 8ceeba4ad1..e2b007ece0 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -8,9 +8,43 @@
>> #include <asm/setup.h>
>> #include <asm/smp.h>
>> 
>> +typedef struct {
>> +    char *name;
>> +    unsigned int name_len;
>> +    EFI_PHYSICAL_ADDRESS addr;
>> +    UINTN size;
>> +} dom0less_module_name;
>> +
>> +/*
>> + * Binaries will be translated into bootmodules, the maximum number for them is
>> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
>> + */
>> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
>> +static struct file __initdata dom0less_file;
>> +static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
>> +static unsigned int __initdata dom0less_modules_available =
>> +                               MAX_DOM0LESS_MODULES;
>> +static unsigned int __initdata dom0less_modules_idx;
> 
> This is a lot better!
> 
> We don't need both dom0less_modules_idx and dom0less_modules_available.
> You can just do:
> 
> #define dom0less_modules_available (MAX_DOM0LESS_MODULES - dom0less_modules_idx)
> static unsigned int __initdata dom0less_modules_idx;
> 
> But maybe we can even get rid of dom0less_modules_available entirely?
> 
> We can change the check at the beginning of allocate_dom0less_file to:
> 
> if ( dom0less_modules_idx == dom0less_modules_available )
>    blexit
> 
> Would that work?

I thought about it but I think they need to stay, because dom0less_modules_available is the
upper bound for the additional dom0less modules (it is decremented each time a dom0 module
Is added), instead dom0less_modules_idx is the typical index for the array of dom0less modules.

> 
> 
>> +#define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
>> +
>> void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>> void __flush_dcache_area(const void *vaddr, unsigned long size);
>> 
>> +static int get_dom0less_file_index(const char *name, unsigned int name_len);
>> +static unsigned int allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>> +                                           const char *name,
>> +                                           unsigned int name_len);
>> +static void handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
>> +                                        int module_node_offset,
>> +                                        int reg_addr_cells,
>> +                                        int reg_size_cells);
>> +static void handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> +                                        int domain_node,
>> +                                        int addr_cells,
>> +                                        int size_cells);
>> +static bool efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle);
>> 
>> #define DEVICE_TREE_GUID \
>> {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
>> 
>> @@ -552,8 +586,218 @@ static void __init efi_arch_handle_module(const struct file *file,
>>                          kernel.size) < 0 )
>>             blexit(L"Unable to set reg property.");
>>     }
>> -    else
>> +    else if ( file != &dom0less_file )
>> +        /*
>> +         * If file is not a dom0 module file and it's not a domU module,
>> +         * stop here.
>> +         */
>>         blexit(L"Unknown module type");
>> +    /*
>> +     * dom0less_modules_available is decremented here because for each dom0
>> +     * file added, there will be an additional bootmodule, so the number
>> +     * of dom0less module files will be decremented because there is
>> +     * a maximum amount of bootmodules that can be loaded.
>> +     */
>> +    dom0less_modules_available--;
>> +}
>> +
>> +/*
>> + * This function checks for a binary previously loaded with a give name, it
>> + * returns the index of the file in the dom0less_files array or a negative
>> + * number if no file with that name is found.
>> + */
>> +static int __init get_dom0less_file_index(const char *name,
>> +                                          unsigned int name_len)
>> +{
>> +    unsigned int i;
>> +    int ret = ERROR_DOM0LESS_FILE_NOT_FOUND;
>> +
>> +    for (i = 0; i < dom0less_modules_idx; i++)
>> +    {
>> +        dom0less_module_name *mod = &dom0less_modules[i];
>> +        if ( (mod->name_len == name_len) &&
>> +             (strncmp(mod->name, name, name_len) == 0) )
>> +        {
>> +            ret = i;
>> +            break;
>> +        }
>> +    }
>> +    return ret;
>> +}
>> +
>> +/*
>> + * This function allocates a binary and keeps track of its name, it
>> + * returns the index of the file in the dom0less_files array.
>> + */
>> +static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>> +                                                  const char *name,
>> +                                                  unsigned int name_len)
>> +{
>> +    dom0less_module_name* file_name;
>> +    union string module_name;
>> +    unsigned int ret_idx;
>> +
>> +    /*
>> +     * Check if there is any space left for a domU module, the variable
>> +     * dom0less_modules_available is updated each time we use read_file(...)
>> +     * successfully.
>> +     */
>> +    if ( !dom0less_modules_available )
>> +        blexit(L"No space left for domU modules");
> 
> This is the check that could be based on dom0less_modules_idx
> 

The only way I see to have it based on dom0less_modules_idx will be to compare it
to the amount of modules still available, that is not constant because it is dependent
on how many dom0 modules are loaded, so still two variables needed.
Don’t know if I’m missing something.

> 
>> +    module_name.s = (char*) name;
>> +    ret_idx = dom0less_modules_idx;
>> +
>> +    /* Save at this index the name of this binary */
>> +    file_name = &dom0less_modules[ret_idx];
>> +
>> +    if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
>> +                              (void**)&file_name->name) != EFI_SUCCESS )
>> +        blexit(L"Error allocating memory for dom0less binary name");
>> +
>> +    /* Save name and length of the binary in the data structure */
>> +    strlcpy(file_name->name, name, name_len);
>> +    file_name->name_len = name_len;
> 
> Sorry for the silly question but do we need to set the '\0' at the end
> of the string or can we assumed the memory returned by AllocatePool is
> already zeroed? Or should we call strlcpy with name_len+1 ?

Not silly at all, I will call strlcpy with name_len+1 and strlcpy will put the
termination.

> 
> 
>> +
>> +    /* Load the binary in memory */
>> +    read_file(dir_handle, s2w(&module_name), &dom0less_file, NULL);
>> +
>> +    /* Save address and size */
>> +    file_name->addr = dom0less_file.addr;
>> +    file_name->size = dom0less_file.size;
>> +
>> +    /* s2w(...) allocates some memory, free it */
>> +    efi_bs->FreePool(module_name.w);
>> +
>> +    dom0less_modules_idx++;
>> +
>> +    return ret_idx;
>> +}
>> +
>> +/*
>> + * This function checks for the presence of the uefi,binary property in the
>> + * module, if found it loads the binary as dom0less module and sets the right
>> + * address for the reg property into the module DT node.
>> + */
>> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
>> +                                          int module_node_offset,
>> +                                          int reg_addr_cells,
>> +                                          int reg_size_cells)
>> +{
>> +    const void *uefi_name_prop;
>> +    char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
>> +    int uefi_name_len, file_idx;
>> +    dom0less_module_name *file;
>> +
>> +    /* Read uefi,binary property to get the file name. */
>> +    uefi_name_prop = fdt_getprop(fdt, module_node_offset, "uefi,binary",
>> +                                 &uefi_name_len);
>> +
>> +    if ( !uefi_name_prop )
>> +        /* Property not found */
>> +        return;
>> +
>> +    file_idx = get_dom0less_file_index(uefi_name_prop, uefi_name_len);
>> +    if (file_idx < 0)
>> +        file_idx = allocate_dom0less_file(dir_handle, uefi_name_prop,
>> +                                          uefi_name_len);
>> +
>> +    file = &dom0less_modules[file_idx];
>> +
>> +    snprintf(mod_string, sizeof(mod_string), "module@%"PRIx64, file->addr);
>> +
>> +    /* Rename the module to be module@{address} */
>> +    if ( fdt_set_name(fdt, module_node_offset, mod_string) < 0 )
>> +        blexit(L"Unable to add domU ramdisk FDT node.");
>> +
>> +    if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
>> +                     file->addr, file->size) < 0 )
>> +        blexit(L"Unable to set reg property.");
>> +}
>> +
>> +/*
>> + * This function checks for boot modules under the domU guest domain node
>> + * in the DT.
>> + */
>> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> +                                               int domain_node,
>> +                                               int addr_cells,
>> +                                               int size_cells)
>> +{
>> +    int module_node;
>> +    /*
>> +     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
>> +     * inside this node
>> +     */
>> +    for ( module_node = fdt_first_subnode(fdt, domain_node);
>> +          module_node > 0;
>> +          module_node = fdt_next_subnode(fdt, module_node) )
>> +        if ( (fdt_node_check_compatible(fdt, module_node,
>> +                                        "multiboot,kernel") == 0) ||
>> +             (fdt_node_check_compatible(fdt, module_node,
>> +                                        "multiboot,ramdisk") == 0) ||
>> +             (fdt_node_check_compatible(fdt, module_node,
>> +                                        "multiboot,device-tree") == 0) )
>> +            /* The compatible is one of the strings above, check the module */
>> +            handle_dom0less_module_node(dir_handle, module_node, addr_cells,
>> +                                        size_cells);
>> +}
>> +
>> +/*
>> + * This function checks for xen domain nodes under the /chosen node for possible
>> + * domU guests to be loaded.
>> + */
>> +static bool __init efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle)
>> +{
>> +    int chosen;
>> +    int addr_len, size_len;
>> +    unsigned int i = 0;
>> +
>> +    /* Check for the chosen node in the current DTB */
>> +    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
>> +    if ( chosen < 0 )
>> +        blexit(L"Unable to setup chosen node");
>> +
>> +    /* Check for nodes compatible with xen,domain under the chosen node */
>> +    for ( int node = fdt_first_subnode(fdt, chosen);
>> +          node > 0;
>> +          node = fdt_next_subnode(fdt, node) )
>> +    {
>> +        int addr_cells, size_cells, len;
>> +        const struct fdt_property *prop;
>> +
>> +        if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
>> +            continue;
>> +
>> +        /* Get or set #address-cells and #size-cells */
>> +        prop = fdt_get_property(fdt, node, "#address-cells", &len);
>> +        if ( !prop )
>> +            blexit(L"#address-cells not found in domain node.");
>> +
>> +        addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
>> +
>> +        prop = fdt_get_property(fdt, node, "#size-cells", &len);
>> +        if ( !prop )
>> +            blexit(L"#size-cells not found in domain node.");
>> +
>> +        size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
>> +
>> +        /* Found a node with compatible xen,domain; handle this node. */
>> +        handle_dom0less_domain_node(dir_handle, node, addr_cells, size_cells);
>> +    }
>> +
>> +    /* Free dom0less file names if any */
>> +    for ( ; i < dom0less_modules_idx; i++ )
>> +    {
>> +        /* Free dom0less binary names */
>> +        efi_bs->FreePool(dom0less_modules[i].name);
>> +    }
>> +
>> +    if ( dom0less_modules_idx > 0 )
>> +        return true;
>> +
>> +    return false;
>> }
>> 
>> static void __init efi_arch_cpu(void)
>> @@ -562,8 +806,19 @@ static void __init efi_arch_cpu(void)
>> 
>> static void __init efi_arch_blexit(void)
>> {
>> +    unsigned int i = 0;
>> +
>>     if ( dtbfile.need_to_free )
>>         efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
>> +    /* Free dom0less files if any */
>> +    for ( ; i < dom0less_modules_idx; i++ )
>> +    {
>> +        /* Free dom0less binary names */
>> +        efi_bs->FreePool(dom0less_modules[i].name);
>> +        /* Free dom0less binaries */
>> +        efi_bs->FreePages(dom0less_modules[i].addr,
>> +                          PFN_UP(dom0less_modules[i].size));
>> +    }
>>     if ( memmap )
>>         efi_bs->FreePool(memmap);
>> }
>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
>> index 9b0cc29aae..950fdf16b7 100644
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -678,6 +678,12 @@ static void __init efi_arch_handle_module(const struct file *file,
>>     efi_bs->FreePool(ptr);
>> }
>> 
>> +static bool __init efi_arch_check_dom0less_boot(EFI_FILE_HANDLE dir_handle)
>> +{
>> +    /* x86 doesn't support dom0less */
>> +    return false;
>> +}
>> +
>> static void __init efi_arch_cpu(void)
>> {
>>     uint32_t eax = cpuid_eax(0x80000000);
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index 758f9d74d2..7d8734199e 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1127,15 +1127,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
>>     EFI_LOADED_IMAGE *loaded_image;
>>     EFI_STATUS status;
>> -    unsigned int i, argc;
>> -    CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
>> +    unsigned int i, argc = 0;
>> +    CHAR16 **argv, *file_name = NULL, *cfg_file_name = NULL, *options = NULL;
>>     UINTN gop_mode = ~0;
>>     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>>     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>>     union string section = { NULL }, name;
>>     bool base_video = false;
>> -    const char *option_str;
>> +    const char *option_str = NULL;
>>     bool use_cfg_file;
>> +    EFI_FILE_HANDLE dir_handle;
>> 
>>     __set_bit(EFI_BOOT, &efi_flags);
>>     __set_bit(EFI_LOADER, &efi_flags);
>> @@ -1216,9 +1217,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>> 
>>     efi_arch_relocate_image(0);
>> 
>> +    /* Get the file system interface. */
>> +    dir_handle = get_parent_handle(loaded_image, &file_name);
>> +
>>     if ( use_cfg_file )
>>     {
>> -        EFI_FILE_HANDLE dir_handle;
>>         UINTN depth, cols, rows, size;
>> 
>>         size = cols = rows = depth = 0;
>> @@ -1229,9 +1232,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>> 
>>         gop = efi_get_gop();
>> 
>> -        /* Get the file system interface. */
>> -        dir_handle = get_parent_handle(loaded_image, &file_name);
>> -
>>         /* Read and parse the config file. */
>>         if ( read_section(loaded_image, L"config", &cfg, NULL) )
>>             PrintStr(L"Using builtin config file\r\n");
>> @@ -1285,14 +1285,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>             efi_bs->FreePool(name.w);
>>         }
>> 
>> -        if ( !name.s )
>> -            blexit(L"No Dom0 kernel image specified.");
>> -
>>         efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
>> 
>> -        option_str = split_string(name.s);
>> +        if ( name.s )
>> +            option_str = split_string(name.s);
>> 
>> -        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) )
>> +        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) &&
>> +             name.s )
> 
> I'd probably indent this and put it under the previous if, instead of
> adding "&& name.s". But I'll leave it up to you.

If name.s is not specified we might want to check if the kernel is in the image section anyway.
When read_section is executed, it it returns true, then the kernel is loaded even if the name.s
Is NULL (it is NULL because we didn’t specify kernel=<image> <args>)
The code there is a little bit tricky I can tell.

> 
> 
>>         {
>>             read_file(dir_handle, s2w(&name), &kernel, option_str);
>>             efi_bs->FreePool(name.w);
>> @@ -1361,12 +1360,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>>         cfg.addr = 0;
>> 
>> -        dir_handle->Close(dir_handle);
>> -
>>         if ( gop && !base_video )
>>             gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>>     }
>> 
>> +    /*
>> +     * Check if a proper configuration is provided to start Xen:
>> +     *  - Dom0 specified (minimum required)
>> +     *  - Dom0 and DomU(s) specified
>> +     *  - DomU(s) specified
>> +     */
>> +    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
>> +        blexit(L"No Dom0 kernel image specified.");
>> +
>> +    dir_handle->Close(dir_handle);
>> +
>>     efi_arch_edd();
>> 
>>     /* XXX Collect EDID info. */
>> -- 
>> 2.17.1

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4878 bytes --]

On Thu, 23 Sep 2021, Luca Fancellu wrote:
> >> +/*
> >> + * Binaries will be translated into bootmodules, the maximum number for them is
> >> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> >> + */
> >> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> >> +static struct file __initdata dom0less_file;
> >> +static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
> >> +static unsigned int __initdata dom0less_modules_available =
> >> +                               MAX_DOM0LESS_MODULES;
> >> +static unsigned int __initdata dom0less_modules_idx;
> > 
> > This is a lot better!
> > 
> > We don't need both dom0less_modules_idx and dom0less_modules_available.
> > You can just do:
> > 
> > #define dom0less_modules_available (MAX_DOM0LESS_MODULES - dom0less_modules_idx)
> > static unsigned int __initdata dom0less_modules_idx;
> > 
> > But maybe we can even get rid of dom0less_modules_available entirely?
> > 
> > We can change the check at the beginning of allocate_dom0less_file to:
> > 
> > if ( dom0less_modules_idx == dom0less_modules_available )
> >    blexit
> > 
> > Would that work?
> 
> I thought about it but I think they need to stay, because dom0less_modules_available is the
> upper bound for the additional dom0less modules (it is decremented each time a dom0 module
> Is added), instead dom0less_modules_idx is the typical index for the array of dom0less modules.

[...]


> >> +    /*
> >> +     * Check if there is any space left for a domU module, the variable
> >> +     * dom0less_modules_available is updated each time we use read_file(...)
> >> +     * successfully.
> >> +     */
> >> +    if ( !dom0less_modules_available )
> >> +        blexit(L"No space left for domU modules");
> > 
> > This is the check that could be based on dom0less_modules_idx
> > 
> 
> The only way I see to have it based on dom0less_modules_idx will be to compare it
> to the amount of modules still available, that is not constant because it is dependent
> on how many dom0 modules are loaded, so still two variables needed.
> Don’t know if I’m missing something.

I think I understand where the confusion comes from. I am appending a
small patch to show what I had in mind. We are already accounting for
Xen and the DTB when declaring MAX_DOM0LESS_MODULES (MAX_MODULES - 2).
The other binaries are the Dom0 kernel and ramdisk, however, in my setup
they don't trigger a call to handle_dom0less_module_node because they
are compatible xen,linux-zimage and xen,linux-initrd.

However, the Dom0 kernel and ramdisk can be also compatible
multiboot,kernel and multiboot,ramdisk. If that is the case, then they
would indeed trigger a call to handle_dom0less_module_node.

I think that is not a good idea: a function called
handle_dom0less_module_node should only be called for dom0less modules
(domUs) and not dom0.

But from the memory consumption point of view, it would be better
actually to catch dom0 modules too as you intended. In that case we need to:

- add a check for xen,linux-zimage and xen,linux-initrd in
  handle_dom0less_module_node also

- rename handle_dom0less_domain_node, handle_dom0less_module_node,
  dom0less_file, dom0less_modules, dom0less_modules_idx to something
  else more generic


For instance they could be called:

handle_domain_node
handle_module_node
module_file
modules
modules_idx




diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index e2b007ece0..812d0bd607 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -22,8 +22,6 @@ typedef struct {
 #define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
 static struct file __initdata dom0less_file;
 static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
-static unsigned int __initdata dom0less_modules_available =
-                               MAX_DOM0LESS_MODULES;
 static unsigned int __initdata dom0less_modules_idx;
 
 #define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
@@ -592,14 +590,6 @@ static void __init efi_arch_handle_module(const struct file *file,
          * stop here.
          */
         blexit(L"Unknown module type");
-
-    /*
-     * dom0less_modules_available is decremented here because for each dom0
-     * file added, there will be an additional bootmodule, so the number
-     * of dom0less module files will be decremented because there is
-     * a maximum amount of bootmodules that can be loaded.
-     */
-    dom0less_modules_available--;
 }
 
 /*
@@ -643,7 +633,7 @@ static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
      * dom0less_modules_available is updated each time we use read_file(...)
      * successfully.
      */
-    if ( !dom0less_modules_available )
+    if ( dom0less_modules_idx == MAX_DOM0LESS_MODULES )
         blexit(L"No space left for domU modules");
 
     module_name.s = (char*) name;

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Luca Fancellu]

> On 23 Sep 2021, at 17:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 23 Sep 2021, Luca Fancellu wrote:
>>>> +/*
>>>> + * Binaries will be translated into bootmodules, the maximum number for them is
>>>> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
>>>> + */
>>>> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
>>>> +static struct file __initdata dom0less_file;
>>>> +static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
>>>> +static unsigned int __initdata dom0less_modules_available =
>>>> +                               MAX_DOM0LESS_MODULES;
>>>> +static unsigned int __initdata dom0less_modules_idx;
>>> 
>>> This is a lot better!
>>> 
>>> We don't need both dom0less_modules_idx and dom0less_modules_available.
>>> You can just do:
>>> 
>>> #define dom0less_modules_available (MAX_DOM0LESS_MODULES - dom0less_modules_idx)
>>> static unsigned int __initdata dom0less_modules_idx;
>>> 
>>> But maybe we can even get rid of dom0less_modules_available entirely?
>>> 
>>> We can change the check at the beginning of allocate_dom0less_file to:
>>> 
>>> if ( dom0less_modules_idx == dom0less_modules_available )
>>>   blexit
>>> 
>>> Would that work?
>> 
>> I thought about it but I think they need to stay, because dom0less_modules_available is the
>> upper bound for the additional dom0less modules (it is decremented each time a dom0 module
>> Is added), instead dom0less_modules_idx is the typical index for the array of dom0less modules.
> 
> [...]
> 
> 
>>>> +    /*
>>>> +     * Check if there is any space left for a domU module, the variable
>>>> +     * dom0less_modules_available is updated each time we use read_file(...)
>>>> +     * successfully.
>>>> +     */
>>>> +    if ( !dom0less_modules_available )
>>>> +        blexit(L"No space left for domU modules");
>>> 
>>> This is the check that could be based on dom0less_modules_idx
>>> 
>> 
>> The only way I see to have it based on dom0less_modules_idx will be to compare it
>> to the amount of modules still available, that is not constant because it is dependent
>> on how many dom0 modules are loaded, so still two variables needed.
>> Don’t know if I’m missing something.
> 
> I think I understand where the confusion comes from. I am appending a
> small patch to show what I had in mind. We are already accounting for
> Xen and the DTB when declaring MAX_DOM0LESS_MODULES (MAX_MODULES - 2).
> The other binaries are the Dom0 kernel and ramdisk, however, in my setup
> they don't trigger a call to handle_dom0less_module_node because they
> are compatible xen,linux-zimage and xen,linux-initrd.
> 
> However, the Dom0 kernel and ramdisk can be also compatible
> multiboot,kernel and multiboot,ramdisk. If that is the case, then they
> would indeed trigger a call to handle_dom0less_module_node.
> 
> I think that is not a good idea: a function called
> handle_dom0less_module_node should only be called for dom0less modules
> (domUs) and not dom0.
> 
> But from the memory consumption point of view, it would be better
> actually to catch dom0 modules too as you intended. In that case we need to:
> 
> - add a check for xen,linux-zimage and xen,linux-initrd in
>  handle_dom0less_module_node also
> 
> - rename handle_dom0less_domain_node, handle_dom0less_module_node,
>  dom0less_file, dom0less_modules, dom0less_modules_idx to something
>  else more generic
> 
> 
> For instance they could be called:
> 
> handle_domain_node
> handle_module_node
> module_file
> modules
> modules_idx
> 
> 
> 
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index e2b007ece0..812d0bd607 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -22,8 +22,6 @@ typedef struct {
> #define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> static struct file __initdata dom0less_file;
> static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
> -static unsigned int __initdata dom0less_modules_available =
> -                               MAX_DOM0LESS_MODULES;
> static unsigned int __initdata dom0less_modules_idx;
> 
> #define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
> @@ -592,14 +590,6 @@ static void __init efi_arch_handle_module(const struct file *file,
>          * stop here.
>          */
>         blexit(L"Unknown module type");
> -
> -    /*
> -     * dom0less_modules_available is decremented here because for each dom0
> -     * file added, there will be an additional bootmodule, so the number
> -     * of dom0less module files will be decremented because there is
> -     * a maximum amount of bootmodules that can be loaded.
> -     */
> -    dom0less_modules_available--;
> }
> 
> /*
> @@ -643,7 +633,7 @@ static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>      * dom0less_modules_available is updated each time we use read_file(...)
>      * successfully.
>      */
> -    if ( !dom0less_modules_available )
> +    if ( dom0less_modules_idx == MAX_DOM0LESS_MODULES )
>         blexit(L"No space left for domU modules");
> 
>     module_name.s = (char*) name;

Hi Stefano,

Yes I understand what you mean, unfortunately it can’t be done, I will explain why in details
because the UEFI code is very tricky in the way it was written.

Dom0 modules and xsm-policy are handled in boot.c by read_section(…) or read_file(…) and
both call handle_file_info(…) that inside calls efi_arch_handle_module(…).
Dom0less modules (xen,domain child modules) are handled in efi-boot.h by handle_dom0less_module_node(…)
that may call allocate_dom0less_file(…) and (to reuse code) calls read_file(…).

So there can be maximum (MAX_MODULES-2) boot modules because at start in start_xen(…)
we add Xen and the DTB as boot modules.

This amount is to be shared among dom0 modules (kernel, ramdisk), xsm-policy and domU
modules (kernel, ramdisk, dtb).

The thing is that we don’t know how many dom0 modules will be specified and also for the xsm-policy.

In your code example, let’s say I have MAX_DOM0LESS_MODULES=(32-2) so 30 modules available,
If I declare a Dom0 kernel and 30 DomU kernels, it will pass the check, but on boot I think it will ignore
the exceeding modules.

For that reason we need to keep track of how many “slots” are available, so in my code every time the
read_file(…)/read_section(…) is called, the dom0less_modules_available is decremented.

If we want to get rid of one variable, we can just assume the dom0 modules and xsm-policy will be always
loaded and we can set MAX_DOM0LESS_MODULES=(MAX_MODULES - 2 - 3) where 3 is dom0 kernel,
dom0 ramdisk and xsm-policy. If the user doesn’t load any of them, we just lost 3 slots.

Another solution can be keeping just the dom0less_modules_available and every time loop backward in the array
starting from that position and stopping when we have a dom0less_module_name.name == NULL so we
know we have an available slot or we reach below zero and we know there is no space. But I think it’s not
worthy.

So if you really want to have only one variable, I will remove space from MAX_DOM0LESS_MODULES and
I will push it in v3.

Cheers,
Luca

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Jan Beulich]

On 22.09.2021 16:13, Luca Fancellu wrote:
> +static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                                  const char *name,
> +                                                  unsigned int name_len)
> +{
> +    dom0less_module_name* file_name;
> +    union string module_name;
> +    unsigned int ret_idx;
> +
> +    /*
> +     * Check if there is any space left for a domU module, the variable
> +     * dom0less_modules_available is updated each time we use read_file(...)
> +     * successfully.
> +     */
> +    if ( !dom0less_modules_available )
> +        blexit(L"No space left for domU modules");
> +
> +    module_name.s = (char*) name;

Unfortunately there are too many style issues in these Arm additions to
really enumerate; I'd like to ask that you go through yourself with
./CODING_STYLE, surrounding code, and review comments on earlier patches
of yours in mind. This cast stands out, though: I'm pretty sure you were
told before that casts are often dangerous and hence should be avoided
whenever (easily) possible. There was a prior case where union string
was used in a similar way, not all that long ago. Hence why it now has
a "const char *" member. (That's still somewhat risky, but imo way
better than a cast.)

> @@ -1361,12 +1360,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>          cfg.addr = 0;
>  
> -        dir_handle->Close(dir_handle);
> -
>          if ( gop && !base_video )
>              gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>      }
>  
> +    /*
> +     * Check if a proper configuration is provided to start Xen:
> +     *  - Dom0 specified (minimum required)
> +     *  - Dom0 and DomU(s) specified
> +     *  - DomU(s) specified
> +     */
> +    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
> +        blexit(L"No Dom0 kernel image specified.");
> +
> +    dir_handle->Close(dir_handle);

So far I was under the impression that handles and alike need closing
before calling Exit(), to prevent resource leaks. While I will admit
that likely there are more (pre-existing) affected paths, I think that
- if that understanding of mine is correct - it would be nice to avoid
adding yet more instances.

Jan

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Luca Fancellu]

> On 24 Sep 2021, at 15:02, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 22.09.2021 16:13, Luca Fancellu wrote:
>> +static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>> +                                                  const char *name,
>> +                                                  unsigned int name_len)
>> +{
>> +    dom0less_module_name* file_name;
>> +    union string module_name;
>> +    unsigned int ret_idx;
>> +
>> +    /*
>> +     * Check if there is any space left for a domU module, the variable
>> +     * dom0less_modules_available is updated each time we use read_file(...)
>> +     * successfully.
>> +     */
>> +    if ( !dom0less_modules_available )
>> +        blexit(L"No space left for domU modules");
>> +
>> +    module_name.s = (char*) name;
> 
> Unfortunately there are too many style issues in these Arm additions to
> really enumerate; I'd like to ask that you go through yourself with
> ./CODING_STYLE, surrounding code, and review comments on earlier patches
> of yours in mind. This cast stands out, though: I'm pretty sure you were
> told before that casts are often dangerous and hence should be avoided
> whenever (easily) possible. There was a prior case where union string
> was used in a similar way, not all that long ago. Hence why it now has
> a "const char *" member. (That's still somewhat risky, but imo way
> better than a cast.)

Hi Jan,

Yes I will use the .cs member, I will have also a better look on the patch to
find the style issues.

> 
>> @@ -1361,12 +1360,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>>         cfg.addr = 0;
>> 
>> -        dir_handle->Close(dir_handle);
>> -
>>         if ( gop && !base_video )
>>             gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>>     }
>> 
>> +    /*
>> +     * Check if a proper configuration is provided to start Xen:
>> +     *  - Dom0 specified (minimum required)
>> +     *  - Dom0 and DomU(s) specified
>> +     *  - DomU(s) specified
>> +     */
>> +    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
>> +        blexit(L"No Dom0 kernel image specified.");
>> +
>> +    dir_handle->Close(dir_handle);
> 
> So far I was under the impression that handles and alike need closing
> before calling Exit(), to prevent resource leaks. While I will admit
> that likely there are more (pre-existing) affected paths, I think that
> - if that understanding of mine is correct - it would be nice to avoid
> adding yet more instances.

Ok sure, I will close the handle before the blexit.

Cheers,
Luca

> 
> Jan
> 

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8155 bytes --]

On Fri, 24 Sep 2021, Luca Fancellu wrote:
> > On 23 Sep 2021, at 17:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Thu, 23 Sep 2021, Luca Fancellu wrote:
> >>>> +/*
> >>>> + * Binaries will be translated into bootmodules, the maximum number for them is
> >>>> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> >>>> + */
> >>>> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> >>>> +static struct file __initdata dom0less_file;
> >>>> +static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
> >>>> +static unsigned int __initdata dom0less_modules_available =
> >>>> +                               MAX_DOM0LESS_MODULES;
> >>>> +static unsigned int __initdata dom0less_modules_idx;
> >>> 
> >>> This is a lot better!
> >>> 
> >>> We don't need both dom0less_modules_idx and dom0less_modules_available.
> >>> You can just do:
> >>> 
> >>> #define dom0less_modules_available (MAX_DOM0LESS_MODULES - dom0less_modules_idx)
> >>> static unsigned int __initdata dom0less_modules_idx;
> >>> 
> >>> But maybe we can even get rid of dom0less_modules_available entirely?
> >>> 
> >>> We can change the check at the beginning of allocate_dom0less_file to:
> >>> 
> >>> if ( dom0less_modules_idx == dom0less_modules_available )
> >>>   blexit
> >>> 
> >>> Would that work?
> >> 
> >> I thought about it but I think they need to stay, because dom0less_modules_available is the
> >> upper bound for the additional dom0less modules (it is decremented each time a dom0 module
> >> Is added), instead dom0less_modules_idx is the typical index for the array of dom0less modules.
> > 
> > [...]
> > 
> > 
> >>>> +    /*
> >>>> +     * Check if there is any space left for a domU module, the variable
> >>>> +     * dom0less_modules_available is updated each time we use read_file(...)
> >>>> +     * successfully.
> >>>> +     */
> >>>> +    if ( !dom0less_modules_available )
> >>>> +        blexit(L"No space left for domU modules");
> >>> 
> >>> This is the check that could be based on dom0less_modules_idx
> >>> 
> >> 
> >> The only way I see to have it based on dom0less_modules_idx will be to compare it
> >> to the amount of modules still available, that is not constant because it is dependent
> >> on how many dom0 modules are loaded, so still two variables needed.
> >> Don’t know if I’m missing something.
> > 
> > I think I understand where the confusion comes from. I am appending a
> > small patch to show what I had in mind. We are already accounting for
> > Xen and the DTB when declaring MAX_DOM0LESS_MODULES (MAX_MODULES - 2).
> > The other binaries are the Dom0 kernel and ramdisk, however, in my setup
> > they don't trigger a call to handle_dom0less_module_node because they
> > are compatible xen,linux-zimage and xen,linux-initrd.
> > 
> > However, the Dom0 kernel and ramdisk can be also compatible
> > multiboot,kernel and multiboot,ramdisk. If that is the case, then they
> > would indeed trigger a call to handle_dom0less_module_node.
> > 
> > I think that is not a good idea: a function called
> > handle_dom0less_module_node should only be called for dom0less modules
> > (domUs) and not dom0.

I can see that I misread the code yesterday: Dom0 modules don't go
through handle_dom0less_module_node thanks to the xen,domain check in
efi_arch_check_dom0less_boot.


> > But from the memory consumption point of view, it would be better
> > actually to catch dom0 modules too as you intended. In that case we need to:
> > 
> > - add a check for xen,linux-zimage and xen,linux-initrd in
> >  handle_dom0less_module_node also
> > 
> > - rename handle_dom0less_domain_node, handle_dom0less_module_node,
> >  dom0less_file, dom0less_modules, dom0less_modules_idx to something
> >  else more generic
> > 
> > 
> > For instance they could be called:
> > 
> > handle_domain_node
> > handle_module_node
> > module_file
> > modules
> > modules_idx
> > 
> > 
> > 
> > 
> > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> > index e2b007ece0..812d0bd607 100644
> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -22,8 +22,6 @@ typedef struct {
> > #define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> > static struct file __initdata dom0less_file;
> > static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
> > -static unsigned int __initdata dom0less_modules_available =
> > -                               MAX_DOM0LESS_MODULES;
> > static unsigned int __initdata dom0less_modules_idx;
> > 
> > #define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
> > @@ -592,14 +590,6 @@ static void __init efi_arch_handle_module(const struct file *file,
> >          * stop here.
> >          */
> >         blexit(L"Unknown module type");
> > -
> > -    /*
> > -     * dom0less_modules_available is decremented here because for each dom0
> > -     * file added, there will be an additional bootmodule, so the number
> > -     * of dom0less module files will be decremented because there is
> > -     * a maximum amount of bootmodules that can be loaded.
> > -     */
> > -    dom0less_modules_available--;
> > }
> > 
> > /*
> > @@ -643,7 +633,7 @@ static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> >      * dom0less_modules_available is updated each time we use read_file(...)
> >      * successfully.
> >      */
> > -    if ( !dom0less_modules_available )
> > +    if ( dom0less_modules_idx == MAX_DOM0LESS_MODULES )
> >         blexit(L"No space left for domU modules");
> > 
> >     module_name.s = (char*) name;
> 
> Hi Stefano,
> 
> Yes I understand what you mean, unfortunately it can’t be done, I will explain why in details
> because the UEFI code is very tricky in the way it was written.
> 
> Dom0 modules and xsm-policy are handled in boot.c by read_section(…) or read_file(…) and
> both call handle_file_info(…) that inside calls efi_arch_handle_module(…).
> Dom0less modules (xen,domain child modules) are handled in efi-boot.h by handle_dom0less_module_node(…)
> that may call allocate_dom0less_file(…) and (to reuse code) calls read_file(…).
> 
> So there can be maximum (MAX_MODULES-2) boot modules because at start in start_xen(…)
> we add Xen and the DTB as boot modules.
> 
> This amount is to be shared among dom0 modules (kernel, ramdisk), xsm-policy and domU
> modules (kernel, ramdisk, dtb).
> 
> The thing is that we don’t know how many dom0 modules will be specified and also for the xsm-policy.
> 
> In your code example, let’s say I have MAX_DOM0LESS_MODULES=(32-2) so 30 modules available,
> If I declare a Dom0 kernel and 30 DomU kernels, it will pass the check, but on boot I think it will ignore
> the exceeding modules.
> 
> For that reason we need to keep track of how many “slots” are available, so in my code every time the
> read_file(…)/read_section(…) is called, the dom0less_modules_available is decremented.
> 
> If we want to get rid of one variable, we can just assume the dom0 modules and xsm-policy will be always
> loaded and we can set MAX_DOM0LESS_MODULES=(MAX_MODULES - 2 - 3) where 3 is dom0 kernel,
> dom0 ramdisk and xsm-policy. If the user doesn’t load any of them, we just lost 3 slots.
> 
> Another solution can be keeping just the dom0less_modules_available and every time loop backward in the array
> starting from that position and stopping when we have a dom0less_module_name.name == NULL so we
> know we have an available slot or we reach below zero and we know there is no space. But I think it’s not
> worthy.
> 
> So if you really want to have only one variable, I will remove space from MAX_DOM0LESS_MODULES and
> I will push it in v3.
 
You are right, let's just keep dom0less_modules_available, thanks for
the explanation.

One suggestion for future improvement (this patch series is OK as is)
would be to extend get_dom0less_file_index to also be able to search for
the dom0 kernel and ramdisk modules so that if a dom0less kernel or
ramdisk has the same filename as the dom0 kernel or ramdisk, the file
doesn't need to be loaded twice. It could be a follow-up patch after
this series gets committed.