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

[Stefano Stabellini <sstabellini@kernel.org>]

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?