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

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

[Luca Fancellu]

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

Luca Fancellu (2):
  xen/efi: Restrict check for DT boot modules on EFI boot
  arm/efi: Use dom0less configuration when using EFI boot

 docs/misc/efi.pandoc        |  37 +++++
 xen/arch/arm/efi/efi-boot.h | 274 +++++++++++++++++++++++++++++++++++-
 xen/common/efi/boot.c       |  20 ++-
 3 files changed, 322 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH 1/2] xen/efi: Restrict check for DT boot modules on EFI boot

[Luca Fancellu]

When Xen is started as EFI application, it is checking
the presence of multiboot,module in the DT, if any is
found, the configuration file is skipped.
Restrict this check to just any multiboot,module that
is a direct child of the /chosen node.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/efi/efi-boot.h | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index cf9c37153f..5ff626c6a0 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -581,6 +581,8 @@ 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)
 {
+    int node;
+    bool dom0_module_found = false;
     /*
      * For arm, we may get a device tree from GRUB (or other bootloader)
      * that contains modules that have already been loaded into memory.  In
@@ -592,11 +594,35 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
     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 )
+
+    /* Locate chosen node */
+    node = fdt_subnode_offset(fdt, 0, "chosen");
+
+    /* Cycle through every node inside chosen having multiboot,module */
+    do {
+        int depth = 0;
+        node = fdt_node_offset_by_compatible(fdt, node, "multiboot,module");
+        /*
+         * If the multiboot,module just found is placed at depth less than 3,
+         * it means that it is here: /chosen/<module> so it is a module to
+         * start dom0. (root is counted as 0)
+         */
+        if ( node > 0 )
+        {
+            depth = fdt_node_depth(fdt, node);
+            if ( (depth >= 0) && (depth < 3) )
+            {
+                dom0_module_found = true;
+                break;
+            }
+        }
+    } while(node > 0);
+
+    if ( !fdt || !dom0_module_found )
     {
         /*
          * 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

Re: [PATCH 1/2] xen/efi: Restrict check for DT boot modules on EFI boot

[Stefano Stabellini]

Adding Jan for an opinion on the EFI common code changes. See below.


On Wed, 15 Sep 2021, Luca Fancellu wrote:
> When Xen is started as EFI application, it is checking
> the presence of multiboot,module in the DT, if any is
> found, the configuration file is skipped.
> Restrict this check to just any multiboot,module that
> is a direct child of the /chosen node.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/arch/arm/efi/efi-boot.h | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index cf9c37153f..5ff626c6a0 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -581,6 +581,8 @@ 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)
>  {
> +    int node;
> +    bool dom0_module_found = false;
>      /*
>       * For arm, we may get a device tree from GRUB (or other bootloader)
>       * that contains modules that have already been loaded into memory.  In
> @@ -592,11 +594,35 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>      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 )
> +
> +    /* Locate chosen node */
> +    node = fdt_subnode_offset(fdt, 0, "chosen");
> +
> +    /* Cycle through every node inside chosen having multiboot,module */
> +    do {
> +        int depth = 0;
> +        node = fdt_node_offset_by_compatible(fdt, node, "multiboot,module");
> +        /*
> +         * If the multiboot,module just found is placed at depth less than 3,
> +         * it means that it is here: /chosen/<module> so it is a module to
> +         * start dom0. (root is counted as 0)
> +         */
> +        if ( node > 0 )
> +        {
> +            depth = fdt_node_depth(fdt, node);
> +            if ( (depth >= 0) && (depth < 3) )
> +            {
> +                dom0_module_found = true;
> +                break;
> +            }
> +        }
> +    } while(node > 0);

It should be possible to enable the uefi,binary bootflow for Dom0 and
the Dom0 ramdisk too. It would be nice as we could have a 100% UEFI
boot, not dependent on U-Boot, both Dom0 and Dom0less, without the
xen.cfg file. It doesn't have to be done now by this series, but it
should be possible from a device tree bindings perspective.

With that in mind, is this check accurate? This patch is saying that if
Dom0 is not present in the device tree, then load xen.cfg. But what if
it is a true dom0less configuration? Then we would have no dom0, only
dom0less VMs, and we might still not want to load xen.cfg. True dom0less
is another one of those configurations that don't have to be enabled now
by this series but they should be possible from a device tree bindings
perspective.


I tried to think of ways to improve this check, for instance searching
for a module that doesn't have "uefi,binary" but has the regular "reg"
property. If there is one, then we could skip loading xen.cfg. But that
doesn't work if we have a UEFI-only true dom0less configuration.

So I am thinking that we have no choice but introducing a new property
to tell us whether we should or should not load xen.cfg when
multiboot,modules are present.

Taking inspiration from HyperLaunch, it could be a new node:

chosen {
    cfg {
        compatible = "xen,uefi-config", "multiboot,module";
        uefi,binary = "xen.cfg";
    };
};

In efi_arch_use_config_file we would check if there are any nodes
compatible with "multiboot,module". If there are none, it returns true.

If there are any, and one of them is also compatible "xen,uefi-config",
then efi_arch_use_config_file returns true and also the specified
configuration filename.

If there are nodes compatible to "multiboot,module" but none of them is
compatible to "module,uefi-config", then efi_arch_use_config_file
returns false. We use the device tree only.

I think that would be clearer and more consistent from a specification
perspective, but it requires one change in common code:
efi_arch_use_config_file would not just return bool but it would also
return a filename if found (it could be a pointer parameter to the
function).


Otherwise, we could add a simple property like the following, without a
specific value and without a filename:

chosen {
    xen,uefi-config;
};

The presence of xen,uefi-config could mean that Xen should go through
the usual guessing game to figure out the right cfg file to load. This
would not require any common code changes because
efi_arch_use_config_file could simply return bool as it does today.

My preference is the xen,uefi-config compatible node, but I think the
property would also work.


Jan, do you have an opinion on whether efi_arch_use_config_file has to
stay as it is today, or would you be open to the possibility of making
efi_arch_use_config_file return a filename too?

Re: [PATCH 1/2] xen/efi: Restrict check for DT boot modules on EFI boot

[Jan Beulich]

On 16.09.2021 02:16, Stefano Stabellini wrote:
> So I am thinking that we have no choice but introducing a new property
> to tell us whether we should or should not load xen.cfg when
> multiboot,modules are present.
> 
> Taking inspiration from HyperLaunch, it could be a new node:
> 
> chosen {
>     cfg {
>         compatible = "xen,uefi-config", "multiboot,module";
>         uefi,binary = "xen.cfg";
>     };
> };
> 
> In efi_arch_use_config_file we would check if there are any nodes
> compatible with "multiboot,module". If there are none, it returns true.
> 
> If there are any, and one of them is also compatible "xen,uefi-config",
> then efi_arch_use_config_file returns true and also the specified
> configuration filename.
> 
> If there are nodes compatible to "multiboot,module" but none of them is
> compatible to "module,uefi-config", then efi_arch_use_config_file
> returns false. We use the device tree only.
> 
> I think that would be clearer and more consistent from a specification
> perspective, but it requires one change in common code:
> efi_arch_use_config_file would not just return bool but it would also
> return a filename if found (it could be a pointer parameter to the
> function).
> 
> 
> Otherwise, we could add a simple property like the following, without a
> specific value and without a filename:
> 
> chosen {
>     xen,uefi-config;
> };
> 
> The presence of xen,uefi-config could mean that Xen should go through
> the usual guessing game to figure out the right cfg file to load. This
> would not require any common code changes because
> efi_arch_use_config_file could simply return bool as it does today.
> 
> My preference is the xen,uefi-config compatible node, but I think the
> property would also work.
> 
> 
> Jan, do you have an opinion on whether efi_arch_use_config_file has to
> stay as it is today, or would you be open to the possibility of making
> efi_arch_use_config_file return a filename too?

I see no issue with making such a change; my preference would be an
altered return type, provided all present cases can be expressed
alongside the new one you need.

Jan

Re: [PATCH 1/2] xen/efi: Restrict check for DT boot modules on EFI boot

[Luca Fancellu]

> On 16 Sep 2021, at 01:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> Adding Jan for an opinion on the EFI common code changes. See below.
> 
> 
> On Wed, 15 Sep 2021, Luca Fancellu wrote:
>> When Xen is started as EFI application, it is checking
>> the presence of multiboot,module in the DT, if any is
>> found, the configuration file is skipped.
>> Restrict this check to just any multiboot,module that
>> is a direct child of the /chosen node.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> xen/arch/arm/efi/efi-boot.h | 30 ++++++++++++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index cf9c37153f..5ff626c6a0 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -581,6 +581,8 @@ 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)
>> {
>> +    int node;
>> +    bool dom0_module_found = false;
>>     /*
>>      * For arm, we may get a device tree from GRUB (or other bootloader)
>>      * that contains modules that have already been loaded into memory.  In
>> @@ -592,11 +594,35 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>>     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 )
>> +
>> +    /* Locate chosen node */
>> +    node = fdt_subnode_offset(fdt, 0, "chosen");
>> +
>> +    /* Cycle through every node inside chosen having multiboot,module */
>> +    do {
>> +        int depth = 0;
>> +        node = fdt_node_offset_by_compatible(fdt, node, "multiboot,module");
>> +        /*
>> +         * If the multiboot,module just found is placed at depth less than 3,
>> +         * it means that it is here: /chosen/<module> so it is a module to
>> +         * start dom0. (root is counted as 0)
>> +         */
>> +        if ( node > 0 )
>> +        {
>> +            depth = fdt_node_depth(fdt, node);
>> +            if ( (depth >= 0) && (depth < 3) )
>> +            {
>> +                dom0_module_found = true;
>> +                break;
>> +            }
>> +        }
>> +    } while(node > 0);
> 
> It should be possible to enable the uefi,binary bootflow for Dom0 and
> the Dom0 ramdisk too. It would be nice as we could have a 100% UEFI
> boot, not dependent on U-Boot, both Dom0 and Dom0less, without the
> xen.cfg file. It doesn't have to be done now by this series, but it
> should be possible from a device tree bindings perspective.
> 
> With that in mind, is this check accurate? This patch is saying that if
> Dom0 is not present in the device tree, then load xen.cfg. But what if
> it is a true dom0less configuration? Then we would have no dom0, only
> dom0less VMs, and we might still not want to load xen.cfg. True dom0less
> is another one of those configurations that don't have to be enabled now
> by this series but they should be possible from a device tree bindings
> perspective.
> 
> 
> I tried to think of ways to improve this check, for instance searching
> for a module that doesn't have "uefi,binary" but has the regular "reg"
> property. If there is one, then we could skip loading xen.cfg. But that
> doesn't work if we have a UEFI-only true dom0less configuration.
> 
> So I am thinking that we have no choice but introducing a new property
> to tell us whether we should or should not load xen.cfg when
> multiboot,modules are present.
> 
> Taking inspiration from HyperLaunch, it could be a new node:
> 
> chosen {
>    cfg {
>        compatible = "xen,uefi-config", "multiboot,module";
>        uefi,binary = "xen.cfg";
>    };
> };
> 
> In efi_arch_use_config_file we would check if there are any nodes
> compatible with "multiboot,module". If there are none, it returns true.
> 
> If there are any, and one of them is also compatible "xen,uefi-config",
> then efi_arch_use_config_file returns true and also the specified
> configuration filename.
> 
> If there are nodes compatible to "multiboot,module" but none of them is
> compatible to "module,uefi-config", then efi_arch_use_config_file
> returns false. We use the device tree only.
> 
> I think that would be clearer and more consistent from a specification
> perspective, but it requires one change in common code:
> efi_arch_use_config_file would not just return bool but it would also
> return a filename if found (it could be a pointer parameter to the
> function).
> 
> 
> Otherwise, we could add a simple property like the following, without a
> specific value and without a filename:
> 
> chosen {
>    xen,uefi-config;
> };
> 
> The presence of xen,uefi-config could mean that Xen should go through
> the usual guessing game to figure out the right cfg file to load. This
> would not require any common code changes because
> efi_arch_use_config_file could simply return bool as it does today.
> 
> My preference is the xen,uefi-config compatible node, but I think the
> property would also work.
> 
> 
> Jan, do you have an opinion on whether efi_arch_use_config_file has to
> stay as it is today, or would you be open to the possibility of making
> efi_arch_use_config_file return a filename too?

Hi Stefano,

True dom0less is a configuration that this serie enables: if there is no dom0 kernel
specified in the xen.cfg then only the domUs will be loaded and started.

The following cases are valid:
1) start only dom0 [dom0 modules in xen.cfg or embedded in Xen image]
2) start only domUs, true dom0less [no dom0 modules in xen.cfg and inside Xen image, domUs on DT]
3) start dom0 and domUs [(dom0 modules in xen.cfg or inside Xen image) and domUs on DT]

I don’t understand why we want to add new properties to avoid/not avoid to read the xen.cfg, am I missing
something?

Cheers,
Luca

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

[Luca Fancellu]

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

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

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

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 docs/misc/efi.pandoc        |  37 ++++++
 xen/arch/arm/efi/efi-boot.h | 244 +++++++++++++++++++++++++++++++++++-
 xen/common/efi/boot.c       |  20 ++-
 3 files changed, 294 insertions(+), 7 deletions(-)

diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index ac3cd58cae..db9b3273f8 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -165,3 +165,40 @@ 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";
+        };
+    };
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 5ff626c6a0..8d7ced70f2 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -8,9 +8,39 @@
 #include <asm/setup.h>
 #include <asm/smp.h>
 
+typedef struct {
+    char* name;
+    int name_len;
+} dom0less_module_name;
+
+/*
+ * Binaries will be translated into bootmodules, the maximum number for them is
+ * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
+ */
+#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
+static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
+static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
+static uint32_t __initdata dom0less_modules_available = MAX_DOM0LESS_MODULES;
+static uint32_t __initdata dom0less_modules_idx = 0;
+
+#define ERROR_DOM0LESS_FILE_NOT_FOUND -1
+
 void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
 void __flush_dcache_area(const void *vaddr, unsigned long size);
 
+static int __init get_dom0less_file_index(const char* name, int name_len);
+static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
+                                              const char* name, int name_len);
+static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
+                                               int module_node_offset,
+                                               int reg_addr_cells,
+                                               int reg_size_cells);
+static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
+                                               int domain_node,
+                                               int addr_cells,
+                                               int size_cells);
+static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle);
+
 #define DEVICE_TREE_GUID \
 {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
 
@@ -552,8 +582,209 @@ static void __init efi_arch_handle_module(const struct file *file,
                          kernel.size) < 0 )
             blexit(L"Unable to set reg property.");
     }
-    else
+    else if ( !((file >= &dom0less_files[0]) &&
+               (file <= &dom0less_files[MAX_DOM0LESS_MODULES-1])) )
+        /*
+         * If file is not a dom0 module file and it's not any domU modules,
+         * stop here.
+         */
         blexit(L"Unknown module type");
