Re: [PATCH v4 1/3] arm/efi: Introduce xen,uefi-cfg-load DT property

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

Sorry for jumping late in the conversation. While skimming through what 
has been committed, I noticed one potential issue in this patch and have 
also a question.
On 30/09/2021 16:28, Luca Fancellu wrote:
> Introduce the xen,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 xen,uefi-cfg-load property is used to see
> if the UEFI Xen configuration file is needed.
>
> Modify a comment in efi_arch_use_config_file, removing
> the part that states "dom0 required" because it's not
> true anymore with this commit.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
> v4 changes:
> - modify property name to xen,uefi-cfg-load
> v3 changes:
> - add documentation to misc/arm/device-tree/booting.txt
> - Modified variable name and logic from skip_cfg_file to
> load_cfg_file
> - Add in the commit message that I'm modifying a comment.
> v2 changes:
> - Introduced uefi,cfg-load property
> - Add documentation about the property
> ---
>   docs/misc/arm/device-tree/booting.txt |  8 ++++++++
>   docs/misc/efi.pandoc                  |  2 ++
>   xen/arch/arm/efi/efi-boot.h           | 28 ++++++++++++++++++++++-----
>   3 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index 44cd9e1a9a..352b0ec43a 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -121,6 +121,14 @@ A Xen-aware bootloader would set xen,xen-bootargs for Xen, 
> xen,dom0-bootargs
>   for Dom0 and bootargs for native Linux.
>   
>   
> +UEFI boot and DT
> +================
> +
> +When Xen is booted using UEFI, it doesn't read the configuration file if any
> +multiboot module is specified. To force Xen to load the configuration file, the
> +boolean property xen,uefi-cfg-load must be declared in the /chosen node.
> +
> +
>   Creating Multiple Domains directly from Xen
>   ===========================================
>   
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index ac3cd58cae..ed85351541 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 "xen,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 this wants to be clarified. Lets imagine both the Device-Tree 
and the cfg provides a kernel. Which one will get used?

>   
>   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..a3e46453d4 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -581,22 +581,40 @@ static void __init 
> efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>   
>   static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>   {
> +    bool load_cfg_file = true;
>       /*
>        * For arm, we may get a device tree from GRUB (or other bootloader)
>        * that contains modules that have already been loaded into memory.  In
> -     * this case, we do not use a configuration file, and rely on the
> -     * bootloader to have loaded all required modules and appropriate
> -     * options.
> +     * this case, we search for the property xen,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 )
AFAICT, fdt_node_offset_by_compatible expects 'fdt' to be non-NULL. 
However, lookup_fdt_config_table() may return NULL on platform with no 
Device-Tree (server tends to be ACPI only). So wouldn't this result to 
dereference NULL and crash?
> +    {
> +        /* 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 xen,uefi-cfg-load property exists */
> +            cfg_load_prop = fdt_getprop(fdt, node, "xen,uefi-cfg-load",
> +                                        &cfg_load_len);
> +            if ( !cfg_load_prop )
> +                load_cfg_file = false;
> +        }
> +    }
> +
> +    if ( !fdt || load_cfg_file )
>       {
>           /*
>            * We either have no FDT, or one without modules, so we must have a
> -         * Xen EFI configuration file to specify modules.  (dom0 required)
> +         * Xen EFI configuration file to specify modules.
>            */
>           return true;
>       }
Cheers,

--
Julien Grall