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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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

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


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

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

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


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

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

    bool load_cfg_file = true;

?


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

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


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