+
+    /*
+     * dom0less_modules_available is decremented here because for each dom0
+     * file added, there will be an additional bootmodule, so the number
+     * of dom0less module files will be decremented because there is
+     * a maximum amount of bootmodules that can be loaded.
+     */
+    dom0less_modules_available--;
+}
+
+/*
+ * This function checks for a binary previously loaded with a give name, it
+ * returns the index of the file in the dom0less_files array or a negative
+ * number if no file with that name is found.
+ */
+static int __init get_dom0less_file_index(const char* name, int name_len)
+{
+    int ret = ERROR_DOM0LESS_FILE_NOT_FOUND;
+
+    for (uint32_t i = 0; i < dom0less_modules_idx; i++)
+    {
+        dom0less_module_name* mod = &dom0less_bin_names[i];
+        if ( (mod->name_len == name_len) &&
+             (strncmp(mod->name, name, name_len) == 0) )
+        {
+            ret = i;
+            break;
+        }
+    }
+    return ret;
+}
+
+/*
+ * This function allocates a binary and keeps track of its name, it
+ * returns the index of the file in the dom0less_files array.
+ */
+static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
+                                              const char* name, int name_len)
+{
+    dom0less_module_name* file_name;
+    union string module_name;
+    struct file* file;
+    uint32_t ret_idx;
+
+    /*
+     * Check if there is any space left for a domU module, the variable
+     * dom0less_modules_available is updated each time we use read_file(...)
+     * successfully.
+     */
+    if ( !dom0less_modules_available )
+        blexit(L"No space left for domU modules");
+
+    module_name.s = (char*) name;
+    ret_idx = dom0less_modules_idx;
+    file = &dom0less_files[ret_idx];
+
+    /* Save at this index the name of this binary */
+    file_name = &dom0less_bin_names[ret_idx];
+
+    if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
+                              (void**)&file_name->name) != EFI_SUCCESS )
+        blexit(L"Error allocating memory for dom0less binary name");
+
+    /* Save name and length of the binary in the data structure */
+    strlcpy(file_name->name, name, name_len);
+    file_name->name_len = name_len;
+
+    /* Load the binary in memory */
+    read_file(dir_handle, s2w(&module_name), file, NULL);
+
+    /* s2w(...) allocates some memory, free it */
+    efi_bs->FreePool(module_name.w);
+
+    dom0less_modules_idx++;
+
+    return ret_idx;
+}
+
+/*
+ * This function checks for the presence of the uefi,binary property in the
+ * module, if found it loads the binary as dom0less module and sets the right
+ * address for the reg property into the module DT node.
+ */
+static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
+                                          int module_node_offset,
+                                          int reg_addr_cells,
+                                          int reg_size_cells)
+{
+    const void* uefi_name_prop;
+    char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
+    int uefi_name_len, file_idx;
+    struct file* 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 ( NULL == uefi_name_prop )
+        /* Property not found */
+        return;
+
+    file_idx = get_dom0less_file_index(uefi_name_prop, uefi_name_len);
+    if (file_idx < 0)
+        file_idx = allocate_dom0less_file(dir_handle, uefi_name_prop,
+                                          uefi_name_len);
+
+    file = &dom0less_files[file_idx];
+
+    snprintf(mod_string, sizeof(mod_string), "module@%"PRIx64, file->addr);
+
+    /* Rename the module to be module@{address} */
+    if ( fdt_set_name(fdt, module_node_offset, mod_string) < 0 )
+        blexit(L"Unable to add domU ramdisk FDT node.");
+
+    if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
+                     file->addr, file->size) < 0 )
+        blexit(L"Unable to set reg property.");
+}
+
+/*
+ * This function checks for boot modules under the domU guest domain node
+ * in the DT.
+ */
+static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
+                                               int domain_node,
+                                               int addr_cells,
+                                               int size_cells)
+{
+    /*
+     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
+     * inside this node
+     */
+    for ( int module_node = fdt_first_subnode(fdt, domain_node);
+          module_node > 0;
+          module_node = fdt_next_subnode(fdt, module_node) )
+    {
+        if ( (fdt_node_check_compatible(fdt, module_node,
+                                        "multiboot,kernel") == 0) ||
+             (fdt_node_check_compatible(fdt, module_node,
+                                        "multiboot,ramdisk") == 0) ||
+             (fdt_node_check_compatible(fdt, module_node,
+                                        "multiboot,device-tree") == 0) )
+        {
+            /* The compatible is one of the strings above, check the module */
+            handle_dom0less_module_node(dir_handle, module_node, addr_cells,
+                                        size_cells);
+        }
+    }
+}
+
+/*
+ * This function checks for xen domain nodes under the /chosen node for possible
+ * domU guests to be loaded.
+ */
+static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle)
+{
+    int chosen;
+    int addr_len, size_len;
+
+    /* Check for the chosen node in the current DTB */
+    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
+    if ( chosen < 0 )
+        blexit(L"Unable to setup chosen node");
+
+    /* Check for nodes compatible with xen,domain under the chosen node */
+    for ( int node = fdt_first_subnode(fdt, chosen);
+          node > 0;
+          node = fdt_next_subnode(fdt, node) )
+    {
+        int addr_cells, size_cells, len;
+        const struct fdt_property *prop;
+
+        if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
+            continue;
+
+        /* Get or set #address-cells and #size-cells */
+        prop = fdt_get_property(fdt, node, "#address-cells", &len);
+        if ( !prop )
+            blexit(L"#address-cells not found in domain node.");
+
+        addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+        prop = fdt_get_property(fdt, node, "#size-cells", &len);
+        if ( !prop )
+            blexit(L"#size-cells not found in domain node.");
+
+        size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
+
+        /* Found a node with compatible xen,domain; handle this node. */
+        handle_dom0less_domain_node(dir_handle, node, addr_cells, size_cells);
+    }
+
+    if ( dom0less_modules_idx > 0 )
+        return true;
+
+    return false;
 }
 
 static void __init efi_arch_cpu(void)
@@ -562,8 +793,19 @@ static void __init efi_arch_cpu(void)
 
 static void __init efi_arch_blexit(void)
 {
+    uint32_t i = 0;
     if ( dtbfile.need_to_free )
         efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
+    /* Free dom0less files if any */
+    for ( ; i < dom0less_modules_idx; i++ )
+    {
+        /* Free dom0less binary names */
+        efi_bs->FreePool(dom0less_bin_names[i].name);
+        /* Free dom0less binaries */
+        if ( dom0less_files[i].need_to_free )
+            efi_bs->FreePages(dom0less_files[i].addr,
+                              PFN_UP(dom0less_files[i].size));
+    }
     if ( memmap )
         efi_bs->FreePool(memmap);
 }
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 758f9d74d2..65493c4b46 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1134,8 +1134,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     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;
+    bool dom0less_found = false;
 
     __set_bit(EFI_BOOT, &efi_flags);
     __set_bit(EFI_LOADER, &efi_flags);
@@ -1285,14 +1286,21 @@ 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);
+#ifdef CONFIG_ARM
+        /* dom0less feature is supported only on ARM */
+        dom0less_found = check_dom0less_efi_boot(dir_handle);
+#endif
+
+        if ( !name.s && !dom0less_found )
+            blexit(L"No Dom0 kernel image specified.");
+
+        if ( name.s != NULL )
+            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 != NULL) )
         {
             read_file(dir_handle, s2w(&name), &kernel, option_str);
             efi_bs->FreePool(name.w);
-- 
2.17.1

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

[Stefano Stabellini]

On Wed, 15 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>
> ---
>  docs/misc/efi.pandoc        |  37 ++++++
>  xen/arch/arm/efi/efi-boot.h | 244 +++++++++++++++++++++++++++++++++++-
>  xen/common/efi/boot.c       |  20 ++-
>  3 files changed, 294 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index ac3cd58cae..db9b3273f8 100644
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -165,3 +165,40 @@ 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";
> +        };
> +    };

Can you please also update docs/misc/arm/device-tree/booting.txt ?
Either a link to docs/misc/efi.pandoc or a definition of the uefi,binary
property (mentioning that it is EFI-only.)


> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 5ff626c6a0..8d7ced70f2 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -8,9 +8,39 @@
>  #include <asm/setup.h>
>  #include <asm/smp.h>
>  
> +typedef struct {
> +    char* name;
> +    int name_len;
> +} dom0less_module_name;
> +
> +/*
> + * Binaries will be translated into bootmodules, the maximum number for them is
> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> + */
> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
> +static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];

I suggest a slightly different model where we don't call AllocatePool to
allocate dom0less_module_name.name and instead we just set the pointer
directly to the fdt string. There is no risk of the fdt going away at
this point so it should be safe to use.

Also, I don't think we need a global array of struct file, we only
really need 1 struct file which would be freed immediately after loading
to memory. We do need to remember the address and size in memory though.
So I would do something like:

typedef struct {
    const char* name;
    int name_len;
    EFI_PHYSICAL_ADDRESS addr;
    UINTN size;
} dom0less_module_name;

/*
 * Binaries will be translated into bootmodules, the maximum number for them is
 * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
 */
#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];


The purpose is to reduce memory allocations and memory consumption.


> +static uint32_t __initdata dom0less_modules_available = MAX_DOM0LESS_MODULES;
> +static uint32_t __initdata dom0less_modules_idx = 0;
> +
> +#define ERROR_DOM0LESS_FILE_NOT_FOUND -1
> +
>  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>  void __flush_dcache_area(const void *vaddr, unsigned long size);
>  
> +static int __init get_dom0less_file_index(const char* name, int name_len);
> +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                              const char* name, int name_len);
> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
> +                                               int module_node_offset,
> +                                               int reg_addr_cells,
> +                                               int reg_size_cells);
> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> +                                               int domain_node,
> +                                               int addr_cells,
> +                                               int size_cells);
> +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle);
> +
>  #define DEVICE_TREE_GUID \
>  {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
>  
> @@ -552,8 +582,209 @@ static void __init efi_arch_handle_module(const struct file *file,
>                           kernel.size) < 0 )
>              blexit(L"Unable to set reg property.");
>      }
> -    else
> +    else if ( !((file >= &dom0less_files[0]) &&
> +               (file <= &dom0less_files[MAX_DOM0LESS_MODULES-1])) )
> +        /*
> +         * If file is not a dom0 module file and it's not any domU modules,
> +         * stop here.
> +         */
>          blexit(L"Unknown module type");

Without &dom0less_files we would have to do without this sanity check.


> +    /*
> +     * dom0less_modules_available is decremented here because for each dom0
> +     * file added, there will be an additional bootmodule, so the number
> +     * of dom0less module files will be decremented because there is
> +     * a maximum amount of bootmodules that can be loaded.
> +     */
> +    dom0less_modules_available--;
> +}
> +
> +/*
> + * This function checks for a binary previously loaded with a give name, it
> + * returns the index of the file in the dom0less_files array or a negative
> + * number if no file with that name is found.
> + */
> +static int __init get_dom0less_file_index(const char* name, int name_len)
> +{
> +    int ret = ERROR_DOM0LESS_FILE_NOT_FOUND;
> +
> +    for (uint32_t i = 0; i < dom0less_modules_idx; i++)

uint32_t i;

for ( i = 0; i < dom0less_modules_idx; i++ )


> +    {
> +        dom0less_module_name* mod = &dom0less_bin_names[i];
> +        if ( (mod->name_len == name_len) &&
> +             (strncmp(mod->name, name, name_len) == 0) )
> +        {
> +            ret = i;
> +            break;
> +        }
> +    }
> +    return ret;
> +}
> +
> +/*
> + * This function allocates a binary and keeps track of its name, it
> + * returns the index of the file in the dom0less_files array.
> + */
> +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                              const char* name, int name_len)
> +{
> +    dom0less_module_name* file_name;
> +    union string module_name;
> +    struct file* file;
> +    uint32_t ret_idx;
> +
> +    /*
> +     * Check if there is any space left for a domU module, the variable
> +     * dom0less_modules_available is updated each time we use read_file(...)
> +     * successfully.
> +     */
> +    if ( !dom0less_modules_available )
> +        blexit(L"No space left for domU modules");
> +    module_name.s = (char*) name;
> +    ret_idx = dom0less_modules_idx;
> +    file = &dom0less_files[ret_idx];
> +
> +    /* Save at this index the name of this binary */
> +    file_name = &dom0less_bin_names[ret_idx];
> +
> +    if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
> +                              (void**)&file_name->name) != EFI_SUCCESS )
> +        blexit(L"Error allocating memory for dom0less binary name");

As far as I can tell we could just set file_name = name;


