* [PATCH v3 0/3] arm/efi: Add dom0less support to UEFI boot
@ 2021-09-28 16:32 Luca Fancellu
2021-09-28 16:32 ` [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property Luca Fancellu
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Luca Fancellu @ 2021-09-28 16:32 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu, Roger Pau Monné
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 (3):
arm/efi: Introduce uefi,cfg-load DT property
arm/efi: Use dom0less configuration when using EFI boot
arm/efi: load dom0 modules from DT using UEFI
docs/misc/arm/device-tree/booting.txt | 37 +++
docs/misc/efi.pandoc | 263 +++++++++++++++++++
xen/arch/arm/efi/efi-boot.h | 361 +++++++++++++++++++++++++-
xen/arch/x86/efi/efi-boot.h | 6 +
xen/common/efi/boot.c | 54 ++--
5 files changed, 696 insertions(+), 25 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property
2021-09-28 16:32 [PATCH v3 0/3] arm/efi: Add dom0less support to UEFI boot Luca Fancellu
@ 2021-09-28 16:32 ` Luca Fancellu
2021-09-28 22:03 ` Stefano Stabellini
2021-09-28 16:32 ` [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot Luca Fancellu
2021-09-28 16:32 ` [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI Luca Fancellu
2 siblings, 1 reply; 15+ messages in thread
From: Luca Fancellu @ 2021-09-28 16:32 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu
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.
Modify a comment in efi_arch_use_config_file, removing
the part that states "dom0 required" because it's not
true anymore with this commit.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v3 changes:
- add documentation to misc/arm/device-tree/booting.txt
- Modified variable name and logic from skip_cfg_file to
load_cfg_file
- Add in the commit message that I'm modifying a comment.
v2 changes:
- Introduced uefi,cfg-load property
- Add documentation about the property
---
docs/misc/arm/device-tree/booting.txt | 8 ++++++++
docs/misc/efi.pandoc | 2 ++
xen/arch/arm/efi/efi-boot.h | 28 ++++++++++++++++++++++-----
3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 44cd9e1a9a..cf878b478e 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -121,6 +121,14 @@ A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
for Dom0 and bootargs for native Linux.
+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.
+
+
Creating Multiple Domains directly from Xen
===========================================
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..4f1b01757d 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 load_cfg_file = true;
/*
* 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 )
+ load_cfg_file = false;
+ }
+ }
+
+ if ( !fdt || load_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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot
2021-09-28 16:32 [PATCH v3 0/3] arm/efi: Add dom0less support to UEFI boot Luca Fancellu
2021-09-28 16:32 ` [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property Luca Fancellu
@ 2021-09-28 16:32 ` Luca Fancellu
2021-09-28 22:47 ` Stefano Stabellini
2021-09-29 7:50 ` Jan Beulich
2021-09-28 16:32 ` [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI Luca Fancellu
2 siblings, 2 replies; 15+ messages in thread
From: Luca Fancellu @ 2021-09-28 16:32 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu, Roger Pau Monné
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 v3:
- fixed documentation
- fixed name len in strlcpy
- fixed some style issues
- closed filesystem handle before calling blexit
- passed runtime errors up to the stack instead
of calling blexit
- renamed names and function to make them more
general in prevision to load also Dom0 kernel
and ramdisk from DT
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 | 305 +++++++++++++++++++++++++-
xen/arch/x86/efi/efi-boot.h | 6 +
xen/common/efi/boot.c | 42 ++--
5 files changed, 562 insertions(+), 15 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index cf878b478e..354bb43fe1 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -190,6 +190,13 @@ The kernel sub-node has the following properties:
Command line parameters for the guest kernel.
+- uefi,binary (UEFI boot only)
+
+ String property that specifies the file name to be loaded by the UEFI boot
+ for this module. If this is specified, there is no need to specify the reg
+ property because it will be created by the UEFI stub on boot.
+ This option is needed only when UEFI boot is used.
+
The ramdisk sub-node has the following properties:
- compatible
@@ -201,6 +208,13 @@ The ramdisk sub-node has the following properties:
Specifies the physical address of the ramdisk in RAM and its
length.
+- uefi,binary (UEFI boot only)
+
+ String property that specifies the file name to be loaded by the UEFI boot
+ for this module. If this is specified, there is no need to specify the reg
+ property because it will be created by the UEFI stub on boot.
+ This option is needed only when UEFI boot is used.
+
Example
=======
@@ -265,6 +279,13 @@ The dtb sub-node should have the following properties:
Specifies the physical address of the device tree binary fragment
RAM and its length.
+- uefi,binary (UEFI boot only)
+
+ String property that specifies the file name to be loaded by the UEFI boot
+ for this module. If this is specified, there is no need to specify the reg
+ property because it will be created by the UEFI stub on boot.
+ This option is needed only when UEFI boot is used.
+
As an example:
module@0xc000000 {
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index e289c5e7ba..800e67a233 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
+
+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.
+When adding DomU modules to device tree, also add the property
+uefi,cfg-load under chosen for Xen to 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 4f1b01757d..4f7c913f86 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -8,9 +8,49 @@
#include <asm/setup.h>
#include <asm/smp.h>
+typedef struct {
+ char *name;
+ unsigned int name_len;
+ EFI_PHYSICAL_ADDRESS addr;
+ UINTN size;
+} 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_UEFI_MODULES (MAX_MODULES - 2)
+static struct file __initdata module_binary;
+static module_name __initdata modules[MAX_UEFI_MODULES];
+static unsigned int __initdata modules_available = MAX_UEFI_MODULES;
+static unsigned int __initdata modules_idx;
+
+#define ERROR_BINARY_FILE_NOT_FOUND (-1)
+#define ERROR_ALLOC_MODULE_NO_SPACE (-1)
+#define ERROR_ALLOC_MODULE_NAME (-2)
+#define ERROR_MISSING_DT_PROPERTY (-3)
+#define ERROR_RENAME_MODULE_NAME (-4)
+#define ERROR_SET_REG_PROPERTY (-5)
+#define ERROR_DT_MODULE_DOMU (-1)
+#define ERROR_DT_CHOSEN_NODE (-2)
+
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_module_file_index(const char *name, unsigned int name_len);
+static void PrintMessage(const CHAR16 *s);
+static int allocate_module_file(EFI_FILE_HANDLE dir_handle,
+ const char *name,
+ unsigned int name_len);
+static int handle_module_node(EFI_FILE_HANDLE dir_handle,
+ int module_node_offset,
+ int reg_addr_cells,
+ int reg_size_cells);
+static bool is_boot_module(int dt_module_offset);
+static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
+ int domain_node);
+static int efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle);
+
#define DEVICE_TREE_GUID \
{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
@@ -552,8 +592,260 @@ 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 != &module_binary )
+ /*
+ * If file is not a dom0 module file and it's not a domU module,
+ * stop here.
+ */
blexit(L"Unknown module type");
+
+ /*
+ * 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.
+ */
+ modules_available--;
+}
+
+/*
+ * This function checks for a binary previously loaded with a give name, it
+ * returns the index of the file in the modules array or a negative number if no
+ * file with that name is found.
+ */
+static int __init get_module_file_index(const char *name,
+ unsigned int name_len)
+{
+ unsigned int i;
+ int ret = ERROR_BINARY_FILE_NOT_FOUND;
+
+ for ( i = 0; i < modules_idx; i++ )
+ {
+ module_name *mod = &modules[i];
+ if ( (mod->name_len == name_len) &&
+ (strncmp(mod->name, name, name_len) == 0) )
+ {
+ ret = i;
+ break;
+ }
+ }
+ return ret;
+}
+
+static void __init PrintMessage(const CHAR16 *s)
+{
+ PrintStr(s);
+ PrintStr(newline);
+}
+
+/*
+ * This function allocates a binary and keeps track of its name, it returns the
+ * index of the file in the modules array or a negative number on error.
+ */
+static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle,
+ const char *name,
+ unsigned int name_len)
+{
+ module_name *file_name;
+ union string module_name;
+ int ret;
+
+ /*
+ * Check if there is any space left for a module, the variable
+ * modules_available is updated each time we use read_file(...)
+ * successfully.
+ */
+ if ( !modules_available )
+ {
+ PrintMessage(L"No space left for modules");
+ return ERROR_ALLOC_MODULE_NO_SPACE;
+ }
+
+ module_name.cs = name;
+ ret = modules_idx;
+
+ /* Save at this index the name of this binary */
+ file_name = &modules[ret];
+
+ if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
+ (void**)&file_name->name) != EFI_SUCCESS )
+ {
+ PrintMessage(L"Error allocating memory for module binary name");
+ return ERROR_ALLOC_MODULE_NAME;
+ }
+
+ /* Save name and length of the binary in the data structure */
+ strlcpy(file_name->name, name, name_len + 1);
+ file_name->name_len = name_len;
+
+ /* Load the binary in memory */
+ read_file(dir_handle, s2w(&module_name), &module_binary, NULL);
+
+ /* Save address and size */
+ file_name->addr = module_binary.addr;
+ file_name->size = module_binary.size;
+
+ /* s2w(...) allocates some memory, free it */
+ efi_bs->FreePool(module_name.w);
+
+ modules_idx++;
+
+ return ret;
+}
+
+/*
+ * This function checks for the presence of the uefi,binary property in the
+ * module, if found it loads the binary as module and sets the right address
+ * for the reg property into the module DT node.
+ */
+static int __init handle_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;
+ 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 0;
+
+ file_idx = get_module_file_index(uefi_name_prop, uefi_name_len);
+ if ( file_idx < 0 )
+ {
+ file_idx = allocate_module_file(dir_handle, uefi_name_prop,
+ uefi_name_len);
+ if ( file_idx < 0 )
+ return file_idx;
+ }
+
+ file = &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 )
+ {
+ PrintMessage(L"Unable to modify module node name.");
+ return ERROR_RENAME_MODULE_NAME;
+ }
+
+ if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
+ file->addr, file->size) < 0 )
+ {
+ PrintMessage(L"Unable to set module reg property.");
+ return ERROR_SET_REG_PROPERTY;
+ }
+
+ return 0;
+}
+
+static bool __init is_boot_module(int dt_module_offset)
+{
+ if ( (fdt_node_check_compatible(fdt, dt_module_offset,
+ "multiboot,kernel") == 0) ||
+ (fdt_node_check_compatible(fdt, dt_module_offset,
+ "multiboot,ramdisk") == 0) ||
+ (fdt_node_check_compatible(fdt, dt_module_offset,
+ "multiboot,device-tree") == 0) )
+ return true;
+
+ return false;
+}
+
+/*
+ * This function checks for boot modules under the domU guest domain node
+ * in the DT.
+ * Returns 0 on success, negative number on error.
+ */
+static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
+ int domain_node)
+{
+ int module_node, addr_cells, size_cells, len;
+ const struct fdt_property *prop;
+
+ /* Get #address-cells and #size-cells from domain node */
+ prop = fdt_get_property(fdt, domain_node, "#address-cells", &len);
+ if ( !prop )
+ {
+ PrintMessage(L"#address-cells not found in domain node.");
+ return ERROR_MISSING_DT_PROPERTY;
+ }
+
+ addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+ prop = fdt_get_property(fdt, domain_node, "#size-cells", &len);
+ if ( !prop )
+ {
+ PrintMessage(L"#size-cells not found in domain node.");
+ return ERROR_MISSING_DT_PROPERTY;
+ }
+
+ size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+ /*
+ * 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 ( is_boot_module(module_node) )
+ {
+ int ret = handle_module_node(dir_handle, module_node, addr_cells,
+ size_cells);
+ if ( ret < 0 )
+ return ret;
+ }
+
+ return 0;
+}
+
+/*
+ * This function checks for xen domain nodes under the /chosen node for possible
+ * domU guests to be loaded.
+ * Returns the number of modules loaded or a negative number for error.
+ */
+static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
+{
+ int chosen, node, 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 )
+ {
+ PrintMessage(L"Unable to setup chosen node");
+ return ERROR_DT_CHOSEN_NODE;
+ }
+
+ /* Check for nodes compatible with xen,domain under the chosen node */
+ for ( node = fdt_first_subnode(fdt, chosen);
+ node > 0;
+ node = fdt_next_subnode(fdt, node) )
+ {
+ if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
+ {
+ /* Found a node with compatible xen,domain; handle this node. */
+ if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
+ return ERROR_DT_MODULE_DOMU;
+ }
+ }
+
+ /* Free dom0less file names if any */
+ for ( ; i < modules_idx; i++ )
+ {
+ /* Free dom0less binary names */
+ efi_bs->FreePool(modules[i].name);
+ }
+
+ return modules_idx;
}
static void __init efi_arch_cpu(void)
@@ -562,8 +854,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 < modules_idx; i++ )
+ {
+ /* Free dom0less binary names */
+ efi_bs->FreePool(modules[i].name);
+ /* Free dom0less binaries */
+ efi_bs->FreePages(modules[i].addr,
+ PFN_UP(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..f5538bfad4 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 int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
+{
+ /* x86 doesn't support device tree boot */
+ return 0;
+}
+
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..c8c57fbb54 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1127,15 +1127,17 @@ 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;
+ int dt_module_found;
+ EFI_FILE_HANDLE dir_handle;
__set_bit(EFI_BOOT, &efi_flags);
__set_bit(EFI_LOADER, &efi_flags);
@@ -1216,9 +1218,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 +1233,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 +1286,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 +1361,26 @@ 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);
}
+ dt_module_found = efi_arch_check_dt_boot(dir_handle);
+
+ dir_handle->Close(dir_handle);
+
+ if (dt_module_found < 0)
+ /* efi_arch_check_dt_boot throws some error */
+ blexit(L"Error processing boot modules on DT.");
+ /*
+ * Check if a proper configuration is provided to start Xen:
+ * - Dom0 specified (minimum required)
+ * - Dom0 and DomU(s) specified
+ * - DomU(s) specified
+ */
+ if ( !dt_module_found && !kernel.addr )
+ blexit(L"No Dom0 kernel image specified.");
+
efi_arch_edd();
/* XXX Collect EDID info. */
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
2021-09-28 16:32 [PATCH v3 0/3] arm/efi: Add dom0less support to UEFI boot Luca Fancellu
2021-09-28 16:32 ` [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property Luca Fancellu
2021-09-28 16:32 ` [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot Luca Fancellu
@ 2021-09-28 16:32 ` Luca Fancellu
2021-09-28 22:46 ` Stefano Stabellini
2021-09-29 8:00 ` Jan Beulich
2 siblings, 2 replies; 15+ messages in thread
From: Luca Fancellu @ 2021-09-28 16:32 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu
Add support to load Dom0 boot modules from
the device tree using the uefi,binary property.
Update documentation about that.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes in v3:
- new patch
---
docs/misc/arm/device-tree/booting.txt | 8 ++++
docs/misc/efi.pandoc | 64 +++++++++++++++++++++++++--
xen/arch/arm/efi/efi-boot.h | 36 +++++++++++++--
xen/common/efi/boot.c | 12 ++---
4 files changed, 108 insertions(+), 12 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 354bb43fe1..e73f6476d4 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -70,6 +70,14 @@ Each node contains the following properties:
priority of this field vs. other mechanisms of specifying the
bootargs for the kernel.
+- uefi,binary (UEFI boot only)
+
+ String property that specifies the file name to be loaded by the UEFI
+ boot for this module. If this is specified, there is no need to specify
+ the reg property because it will be created by the UEFI stub on boot.
+ This option is needed only when UEFI boot is used, the node needs to be
+ compatible with multiboot,kernel or multiboot,ramdisk.
+
Examples
========
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 800e67a233..4cebc47a18 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -167,6 +167,28 @@ sbsign \
--output xen.signed.efi \
xen.unified.efi
```
+## UEFI boot and Dom0 modules on ARM
+
+When booting using UEFI on ARM, it is possible to specify the Dom0 modules
+directly from the device tree without using the Xen configuration file, here an
+example:
+
+chosen {
+ #size-cells = <0x1>;
+ #address-cells = <0x1>;
+ xen,xen-bootargs = "[Xen boot arguments]"
+
+ module@1 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ uefi,binary = "vmlinuz-3.0.31-0.4-xen";
+ bootargs = "[domain 0 command line options]";
+ };
+
+ module@2 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ uefi,binary = "initrd-3.0.31-0.4-xen";
+ };
+}
## UEFI boot and dom0less on ARM
@@ -326,10 +348,10 @@ chosen {
### Boot Xen, Dom0 and DomU(s)
This configuration is a mix of the two configuration above, to boot this one
-the configuration file must be processed so the /chosen node must have the
-"uefi,cfg-load" property.
+the configuration file can be processed or the Dom0 modules can be read from
+the device tree.
-Here an example:
+Here the first example:
Xen configuration file:
@@ -369,4 +391,40 @@ chosen {
};
```
+Here the second example:
+
+Device tree:
+
+```
+chosen {
+ #size-cells = <0x1>;
+ #address-cells = <0x1>;
+ xen,xen-bootargs = "[Xen boot arguments]"
+
+ module@1 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ uefi,binary = "vmlinuz-3.0.31-0.4-xen";
+ bootargs = "[domain 0 command line options]";
+ };
+
+ module@2 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ uefi,binary = "initrd-3.0.31-0.4-xen";
+ };
+
+ domU1 {
+ #size-cells = <0x1>;
+ #address-cells = <0x1>;
+ compatible = "xen,domain";
+ cpus = <0x1>;
+ memory = <0x0 0xc0000>;
+ vpl011;
+ module@1 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ uefi,binary = "Image-domu1.bin";
+ bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
+ };
+ };
+};
+```
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 4f7c913f86..df63387136 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -31,8 +31,10 @@ static unsigned int __initdata modules_idx;
#define ERROR_MISSING_DT_PROPERTY (-3)
#define ERROR_RENAME_MODULE_NAME (-4)
#define ERROR_SET_REG_PROPERTY (-5)
+#define ERROR_DOM0_ALREADY_FOUND (-6)
#define ERROR_DT_MODULE_DOMU (-1)
#define ERROR_DT_CHOSEN_NODE (-2)
+#define ERROR_DT_MODULE_DOM0 (-3)
void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
void __flush_dcache_area(const void *vaddr, unsigned long size);
@@ -45,7 +47,8 @@ static int allocate_module_file(EFI_FILE_HANDLE dir_handle,
static int handle_module_node(EFI_FILE_HANDLE dir_handle,
int module_node_offset,
int reg_addr_cells,
- int reg_size_cells);
+ int reg_size_cells,
+ bool is_domu_module);
static bool is_boot_module(int dt_module_offset);
static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
int domain_node);
@@ -701,7 +704,8 @@ static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle,
static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
int module_node_offset,
int reg_addr_cells,
- int reg_size_cells)
+ int reg_size_cells,
+ bool is_domu_module)
{
const void *uefi_name_prop;
char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
@@ -743,6 +747,24 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
return ERROR_SET_REG_PROPERTY;
}
+ if ( !is_domu_module &&
+ (fdt_node_check_compatible(fdt, module_node_offset,
+ "multiboot,kernel") == 0) )
+ {
+ /*
+ * This is the Dom0 kernel, wire it to the kernel variable because it
+ * will be verified by the shim lock protocol later in the common code.
+ */
+ if ( kernel.addr )
+ {
+ PrintMessage(L"Dom0 kernel already found in cfg file.");
+ return ERROR_DOM0_ALREADY_FOUND;
+ }
+ kernel.need_to_free = false; /* Freed using the module array */
+ kernel.addr = file->addr;
+ kernel.size = file->size;
+ }
+
return 0;
}
@@ -799,7 +821,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
if ( is_boot_module(module_node) )
{
int ret = handle_module_node(dir_handle, module_node, addr_cells,
- size_cells);
+ size_cells, true);
if ( ret < 0 )
return ret;
}
@@ -809,7 +831,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
/*
* This function checks for xen domain nodes under the /chosen node for possible
- * domU guests to be loaded.
+ * dom0 and domU guests to be loaded.
* Returns the number of modules loaded or a negative number for error.
*/
static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
@@ -836,6 +858,12 @@ static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
return ERROR_DT_MODULE_DOMU;
}
+ else if ( is_boot_module(node) )
+ {
+ if ( handle_module_node(dir_handle, node, addr_len, size_len,
+ false) < 0 )
+ return ERROR_DT_MODULE_DOM0;
+ }
}
/* Free dom0less file names if any */
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index c8c57fbb54..b221494a06 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
{
read_file(dir_handle, s2w(&name), &kernel, option_str);
efi_bs->FreePool(name.w);
-
- if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
- (void **)&shim_lock)) &&
- (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
- PrintErrMesg(L"Dom0 kernel image could not be verified", status);
}
if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
@@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
if (dt_module_found < 0)
/* efi_arch_check_dt_boot throws some error */
blexit(L"Error processing boot modules on DT.");
+
+ /* If Dom0 is specified, verify it */
+ if ( kernel.ptr &&
+ !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
+ (void **)&shim_lock)) &&
+ (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
+ PrintErrMesg(L"Dom0 kernel image could not be verified", status);
/*
* Check if a proper configuration is provided to start Xen:
* - Dom0 specified (minimum required)
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property
2021-09-28 16:32 ` [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property Luca Fancellu
@ 2021-09-28 22:03 ` Stefano Stabellini
[not found] ` <AB8FEF63-EA1F-427F-AF2B-13C1E930F682@arm.com>
0 siblings, 1 reply; 15+ messages in thread
From: Stefano Stabellini @ 2021-09-28 22:03 UTC (permalink / raw)
To: Luca Fancellu
Cc: xen-devel, bertrand.marquis, wei.chen, Stefano Stabellini,
Julien Grall, Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Wei Liu
On Tue, 28 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.
>
> Modify a comment in efi_arch_use_config_file, removing
> the part that states "dom0 required" because it's not
> true anymore with this commit.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
The patch looks good. Only one minor change: given that this is a Xen
parameter that we are introducing and not a parameter defined by UEFI
Forum, I think uefi,cfg-load should be called xen,uefi-cfg-load instead.
Because "xen," is our prefix, while "uefi," is not.
With that minor change:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Note that the uefi,binary property is different because that property is
for xen,domain nodes, so we are already in a Xen specific namespace when
we are using it. Instead this property is for /chosen which is not a Xen
specific node.
> ---
> v3 changes:
> - add documentation to misc/arm/device-tree/booting.txt
> - Modified variable name and logic from skip_cfg_file to
> load_cfg_file
> - Add in the commit message that I'm modifying a comment.
> v2 changes:
> - Introduced uefi,cfg-load property
> - Add documentation about the property
> ---
> docs/misc/arm/device-tree/booting.txt | 8 ++++++++
> docs/misc/efi.pandoc | 2 ++
> xen/arch/arm/efi/efi-boot.h | 28 ++++++++++++++++++++++-----
> 3 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 44cd9e1a9a..cf878b478e 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -121,6 +121,14 @@ A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
> for Dom0 and bootargs for native Linux.
>
>
> +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.
> +
> +
> Creating Multiple Domains directly from Xen
> ===========================================
>
> 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..4f1b01757d 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 load_cfg_file = true;
> /*
> * 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 )
> + load_cfg_file = false;
> + }
> + }
> +
> + if ( !fdt || load_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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
2021-09-28 16:32 ` [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI Luca Fancellu
@ 2021-09-28 22:46 ` Stefano Stabellini
[not found] ` <1E500DE6-1680-41F6-BE8E-5B7ECBAD24BA@arm.com>
2021-09-29 8:00 ` Jan Beulich
1 sibling, 1 reply; 15+ messages in thread
From: Stefano Stabellini @ 2021-09-28 22:46 UTC (permalink / raw)
To: Luca Fancellu
Cc: xen-devel, bertrand.marquis, wei.chen, Stefano Stabellini,
Julien Grall, Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Wei Liu
On Tue, 28 Sep 2021, Luca Fancellu wrote:
> Add support to load Dom0 boot modules from
> the device tree using the uefi,binary property.
>
> Update documentation about that.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
It is great how simple this patch is!
The patch looks all correct. Only one question: do we need a check to
make sure the dom0 ramdisk is not loaded twice? Once via uefi,binary and
another time via the config file? In other words...
> ---
> Changes in v3:
> - new patch
> ---
> docs/misc/arm/device-tree/booting.txt | 8 ++++
> docs/misc/efi.pandoc | 64 +++++++++++++++++++++++++--
> xen/arch/arm/efi/efi-boot.h | 36 +++++++++++++--
> xen/common/efi/boot.c | 12 ++---
> 4 files changed, 108 insertions(+), 12 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 354bb43fe1..e73f6476d4 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -70,6 +70,14 @@ Each node contains the following properties:
> priority of this field vs. other mechanisms of specifying the
> bootargs for the kernel.
>
> +- uefi,binary (UEFI boot only)
> +
> + String property that specifies the file name to be loaded by the UEFI
> + boot for this module. If this is specified, there is no need to specify
> + the reg property because it will be created by the UEFI stub on boot.
> + This option is needed only when UEFI boot is used, the node needs to be
> + compatible with multiboot,kernel or multiboot,ramdisk.
> +
> Examples
> ========
>
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index 800e67a233..4cebc47a18 100644
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -167,6 +167,28 @@ sbsign \
> --output xen.signed.efi \
> xen.unified.efi
> ```
> +## UEFI boot and Dom0 modules on ARM
> +
> +When booting using UEFI on ARM, it is possible to specify the Dom0 modules
> +directly from the device tree without using the Xen configuration file, here an
> +example:
> +
> +chosen {
> + #size-cells = <0x1>;
> + #address-cells = <0x1>;
> + xen,xen-bootargs = "[Xen boot arguments]"
> +
> + module@1 {
> + compatible = "multiboot,kernel", "multiboot,module";
> + uefi,binary = "vmlinuz-3.0.31-0.4-xen";
> + bootargs = "[domain 0 command line options]";
> + };
> +
> + module@2 {
> + compatible = "multiboot,ramdisk", "multiboot,module";
> + uefi,binary = "initrd-3.0.31-0.4-xen";
> + };
> +}
>
> ## UEFI boot and dom0less on ARM
>
> @@ -326,10 +348,10 @@ chosen {
> ### Boot Xen, Dom0 and DomU(s)
>
> This configuration is a mix of the two configuration above, to boot this one
> -the configuration file must be processed so the /chosen node must have the
> -"uefi,cfg-load" property.
> +the configuration file can be processed or the Dom0 modules can be read from
> +the device tree.
>
> -Here an example:
> +Here the first example:
>
> Xen configuration file:
>
> @@ -369,4 +391,40 @@ chosen {
> };
> ```
>
> +Here the second example:
> +
> +Device tree:
> +
> +```
> +chosen {
> + #size-cells = <0x1>;
> + #address-cells = <0x1>;
> + xen,xen-bootargs = "[Xen boot arguments]"
> +
> + module@1 {
> + compatible = "multiboot,kernel", "multiboot,module";
> + uefi,binary = "vmlinuz-3.0.31-0.4-xen";
> + bootargs = "[domain 0 command line options]";
> + };
> +
> + module@2 {
> + compatible = "multiboot,ramdisk", "multiboot,module";
> + uefi,binary = "initrd-3.0.31-0.4-xen";
> + };
> +
> + domU1 {
> + #size-cells = <0x1>;
> + #address-cells = <0x1>;
> + compatible = "xen,domain";
> + cpus = <0x1>;
> + memory = <0x0 0xc0000>;
> + vpl011;
>
> + module@1 {
> + compatible = "multiboot,kernel", "multiboot,module";
> + uefi,binary = "Image-domu1.bin";
> + bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
> + };
> + };
> +};
> +```
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 4f7c913f86..df63387136 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -31,8 +31,10 @@ static unsigned int __initdata modules_idx;
> #define ERROR_MISSING_DT_PROPERTY (-3)
> #define ERROR_RENAME_MODULE_NAME (-4)
> #define ERROR_SET_REG_PROPERTY (-5)
> +#define ERROR_DOM0_ALREADY_FOUND (-6)
> #define ERROR_DT_MODULE_DOMU (-1)
> #define ERROR_DT_CHOSEN_NODE (-2)
> +#define ERROR_DT_MODULE_DOM0 (-3)
>
> void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
> void __flush_dcache_area(const void *vaddr, unsigned long size);
> @@ -45,7 +47,8 @@ static int allocate_module_file(EFI_FILE_HANDLE dir_handle,
> static int handle_module_node(EFI_FILE_HANDLE dir_handle,
> int module_node_offset,
> int reg_addr_cells,
> - int reg_size_cells);
> + int reg_size_cells,
> + bool is_domu_module);
> static bool is_boot_module(int dt_module_offset);
> static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> int domain_node);
> @@ -701,7 +704,8 @@ static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle,
> static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
> int module_node_offset,
> int reg_addr_cells,
> - int reg_size_cells)
> + int reg_size_cells,
> + bool is_domu_module)
> {
> const void *uefi_name_prop;
> char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
> @@ -743,6 +747,24 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
> return ERROR_SET_REG_PROPERTY;
> }
>
> + if ( !is_domu_module &&
> + (fdt_node_check_compatible(fdt, module_node_offset,
> + "multiboot,kernel") == 0) )
> + {
> + /*
> + * This is the Dom0 kernel, wire it to the kernel variable because it
> + * will be verified by the shim lock protocol later in the common code.
> + */
> + if ( kernel.addr )
> + {
> + PrintMessage(L"Dom0 kernel already found in cfg file.");
> + return ERROR_DOM0_ALREADY_FOUND;
> + }
> + kernel.need_to_free = false; /* Freed using the module array */
> + kernel.addr = file->addr;
> + kernel.size = file->size;
> + }
... is it necessary to update ramdisk as well or check if it is already
set? As far as I can tell it is the only other potential conflict that
we could have.
> return 0;
> }
>
> @@ -799,7 +821,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> if ( is_boot_module(module_node) )
> {
> int ret = handle_module_node(dir_handle, module_node, addr_cells,
> - size_cells);
> + size_cells, true);
> if ( ret < 0 )
> return ret;
> }
> @@ -809,7 +831,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>
> /*
> * This function checks for xen domain nodes under the /chosen node for possible
> - * domU guests to be loaded.
> + * dom0 and domU guests to be loaded.
> * Returns the number of modules loaded or a negative number for error.
> */
> static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> @@ -836,6 +858,12 @@ static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
> return ERROR_DT_MODULE_DOMU;
> }
> + else if ( is_boot_module(node) )
> + {
> + if ( handle_module_node(dir_handle, node, addr_len, size_len,
> + false) < 0 )
> + return ERROR_DT_MODULE_DOM0;
> + }
> }
>
> /* Free dom0less file names if any */
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index c8c57fbb54..b221494a06 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> {
> read_file(dir_handle, s2w(&name), &kernel, option_str);
> efi_bs->FreePool(name.w);
> -
> - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> - (void **)&shim_lock)) &&
> - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> - PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> }
>
> if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> if (dt_module_found < 0)
> /* efi_arch_check_dt_boot throws some error */
> blexit(L"Error processing boot modules on DT.");
> +
> + /* If Dom0 is specified, verify it */
> + if ( kernel.ptr &&
> + !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> + (void **)&shim_lock)) &&
> + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> + PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> /*
> * Check if a proper configuration is provided to start Xen:
> * - Dom0 specified (minimum required)
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot
2021-09-28 16:32 ` [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot Luca Fancellu
@ 2021-09-28 22:47 ` Stefano Stabellini
2021-09-29 7:50 ` Jan Beulich
1 sibling, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2021-09-28 22:47 UTC (permalink / raw)
To: Luca Fancellu
Cc: xen-devel, bertrand.marquis, wei.chen, Stefano Stabellini,
Julien Grall, Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Wei Liu, Roger Pau Monné
On Tue, 28 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>
Some minor feedback about code style and comments below. With those
addressed:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v3:
> - fixed documentation
> - fixed name len in strlcpy
> - fixed some style issues
> - closed filesystem handle before calling blexit
> - passed runtime errors up to the stack instead
> of calling blexit
> - renamed names and function to make them more
> general in prevision to load also Dom0 kernel
> and ramdisk from DT
> 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 | 305 +++++++++++++++++++++++++-
> xen/arch/x86/efi/efi-boot.h | 6 +
> xen/common/efi/boot.c | 42 ++--
> 5 files changed, 562 insertions(+), 15 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index cf878b478e..354bb43fe1 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -190,6 +190,13 @@ The kernel sub-node has the following properties:
>
> Command line parameters for the guest kernel.
>
> +- uefi,binary (UEFI boot only)
> +
> + String property that specifies the file name to be loaded by the UEFI boot
> + for this module. If this is specified, there is no need to specify the reg
> + property because it will be created by the UEFI stub on boot.
> + This option is needed only when UEFI boot is used.
> +
> The ramdisk sub-node has the following properties:
>
> - compatible
> @@ -201,6 +208,13 @@ The ramdisk sub-node has the following properties:
> Specifies the physical address of the ramdisk in RAM and its
> length.
>
> +- uefi,binary (UEFI boot only)
> +
> + String property that specifies the file name to be loaded by the UEFI boot
> + for this module. If this is specified, there is no need to specify the reg
> + property because it will be created by the UEFI stub on boot.
> + This option is needed only when UEFI boot is used.
> +
>
> Example
> =======
> @@ -265,6 +279,13 @@ The dtb sub-node should have the following properties:
> Specifies the physical address of the device tree binary fragment
> RAM and its length.
>
> +- uefi,binary (UEFI boot only)
> +
> + String property that specifies the file name to be loaded by the UEFI boot
> + for this module. If this is specified, there is no need to specify the reg
> + property because it will be created by the UEFI stub on boot.
> + This option is needed only when UEFI boot is used.
> +
> As an example:
>
> module@0xc000000 {
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index e289c5e7ba..800e67a233 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
> +
> +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.
> +When adding DomU modules to device tree, also add the property
> +uefi,cfg-load under chosen for Xen to load the Xen config file.
Just a note to remind to update uefi,cfg-load to xen,uefi-cfg-load
> +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.
also here
> +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 4f1b01757d..4f7c913f86 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -8,9 +8,49 @@
> #include <asm/setup.h>
> #include <asm/smp.h>
>
> +typedef struct {
> + char *name;
> + unsigned int name_len;
> + EFI_PHYSICAL_ADDRESS addr;
> + UINTN size;
> +} 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_UEFI_MODULES (MAX_MODULES - 2)
> +static struct file __initdata module_binary;
> +static module_name __initdata modules[MAX_UEFI_MODULES];
> +static unsigned int __initdata modules_available = MAX_UEFI_MODULES;
> +static unsigned int __initdata modules_idx;
> +
> +#define ERROR_BINARY_FILE_NOT_FOUND (-1)
> +#define ERROR_ALLOC_MODULE_NO_SPACE (-1)
> +#define ERROR_ALLOC_MODULE_NAME (-2)
> +#define ERROR_MISSING_DT_PROPERTY (-3)
> +#define ERROR_RENAME_MODULE_NAME (-4)
> +#define ERROR_SET_REG_PROPERTY (-5)
> +#define ERROR_DT_MODULE_DOMU (-1)
> +#define ERROR_DT_CHOSEN_NODE (-2)
> +
> 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_module_file_index(const char *name, unsigned int name_len);
> +static void PrintMessage(const CHAR16 *s);
> +static int allocate_module_file(EFI_FILE_HANDLE dir_handle,
> + const char *name,
> + unsigned int name_len);
> +static int handle_module_node(EFI_FILE_HANDLE dir_handle,
> + int module_node_offset,
> + int reg_addr_cells,
> + int reg_size_cells);
> +static bool is_boot_module(int dt_module_offset);
> +static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> + int domain_node);
> +static int efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle);
> +
> #define DEVICE_TREE_GUID \
> {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
>
> @@ -552,8 +592,260 @@ 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 != &module_binary )
> + /*
> + * If file is not a dom0 module file and it's not a domU module,
> + * stop here.
> + */
> blexit(L"Unknown module type");
> +
> + /*
> + * 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
the number of "boot" module files
> + * a maximum amount of bootmodules that can be loaded.
> + */
> + modules_available--;
> +}
> +
> +/*
> + * This function checks for a binary previously loaded with a give name, it
> + * returns the index of the file in the modules array or a negative number if no
> + * file with that name is found.
> + */
> +static int __init get_module_file_index(const char *name,
> + unsigned int name_len)
> +{
> + unsigned int i;
> + int ret = ERROR_BINARY_FILE_NOT_FOUND;
> +
> + for ( i = 0; i < modules_idx; i++ )
> + {
> + module_name *mod = &modules[i];
> + if ( (mod->name_len == name_len) &&
> + (strncmp(mod->name, name, name_len) == 0) )
> + {
> + ret = i;
> + break;
> + }
> + }
> + return ret;
> +}
> +
> +static void __init PrintMessage(const CHAR16 *s)
> +{
> + PrintStr(s);
> + PrintStr(newline);
> +}
> +
> +/*
> + * This function allocates a binary and keeps track of its name, it returns the
> + * index of the file in the modules array or a negative number on error.
> + */
> +static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle,
> + const char *name,
> + unsigned int name_len)
> +{
> + module_name *file_name;
> + union string module_name;
> + int ret;
> +
> + /*
> + * Check if there is any space left for a module, the variable
> + * modules_available is updated each time we use read_file(...)
> + * successfully.
> + */
> + if ( !modules_available )
> + {
> + PrintMessage(L"No space left for modules");
> + return ERROR_ALLOC_MODULE_NO_SPACE;
> + }
> +
> + module_name.cs = name;
> + ret = modules_idx;
> +
> + /* Save at this index the name of this binary */
> + file_name = &modules[ret];
> +
> + if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
> + (void**)&file_name->name) != EFI_SUCCESS )
> + {
> + PrintMessage(L"Error allocating memory for module binary name");
> + return ERROR_ALLOC_MODULE_NAME;
> + }
> +
> + /* Save name and length of the binary in the data structure */
> + strlcpy(file_name->name, name, name_len + 1);
> + file_name->name_len = name_len;
> +
> + /* Load the binary in memory */
> + read_file(dir_handle, s2w(&module_name), &module_binary, NULL);
> +
> + /* Save address and size */
> + file_name->addr = module_binary.addr;
> + file_name->size = module_binary.size;
> +
> + /* s2w(...) allocates some memory, free it */
> + efi_bs->FreePool(module_name.w);
> +
> + modules_idx++;
> +
> + return ret;
> +}
> +
> +/*
> + * This function checks for the presence of the uefi,binary property in the
> + * module, if found it loads the binary as module and sets the right address
> + * for the reg property into the module DT node.
> + */
> +static int __init handle_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;
> + 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 0;
> +
> + file_idx = get_module_file_index(uefi_name_prop, uefi_name_len);
> + if ( file_idx < 0 )
> + {
> + file_idx = allocate_module_file(dir_handle, uefi_name_prop,
> + uefi_name_len);
> + if ( file_idx < 0 )
> + return file_idx;
> + }
> +
> + file = &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 )
> + {
> + PrintMessage(L"Unable to modify module node name.");
> + return ERROR_RENAME_MODULE_NAME;
> + }
> +
> + if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
> + file->addr, file->size) < 0 )
> + {
> + PrintMessage(L"Unable to set module reg property.");
> + return ERROR_SET_REG_PROPERTY;
> + }
> +
> + return 0;
> +}
> +
> +static bool __init is_boot_module(int dt_module_offset)
> +{
> + if ( (fdt_node_check_compatible(fdt, dt_module_offset,
> + "multiboot,kernel") == 0) ||
> + (fdt_node_check_compatible(fdt, dt_module_offset,
> + "multiboot,ramdisk") == 0) ||
> + (fdt_node_check_compatible(fdt, dt_module_offset,
> + "multiboot,device-tree") == 0) )
> + return true;
> +
> + return false;
> +}
> +
> +/*
> + * This function checks for boot modules under the domU guest domain node
> + * in the DT.
> + * Returns 0 on success, negative number on error.
> + */
> +static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> + int domain_node)
> +{
> + int module_node, addr_cells, size_cells, len;
> + const struct fdt_property *prop;
> +
> + /* Get #address-cells and #size-cells from domain node */
> + prop = fdt_get_property(fdt, domain_node, "#address-cells", &len);
> + if ( !prop )
> + {
> + PrintMessage(L"#address-cells not found in domain node.");
> + return ERROR_MISSING_DT_PROPERTY;
> + }
> +
> + addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
> +
> + prop = fdt_get_property(fdt, domain_node, "#size-cells", &len);
> + if ( !prop )
> + {
> + PrintMessage(L"#size-cells not found in domain node.");
> + return ERROR_MISSING_DT_PROPERTY;
> + }
> +
> + size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
> +
> + /*
> + * 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 ( is_boot_module(module_node) )
> + {
> + int ret = handle_module_node(dir_handle, module_node, addr_cells,
> + size_cells);
> + if ( ret < 0 )
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * This function checks for xen domain nodes under the /chosen node for possible
> + * domU guests to be loaded.
> + * Returns the number of modules loaded or a negative number for error.
> + */
> +static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> +{
> + int chosen, node, 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 )
> + {
> + PrintMessage(L"Unable to setup chosen node");
> + return ERROR_DT_CHOSEN_NODE;
> + }
> +
> + /* Check for nodes compatible with xen,domain under the chosen node */
> + for ( node = fdt_first_subnode(fdt, chosen);
> + node > 0;
> + node = fdt_next_subnode(fdt, node) )
> + {
> + if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
> + {
> + /* Found a node with compatible xen,domain; handle this node. */
> + if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
> + return ERROR_DT_MODULE_DOMU;
> + }
> + }
> +
> + /* Free dom0less file names if any */
Free boot modules file names if any
> + for ( ; i < modules_idx; i++ )
> + {
> + /* Free dom0less binary names */
Free boot modules binary names
> + efi_bs->FreePool(modules[i].name);
> + }
> +
> + return modules_idx;
> }
>
> static void __init efi_arch_cpu(void)
> @@ -562,8 +854,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 */
Free boot module files if any
> + for ( ; i < modules_idx; i++ )
> + {
> + /* Free dom0less binary names */
Free boot modules binary names
> + efi_bs->FreePool(modules[i].name);
> + /* Free dom0less binaries */
Free boot modules binaries
> + efi_bs->FreePages(modules[i].addr,
> + PFN_UP(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..f5538bfad4 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 int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> +{
> + /* x86 doesn't support device tree boot */
> + return 0;
> +}
> +
> 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..c8c57fbb54 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1127,15 +1127,17 @@ 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;
> + int dt_module_found;
> + EFI_FILE_HANDLE dir_handle;
>
> __set_bit(EFI_BOOT, &efi_flags);
> __set_bit(EFI_LOADER, &efi_flags);
> @@ -1216,9 +1218,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 +1233,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 +1286,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 +1361,26 @@ 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);
> }
>
> + dt_module_found = efi_arch_check_dt_boot(dir_handle);
> +
> + dir_handle->Close(dir_handle);
> +
> + if (dt_module_found < 0)
code style if ( dt_module_found < 0 )
> + /* efi_arch_check_dt_boot throws some error */
> + blexit(L"Error processing boot modules on DT.");
> + /*
> + * Check if a proper configuration is provided to start Xen:
> + * - Dom0 specified (minimum required)
> + * - Dom0 and DomU(s) specified
> + * - DomU(s) specified
> + */
> + if ( !dt_module_found && !kernel.addr )
> + blexit(L"No Dom0 kernel image specified.");
> +
> efi_arch_edd();
>
> /* XXX Collect EDID info. */
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot
2021-09-28 16:32 ` [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot Luca Fancellu
2021-09-28 22:47 ` Stefano Stabellini
@ 2021-09-29 7:50 ` Jan Beulich
2021-09-29 11:30 ` Luca Fancellu
1 sibling, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2021-09-29 7:50 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Wei Liu, Roger Pau Monné,
xen-devel
On 28.09.2021 18:32, Luca Fancellu wrote:
> --- 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 int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
> +{
> + /* x86 doesn't support device tree boot */
> + return 0;
> +}
Every time I see this addition I'm getting puzzled. As a result I'm
afraid I now need to finally ask you to do something about this (and
I'm sorry for doing so only now). There would better be no notion of
DT in x86 code, and there would better also not be a need for
architectures not supporting DT to each supply such a stub. Instead
I think you want to put this stub in xen/common/efi/boot.c, inside a
suitable #ifdef.
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1127,15 +1127,17 @@ 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;
> + int dt_module_found;
I think this variable either wants to be bool or be named differently.
> @@ -1361,12 +1361,26 @@ 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);
> }
>
> + dt_module_found = efi_arch_check_dt_boot(dir_handle);
> +
> + dir_handle->Close(dir_handle);
> +
> + if (dt_module_found < 0)
> + /* efi_arch_check_dt_boot throws some error */
> + blexit(L"Error processing boot modules on DT.");
For this use, bool would seem appropriate, but ...
> + /*
> + * Check if a proper configuration is provided to start Xen:
> + * - Dom0 specified (minimum required)
> + * - Dom0 and DomU(s) specified
> + * - DomU(s) specified
> + */
> + if ( !dt_module_found && !kernel.addr )
> + blexit(L"No Dom0 kernel image specified.");
... this (and my brief looking at the Arm code) rather suggests a
count gets returned, and hence it may want renaming instead. Maybe
simply to dt_modules_found.
Considering the new conditional I also wonder whether the error
message can't end up being misleading on Arm (it certainly should
remain as is on x86).
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
2021-09-28 16:32 ` [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI Luca Fancellu
2021-09-28 22:46 ` Stefano Stabellini
@ 2021-09-29 8:00 ` Jan Beulich
2021-09-29 11:22 ` Luca Fancellu
1 sibling, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2021-09-29 8:00 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Wei Liu, xen-devel
On 28.09.2021 18:32, Luca Fancellu wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> {
> read_file(dir_handle, s2w(&name), &kernel, option_str);
> efi_bs->FreePool(name.w);
> -
> - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> - (void **)&shim_lock)) &&
> - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> - PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> }
>
> if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> if (dt_module_found < 0)
> /* efi_arch_check_dt_boot throws some error */
> blexit(L"Error processing boot modules on DT.");
> +
> + /* If Dom0 is specified, verify it */
> + if ( kernel.ptr &&
> + !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> + (void **)&shim_lock)) &&
> + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> + PrintErrMesg(L"Dom0 kernel image could not be verified", status);
Why does this code need moving in the first place? That's (to me at least)
not obvious from looking just at the common (i.e. non-Arm-specific) part.
Hence I would wish for the comment to be slightly extended to indicate
that the kernel image may also have been loaded by <whichever function>.
Also, two nits: You've broken indentation here (you've lost a space too
much compared to the original code) and ...
> /*
> * Check if a proper configuration is provided to start Xen:
> * - Dom0 specified (minimum required)
>
... you will want to insert a blank line not only before, but also after
your insertion (or, imo less desirable, neither of the two blank lines).
Further I wonder whether your addition wouldn't better be after the code
following this comment.
And finally I wonder (looking back at the earlier patch) why you use
kernel.addr there when kernel.ptr is being used here. Unless there's a
reason for the difference, being consistent would be better.
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property
[not found] ` <AB8FEF63-EA1F-427F-AF2B-13C1E930F682@arm.com>
@ 2021-09-29 9:56 ` Luca Fancellu
2021-09-29 16:17 ` Stefano Stabellini
0 siblings, 1 reply; 15+ messages in thread
From: Luca Fancellu @ 2021-09-29 9:56 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, Bertrand Marquis, wei.chen, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu
> On Tue, 28 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.
>>
>> Modify a comment in efi_arch_use_config_file, removing
>> the part that states "dom0 required" because it's not
>> true anymore with this commit.
>>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>
> The patch looks good. Only one minor change: given that this is a Xen
> parameter that we are introducing and not a parameter defined by UEFI
> Forum, I think uefi,cfg-load should be called xen,uefi-cfg-load instead.
> Because "xen," is our prefix, while "uefi," is not.
>
> With that minor change:
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Yes I will rename it.
>
>
> Note that the uefi,binary property is different because that property is
> for xen,domain nodes, so we are already in a Xen specific namespace when
> we are using it. Instead this property is for /chosen which is not a Xen
> specific node.
Given that uefi,binary will be used also for multiboot,module directly under /chosen
for Dom0, do you think I should rename also that to xen,uefi-binary?
Cheers,
Luca
>
>
>
>> ---
>> v3 changes:
>> - add documentation to misc/arm/device-tree/booting.txt
>> - Modified variable name and logic from skip_cfg_file to
>> load_cfg_file
>> - Add in the commit message that I'm modifying a comment.
>> v2 changes:
>> - Introduced uefi,cfg-load property
>> - Add documentation about the property
>> ---
>> docs/misc/arm/device-tree/booting.txt | 8 ++++++++
>> docs/misc/efi.pandoc | 2 ++
>> xen/arch/arm/efi/efi-boot.h | 28 ++++++++++++++++++++++-----
>> 3 files changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 44cd9e1a9a..cf878b478e 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -121,6 +121,14 @@ A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
>> for Dom0 and bootargs for native Linux.
>>
>>
>> +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.
>> +
>> +
>> Creating Multiple Domains directly from Xen
>> ===========================================
>>
>> 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..4f1b01757d 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 load_cfg_file = true;
>> /*
>> * 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 )
>> + load_cfg_file = false;
>> + }
>> + }
>> +
>> + if ( !fdt || load_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
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
[not found] ` <1E500DE6-1680-41F6-BE8E-5B7ECBAD24BA@arm.com>
@ 2021-09-29 10:03 ` Luca Fancellu
0 siblings, 0 replies; 15+ messages in thread
From: Luca Fancellu @ 2021-09-29 10:03 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, Bertrand Marquis, wei.chen, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu
> On Tue, 28 Sep 2021, Luca Fancellu wrote:
>> Add support to load Dom0 boot modules from
>> the device tree using the uefi,binary property.
>>
>> Update documentation about that.
>>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>
> It is great how simple this patch is!
>
> The patch looks all correct. Only one question: do we need a check to
> make sure the dom0 ramdisk is not loaded twice? Once via uefi,binary and
> another time via the config file? In other words...
Yes I will add a check for it
>
>> ---
>> Changes in v3:
>> - new patch
>> ---
>> docs/misc/arm/device-tree/booting.txt | 8 ++++
>> docs/misc/efi.pandoc | 64 +++++++++++++++++++++++++--
>> xen/arch/arm/efi/efi-boot.h | 36 +++++++++++++--
>> xen/common/efi/boot.c | 12 ++---
>> 4 files changed, 108 insertions(+), 12 deletions(-)
>>
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 354bb43fe1..e73f6476d4 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -70,6 +70,14 @@ Each node contains the following properties:
>> priority of this field vs. other mechanisms of specifying the
>> bootargs for the kernel.
>>
>> +- uefi,binary (UEFI boot only)
>> +
>> + String property that specifies the file name to be loaded by the UEFI
>> + boot for this module. If this is specified, there is no need to specify
>> + the reg property because it will be created by the UEFI stub on boot.
>> + This option is needed only when UEFI boot is used, the node needs to be
>> + compatible with multiboot,kernel or multiboot,ramdisk.
>> +
>> Examples
>> ========
>>
>> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
>> index 800e67a233..4cebc47a18 100644
>> --- a/docs/misc/efi.pandoc
>> +++ b/docs/misc/efi.pandoc
>> @@ -167,6 +167,28 @@ sbsign \
>> --output xen.signed.efi \
>> xen.unified.efi
>> ```
>> +## UEFI boot and Dom0 modules on ARM
>> +
>> +When booting using UEFI on ARM, it is possible to specify the Dom0 modules
>> +directly from the device tree without using the Xen configuration file, here an
>> +example:
>> +
>> +chosen {
>> + #size-cells = <0x1>;
>> + #address-cells = <0x1>;
>> + xen,xen-bootargs = "[Xen boot arguments]"
>> +
>> + module@1 {
>> + compatible = "multiboot,kernel", "multiboot,module";
>> + uefi,binary = "vmlinuz-3.0.31-0.4-xen";
>> + bootargs = "[domain 0 command line options]";
>> + };
>> +
>> + module@2 {
>> + compatible = "multiboot,ramdisk", "multiboot,module";
>> + uefi,binary = "initrd-3.0.31-0.4-xen";
>> + };
>> +}
>>
>> ## UEFI boot and dom0less on ARM
>>
>> @@ -326,10 +348,10 @@ chosen {
>> ### Boot Xen, Dom0 and DomU(s)
>>
>> This configuration is a mix of the two configuration above, to boot this one
>> -the configuration file must be processed so the /chosen node must have the
>> -"uefi,cfg-load" property.
>> +the configuration file can be processed or the Dom0 modules can be read from
>> +the device tree.
>>
>> -Here an example:
>> +Here the first example:
>>
>> Xen configuration file:
>>
>> @@ -369,4 +391,40 @@ chosen {
>> };
>> ```
>>
>> +Here the second example:
>> +
>> +Device tree:
>> +
>> +```
>> +chosen {
>> + #size-cells = <0x1>;
>> + #address-cells = <0x1>;
>> + xen,xen-bootargs = "[Xen boot arguments]"
>> +
>> + module@1 {
>> + compatible = "multiboot,kernel", "multiboot,module";
>> + uefi,binary = "vmlinuz-3.0.31-0.4-xen";
>> + bootargs = "[domain 0 command line options]";
>> + };
>> +
>> + module@2 {
>> + compatible = "multiboot,ramdisk", "multiboot,module";
>> + uefi,binary = "initrd-3.0.31-0.4-xen";
>> + };
>> +
>> + domU1 {
>> + #size-cells = <0x1>;
>> + #address-cells = <0x1>;
>> + compatible = "xen,domain";
>> + cpus = <0x1>;
>> + memory = <0x0 0xc0000>;
>> + vpl011;
>>
>> + module@1 {
>> + compatible = "multiboot,kernel", "multiboot,module";
>> + uefi,binary = "Image-domu1.bin";
>> + bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
>> + };
>> + };
>> +};
>> +```
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index 4f7c913f86..df63387136 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -31,8 +31,10 @@ static unsigned int __initdata modules_idx;
>> #define ERROR_MISSING_DT_PROPERTY (-3)
>> #define ERROR_RENAME_MODULE_NAME (-4)
>> #define ERROR_SET_REG_PROPERTY (-5)
>> +#define ERROR_DOM0_ALREADY_FOUND (-6)
>> #define ERROR_DT_MODULE_DOMU (-1)
>> #define ERROR_DT_CHOSEN_NODE (-2)
>> +#define ERROR_DT_MODULE_DOM0 (-3)
>>
>> void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>> void __flush_dcache_area(const void *vaddr, unsigned long size);
>> @@ -45,7 +47,8 @@ static int allocate_module_file(EFI_FILE_HANDLE dir_handle,
>> static int handle_module_node(EFI_FILE_HANDLE dir_handle,
>> int module_node_offset,
>> int reg_addr_cells,
>> - int reg_size_cells);
>> + int reg_size_cells,
>> + bool is_domu_module);
>> static bool is_boot_module(int dt_module_offset);
>> static int handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> int domain_node);
>> @@ -701,7 +704,8 @@ static int __init allocate_module_file(EFI_FILE_HANDLE dir_handle,
>> static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
>> int module_node_offset,
>> int reg_addr_cells,
>> - int reg_size_cells)
>> + int reg_size_cells,
>> + bool is_domu_module)
>> {
>> const void *uefi_name_prop;
>> char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
>> @@ -743,6 +747,24 @@ static int __init handle_module_node(EFI_FILE_HANDLE dir_handle,
>> return ERROR_SET_REG_PROPERTY;
>> }
>>
>> + if ( !is_domu_module &&
>> + (fdt_node_check_compatible(fdt, module_node_offset,
>> + "multiboot,kernel") == 0) )
>> + {
>> + /*
>> + * This is the Dom0 kernel, wire it to the kernel variable because it
>> + * will be verified by the shim lock protocol later in the common code.
>> + */
>> + if ( kernel.addr )
>> + {
>> + PrintMessage(L"Dom0 kernel already found in cfg file.");
>> + return ERROR_DOM0_ALREADY_FOUND;
>> + }
>> + kernel.need_to_free = false; /* Freed using the module array */
>> + kernel.addr = file->addr;
>> + kernel.size = file->size;
>> + }
>
> ... is it necessary to update ramdisk as well or check if it is already
> set? As far as I can tell it is the only other potential conflict that
> we could have.
ramdisk variable can be left unset, we are setting the kernel variable only to
reuse the code that verifies the kernel image, however if the user specify the
ramdisk in both cfg and device tree we will end up in two multiboot,ramdisk
modules being added to the device tree.
I will add a check to return an error if ramdisk was declared previously.
>
>
>> return 0;
>> }
>>
>> @@ -799,7 +821,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> if ( is_boot_module(module_node) )
>> {
>> int ret = handle_module_node(dir_handle, module_node, addr_cells,
>> - size_cells);
>> + size_cells, true);
>> if ( ret < 0 )
>> return ret;
>> }
>> @@ -809,7 +831,7 @@ static int __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>>
>> /*
>> * This function checks for xen domain nodes under the /chosen node for possible
>> - * domU guests to be loaded.
>> + * dom0 and domU guests to be loaded.
>> * Returns the number of modules loaded or a negative number for error.
>> */
>> static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
>> @@ -836,6 +858,12 @@ static int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
>> if ( handle_dom0less_domain_node(dir_handle, node) < 0 )
>> return ERROR_DT_MODULE_DOMU;
>> }
>> + else if ( is_boot_module(node) )
>> + {
>> + if ( handle_module_node(dir_handle, node, addr_len, size_len,
>> + false) < 0 )
>> + return ERROR_DT_MODULE_DOM0;
>> + }
>> }
>>
>> /* Free dom0less file names if any */
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index c8c57fbb54..b221494a06 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>> {
>> read_file(dir_handle, s2w(&name), &kernel, option_str);
>> efi_bs->FreePool(name.w);
>> -
>> - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
>> - (void **)&shim_lock)) &&
>> - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
>> - PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>> }
>>
>> if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
>> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>> if (dt_module_found < 0)
>> /* efi_arch_check_dt_boot throws some error */
>> blexit(L"Error processing boot modules on DT.");
>> +
>> + /* If Dom0 is specified, verify it */
>> + if ( kernel.ptr &&
>> + !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
>> + (void **)&shim_lock)) &&
>> + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
>> + PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>> /*
>> * Check if a proper configuration is provided to start Xen:
>> * - Dom0 specified (minimum required)
>> --
>> 2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI
2021-09-29 8:00 ` Jan Beulich
@ 2021-09-29 11:22 ` Luca Fancellu
0 siblings, 0 replies; 15+ messages in thread
From: Luca Fancellu @ 2021-09-29 11:22 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Wei Liu, xen-devel
> On 29 Sep 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.09.2021 18:32, Luca Fancellu wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1296,11 +1296,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>> {
>> read_file(dir_handle, s2w(&name), &kernel, option_str);
>> efi_bs->FreePool(name.w);
>> -
>> - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
>> - (void **)&shim_lock)) &&
>> - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
>> - PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>> }
>>
>> if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
>> @@ -1372,6 +1367,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>> if (dt_module_found < 0)
>> /* efi_arch_check_dt_boot throws some error */
>> blexit(L"Error processing boot modules on DT.");
>> +
>> + /* If Dom0 is specified, verify it */
>> + if ( kernel.ptr &&
>> + !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
>> + (void **)&shim_lock)) &&
>> + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
>> + PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>
> Why does this code need moving in the first place? That's (to me at least)
> not obvious from looking just at the common (i.e. non-Arm-specific) part.
> Hence I would wish for the comment to be slightly extended to indicate
> that the kernel image may also have been loaded by <whichever function>.
>
Sure I will improve the comment to explain that.
> Also, two nits: You've broken indentation here (you've lost a space too
> much compared to the original code) and ...
Yes sorry for that, we didn’t see it, I will fix it.
>
>> /*
>> * Check if a proper configuration is provided to start Xen:
>> * - Dom0 specified (minimum required)
>>
>
> ... you will want to insert a blank line not only before, but also after
> your insertion (or, imo less desirable, neither of the two blank lines).
>
> Further I wonder whether your addition wouldn't better be after the code
> following this comment.
>
> And finally I wonder (looking back at the earlier patch) why you use
> kernel.addr there when kernel.ptr is being used here. Unless there's a
> reason for the difference, being consistent would be better.
I will do all of the above for the v4.
Thanks for all the feedbacks.
Cheers,
Luca
>
> Jan
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot
2021-09-29 7:50 ` Jan Beulich
@ 2021-09-29 11:30 ` Luca Fancellu
2021-09-29 11:53 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Luca Fancellu @ 2021-09-29 11:30 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Wei Liu, Roger Pau Monné,
xen-devel
> On 29 Sep 2021, at 08:50, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.09.2021 18:32, Luca Fancellu wrote:
>> --- 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 int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
>> +{
>> + /* x86 doesn't support device tree boot */
>> + return 0;
>> +}
>
> Every time I see this addition I'm getting puzzled. As a result I'm
> afraid I now need to finally ask you to do something about this (and
> I'm sorry for doing so only now). There would better be no notion of
> DT in x86 code, and there would better also not be a need for
> architectures not supporting DT to each supply such a stub. Instead
> I think you want to put this stub in xen/common/efi/boot.c, inside a
> suitable #ifdef.
Sure I will enclose it in #ifdef CONFIG_ARM and remove the x86 stub.
>
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1127,15 +1127,17 @@ 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;
>> + int dt_module_found;
>
> I think this variable either wants to be bool or be named differently.
>
>> @@ -1361,12 +1361,26 @@ 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);
>> }
>>
>> + dt_module_found = efi_arch_check_dt_boot(dir_handle);
>> +
>> + dir_handle->Close(dir_handle);
>> +
>> + if (dt_module_found < 0)
>> + /* efi_arch_check_dt_boot throws some error */
>> + blexit(L"Error processing boot modules on DT.");
>
> For this use, bool would seem appropriate, but ...
>
>> + /*
>> + * Check if a proper configuration is provided to start Xen:
>> + * - Dom0 specified (minimum required)
>> + * - Dom0 and DomU(s) specified
>> + * - DomU(s) specified
>> + */
>> + if ( !dt_module_found && !kernel.addr )
>> + blexit(L"No Dom0 kernel image specified.");
>
> ... this (and my brief looking at the Arm code) rather suggests a
> count gets returned, and hence it may want renaming instead. Maybe
> simply to dt_modules_found.
Yes that’s a better name, I will also add a comment just above the
efi_arch_check_dt_boot to explain it is returning the number of modules
found in the DT or an error (<0)
>
> Considering the new conditional I also wonder whether the error
> message can't end up being misleading on Arm (it certainly should
> remain as is on x86).
Do you think that a message like this: “No guest kernel image specified”
can work for both x86 and arm architecture?
>
> Jan
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot
2021-09-29 11:30 ` Luca Fancellu
@ 2021-09-29 11:53 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2021-09-29 11:53 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Wei Liu, Roger Pau Monné,
xen-devel
On 29.09.2021 13:30, Luca Fancellu wrote:
>
>
>> On 29 Sep 2021, at 08:50, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.09.2021 18:32, Luca Fancellu wrote:
>>> --- 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 int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
>>> +{
>>> + /* x86 doesn't support device tree boot */
>>> + return 0;
>>> +}
>>
>> Every time I see this addition I'm getting puzzled. As a result I'm
>> afraid I now need to finally ask you to do something about this (and
>> I'm sorry for doing so only now). There would better be no notion of
>> DT in x86 code, and there would better also not be a need for
>> architectures not supporting DT to each supply such a stub. Instead
>> I think you want to put this stub in xen/common/efi/boot.c, inside a
>> suitable #ifdef.
>
> Sure I will enclose it in #ifdef CONFIG_ARM and remove the x86 stub.
Hmm, so you've taken my reply in way quite different from how it was
meant, albeit I can't see how the last sentence of what I've said could
be interpreted in this way. The more generic stub wants enclosing in
"#ifndef CONFIG_HAS_DEVICE_TREE" imo.
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property
2021-09-29 9:56 ` Luca Fancellu
@ 2021-09-29 16:17 ` Stefano Stabellini
0 siblings, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2021-09-29 16:17 UTC (permalink / raw)
To: Luca Fancellu
Cc: Stefano Stabellini, xen-devel, Bertrand Marquis, wei.chen,
Julien Grall, Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Wei Liu
On Wed, 29 Sep 2021, Luca Fancellu wrote:
> > On Tue, 28 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.
> >>
> >> Modify a comment in efi_arch_use_config_file, removing
> >> the part that states "dom0 required" because it's not
> >> true anymore with this commit.
> >>
> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> >
> > The patch looks good. Only one minor change: given that this is a Xen
> > parameter that we are introducing and not a parameter defined by UEFI
> > Forum, I think uefi,cfg-load should be called xen,uefi-cfg-load instead.
> > Because "xen," is our prefix, while "uefi," is not.
> >
> > With that minor change:
> >
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
> Yes I will rename it.
>
> >
> >
> > Note that the uefi,binary property is different because that property is
> > for xen,domain nodes, so we are already in a Xen specific namespace when
> > we are using it. Instead this property is for /chosen which is not a Xen
> > specific node.
>
> Given that uefi,binary will be used also for multiboot,module directly under /chosen
> for Dom0, do you think I should rename also that to xen,uefi-binary?
Yes, maybe to stay on the safe side, it would be better to also rename
uefi,binary to xen,uefi-binary.
> >> ---
> >> v3 changes:
> >> - add documentation to misc/arm/device-tree/booting.txt
> >> - Modified variable name and logic from skip_cfg_file to
> >> load_cfg_file
> >> - Add in the commit message that I'm modifying a comment.
> >> v2 changes:
> >> - Introduced uefi,cfg-load property
> >> - Add documentation about the property
> >> ---
> >> docs/misc/arm/device-tree/booting.txt | 8 ++++++++
> >> docs/misc/efi.pandoc | 2 ++
> >> xen/arch/arm/efi/efi-boot.h | 28 ++++++++++++++++++++++-----
> >> 3 files changed, 33 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> >> index 44cd9e1a9a..cf878b478e 100644
> >> --- a/docs/misc/arm/device-tree/booting.txt
> >> +++ b/docs/misc/arm/device-tree/booting.txt
> >> @@ -121,6 +121,14 @@ A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
> >> for Dom0 and bootargs for native Linux.
> >>
> >>
> >> +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.
> >> +
> >> +
> >> Creating Multiple Domains directly from Xen
> >> ===========================================
> >>
> >> 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..4f1b01757d 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 load_cfg_file = true;
> >> /*
> >> * 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 )
> >> + load_cfg_file = false;
> >> + }
> >> + }
> >> +
> >> + if ( !fdt || load_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
> >>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-09-29 16:17 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 16:32 [PATCH v3 0/3] arm/efi: Add dom0less support to UEFI boot Luca Fancellu
2021-09-28 16:32 ` [PATCH v3 1/3] arm/efi: Introduce uefi,cfg-load DT property Luca Fancellu
2021-09-28 22:03 ` Stefano Stabellini
[not found] ` <AB8FEF63-EA1F-427F-AF2B-13C1E930F682@arm.com>
2021-09-29 9:56 ` Luca Fancellu
2021-09-29 16:17 ` Stefano Stabellini
2021-09-28 16:32 ` [PATCH v3 2/3] arm/efi: Use dom0less configuration when using EFI boot Luca Fancellu
2021-09-28 22:47 ` Stefano Stabellini
2021-09-29 7:50 ` Jan Beulich
2021-09-29 11:30 ` Luca Fancellu
2021-09-29 11:53 ` Jan Beulich
2021-09-28 16:32 ` [PATCH v3 3/3] arm/efi: load dom0 modules from DT using UEFI Luca Fancellu
2021-09-28 22:46 ` Stefano Stabellini
[not found] ` <1E500DE6-1680-41F6-BE8E-5B7ECBAD24BA@arm.com>
2021-09-29 10:03 ` Luca Fancellu
2021-09-29 8:00 ` Jan Beulich
2021-09-29 11:22 ` Luca Fancellu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.