> +    /* Save name and length of the binary in the data structure */
> +    strlcpy(file_name->name, name, name_len);
> +    file_name->name_len = name_len;
> +
> +    /* Load the binary in memory */
> +    read_file(dir_handle, s2w(&module_name), file, NULL);
> +
> +    /* s2w(...) allocates some memory, free it */
> +    efi_bs->FreePool(module_name.w);
> +
> +    dom0less_modules_idx++;
> +
> +    return ret_idx;
> +}
> +
> +/*
> + * This function checks for the presence of the uefi,binary property in the
> + * module, if found it loads the binary as dom0less module and sets the right
> + * address for the reg property into the module DT node.
> + */
> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
> +                                          int module_node_offset,
> +                                          int reg_addr_cells,
> +                                          int reg_size_cells)
> +{
> +    const void* uefi_name_prop;
> +    char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
> +    int uefi_name_len, file_idx;
> +    struct file* 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 ( NULL == uefi_name_prop )
> +        /* Property not found */
> +        return;
> +
> +    file_idx = get_dom0less_file_index(uefi_name_prop, uefi_name_len);
> +    if (file_idx < 0)
> +        file_idx = allocate_dom0less_file(dir_handle, uefi_name_prop,
> +                                          uefi_name_len);
> +
> +    file = &dom0less_files[file_idx];
> +
> +    snprintf(mod_string, sizeof(mod_string), "module@%"PRIx64, file->addr);
> +
> +    /* Rename the module to be module@{address} */
> +    if ( fdt_set_name(fdt, module_node_offset, mod_string) < 0 )
> +        blexit(L"Unable to add domU ramdisk FDT node.");
> +
> +    if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
> +                     file->addr, file->size) < 0 )
> +        blexit(L"Unable to set reg property.");
> +}
> +
> +/*
> + * This function checks for boot modules under the domU guest domain node
> + * in the DT.
> + */
> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> +                                               int domain_node,
> +                                               int addr_cells,
> +                                               int size_cells)
> +{
> +    /*
> +     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
> +     * inside this node
> +     */
> +    for ( int module_node = fdt_first_subnode(fdt, domain_node);

int module_node;

for ( module_node = fdt_first_subnode(fdt, domain_node);


> +          module_node > 0;
> +          module_node = fdt_next_subnode(fdt, module_node) )
> +    {
> +        if ( (fdt_node_check_compatible(fdt, module_node,
> +                                        "multiboot,kernel") == 0) ||
> +             (fdt_node_check_compatible(fdt, module_node,
> +                                        "multiboot,ramdisk") == 0) ||
> +             (fdt_node_check_compatible(fdt, module_node,
> +                                        "multiboot,device-tree") == 0) )
> +        {
> +            /* The compatible is one of the strings above, check the module */
> +            handle_dom0less_module_node(dir_handle, module_node, addr_cells,
> +                                        size_cells);
> +        }
> +    }
> +}
> +
> +/*
> + * This function checks for xen domain nodes under the /chosen node for possible
> + * domU guests to be loaded.
> + */
> +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle)
> +{
> +    int chosen;
> +    int addr_len, size_len;
> +
> +    /* Check for the chosen node in the current DTB */
> +    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
> +    if ( chosen < 0 )
> +        blexit(L"Unable to setup chosen node");
> +
> +    /* Check for nodes compatible with xen,domain under the chosen node */
> +    for ( int node = fdt_first_subnode(fdt, chosen);
> +          node > 0;
> +          node = fdt_next_subnode(fdt, node) )
> +    {
> +        int addr_cells, size_cells, len;
> +        const struct fdt_property *prop;
> +
> +        if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
> +            continue;
> +
> +        /* Get or set #address-cells and #size-cells */
> +        prop = fdt_get_property(fdt, node, "#address-cells", &len);
> +        if ( !prop )
> +            blexit(L"#address-cells not found in domain node.");
> +
> +        addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
> +
> +        prop = fdt_get_property(fdt, node, "#size-cells", &len);
> +        if ( !prop )
> +            blexit(L"#size-cells not found in domain node.");
> +
> +        size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
> +
> +        /* Found a node with compatible xen,domain; handle this node. */
> +        handle_dom0less_domain_node(dir_handle, node, addr_cells, size_cells);
> +    }
> +
> +    if ( dom0less_modules_idx > 0 )
> +        return true;
> +
> +    return false;
>  }
>  
>  static void __init efi_arch_cpu(void)
> @@ -562,8 +793,19 @@ static void __init efi_arch_cpu(void)
>  
>  static void __init efi_arch_blexit(void)
>  {
> +    uint32_t i = 0;
>      if ( dtbfile.need_to_free )
>          efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
> +    /* Free dom0less files if any */
> +    for ( ; i < dom0less_modules_idx; i++ )
> +    {
> +        /* Free dom0less binary names */
> +        efi_bs->FreePool(dom0less_bin_names[i].name);
> +        /* Free dom0less binaries */
> +        if ( dom0less_files[i].need_to_free )
> +            efi_bs->FreePages(dom0less_files[i].addr,
> +                              PFN_UP(dom0less_files[i].size));
> +    }
>      if ( memmap )
>          efi_bs->FreePool(memmap);
>  }
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 758f9d74d2..65493c4b46 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1134,8 +1134,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      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;
> +    bool dom0less_found = false;
>  
>      __set_bit(EFI_BOOT, &efi_flags);
>      __set_bit(EFI_LOADER, &efi_flags);
> @@ -1285,14 +1286,21 @@ 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);
> +#ifdef CONFIG_ARM
> +        /* dom0less feature is supported only on ARM */
> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
> +#endif

Rather than an #ifdef here you can simply implement
check_dom0less_efi_boot on x86 as a static inline returning always
false.

Also, we are under the if ( use_cfg_file ) code path. So maybe it is
reasonable that dom0 is required if we are booting with use_cfg_file
= true. After all, it is specified as a required property today of
xen.cfg.

If you follow my suggestion with an explicit enabled/disabled of xen.cfg
from device tree, a true dom0less configuration could be fully specified
without xen.cfg.

If we do that, then here probable we don't need to change this code path.



> +        if ( !name.s && !dom0less_found )
> +            blexit(L"No Dom0 kernel image specified.");
> +
> +        if ( name.s != NULL )
> +            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 != NULL) )
>          {
>              read_file(dir_handle, s2w(&name), &kernel, option_str);
>              efi_bs->FreePool(name.w);

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

[Jan Beulich]

On 16.09.2021 03:16, Stefano Stabellini wrote:
> On Wed, 15 Sep 2021, Luca Fancellu wrote:
>> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> +                                               int domain_node,
>> +                                               int addr_cells,
>> +                                               int size_cells)
>> +{
>> +    /*
>> +     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
>> +     * inside this node
>> +     */
>> +    for ( int module_node = fdt_first_subnode(fdt, domain_node);
> 
> int module_node;
> 
> for ( module_node = fdt_first_subnode(fdt, domain_node);

Not just here iirc from briefly looking over the patch as a whole
yesterday: Use of plain "int" would better be limited to cases where
values may also be negative. I don't suppose that's possible here as
well as in a number of other cases.

>> @@ -1285,14 +1286,21 @@ 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);
>> +#ifdef CONFIG_ARM
>> +        /* dom0less feature is supported only on ARM */
>> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
>> +#endif
> 
> Rather than an #ifdef here you can simply implement
> check_dom0less_efi_boot on x86 as a static inline returning always
> false.

Indeed, and the properly named (efi_arch_...()).

Jan

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

[Luca Fancellu]

> On 16 Sep 2021, at 07:50, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.09.2021 03:16, Stefano Stabellini wrote:
>> On Wed, 15 Sep 2021, Luca Fancellu wrote:
>>> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>>> +                                               int domain_node,
>>> +                                               int addr_cells,
>>> +                                               int size_cells)
>>> +{
>>> +    /*
>>> +     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
>>> +     * inside this node
>>> +     */
>>> +    for ( int module_node = fdt_first_subnode(fdt, domain_node);
>> 
>> int module_node;
>> 
>> for ( module_node = fdt_first_subnode(fdt, domain_node);
> 
> Not just here iirc from briefly looking over the patch as a whole
> yesterday: Use of plain "int" would better be limited to cases where
> values may also be negative. I don't suppose that's possible here as
> well as in a number of other cases.

Hi Jan,

fdt_first_subnode(…) can return -FDT_ERR_NOTFOUND.

> 
>>> @@ -1285,14 +1286,21 @@ 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);
>>> +#ifdef CONFIG_ARM
>>> +        /* dom0less feature is supported only on ARM */
>>> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
>>> +#endif
>> 
>> Rather than an #ifdef here you can simply implement
>> check_dom0less_efi_boot on x86 as a static inline returning always
>> false.
> 
> Indeed, and the properly named (efi_arch_...()).

Ok, I was unsure if a solution like that was going to be accepted, I will update the code then.

Cheers,
Luca

> 
> Jan
> 

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

[Luca Fancellu]

> On 16 Sep 2021, at 02:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 15 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>
>> ---
>> docs/misc/efi.pandoc        |  37 ++++++
>> xen/arch/arm/efi/efi-boot.h | 244 +++++++++++++++++++++++++++++++++++-
>> xen/common/efi/boot.c       |  20 ++-
>> 3 files changed, 294 insertions(+), 7 deletions(-)
>> 
>> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
>> index ac3cd58cae..db9b3273f8 100644
>> --- a/docs/misc/efi.pandoc
>> +++ b/docs/misc/efi.pandoc
>> @@ -165,3 +165,40 @@ 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";
>> +        };
>> +    };
> 
> Can you please also update docs/misc/arm/device-tree/booting.txt ?
> Either a link to docs/misc/efi.pandoc or a definition of the uefi,binary
> property (mentioning that it is EFI-only.)

Yes I will update it.

> 
> 
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index 5ff626c6a0..8d7ced70f2 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -8,9 +8,39 @@
>> #include <asm/setup.h>
>> #include <asm/smp.h>
>> 
>> +typedef struct {
>> +    char* name;
>> +    int name_len;
>> +} dom0less_module_name;
>> +
>> +/*
>> + * Binaries will be translated into bootmodules, the maximum number for them is
>> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
>> + */
>> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
>> +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
>> +static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
> 
> I suggest a slightly different model where we don't call AllocatePool to
> allocate dom0less_module_name.name and instead we just set the pointer
> directly to the fdt string. There is no risk of the fdt going away at
> this point so it should be safe to use.

Yes I thought about this approach but since I was not sure how the DTB behaves when we modify
It to add the reg property or to modify the module name, then I used this other approach.
Are you sure that the pointed memory will stay the same after we modify the DTB? My main concern
was that the DTB structure was going to be modified and the string I was pointing in the DTB memory
can be relocated elsewhere. 

> 
> Also, I don't think we need a global array of struct file, we only
> really need 1 struct file which would be freed immediately after loading
> to memory. We do need to remember the address and size in memory though.
> So I would do something like:
> 
> typedef struct {
>    const char* name;
>    int name_len;
>    EFI_PHYSICAL_ADDRESS addr;
>    UINTN size;
> } dom0less_module_name;
> 
> /*
> * Binaries will be translated into bootmodules, the maximum number for them is
> * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> */
> #define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
> 
> 
> The purpose is to reduce memory allocations and memory consumption.

Yes I can do that.

> 
> 
>> +static uint32_t __initdata dom0less_modules_available = MAX_DOM0LESS_MODULES;
>> +static uint32_t __initdata dom0less_modules_idx = 0;
>> +
>> +#define ERROR_DOM0LESS_FILE_NOT_FOUND -1
>> +
>> void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>> void __flush_dcache_area(const void *vaddr, unsigned long size);
>> 
>> +static int __init get_dom0less_file_index(const char* name, int name_len);
>> +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>> +                                              const char* name, int name_len);
>> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
>> +                                               int module_node_offset,
>> +                                               int reg_addr_cells,
>> +                                               int reg_size_cells);
>> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> +                                               int domain_node,
>> +                                               int addr_cells,
>> +                                               int size_cells);
>> +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle);
>> +
>> #define DEVICE_TREE_GUID \
>> {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
>> 
>> @@ -552,8 +582,209 @@ static void __init efi_arch_handle_module(const struct file *file,
>>                          kernel.size) < 0 )
>>             blexit(L"Unable to set reg property.");
>>     }
>> -    else
>> +    else if ( !((file >= &dom0less_files[0]) &&
>> +               (file <= &dom0less_files[MAX_DOM0LESS_MODULES-1])) )
>> +        /*
>> +         * If file is not a dom0 module file and it's not any domU modules,
>> +         * stop here.
>> +         */
>>         blexit(L"Unknown module type");
> 
> Without &dom0less_files we would have to do without this sanity check.

Sure, it will simplify to 
+ else if ( file != &dom0less_file )         

> 
> 
>> +    /*
>> +     * dom0less_modules_available is decremented here because for each dom0
>> +     * file added, there will be an additional bootmodule, so the number
>> +     * of dom0less module files will be decremented because there is
>> +     * a maximum amount of bootmodules that can be loaded.
>> +     */
>> +    dom0less_modules_available--;
>> +}
>> +
>> +/*
>> + * This function checks for a binary previously loaded with a give name, it
>> + * returns the index of the file in the dom0less_files array or a negative
>> + * number if no file with that name is found.
>> + */
>> +static int __init get_dom0less_file_index(const char* name, int name_len)
>> +{
>> +    int ret = ERROR_DOM0LESS_FILE_NOT_FOUND;
>> +
>> +    for (uint32_t i = 0; i < dom0less_modules_idx; i++)
> 
> uint32_t i;
> 
> for ( i = 0; i < dom0less_modules_idx; i++ )

Will fix that.

> 
> 
>> +    {
>> +        dom0less_module_name* mod = &dom0less_bin_names[i];
>> +        if ( (mod->name_len == name_len) &&
>> +             (strncmp(mod->name, name, name_len) == 0) )
>> +        {
>> +            ret = i;
>> +            break;
>> +        }
>> +    }
>> +    return ret;
>> +}
>> +
>> +/*
>> + * This function allocates a binary and keeps track of its name, it
>> + * returns the index of the file in the dom0less_files array.
>> + */
>> +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>> +                                              const char* name, int name_len)
>> +{
>> +    dom0less_module_name* file_name;
>> +    union string module_name;
>> +    struct file* file;
>> +    uint32_t ret_idx;
>> +
>> +    /*
>> +     * Check if there is any space left for a domU module, the variable
>> +     * dom0less_modules_available is updated each time we use read_file(...)
>> +     * successfully.
>> +     */
>> +    if ( !dom0less_modules_available )
>> +        blexit(L"No space left for domU modules");
>> +    module_name.s = (char*) name;
>> +    ret_idx = dom0less_modules_idx;
>> +    file = &dom0less_files[ret_idx];
>> +
>> +    /* Save at this index the name of this binary */
>> +    file_name = &dom0less_bin_names[ret_idx];
>> +
>> +    if ( efi_bs->AllocatePool(EfiLoaderData, (name_len + 1) * sizeof(char),
>> +                              (void**)&file_name->name) != EFI_SUCCESS )
>> +        blexit(L"Error allocating memory for dom0less binary name");
> 
> As far as I can tell we could just set file_name = name;

If you are sure I will modify that, I will wait your confirmation.

> 
> 
>> +    /* Save name and length of the binary in the data structure */
>> +    strlcpy(file_name->name, name, name_len);
>> +    file_name->name_len = name_len;
>> +
>> +    /* Load the binary in memory */
>> +    read_file(dir_handle, s2w(&module_name), file, NULL);
>> +
>> +    /* s2w(...) allocates some memory, free it */
>> +    efi_bs->FreePool(module_name.w);
>> +
>> +    dom0less_modules_idx++;
>> +
>> +    return ret_idx;
>> +}
>> +
>> +/*
>> + * This function checks for the presence of the uefi,binary property in the
>> + * module, if found it loads the binary as dom0less module and sets the right
>> + * address for the reg property into the module DT node.
>> + */
>> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
>> +                                          int module_node_offset,
>> +                                          int reg_addr_cells,
>> +                                          int reg_size_cells)
>> +{
>> +    const void* uefi_name_prop;
>> +    char mod_string[24]; /* Placeholder for module@ + a 64-bit number + \0 */
>> +    int uefi_name_len, file_idx;
>> +    struct file* 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 ( NULL == uefi_name_prop )
>> +        /* Property not found */
>> +        return;
>> +
>> +    file_idx = get_dom0less_file_index(uefi_name_prop, uefi_name_len);
>> +    if (file_idx < 0)
>> +        file_idx = allocate_dom0less_file(dir_handle, uefi_name_prop,
>> +                                          uefi_name_len);
>> +
>> +    file = &dom0less_files[file_idx];
>> +
>> +    snprintf(mod_string, sizeof(mod_string), "module@%"PRIx64, file->addr);
>> +
>> +    /* Rename the module to be module@{address} */
>> +    if ( fdt_set_name(fdt, module_node_offset, mod_string) < 0 )
>> +        blexit(L"Unable to add domU ramdisk FDT node.");
>> +
>> +    if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
>> +                     file->addr, file->size) < 0 )
>> +        blexit(L"Unable to set reg property.");
>> +}
>> +
>> +/*
>> + * This function checks for boot modules under the domU guest domain node
>> + * in the DT.
>> + */
>> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> +                                               int domain_node,
>> +                                               int addr_cells,
>> +                                               int size_cells)
>> +{
>> +    /*
>> +     * Check for nodes compatible with multiboot,{kernel,ramdisk,device-tree}
>> +     * inside this node
>> +     */
>> +    for ( int module_node = fdt_first_subnode(fdt, domain_node);
> 
> int module_node;
> 
> for ( module_node = fdt_first_subnode(fdt, domain_node);
> 

Will fix that.

> 
>> +          module_node > 0;
>> +          module_node = fdt_next_subnode(fdt, module_node) )
>> +    {
>> +        if ( (fdt_node_check_compatible(fdt, module_node,
>> +                                        "multiboot,kernel") == 0) ||
>> +             (fdt_node_check_compatible(fdt, module_node,
>> +                                        "multiboot,ramdisk") == 0) ||
>> +             (fdt_node_check_compatible(fdt, module_node,
>> +                                        "multiboot,device-tree") == 0) )
>> +        {
>> +            /* The compatible is one of the strings above, check the module */
>> +            handle_dom0less_module_node(dir_handle, module_node, addr_cells,
>> +                                        size_cells);
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * This function checks for xen domain nodes under the /chosen node for possible
>> + * domU guests to be loaded.
>> + */
>> +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle)
>> +{
>> +    int chosen;
>> +    int addr_len, size_len;
>> +
>> +    /* Check for the chosen node in the current DTB */
>> +    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
>> +    if ( chosen < 0 )
>> +        blexit(L"Unable to setup chosen node");
>> +
>> +    /* Check for nodes compatible with xen,domain under the chosen node */
>> +    for ( int node = fdt_first_subnode(fdt, chosen);
>> +          node > 0;
>> +          node = fdt_next_subnode(fdt, node) )
>> +    {
>> +        int addr_cells, size_cells, len;
>> +        const struct fdt_property *prop;
>> +
>> +        if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
>> +            continue;
>> +
>> +        /* Get or set #address-cells and #size-cells */
>> +        prop = fdt_get_property(fdt, node, "#address-cells", &len);
>> +        if ( !prop )
>> +            blexit(L"#address-cells not found in domain node.");
>> +
>> +        addr_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
>> +
>> +        prop = fdt_get_property(fdt, node, "#size-cells", &len);
>> +        if ( !prop )
>> +            blexit(L"#size-cells not found in domain node.");
>> +
>> +        size_cells = fdt32_to_cpu(*((uint32_t *)prop->data));
>> +
>> +        /* Found a node with compatible xen,domain; handle this node. */
>> +        handle_dom0less_domain_node(dir_handle, node, addr_cells, size_cells);
>> +    }
>> +
>> +    if ( dom0less_modules_idx > 0 )
>> +        return true;
>> +
>> +    return false;
>> }
>> 
>> static void __init efi_arch_cpu(void)
>> @@ -562,8 +793,19 @@ static void __init efi_arch_cpu(void)
>> 
>> static void __init efi_arch_blexit(void)
>> {
>> +    uint32_t i = 0;
>>     if ( dtbfile.need_to_free )
>>         efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
>> +    /* Free dom0less files if any */
>> +    for ( ; i < dom0less_modules_idx; i++ )
>> +    {
>> +        /* Free dom0less binary names */
>> +        efi_bs->FreePool(dom0less_bin_names[i].name);
>> +        /* Free dom0less binaries */
>> +        if ( dom0less_files[i].need_to_free )
>> +            efi_bs->FreePages(dom0less_files[i].addr,
>> +                              PFN_UP(dom0less_files[i].size));
>> +    }
>>     if ( memmap )
>>         efi_bs->FreePool(memmap);
>> }
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index 758f9d74d2..65493c4b46 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1134,8 +1134,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>     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;
>> +    bool dom0less_found = false;
>> 
>>     __set_bit(EFI_BOOT, &efi_flags);
>>     __set_bit(EFI_LOADER, &efi_flags);
>> @@ -1285,14 +1286,21 @@ 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);
>> +#ifdef CONFIG_ARM
>> +        /* dom0less feature is supported only on ARM */
>> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
>> +#endif
> 
> Rather than an #ifdef here you can simply implement
> check_dom0less_efi_boot on x86 as a static inline returning always
> false.

Sure I will create that on x86 code and I will update the code here.

> 
> Also, we are under the if ( use_cfg_file ) code path. So maybe it is
> reasonable that dom0 is required if we are booting with use_cfg_file
> = true. After all, it is specified as a required property today of
> xen.cfg.
> 
> If you follow my suggestion with an explicit enabled/disabled of xen.cfg
> from device tree, a true dom0less configuration could be fully specified
> without xen.cfg.
> 
> If we do that, then here probable we don't need to change this code path.
> 

Please check my reply on the previous patch.

Cheers,

Luca

> 
> 
>> +        if ( !name.s && !dom0less_found )
>> +            blexit(L"No Dom0 kernel image specified.");
>> +
>> +        if ( name.s != NULL )
>> +            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 != NULL) )
>>         {
>>             read_file(dir_handle, s2w(&name), &kernel, option_str);
>>             efi_bs->FreePool(name.w);

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

[Stefano Stabellini]

On Thu, 16 Sep 2021, Luca Fancellu wrote:
> > On 16 Sep 2021, at 02:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Wed, 15 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>
> >> ---
> >> docs/misc/efi.pandoc        |  37 ++++++
> >> xen/arch/arm/efi/efi-boot.h | 244 +++++++++++++++++++++++++++++++++++-
> >> xen/common/efi/boot.c       |  20 ++-
> >> 3 files changed, 294 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> >> index ac3cd58cae..db9b3273f8 100644
> >> --- a/docs/misc/efi.pandoc
> >> +++ b/docs/misc/efi.pandoc
> >> @@ -165,3 +165,40 @@ 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";
> >> +        };
> >> +    };
> > 
> > Can you please also update docs/misc/arm/device-tree/booting.txt ?
> > Either a link to docs/misc/efi.pandoc or a definition of the uefi,binary
> > property (mentioning that it is EFI-only.)
> 
> Yes I will update it.
> 
> > 
> > 
> >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> >> index 5ff626c6a0..8d7ced70f2 100644
> >> --- a/xen/arch/arm/efi/efi-boot.h
> >> +++ b/xen/arch/arm/efi/efi-boot.h
> >> @@ -8,9 +8,39 @@
> >> #include <asm/setup.h>
> >> #include <asm/smp.h>
> >> 
> >> +typedef struct {
> >> +    char* name;
> >> +    int name_len;
> >> +} dom0less_module_name;
> >> +
> >> +/*
> >> + * Binaries will be translated into bootmodules, the maximum number for them is
> >> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> >> + */
> >> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> >> +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
> >> +static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
> > 
> > I suggest a slightly different model where we don't call AllocatePool to
> > allocate dom0less_module_name.name and instead we just set the pointer
> > directly to the fdt string. There is no risk of the fdt going away at
> > this point so it should be safe to use.
> 
> Yes I thought about this approach but since I was not sure how the DTB behaves when we modify
> It to add the reg property or to modify the module name, then I used this other approach.
> Are you sure that the pointed memory will stay the same after we modify the DTB? My main concern
> was that the DTB structure was going to be modified and the string I was pointing in the DTB memory
> can be relocated elsewhere. 

You are right: fdt_set_name and fdt_set_reg can cause a memmove to be
called, which might change the pointers. Which means we cannot simply
set the char* pointer to the device tree string as it might change.
That's unfortunate. For the lack of a better suggestion, go ahead and
keep AllocatePool/FreePool for the next version.

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

[Jan Beulich]

A number of nits, sorry:

On 15.09.2021 16:26, Luca Fancellu wrote:
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -8,9 +8,39 @@
>  #include <asm/setup.h>
>  #include <asm/smp.h>
>  
> +typedef struct {
> +    char* name;

Misplaced *.

> +    int name_len;

Surely this can't go negative? (Same issue elsewhere.)

> +} dom0less_module_name;
> +
> +/*
> + * Binaries will be translated into bootmodules, the maximum number for them is
> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> + */
> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
> +static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
> +static uint32_t __initdata dom0less_modules_available = MAX_DOM0LESS_MODULES;
> +static uint32_t __initdata dom0less_modules_idx = 0;

Please see ./CODING_STYLE for your (ab)use of uint32_t here and
elsewhere.

> +#define ERROR_DOM0LESS_FILE_NOT_FOUND -1

Macros expanding to more than a single token should be suitably
parenthesized at least when the expression can possibly be mistaken
precedence wise (i.e. array[n] is in principle fine without
parentheses, because the meaning won't change no matter how it's
used in an expression).

>  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>  void __flush_dcache_area(const void *vaddr, unsigned long size);
>  
> +static int __init get_dom0less_file_index(const char* name, int name_len);
> +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                              const char* name, int name_len);
> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
> +                                               int module_node_offset,
> +                                               int reg_addr_cells,
> +                                               int reg_size_cells);
> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
> +                                               int domain_node,
> +                                               int addr_cells,
> +                                               int size_cells);
> +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle);

There are attributes (e.g. __must_check) which belong on the
declarations. __init, however, belongs on the definitions.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1134,8 +1134,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      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;
> +    bool dom0less_found = false;
>  
>      __set_bit(EFI_BOOT, &efi_flags);
>      __set_bit(EFI_LOADER, &efi_flags);
> @@ -1285,14 +1286,21 @@ 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);
> +#ifdef CONFIG_ARM
> +        /* dom0less feature is supported only on ARM */
> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
> +#endif
> +
> +        if ( !name.s && !dom0less_found )

Here you (properly ) use !name.s,

> +            blexit(L"No Dom0 kernel image specified.");
> +
> +        if ( name.s != NULL )

Here you then mean to omit the "!= NULL" for consistency and brevity.

> +            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)) &&

Stray parentheses.

> +             (name.s != NULL) )

See above.

I also don't think this logic is quite right: If you're dom0less,
why would you want to look for an embedded Dom0 kernel image?

Jan

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

[Luca Fancellu]

> On 16 Sep 2021, at 09:46, Jan Beulich <jbeulich@suse.com> wrote:
> 
> A number of nits, sorry:
> 
> On 15.09.2021 16:26, Luca Fancellu wrote:
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -8,9 +8,39 @@
>> #include <asm/setup.h>
>> #include <asm/smp.h>
>> 
>> +typedef struct {
>> +    char* name;
> 
> Misplaced *.

I was looking in the CODING_STYLE and I didn’t found anything that mandates
char *name; instead of char* name; but if you prefer I can change it since I have
to do some modification to the patch.

> 
>> +    int name_len;
> 
> Surely this can't go negative? (Same issue elsewhere.)

I will change that to unsigned int.

> 
>> +} dom0less_module_name;
>> +
>> +/*
>> + * Binaries will be translated into bootmodules, the maximum number for them is
>> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
>> + */
>> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
>> +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES];
>> +static dom0less_module_name __initdata dom0less_bin_names[MAX_DOM0LESS_MODULES];
>> +static uint32_t __initdata dom0less_modules_available = MAX_DOM0LESS_MODULES;
>> +static uint32_t __initdata dom0less_modules_idx = 0;
> 
> Please see ./CODING_STYLE for your (ab)use of uint32_t here and
> elsewhere.

Ok, I will change them to unsigned int

> 
>> +#define ERROR_DOM0LESS_FILE_NOT_FOUND -1
> 
> Macros expanding to more than a single token should be suitably
> parenthesized at least when the expression can possibly be mistaken
> precedence wise (i.e. array[n] is in principle fine without
> parentheses, because the meaning won't change no matter how it's
> used in an expression).

I will fix it to be (-1)

> 
>> void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>> void __flush_dcache_area(const void *vaddr, unsigned long size);
>> 
>> +static int __init get_dom0less_file_index(const char* name, int name_len);
>> +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
>> +                                              const char* name, int name_len);
>> +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle,
>> +                                               int module_node_offset,
>> +                                               int reg_addr_cells,
>> +                                               int reg_size_cells);
>> +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle,
>> +                                               int domain_node,
>> +                                               int addr_cells,
>> +                                               int size_cells);
>> +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle);
> 
> There are attributes (e.g. __must_check) which belong on the
> declarations. __init, however, belongs on the definitions.

Ok, I will remove __init from declarations.

> 
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1134,8 +1134,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>     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;
>> +    bool dom0less_found = false;
>> 
>>     __set_bit(EFI_BOOT, &efi_flags);
>>     __set_bit(EFI_LOADER, &efi_flags);
>> @@ -1285,14 +1286,21 @@ 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);
>> +#ifdef CONFIG_ARM
>> +        /* dom0less feature is supported only on ARM */
>> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
>> +#endif
>> +
>> +        if ( !name.s && !dom0less_found )
> 
> Here you (properly ) use !name.s,

This is not my code, I just added && !dom0less

> 
>> +            blexit(L"No Dom0 kernel image specified.");
>> +
>> +        if ( name.s != NULL )
> 
> Here you then mean to omit the "!= NULL" for consistency and brevity.

I usually check explicitely pointers with NULL, is it something to be avoided in Xen?
There are some industrial coding standards that says to avoid the use of ! operator
with pointers. Is it important here to do !name.s instead of the solution above?

> 
>> +            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)) &&
> 
> Stray parentheses.

Will fix

> 
>> +             (name.s != NULL) )
> 
> See above.

Will fix

> 
> I also don't think this logic is quite right: If you're dom0less,
> why would you want to look for an embedded Dom0 kernel image?

This is common code, that check is not from my patch.

Before this serie, EFI boot requires that a dom0 module was passed, otherwise
the boot was stopped.

This serie instead removes this requirement, letting the boot continue if there is no dom0
kernel.

However (as in the old code) if the user embed the dom0 kernel in the image, then it is
legit to use it and if there are also other domUs specified by DT, then the system will
start the dom0 kernel and the domUs kernel as well.

Cheers,
Luca 


> 
> Jan
> 

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

[Jan Beulich]

On 16.09.2021 13:28, Luca Fancellu wrote:
>> On 16 Sep 2021, at 09:46, Jan Beulich <jbeulich@suse.com> wrote:
>> On 15.09.2021 16:26, Luca Fancellu wrote:
>>> --- a/xen/arch/arm/efi/efi-boot.h
>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>> @@ -8,9 +8,39 @@
>>> #include <asm/setup.h>
>>> #include <asm/smp.h>
>>>
>>> +typedef struct {
>>> +    char* name;
>>
>> Misplaced *.
> 
> I was looking in the CODING_STYLE and I didn’t found anything that mandates
> char *name; instead of char* name; but if you prefer I can change it since I have
> to do some modification to the patch.

I don't think it's reasonable to spell out there every little detail.
You should also take adjacent code into consideration, making yours
match. Issues only arise when there's bad code that you happen to
look at.

>>> @@ -1285,14 +1286,21 @@ 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);
>>> +#ifdef CONFIG_ARM
>>> +        /* dom0less feature is supported only on ARM */
>>> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
>>> +#endif
>>> +
>>> +        if ( !name.s && !dom0less_found )
>>
>> Here you (properly ) use !name.s,
> 
> This is not my code, I just added && !dom0less

Correct, which is why this is fine.

>>> +            blexit(L"No Dom0 kernel image specified.");
>>> +
>>> +        if ( name.s != NULL )
>>
>> Here you then mean to omit the "!= NULL" for consistency and brevity.
> 
> I usually check explicitely pointers with NULL, is it something to be avoided in Xen?
> There are some industrial coding standards that says to avoid the use of ! operator
> with pointers. Is it important here to do !name.s instead of the solution above?

As you can see from neighboring code, we prefer the shorter forms,
for being easier/shorter to read.

>>> +            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)) &&
>>
>> Stray parentheses.
> 
> Will fix
> 
>>
>>> +             (name.s != NULL) )
>>
>> See above.
> 
> Will fix
> 
>>
>> I also don't think this logic is quite right: If you're dom0less,
>> why would you want to look for an embedded Dom0 kernel image?
> 
> This is common code, that check is not from my patch.

It is you who is adding the name.s != NULL part, isn't it?

> Before this serie, EFI boot requires that a dom0 module was passed, otherwise
> the boot was stopped.
> 
> This serie instead removes this requirement, letting the boot continue if there is no dom0
> kernel.
> 
> However (as in the old code) if the user embed the dom0 kernel in the image, then it is
> legit to use it and if there are also other domUs specified by DT, then the system will
> start the dom0 kernel and the domUs kernel as well.

This doesn't match what I would expect - if configuration tells
to boot dom0less, why would an embedded Dom0 kernel matter? I can
see that views might differ here; you will want to write down
somewhere what the intended behavior in such a case is.

Jan

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

[Luca Fancellu]

> On 16 Sep 2021, at 13:15, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.09.2021 13:28, Luca Fancellu wrote:
>>> On 16 Sep 2021, at 09:46, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 15.09.2021 16:26, Luca Fancellu wrote:
>>>> --- a/xen/arch/arm/efi/efi-boot.h
>>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>>> @@ -8,9 +8,39 @@
>>>> #include <asm/setup.h>
>>>> #include <asm/smp.h>
>>>> 
>>>> +typedef struct {
>>>> +    char* name;
>>> 
>>> Misplaced *.
>> 
>> I was looking in the CODING_STYLE and I didn’t found anything that mandates
>> char *name; instead of char* name; but if you prefer I can change it since I have
>> to do some modification to the patch.
> 
> I don't think it's reasonable to spell out there every little detail.
> You should also take adjacent code into consideration, making yours
> match. Issues only arise when there's bad code that you happen to
> look at.
> 
>>>> @@ -1285,14 +1286,21 @@ 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);
>>>> +#ifdef CONFIG_ARM
>>>> +        /* dom0less feature is supported only on ARM */
>>>> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
>>>> +#endif
>>>> +
>>>> +        if ( !name.s && !dom0less_found )
>>> 
>>> Here you (properly ) use !name.s,
>> 
>> This is not my code, I just added && !dom0less
> 
> Correct, which is why this is fine.
> 
>>>> +            blexit(L"No Dom0 kernel image specified.");
>>>> +
>>>> +        if ( name.s != NULL )
>>> 
>>> Here you then mean to omit the "!= NULL" for consistency and brevity.
>> 
>> I usually check explicitely pointers with NULL, is it something to be avoided in Xen?
>> There are some industrial coding standards that says to avoid the use of ! operator
>> with pointers. Is it important here to do !name.s instead of the solution above?
> 
> As you can see from neighboring code, we prefer the shorter forms,
> for being easier/shorter to read.

Ok

> 
>>>> +            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)) &&
>>> 
>>> Stray parentheses.
>> 
>> Will fix
>> 
>>> 
>>>> +             (name.s != NULL) )
>>> 
>>> See above.
>> 
>> Will fix
>> 
>>> 
>>> I also don't think this logic is quite right: If you're dom0less,
>>> why would you want to look for an embedded Dom0 kernel image?
>> 
>> This is common code, that check is not from my patch.
> 
> It is you who is adding the name.s != NULL part, isn't it?

Ok I think the logic needs to be explained because also the name dom0less
Is a little bit misleading, I will explain below.

Short answer for now: if there is no dom0 kernel embedded in Xen and name.s is NULL,
do not try to load something or check dom0 command line because there is no dom0.

> 
>> Before this serie, EFI boot requires that a dom0 module was passed, otherwise
>> the boot was stopped.
>> 
>> This serie instead removes this requirement, letting the boot continue if there is no dom0
>> kernel.
>> 
>> However (as in the old code) if the user embed the dom0 kernel in the image, then it is
>> legit to use it and if there are also other domUs specified by DT, then the system will
>> start the dom0 kernel and the domUs kernel as well.
> 
> This doesn't match what I would expect - if configuration tells
> to boot dom0less, why would an embedded Dom0 kernel matter? I can
> see that views might differ here; you will want to write down
> somewhere what the intended behavior in such a case is.

I explain here my understanding on dom0less, this feature is used to start domUs at
Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.

So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
doesn’t have to be skipped.

Here the possible user cases:
1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]

Let me know your thoughts about that.

Cheers,

Luca


> 
> Jan
> 
> 

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

[Jan Beulich]

On 16.09.2021 17:07, Luca Fancellu wrote:
> I explain here my understanding on dom0less, this feature is used to start domUs at
> Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
> 
> So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
> doesn’t have to be skipped.
> 
> Here the possible user cases:
> 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
> 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
> 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]

If that's the intention - fine. Stefano?

Jan

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

[Stefano Stabellini]

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

On Thu, 16 Sep 2021, Jan Beulich wrote:
> On 16.09.2021 17:07, Luca Fancellu wrote:
> > I explain here my understanding on dom0less, this feature is used to start domUs at
> > Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
> > 
> > So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
> > doesn’t have to be skipped.
> > 
> > Here the possible user cases:
> > 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
> > 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
> > 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]
> 
> If that's the intention - fine. Stefano?

What do you mean by dom0 modules embedded in the Xen image? I am not
familiar with it, but I imagine it doesn't involve any multiboot,module
nodes in device tree, right?

Putting aside "dom0 modules embedded in Xen image" that I didn't fully
understand, there are three ways to load Dom0:

- dom0 kernel (and ramdisk, optional) on xen.cfg
- dom0 kernel (and ramdisk, optional) on device tree using the "reg" property
- dom0 kernel (and ramdisk, optional) on device tree using the "uefi,binary" property

Then, the other use cases are:

- true dom0less, domUs on device tree using the "reg" property
- true dom0less, domUs on device tree using the "uefi,binary" property

And of course all the possible combinations between Dom0 and DomU
loading.


Currently, patch #1 checks for the presence of a Dom0 kernel node and, if
present, it decides not to proceed with xen.cfg. So if the Dom0 kernel
node is *not* present, efi_arch_use_config_file returns true.

However, this could be a true dom0less configuration without any Dom0
kernel. If so, you might not want to load xen.cfg at all because it is
not needed. In a true dom0less configuration, we probably want
efi_arch_use_config_file to return false.


In general, loading xen.cfg or not loading xen.cfg doesn't seem like it
should be dependent on whether there is or there is not a dom0 kernel
node in device tree. I think there are too many possible combinations to
guess correctly based on the presence of the dom0 kernel node. Instead,
I think it would be better to have an explicit flag.

Today, if a "multiboot,module" node is present, efi_arch_use_config_file
returns false. So I suggested to extend it so that if a
"multiboot,module" node is present we look into a specific
xen.cfg-loading property and if present return true otherwise false.
This way, we are backward compatible. 

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

[Jan Beulich]

On 16.09.2021 22:16, Stefano Stabellini wrote:
> On Thu, 16 Sep 2021, Jan Beulich wrote:
>> On 16.09.2021 17:07, Luca Fancellu wrote:
>>> I explain here my understanding on dom0less, this feature is used to start domUs at
>>> Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
>>>
>>> So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
>>> doesn’t have to be skipped.
>>>
>>> Here the possible user cases:
>>> 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
>>> 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
>>> 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]
>>
>> If that's the intention - fine. Stefano?
> 
> What do you mean by dom0 modules embedded in the Xen image? I am not
> familiar with it, but I imagine it doesn't involve any multiboot,module
> nodes in device tree, right?

There's no DT interaction there afaict. See commit 8a71d50ed40b
("efi: Enable booting unified hypervisor/kernel/initrd images").

> Putting aside "dom0 modules embedded in Xen image" that I didn't fully
> understand, there are three ways to load Dom0:
> 
> - dom0 kernel (and ramdisk, optional) on xen.cfg
> - dom0 kernel (and ramdisk, optional) on device tree using the "reg" property
> - dom0 kernel (and ramdisk, optional) on device tree using the "uefi,binary" property
> 
> Then, the other use cases are:
> 
> - true dom0less, domUs on device tree using the "reg" property
> - true dom0less, domUs on device tree using the "uefi,binary" property
> 
> And of course all the possible combinations between Dom0 and DomU
> loading.

Okay, so as Luca says "dom0less" is a misnomer really. I wasn't
aware of this.

Jan

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

[Luca Fancellu]

> On 16 Sep 2021, at 21:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 16 Sep 2021, Jan Beulich wrote:
>> On 16.09.2021 17:07, Luca Fancellu wrote:
>>> I explain here my understanding on dom0less, this feature is used to start domUs at
>>> Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
>>> 
>>> So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
>>> doesn’t have to be skipped.
>>> 
>>> Here the possible user cases:
>>> 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
>>> 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
>>> 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]
>> 
>> If that's the intention - fine. Stefano?
> 

Hi Stefano,

> What do you mean by dom0 modules embedded in the Xen image? I am not
> familiar with it, but I imagine it doesn't involve any multiboot,module
> nodes in device tree, right?
> 
> Putting aside "dom0 modules embedded in Xen image" that I didn't fully
> understand, there are three ways to load Dom0:
> 
> - dom0 kernel (and ramdisk, optional) on xen.cfg
> - dom0 kernel (and ramdisk, optional) on device tree using the "reg" property
> - dom0 kernel (and ramdisk, optional) on device tree using the "uefi,binary" property

True for the #1 and #2, the last one is not implemented. The uefi,binary property
for now is only used to load domU modules.

> 
> Then, the other use cases are:
> 
> - true dom0less, domUs on device tree using the "reg" property
> - true dom0less, domUs on device tree using the "uefi,binary" property
> 
> And of course all the possible combinations between Dom0 and DomU
> loading.
> 
> 
> Currently, patch #1 checks for the presence of a Dom0 kernel node and, if
> present, it decides not to proceed with xen.cfg. So if the Dom0 kernel
> node is *not* present, efi_arch_use_config_file returns true.
> 
> However, this could be a true dom0less configuration without any Dom0
> kernel. If so, you might not want to load xen.cfg at all because it is
> not needed. In a true dom0less configuration, we probably want
> efi_arch_use_config_file to return false.

In a true dom0less configuration we might need to read xen.cfg to retrieve the
Xen command line, but following the actual implementation of the common code
there is more. I’m going to explain.

What efi_arch_use_config_file really does is not only choosing to read xen.cfg
or not. Following the common code (xen/common/efi/boot.c) and what its result activate
along the path, it basically decides if the UEFI stub is used as a loader from filesystem
or not. We need the UEFI stub as a loader to be 100% UEFI and load our modules.

The original check basically says “if there are multiboot,module in the DT, then some
bootloader has loaded in memory the required modules so I’m not gonna load anything
from the filesystem because I assume it puts everything in place for me to boot.”

From misc/efi.txt:
When booted as an EFI application, Xen requires a configuration file as described below unless a bootloader,
such as GRUB, has 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.

What I’m doing in patch #1 is restricting that check to just the multiboot,module that are
Dom0 module, why? Because with the introduction of dom0less we need to specify
multiboot,modules for domUs, but the presence or not of dom0 modules is the only
Information we need to understand if the user decided to start Xen with everything
in places (modules in memory, xen command line, dtb) or if the job is demanded to the
UEFI stub and its configuration file.

By the configuration file you can also load in memory the Xen dtb, so Xen can
be started as an EFI application without the DTB and then load it using the EFI stub.

I’m not against this new property “xen,cfg-loading”, I just think it is not needed because
we have all the information we need without it and in any case we need to read the
configuration file because otherwise we won’t have access to the Xen command line.

Now I’m going to show you examples of all use cases that are valid with the introduction
of this serie:

1) Start Xen as EFI application and load only Dom0

    Xen.cfg:
    [global]
    default=xen_dom0

    [xen_dom0]
    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=<xen DTB>

    DT:
    {no modification}

2) Start Xen as EFI application to load a true dom0less setup

    Xen.cfg:
    [global]
    default=xen_true_dom0less

    [xen_true_dom0less]
    options=<Xen command line>
    dtb=<xen DTB>

    DT:
    chosen {
        #size-cells = <0x1>;
	#address-cells = <0x1>;

	domU1 {
            #size-cells = <0x1>; 
            #address-cells = <0x1>;
            compatible = "xen,domain”;
            cpus = <1>;
            memory = <0 0xC0000>;
           
            module@1 {
                compatible = "multiboot,kernel", "multiboot,module”;
                bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
                uefi,binary = “domU_kernel1.bin"
            };

            module@2 {
                compatible = “multiboot,ramdisk", "multiboot,module”;
                uefi,binary = “domU_ramdisk1.bin"
            };

            module@3 {
                compatible = "multiboot,device-tree", "multiboot,module”;
                uefi,binary = “domU_passthrough1.dtb"
            }; 
        };
        
        domU2 { <as above> };
    }

3) Start Xen as EFI application to load Dom0 and DomUs

    Xen.cfg:
    [global]
    default=xen_dom0_domUs

    [xen_dom0_domUs]
    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=<xen DTB>

    DT:
    chosen {
        #size-cells = <0x1>;
	#address-cells = <0x1>;

	domU1 {
            #size-cells = <0x1>; 
            #address-cells = <0x1>;
            compatible = "xen,domain”;
            cpus = <1>;
            memory = <0 0xC0000>;
           
            module@1 {
                compatible = "multiboot,kernel", "multiboot,module”;
                bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
                uefi,binary = “domU_kernel1.bin"
            };

            module@2 {
                compatible = “multiboot,ramdisk", "multiboot,module”;
                uefi,binary = “domU_ramdisk1.bin"
            };

            module@3 {
                compatible = "multiboot,device-tree", "multiboot,module”;
                uefi,binary = “domU_passthrough1.dtb"
            }; 
        };
        
        domU2 { <as above> };
    }

So as you see every case is covered without the introduction of the
property.

Please let me know what do you think.

Cheers,
Luca

> 
> 
> In general, loading xen.cfg or not loading xen.cfg doesn't seem like it
> should be dependent on whether there is or there is not a dom0 kernel
> node in device tree. I think there are too many possible combinations to
> guess correctly based on the presence of the dom0 kernel node. Instead,
> I think it would be better to have an explicit flag.
> 
> Today, if a "multiboot,module" node is present, efi_arch_use_config_file
> returns false. So I suggested to extend it so that if a
> "multiboot,module" node is present we look into a specific
> xen.cfg-loading property and if present return true otherwise false.
> This way, we are backward compatible. 

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

[Stefano Stabellini]

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

On Fri, 17 Sep 2021, Luca Fancellu wrote:
> > On 16 Sep 2021, at 21:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Thu, 16 Sep 2021, Jan Beulich wrote:
> >> On 16.09.2021 17:07, Luca Fancellu wrote:
> >>> I explain here my understanding on dom0less, this feature is used to start domUs at
> >>> Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
> >>> 
> >>> So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
> >>> doesn’t have to be skipped.
> >>> 
> >>> Here the possible user cases:
> >>> 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
> >>> 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
> >>> 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]
> >> 
> >> If that's the intention - fine. Stefano?
> > 
> 
> Hi Stefano,
> 
> > What do you mean by dom0 modules embedded in the Xen image? I am not
> > familiar with it, but I imagine it doesn't involve any multiboot,module
> > nodes in device tree, right?
> > 
> > Putting aside "dom0 modules embedded in Xen image" that I didn't fully
> > understand, there are three ways to load Dom0:
> > 
> > - dom0 kernel (and ramdisk, optional) on xen.cfg
> > - dom0 kernel (and ramdisk, optional) on device tree using the "reg" property
> > - dom0 kernel (and ramdisk, optional) on device tree using the "uefi,binary" property
> 
> True for the #1 and #2, the last one is not implemented. The uefi,binary property
> for now is only used to load domU modules.

Yeah, it is no problem that is not currently implemented, but from a
device tree binding / efi interface perspective it should be possible.

 
> > Then, the other use cases are:
> > 
> > - true dom0less, domUs on device tree using the "reg" property
> > - true dom0less, domUs on device tree using the "uefi,binary" property
> > 
> > And of course all the possible combinations between Dom0 and DomU
> > loading.
> > 
> > 
> > Currently, patch #1 checks for the presence of a Dom0 kernel node and, if
> > present, it decides not to proceed with xen.cfg. So if the Dom0 kernel
> > node is *not* present, efi_arch_use_config_file returns true.
> > 
> > However, this could be a true dom0less configuration without any Dom0
> > kernel. If so, you might not want to load xen.cfg at all because it is
> > not needed. In a true dom0less configuration, we probably want
> > efi_arch_use_config_file to return false.
> 
> In a true dom0less configuration we might need to read xen.cfg to retrieve the
> Xen command line, 

The Xen command line could also be on device tree
(/chosen/xen,xen-bootargs).


> but following the actual implementation of the common code
> there is more. I’m going to explain.
> 
> What efi_arch_use_config_file really does is not only choosing to read xen.cfg
> or not. Following the common code (xen/common/efi/boot.c) and what its result activate
> along the path, it basically decides if the UEFI stub is used as a loader from filesystem
> or not. We need the UEFI stub as a loader to be 100% UEFI and load our modules.
>
> The original check basically says “if there are multiboot,module in the DT, then some
> bootloader has loaded in memory the required modules so I’m not gonna load anything
> from the filesystem because I assume it puts everything in place for me to boot.”

OK, I am following. It looks like this is the source of the issue.

 
> >From misc/efi.txt:
> When booted as an EFI application, Xen requires a configuration file as described below unless a bootloader,
> such as GRUB, has 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.
> 
> What I’m doing in patch #1 is restricting that check to just the multiboot,module that are
> Dom0 module, why? Because with the introduction of dom0less we need to specify
> multiboot,modules for domUs, but the presence or not of dom0 modules is the only
> Information we need to understand if the user decided to start Xen with everything
> in places (modules in memory, xen command line, dtb) or if the job is demanded to the
> UEFI stub and its configuration file.

I don't think so. Imagine a case where the user has everything in device
tree, doesn't need xen.cfg, but dom0 and domUs are specified as
uefi,binary.

We don't want xen.cfg but we do want to be able to load files from the
filesystem. This might not be currently implemented but from an bindings
perspective it should be possible.


> By the configuration file you can also load in memory the Xen dtb, so Xen can
> be started as an EFI application without the DTB and then load it using the EFI stub.

This can be very useful but it would follow the !fdt check and return
true from efi_arch_use_config_file. So it doesn't really conflict with
anything we would around multiboot,module and xen,cfg-loading.


> I’m not against this new property “xen,cfg-loading”, I just think it is not needed because
> we have all the information we need without it and in any case we need to read the
> configuration file because otherwise we won’t have access to the Xen command line.

We don't necessarely need to read the Xen command line from xen.cfg :-)


> Now I’m going to show you examples of all use cases that are valid with the introduction
> of this serie:
> 
> 1) Start Xen as EFI application and load only Dom0
> 
>     Xen.cfg:
>     [global]
>     default=xen_dom0
> 
>     [xen_dom0]
>     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=<xen DTB>
> 
>     DT:
>     {no modification}
> 
> 2) Start Xen as EFI application to load a true dom0less setup
> 
>     Xen.cfg:
>     [global]
>     default=xen_true_dom0less
> 
>     [xen_true_dom0less]
>     options=<Xen command line>
>     dtb=<xen DTB>
> 
>     DT:
>     chosen {
>         #size-cells = <0x1>;
> 	#address-cells = <0x1>;
> 
> 	domU1 {
>             #size-cells = <0x1>; 
>             #address-cells = <0x1>;
>             compatible = "xen,domain”;
>             cpus = <1>;
>             memory = <0 0xC0000>;
>            
>             module@1 {
>                 compatible = "multiboot,kernel", "multiboot,module”;
>                 bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>                 uefi,binary = “domU_kernel1.bin"
>             };
> 
>             module@2 {
>                 compatible = “multiboot,ramdisk", "multiboot,module”;
>                 uefi,binary = “domU_ramdisk1.bin"
>             };
> 
>             module@3 {
>                 compatible = "multiboot,device-tree", "multiboot,module”;
>                 uefi,binary = “domU_passthrough1.dtb"
>             }; 
>         };
>         
>         domU2 { <as above> };
>     }
> 
> 3) Start Xen as EFI application to load Dom0 and DomUs
> 
>     Xen.cfg:
>     [global]
>     default=xen_dom0_domUs
> 
>     [xen_dom0_domUs]
>     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=<xen DTB>
> 
>     DT:
>     chosen {
>         #size-cells = <0x1>;
> 	#address-cells = <0x1>;
> 
> 	domU1 {
>             #size-cells = <0x1>; 
>             #address-cells = <0x1>;
>             compatible = "xen,domain”;
>             cpus = <1>;
>             memory = <0 0xC0000>;
>            
>             module@1 {
>                 compatible = "multiboot,kernel", "multiboot,module”;
>                 bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>                 uefi,binary = “domU_kernel1.bin"
>             };
> 
>             module@2 {
>                 compatible = “multiboot,ramdisk", "multiboot,module”;
>                 uefi,binary = “domU_ramdisk1.bin"
>             };
> 
>             module@3 {
>                 compatible = "multiboot,device-tree", "multiboot,module”;
>                 uefi,binary = “domU_passthrough1.dtb"
>             }; 
>         };
>         
>         domU2 { <as above> };
>     }
> 
> So as you see every case is covered without the introduction of the
> property.
> 
> Please let me know what do you think.

I think that from an interface perspective (not a code perspective) we
need to be able to account for cases like:

4) Start Xen as EFI application and load only Dom0
no Xen.cfg
DT:
  xen,xen-bootargs
  dom0/uefi,binary
  domUs/uefi,binary


But in any case, even disregarding this case, past experience has taught
me that it is always better to have an explicit flag to trigger a new
behavior, rather than relying on "guesswork". If we introduce
"xen,cfg-loading", we are going to be a lot more future-proof that if we
don't introduce it in terms of backward and forward compatibility in
case we need to change anything.

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

[Luca Fancellu]

> On 17 Sep 2021, at 23:33, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Fri, 17 Sep 2021, Luca Fancellu wrote:
>>> On 16 Sep 2021, at 21:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Thu, 16 Sep 2021, Jan Beulich wrote:
>>>> On 16.09.2021 17:07, Luca Fancellu wrote:
>>>>> I explain here my understanding on dom0less, this feature is used to start domUs at
>>>>> Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
>>>>> 
>>>>> So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
>>>>> doesn’t have to be skipped.
>>>>> 
>>>>> Here the possible user cases:
>>>>> 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
>>>>> 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
>>>>> 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]
>>>> 
>>>> If that's the intention - fine. Stefano?
>>> 
>> 
>> Hi Stefano,
>> 
>>> What do you mean by dom0 modules embedded in the Xen image? I am not
>>> familiar with it, but I imagine it doesn't involve any multiboot,module
>>> nodes in device tree, right?
>>> 
>>> Putting aside "dom0 modules embedded in Xen image" that I didn't fully
>>> understand, there are three ways to load Dom0:
>>> 
>>> - dom0 kernel (and ramdisk, optional) on xen.cfg
>>> - dom0 kernel (and ramdisk, optional) on device tree using the "reg" property
>>> - dom0 kernel (and ramdisk, optional) on device tree using the "uefi,binary" property
>> 
>> True for the #1 and #2, the last one is not implemented. The uefi,binary property
>> for now is only used to load domU modules.
> 
> Yeah, it is no problem that is not currently implemented, but from a
> device tree binding / efi interface perspective it should be possible.
> 
> 
>>> Then, the other use cases are:
>>> 
>>> - true dom0less, domUs on device tree using the "reg" property
>>> - true dom0less, domUs on device tree using the "uefi,binary" property
>>> 
>>> And of course all the possible combinations between Dom0 and DomU
>>> loading.
>>> 
>>> 
>>> Currently, patch #1 checks for the presence of a Dom0 kernel node and, if
>>> present, it decides not to proceed with xen.cfg. So if the Dom0 kernel
>>> node is *not* present, efi_arch_use_config_file returns true.
>>> 
>>> However, this could be a true dom0less configuration without any Dom0
>>> kernel. If so, you might not want to load xen.cfg at all because it is
>>> not needed. In a true dom0less configuration, we probably want
>>> efi_arch_use_config_file to return false.
>> 
>> In a true dom0less configuration we might need to read xen.cfg to retrieve the
>> Xen command line, 
> 
> The Xen command line could also be on device tree
> (/chosen/xen,xen-bootargs).
> 
> 
>> but following the actual implementation of the common code
>> there is more. I’m going to explain.
>> 
>> What efi_arch_use_config_file really does is not only choosing to read xen.cfg
>> or not. Following the common code (xen/common/efi/boot.c) and what its result activate
>> along the path, it basically decides if the UEFI stub is used as a loader from filesystem
>> or not. We need the UEFI stub as a loader to be 100% UEFI and load our modules.
>> 
>> The original check basically says “if there are multiboot,module in the DT, then some
>> bootloader has loaded in memory the required modules so I’m not gonna load anything
>> from the filesystem because I assume it puts everything in place for me to boot.”
> 
> OK, I am following. It looks like this is the source of the issue.
> 
> 
>>> From misc/efi.txt:
>> When booted as an EFI application, Xen requires a configuration file as described below unless a bootloader,
>> such as GRUB, has 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.
>> 
>> What I’m doing in patch #1 is restricting that check to just the multiboot,module that are
>> Dom0 module, why? Because with the introduction of dom0less we need to specify
>> multiboot,modules for domUs, but the presence or not of dom0 modules is the only
>> Information we need to understand if the user decided to start Xen with everything
>> in places (modules in memory, xen command line, dtb) or if the job is demanded to the
>> UEFI stub and its configuration file.
> 
> I don't think so. Imagine a case where the user has everything in device
> tree, doesn't need xen.cfg, but dom0 and domUs are specified as
> uefi,binary.
> 
> We don't want xen.cfg but we do want to be able to load files from the
> filesystem. This might not be currently implemented but from an bindings
> perspective it should be possible.
> 
> 
>> By the configuration file you can also load in memory the Xen dtb, so Xen can
>> be started as an EFI application without the DTB and then load it using the EFI stub.
> 
> This can be very useful but it would follow the !fdt check and return
> true from efi_arch_use_config_file. So it doesn't really conflict with
> anything we would around multiboot,module and xen,cfg-loading.
> 
> 
>> I’m not against this new property “xen,cfg-loading”, I just think it is not needed because
>> we have all the information we need without it and in any case we need to read the
>> configuration file because otherwise we won’t have access to the Xen command line.
> 
> We don't necessarely need to read the Xen command line from xen.cfg :-)
> 
> 
>> Now I’m going to show you examples of all use cases that are valid with the introduction
>> of this serie:
>> 
>> 1) Start Xen as EFI application and load only Dom0
>> 
>>    Xen.cfg:
>>    [global]
>>    default=xen_dom0
>> 
>>    [xen_dom0]
>>    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=<xen DTB>
>> 
>>    DT:
>>    {no modification}
>> 
>> 2) Start Xen as EFI application to load a true dom0less setup
>> 
>>    Xen.cfg:
>>    [global]
>>    default=xen_true_dom0less
>> 
>>    [xen_true_dom0less]
>>    options=<Xen command line>
>>    dtb=<xen DTB>
>> 
>>    DT:
>>    chosen {
>>        #size-cells = <0x1>;
>> 	#address-cells = <0x1>;
>> 
>> 	domU1 {
>>            #size-cells = <0x1>; 
>>            #address-cells = <0x1>;
>>            compatible = "xen,domain”;
>>            cpus = <1>;
>>            memory = <0 0xC0000>;
>> 
>>            module@1 {
>>                compatible = "multiboot,kernel", "multiboot,module”;
>>                bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>>                uefi,binary = “domU_kernel1.bin"
>>            };
>> 
>>            module@2 {
>>                compatible = “multiboot,ramdisk", "multiboot,module”;
>>                uefi,binary = “domU_ramdisk1.bin"
>>            };
>> 
>>            module@3 {
>>                compatible = "multiboot,device-tree", "multiboot,module”;
>>                uefi,binary = “domU_passthrough1.dtb"
>>            }; 
>>        };
>> 
>>        domU2 { <as above> };
>>    }
>> 
>> 3) Start Xen as EFI application to load Dom0 and DomUs
>> 
>>    Xen.cfg:
>>    [global]
>>    default=xen_dom0_domUs
>> 
>>    [xen_dom0_domUs]
>>    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=<xen DTB>
>> 
>>    DT:
>>    chosen {
>>        #size-cells = <0x1>;
>> 	#address-cells = <0x1>;
>> 
>> 	domU1 {
>>            #size-cells = <0x1>; 
>>            #address-cells = <0x1>;
>>            compatible = "xen,domain”;
>>            cpus = <1>;
>>            memory = <0 0xC0000>;
>> 
>>            module@1 {
>>                compatible = "multiboot,kernel", "multiboot,module”;
>>                bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>>                uefi,binary = “domU_kernel1.bin"
>>            };
>> 
>>            module@2 {
>>                compatible = “multiboot,ramdisk", "multiboot,module”;
>>                uefi,binary = “domU_ramdisk1.bin"
>>            };
>> 
>>            module@3 {
>>                compatible = "multiboot,device-tree", "multiboot,module”;
>>                uefi,binary = “domU_passthrough1.dtb"
>>            }; 
>>        };
>> 
>>        domU2 { <as above> };
>>    }
>> 
>> So as you see every case is covered without the introduction of the
>> property.
>> 
>> Please let me know what do you think.
> 

Hi Stefano,

> I think that from an interface perspective (not a code perspective) we
> need to be able to account for cases like:
> 
> 4) Start Xen as EFI application and load only Dom0
> no Xen.cfg
> DT:
>  xen,xen-bootargs
>  dom0/uefi,binary
>  domUs/uefi,binary
> 
> 
> But in any case, even disregarding this case, past experience has taught
> me that it is always better to have an explicit flag to trigger a new
> behavior, rather than relying on "guesswork". If we introduce
> "xen,cfg-loading", we are going to be a lot more future-proof that if we
> don't introduce it in terms of backward and forward compatibility in
> case we need to change anything.

I see your point, for sure the DT is a more powerful tool than the simple
text configuration file and it would be the best interface.
However I think we are moving into the direction where x86 and arm
are going to diverge even if we can have a common interface for them
(the configuration file).

For that reason I’m asking if you would be willing to accept a solution
where we introduce a new keyword in the configuration file:

dom0less=<dtb> OR domu_guests=<dtb> OR I’m open to suggestion.

Where the pointed dtb contains the domU domains:

/dts-v1/;

/ {
    /* #*cells are here to keep DTC happy */
    #address-cells = <2>;
    #size-cells = <2>;

    domU1 {
           #size-cells = <0x1>; 
           #address-cells = <0x1>;
           compatible = "xen,domain”;
           cpus = <1>;
           memory = <0 0xC0000>;

           module@1 {
               compatible = "multiboot,kernel", "multiboot,module”;
               bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
               uefi,binary = “domU_kernel1.bin"
           };

           module@2 {
               compatible = “multiboot,ramdisk", "multiboot,module”;
               uefi,binary = “domU_ramdisk1.bin"
           };

           module@3 {
               compatible = "multiboot,device-tree", "multiboot,module”;
               uefi,binary = “domU_passthrough1.dtb"
           }; 
    };

    domU2 { <as above> };

};


So that the user cases we discussed are valid:

1) Start Xen and load Dom0:
   
   Xen.cfg:
   [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=<xen DTB>

2) Start Xen and load only domUs (true dom0less)

   Xen.cfg:
   [global]
   default=xen

   [xen]
   options=<Xen command line>
   dom0less=<dom0less DTB>
   dtb=<xen DTB>

3) Start Xen and load Dom0 and DomUs

   Xen.cfg:
   [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
   dom0less=<dom0less DTB>
   dtb=<xen DTB>


With this change we will be consistent across x86 and arm UEFI boot
start procedure, we won’t touch the current check on multiboot,modules
because it will be valid, we will have a way to boot dom0less and it
requires less testing for the changing in the common code.

Please let me know what do you think about that.

Cheers,
Luca

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

[Stefano Stabellini]

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

On Tue, 21 Sep 2021, Luca Fancellu wrote:
> > On 17 Sep 2021, at 23:33, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > On Fri, 17 Sep 2021, Luca Fancellu wrote:
> >>> On 16 Sep 2021, at 21:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >>> 
> >>> On Thu, 16 Sep 2021, Jan Beulich wrote:
> >>>> On 16.09.2021 17:07, Luca Fancellu wrote:
> >>>>> I explain here my understanding on dom0less, this feature is used to start domUs at
> >>>>> Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
> >>>>> 
> >>>>> So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
> >>>>> doesn’t have to be skipped.
> >>>>> 
> >>>>> Here the possible user cases:
> >>>>> 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
> >>>>> 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
> >>>>> 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]
> >>>> 
> >>>> If that's the intention - fine. Stefano?
> >>> 
> >> 
> >> Hi Stefano,
> >> 
> >>> What do you mean by dom0 modules embedded in the Xen image? I am not
> >>> familiar with it, but I imagine it doesn't involve any multiboot,module
> >>> nodes in device tree, right?
> >>> 
> >>> Putting aside "dom0 modules embedded in Xen image" that I didn't fully
> >>> understand, there are three ways to load Dom0:
> >>> 
> >>> - dom0 kernel (and ramdisk, optional) on xen.cfg
> >>> - dom0 kernel (and ramdisk, optional) on device tree using the "reg" property
> >>> - dom0 kernel (and ramdisk, optional) on device tree using the "uefi,binary" property
> >> 
> >> True for the #1 and #2, the last one is not implemented. The uefi,binary property
> >> for now is only used to load domU modules.
> > 
> > Yeah, it is no problem that is not currently implemented, but from a
> > device tree binding / efi interface perspective it should be possible.
> > 
> > 
> >>> Then, the other use cases are:
> >>> 
> >>> - true dom0less, domUs on device tree using the "reg" property
> >>> - true dom0less, domUs on device tree using the "uefi,binary" property
> >>> 
> >>> And of course all the possible combinations between Dom0 and DomU
> >>> loading.
> >>> 
> >>> 
> >>> Currently, patch #1 checks for the presence of a Dom0 kernel node and, if
> >>> present, it decides not to proceed with xen.cfg. So if the Dom0 kernel
> >>> node is *not* present, efi_arch_use_config_file returns true.
> >>> 
> >>> However, this could be a true dom0less configuration without any Dom0
> >>> kernel. If so, you might not want to load xen.cfg at all because it is
> >>> not needed. In a true dom0less configuration, we probably want
> >>> efi_arch_use_config_file to return false.
> >> 
> >> In a true dom0less configuration we might need to read xen.cfg to retrieve the
> >> Xen command line, 
> > 
> > The Xen command line could also be on device tree
> > (/chosen/xen,xen-bootargs).
> > 
> > 
> >> but following the actual implementation of the common code
> >> there is more. I’m going to explain.
> >> 
> >> What efi_arch_use_config_file really does is not only choosing to read xen.cfg
> >> or not. Following the common code (xen/common/efi/boot.c) and what its result activate
> >> along the path, it basically decides if the UEFI stub is used as a loader from filesystem
> >> or not. We need the UEFI stub as a loader to be 100% UEFI and load our modules.
> >> 
> >> The original check basically says “if there are multiboot,module in the DT, then some
> >> bootloader has loaded in memory the required modules so I’m not gonna load anything
> >> from the filesystem because I assume it puts everything in place for me to boot.”
> > 
> > OK, I am following. It looks like this is the source of the issue.
> > 
> > 
> >>> From misc/efi.txt:
> >> When booted as an EFI application, Xen requires a configuration file as described below unless a bootloader,
> >> such as GRUB, has 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.
> >> 
> >> What I’m doing in patch #1 is restricting that check to just the multiboot,module that are
> >> Dom0 module, why? Because with the introduction of dom0less we need to specify
> >> multiboot,modules for domUs, but the presence or not of dom0 modules is the only
> >> Information we need to understand if the user decided to start Xen with everything
> >> in places (modules in memory, xen command line, dtb) or if the job is demanded to the
> >> UEFI stub and its configuration file.
> > 
> > I don't think so. Imagine a case where the user has everything in device
> > tree, doesn't need xen.cfg, but dom0 and domUs are specified as
> > uefi,binary.
> > 
> > We don't want xen.cfg but we do want to be able to load files from the
> > filesystem. This might not be currently implemented but from an bindings
> > perspective it should be possible.
> > 
> > 
> >> By the configuration file you can also load in memory the Xen dtb, so Xen can
> >> be started as an EFI application without the DTB and then load it using the EFI stub.
> > 
> > This can be very useful but it would follow the !fdt check and return
> > true from efi_arch_use_config_file. So it doesn't really conflict with
> > anything we would around multiboot,module and xen,cfg-loading.
> > 
> > 
> >> I’m not against this new property “xen,cfg-loading”, I just think it is not needed because
> >> we have all the information we need without it and in any case we need to read the
> >> configuration file because otherwise we won’t have access to the Xen command line.
> > 
> > We don't necessarely need to read the Xen command line from xen.cfg :-)
> > 
> > 
> >> Now I’m going to show you examples of all use cases that are valid with the introduction
> >> of this serie:
> >> 
> >> 1) Start Xen as EFI application and load only Dom0
> >> 
> >>    Xen.cfg:
> >>    [global]
> >>    default=xen_dom0
> >> 
> >>    [xen_dom0]
> >>    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=<xen DTB>
> >> 
> >>    DT:
> >>    {no modification}
> >> 
> >> 2) Start Xen as EFI application to load a true dom0less setup
> >> 
> >>    Xen.cfg:
> >>    [global]
> >>    default=xen_true_dom0less
> >> 
> >>    [xen_true_dom0less]
> >>    options=<Xen command line>
> >>    dtb=<xen DTB>
> >> 
> >>    DT:
> >>    chosen {
> >>        #size-cells = <0x1>;
> >> 	#address-cells = <0x1>;
> >> 
> >> 	domU1 {
> >>            #size-cells = <0x1>; 
> >>            #address-cells = <0x1>;
> >>            compatible = "xen,domain”;
> >>            cpus = <1>;
> >>            memory = <0 0xC0000>;
> >> 
> >>            module@1 {
> >>                compatible = "multiboot,kernel", "multiboot,module”;
> >>                bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
> >>                uefi,binary = “domU_kernel1.bin"
> >>            };
> >> 
> >>            module@2 {
> >>                compatible = “multiboot,ramdisk", "multiboot,module”;
> >>                uefi,binary = “domU_ramdisk1.bin"
> >>            };
> >> 
> >>            module@3 {
> >>                compatible = "multiboot,device-tree", "multiboot,module”;
> >>                uefi,binary = “domU_passthrough1.dtb"
> >>            }; 
> >>        };
> >> 
> >>        domU2 { <as above> };
> >>    }
> >> 
> >> 3) Start Xen as EFI application to load Dom0 and DomUs
> >> 
> >>    Xen.cfg:
> >>    [global]
> >>    default=xen_dom0_domUs
> >> 
> >>    [xen_dom0_domUs]
> >>    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=<xen DTB>
> >> 
> >>    DT:
> >>    chosen {
> >>        #size-cells = <0x1>;
> >> 	#address-cells = <0x1>;
> >> 
> >> 	domU1 {
> >>            #size-cells = <0x1>; 
> >>            #address-cells = <0x1>;
> >>            compatible = "xen,domain”;
> >>            cpus = <1>;
> >>            memory = <0 0xC0000>;
> >> 
> >>            module@1 {
> >>                compatible = "multiboot,kernel", "multiboot,module”;
> >>                bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
> >>                uefi,binary = “domU_kernel1.bin"
> >>            };
> >> 
> >>            module@2 {
> >>                compatible = “multiboot,ramdisk", "multiboot,module”;
> >>                uefi,binary = “domU_ramdisk1.bin"
> >>            };
> >> 
> >>            module@3 {
> >>                compatible = "multiboot,device-tree", "multiboot,module”;
> >>                uefi,binary = “domU_passthrough1.dtb"
> >>            }; 
> >>        };
> >> 
> >>        domU2 { <as above> };
> >>    }
> >> 
> >> So as you see every case is covered without the introduction of the
> >> property.
> >> 
> >> Please let me know what do you think.
> > 
> 
> Hi Stefano,
> 
> > I think that from an interface perspective (not a code perspective) we
> > need to be able to account for cases like:
> > 
> > 4) Start Xen as EFI application and load only Dom0
> > no Xen.cfg
> > DT:
> >  xen,xen-bootargs
> >  dom0/uefi,binary
> >  domUs/uefi,binary
> > 
> > 
> > But in any case, even disregarding this case, past experience has taught
> > me that it is always better to have an explicit flag to trigger a new
> > behavior, rather than relying on "guesswork". If we introduce
> > "xen,cfg-loading", we are going to be a lot more future-proof that if we
> > don't introduce it in terms of backward and forward compatibility in
> > case we need to change anything.
> 
> I see your point, for sure the DT is a more powerful tool than the simple
> text configuration file and it would be the best interface.
> However I think we are moving into the direction where x86 and arm
> are going to diverge even if we can have a common interface for them
> (the configuration file).

Consider that the configuration file is not the only interface to Xen
any longer. There is also HyperLaunch and I was trying to align to it:
https://marc.info/?l=xen-devel&m=162096368626246 The DT-based approached
would be very well aligned with HyperLaunch.


> For that reason I’m asking if you would be willing to accept a solution
> where we introduce a new keyword in the configuration file:
> 
> dom0less=<dtb> OR domu_guests=<dtb> OR I’m open to suggestion.
> 
> Where the pointed dtb contains the domU domains:
> 
> /dts-v1/;
> 
> / {
>     /* #*cells are here to keep DTC happy */
>     #address-cells = <2>;
>     #size-cells = <2>;
> 
>     domU1 {
>            #size-cells = <0x1>; 
>            #address-cells = <0x1>;
>            compatible = "xen,domain”;
>            cpus = <1>;
>            memory = <0 0xC0000>;
> 
>            module@1 {
>                compatible = "multiboot,kernel", "multiboot,module”;
>                bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>                uefi,binary = “domU_kernel1.bin"
>            };
> 
>            module@2 {
>                compatible = “multiboot,ramdisk", "multiboot,module”;
>                uefi,binary = “domU_ramdisk1.bin"
>            };
> 
>            module@3 {
>                compatible = "multiboot,device-tree", "multiboot,module”;
>                uefi,binary = “domU_passthrough1.dtb"
>            }; 
>     };
> 
>     domU2 { <as above> };
> 
> };
> 
> 
> So that the user cases we discussed are valid:
> 
> 1) Start Xen and load Dom0:
>    
>    Xen.cfg:
>    [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=<xen DTB>
> 
> 2) Start Xen and load only domUs (true dom0less)
> 
>    Xen.cfg:
>    [global]
>    default=xen
> 
>    [xen]
>    options=<Xen command line>
>    dom0less=<dom0less DTB>
>    dtb=<xen DTB>
> 
> 3) Start Xen and load Dom0 and DomUs
> 
>    Xen.cfg:
>    [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
>    dom0less=<dom0less DTB>
>    dtb=<xen DTB>
> 
> 
> With this change we will be consistent across x86 and arm UEFI boot
> start procedure, we won’t touch the current check on multiboot,modules
> because it will be valid, we will have a way to boot dom0less and it
> requires less testing for the changing in the common code.
> 
> Please let me know what do you think about that.

My order of preference from best to worst is:
1) my previous suggestion, e.g. xen,cfg-loading
2) your previous suggestion, e.g. change the multiboot,modules check
   without adding xen,cfg-loading
3) this proposal


Let me explain the reason behind this. This proposal still requires a
device tree but it has to be loaded from the config file, which is
limiting compared to the other approaches. From a user perspective is
just as complex (just as difficult to write) but less flexible because
it prevents using the device tree alone for UEFI booting in the future.
Given that with the two other alternatives the config file could still
be used anyway, I don't think that adding the "dom0less" parameters to
the config file buys us much in terms of alignment with x86. This is
why this is my least favorite option.

Your previous approach is actually quite good. Same complexity but much
more flexible. Similar alignment with x86 in my view. The only
correction I suggested is the addition of xen,cfg-loading to make the
efi_arch_use_config_file check more robust. However, I still prefer your
prevous approach even without xen,cfg-loading.


Let me make one more suggestion based on your previous approach (I mean
this version of the patch series):

- You have already removed the panic for ARM in case a dom0 kernel is
  not specifid in patch #2. We can go farther than that and make the
  absence of xen.cfg not a fatal failure on ARM because it is fine not
  to have dom0 in true dom0less configurations and the xen cmdline is
  optional anyway.

- If the absence of xen.cfg is not a fatal failure, then we don't need
  efi_arch_use_config_file anymore. Let's try to load xen.cfg always. We
  error out if xen.cfg specifies a dom0 kernel and we already have a
  dom0 kernel in DT.

- In the future a "don't load xen.cfg" option (the opposite of
  xen,cfg-loading) could be added but it is not necessary now


This should be a minimal change compared to this version of the patch
series, enable all the use-cases you care about, and also be more
flexible for the future.

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

[Luca Fancellu]

> On 21 Sep 2021, at 22:34, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 21 Sep 2021, Luca Fancellu wrote:
>>> On 17 Sep 2021, at 23:33, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> On Fri, 17 Sep 2021, Luca Fancellu wrote:
>>>>> On 16 Sep 2021, at 21:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>> 
>>>>> On Thu, 16 Sep 2021, Jan Beulich wrote:
>>>>>> On 16.09.2021 17:07, Luca Fancellu wrote:
>>>>>>> I explain here my understanding on dom0less, this feature is used to start domUs at
>>>>>>> Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be absent.
>>>>>>> 
>>>>>>> So if you have a dom0 kernel embed in the image, it's completely fine to start it and it 
>>>>>>> doesn’t have to be skipped.
>>>>>>> 
>>>>>>> Here the possible user cases:
>>>>>>> 1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
>>>>>>> 2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in Xen image, domUs on DT]
>>>>>>> 3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and domUs on DT]
>>>>>> 
>>>>>> If that's the intention - fine. Stefano?
>>>>> 
>>>> 
>>>> Hi Stefano,
>>>> 
>>>>> What do you mean by dom0 modules embedded in the Xen image? I am not
>>>>> familiar with it, but I imagine it doesn't involve any multiboot,module
>>>>> nodes in device tree, right?
>>>>> 
>>>>> Putting aside "dom0 modules embedded in Xen image" that I didn't fully
>>>>> understand, there are three ways to load Dom0:
>>>>> 
>>>>> - dom0 kernel (and ramdisk, optional) on xen.cfg
>>>>> - dom0 kernel (and ramdisk, optional) on device tree using the "reg" property
>>>>> - dom0 kernel (and ramdisk, optional) on device tree using the "uefi,binary" property
>>>> 
>>>> True for the #1 and #2, the last one is not implemented. The uefi,binary property
>>>> for now is only used to load domU modules.
>>> 
>>> Yeah, it is no problem that is not currently implemented, but from a
>>> device tree binding / efi interface perspective it should be possible.
>>> 
>>> 
>>>>> Then, the other use cases are:
>>>>> 
>>>>> - true dom0less, domUs on device tree using the "reg" property
>>>>> - true dom0less, domUs on device tree using the "uefi,binary" property
>>>>> 
>>>>> And of course all the possible combinations between Dom0 and DomU
>>>>> loading.
>>>>> 
>>>>> 
>>>>> Currently, patch #1 checks for the presence of a Dom0 kernel node and, if
>>>>> present, it decides not to proceed with xen.cfg. So if the Dom0 kernel
>>>>> node is *not* present, efi_arch_use_config_file returns true.
>>>>> 
>>>>> However, this could be a true dom0less configuration without any Dom0
>>>>> kernel. If so, you might not want to load xen.cfg at all because it is
>>>>> not needed. In a true dom0less configuration, we probably want
>>>>> efi_arch_use_config_file to return false.
>>>> 
>>>> In a true dom0less configuration we might need to read xen.cfg to retrieve the
>>>> Xen command line, 
>>> 
>>> The Xen command line could also be on device tree
>>> (/chosen/xen,xen-bootargs).
>>> 
>>> 
>>>> but following the actual implementation of the common code
>>>> there is more. I’m going to explain.
>>>> 
>>>> What efi_arch_use_config_file really does is not only choosing to read xen.cfg
>>>> or not. Following the common code (xen/common/efi/boot.c) and what its result activate
>>>> along the path, it basically decides if the UEFI stub is used as a loader from filesystem
>>>> or not. We need the UEFI stub as a loader to be 100% UEFI and load our modules.
>>>> 
>>>> The original check basically says “if there are multiboot,module in the DT, then some
>>>> bootloader has loaded in memory the required modules so I’m not gonna load anything
>>>> from the filesystem because I assume it puts everything in place for me to boot.”
>>> 
>>> OK, I am following. It looks like this is the source of the issue.
>>> 
>>> 
>>>>> From misc/efi.txt:
>>>> When booted as an EFI application, Xen requires a configuration file as described below unless a bootloader,
>>>> such as GRUB, has 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.
>>>> 
>>>> What I’m doing in patch #1 is restricting that check to just the multiboot,module that are
>>>> Dom0 module, why? Because with the introduction of dom0less we need to specify
>>>> multiboot,modules for domUs, but the presence or not of dom0 modules is the only
>>>> Information we need to understand if the user decided to start Xen with everything
>>>> in places (modules in memory, xen command line, dtb) or if the job is demanded to the
>>>> UEFI stub and its configuration file.
>>> 
>>> I don't think so. Imagine a case where the user has everything in device
>>> tree, doesn't need xen.cfg, but dom0 and domUs are specified as
>>> uefi,binary.
>>> 
>>> We don't want xen.cfg but we do want to be able to load files from the
>>> filesystem. This might not be currently implemented but from an bindings
>>> perspective it should be possible.
>>> 
>>> 
>>>> By the configuration file you can also load in memory the Xen dtb, so Xen can
>>>> be started as an EFI application without the DTB and then load it using the EFI stub.
>>> 
>>> This can be very useful but it would follow the !fdt check and return
>>> true from efi_arch_use_config_file. So it doesn't really conflict with
>>> anything we would around multiboot,module and xen,cfg-loading.
>>> 
>>> 
>>>> I’m not against this new property “xen,cfg-loading”, I just think it is not needed because
>>>> we have all the information we need without it and in any case we need to read the
>>>> configuration file because otherwise we won’t have access to the Xen command line.
>>> 
>>> We don't necessarely need to read the Xen command line from xen.cfg :-)
>>> 
>>> 
>>>> Now I’m going to show you examples of all use cases that are valid with the introduction
>>>> of this serie:
>>>> 
>>>> 1) Start Xen as EFI application and load only Dom0
>>>> 
>>>>   Xen.cfg:
>>>>   [global]
>>>>   default=xen_dom0
>>>> 
>>>>   [xen_dom0]
>>>>   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=<xen DTB>
>>>> 
>>>>   DT:
>>>>   {no modification}
>>>> 
>>>> 2) Start Xen as EFI application to load a true dom0less setup
>>>> 
>>>>   Xen.cfg:
>>>>   [global]
>>>>   default=xen_true_dom0less
>>>> 
>>>>   [xen_true_dom0less]
>>>>   options=<Xen command line>
>>>>   dtb=<xen DTB>
>>>> 
>>>>   DT:
>>>>   chosen {
>>>>       #size-cells = <0x1>;
>>>> 	#address-cells = <0x1>;
>>>> 
>>>> 	domU1 {
>>>>           #size-cells = <0x1>; 
>>>>           #address-cells = <0x1>;
>>>>           compatible = "xen,domain”;
>>>>           cpus = <1>;
>>>>           memory = <0 0xC0000>;
>>>> 
>>>>           module@1 {
>>>>               compatible = "multiboot,kernel", "multiboot,module”;
>>>>               bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>>>>               uefi,binary = “domU_kernel1.bin"
>>>>           };
>>>> 
>>>>           module@2 {
>>>>               compatible = “multiboot,ramdisk", "multiboot,module”;
>>>>               uefi,binary = “domU_ramdisk1.bin"
>>>>           };
>>>> 
>>>>           module@3 {
>>>>               compatible = "multiboot,device-tree", "multiboot,module”;
>>>>               uefi,binary = “domU_passthrough1.dtb"
>>>>           }; 
>>>>       };
>>>> 
>>>>       domU2 { <as above> };
>>>>   }
>>>> 
>>>> 3) Start Xen as EFI application to load Dom0 and DomUs
>>>> 
>>>>   Xen.cfg:
>>>>   [global]
>>>>   default=xen_dom0_domUs
>>>> 
>>>>   [xen_dom0_domUs]
>>>>   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=<xen DTB>
>>>> 
>>>>   DT:
>>>>   chosen {
>>>>       #size-cells = <0x1>;
>>>> 	#address-cells = <0x1>;
>>>> 
>>>> 	domU1 {
>>>>           #size-cells = <0x1>; 
>>>>           #address-cells = <0x1>;
>>>>           compatible = "xen,domain”;
>>>>           cpus = <1>;
>>>>           memory = <0 0xC0000>;
>>>> 
>>>>           module@1 {
>>>>               compatible = "multiboot,kernel", "multiboot,module”;
>>>>               bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>>>>               uefi,binary = “domU_kernel1.bin"
>>>>           };
>>>> 
>>>>           module@2 {
>>>>               compatible = “multiboot,ramdisk", "multiboot,module”;
>>>>               uefi,binary = “domU_ramdisk1.bin"
>>>>           };
>>>> 
>>>>           module@3 {
>>>>               compatible = "multiboot,device-tree", "multiboot,module”;
>>>>               uefi,binary = “domU_passthrough1.dtb"
>>>>           }; 
>>>>       };
>>>> 
>>>>       domU2 { <as above> };
>>>>   }
>>>> 
>>>> So as you see every case is covered without the introduction of the
>>>> property.
>>>> 
>>>> Please let me know what do you think.
>>> 
>> 
>> Hi Stefano,
>> 
>>> I think that from an interface perspective (not a code perspective) we
>>> need to be able to account for cases like:
>>> 
>>> 4) Start Xen as EFI application and load only Dom0
>>> no Xen.cfg
>>> DT:
>>> xen,xen-bootargs
>>> dom0/uefi,binary
>>> domUs/uefi,binary
>>> 
>>> 
>>> But in any case, even disregarding this case, past experience has taught
>>> me that it is always better to have an explicit flag to trigger a new
>>> behavior, rather than relying on "guesswork". If we introduce
>>> "xen,cfg-loading", we are going to be a lot more future-proof that if we
>>> don't introduce it in terms of backward and forward compatibility in
>>> case we need to change anything.
>> 
>> I see your point, for sure the DT is a more powerful tool than the simple
>> text configuration file and it would be the best interface.
>> However I think we are moving into the direction where x86 and arm
>> are going to diverge even if we can have a common interface for them
>> (the configuration file).
> 
> Consider that the configuration file is not the only interface to Xen
> any longer. There is also HyperLaunch and I was trying to align to it:
> https://marc.info/?l=xen-devel&m=162096368626246 The DT-based approached
> would be very well aligned with HyperLaunch.
> 
> 
>> For that reason I’m asking if you would be willing to accept a solution
>> where we introduce a new keyword in the configuration file:
>> 
>> dom0less=<dtb> OR domu_guests=<dtb> OR I’m open to suggestion.
>> 
>> Where the pointed dtb contains the domU domains:
>> 
>> /dts-v1/;
>> 
>> / {
>>    /* #*cells are here to keep DTC happy */
>>    #address-cells = <2>;
>>    #size-cells = <2>;
>> 
>>    domU1 {
>>           #size-cells = <0x1>; 
>>           #address-cells = <0x1>;
>>           compatible = "xen,domain”;
>>           cpus = <1>;
>>           memory = <0 0xC0000>;
>> 
>>           module@1 {
>>               compatible = "multiboot,kernel", "multiboot,module”;
>>               bootargs = "console=ttyAMA0 root=/dev/ram0 rw”;
>>               uefi,binary = “domU_kernel1.bin"
>>           };
>> 
>>           module@2 {
>>               compatible = “multiboot,ramdisk", "multiboot,module”;
>>               uefi,binary = “domU_ramdisk1.bin"
>>           };
>> 
>>           module@3 {
>>               compatible = "multiboot,device-tree", "multiboot,module”;
>>               uefi,binary = “domU_passthrough1.dtb"
>>           }; 
>>    };
>> 
>>    domU2 { <as above> };
>> 
>> };
>> 
>> 
>> So that the user cases we discussed are valid:
>> 
>> 1) Start Xen and load Dom0:
>> 
>>   Xen.cfg:
>>   [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=<xen DTB>
>> 
>> 2) Start Xen and load only domUs (true dom0less)
>> 
>>   Xen.cfg:
>>   [global]
>>   default=xen
>> 
>>   [xen]
>>   options=<Xen command line>
>>   dom0less=<dom0less DTB>
>>   dtb=<xen DTB>
>> 
>> 3) Start Xen and load Dom0 and DomUs
>> 
>>   Xen.cfg:
>>   [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
>>   dom0less=<dom0less DTB>
>>   dtb=<xen DTB>
>> 
>> 
>> With this change we will be consistent across x86 and arm UEFI boot
>> start procedure, we won’t touch the current check on multiboot,modules
>> because it will be valid, we will have a way to boot dom0less and it
>> requires less testing for the changing in the common code.
>> 
>> Please let me know what do you think about that.
> 

Hi Stefano,

> My order of preference from best to worst is:
> 1) my previous suggestion, e.g. xen,cfg-loading

Thank you now I have the big picture, I will introduce the xen,cfg-load
In the v2 serie.

Cheers,
Luca

> 2) your previous suggestion, e.g. change the multiboot,modules check
>   without adding xen,cfg-loading
> 3) this proposal
> 
> 
> Let me explain the reason behind this. This proposal still requires a
> device tree but it has to be loaded from the config file, which is
> limiting compared to the other approaches. From a user perspective is
> just as complex (just as difficult to write) but less flexible because
> it prevents using the device tree alone for UEFI booting in the future.
> Given that with the two other alternatives the config file could still
> be used anyway, I don't think that adding the "dom0less" parameters to
> the config file buys us much in terms of alignment with x86. This is
> why this is my least favorite option.
> 
> Your previous approach is actually quite good. Same complexity but much
> more flexible. Similar alignment with x86 in my view. The only
> correction I suggested is the addition of xen,cfg-loading to make the
> efi_arch_use_config_file check more robust. However, I still prefer your
> prevous approach even without xen,cfg-loading.
> 
> 
> Let me make one more suggestion based on your previous approach (I mean
> this version of the patch series):
> 
> - You have already removed the panic for ARM in case a dom0 kernel is
>  not specifid in patch #2. We can go farther than that and make the
>  absence of xen.cfg not a fatal failure on ARM because it is fine not
>  to have dom0 in true dom0less configurations and the xen cmdline is
>  optional anyway.
> 
> - If the absence of xen.cfg is not a fatal failure, then we don't need
>  efi_arch_use_config_file anymore. Let's try to load xen.cfg always. We
>  error out if xen.cfg specifies a dom0 kernel and we already have a
>  dom0 kernel in DT.
> 
> - In the future a "don't load xen.cfg" option (the opposite of
>  xen,cfg-loading) could be added but it is not necessary now
> 
> 
> This should be a minimal change compared to this version of the patch
> series, enable all the use-cases you care about, and also be more
> flexible for the future.