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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 29 Sep 2021, Luca Fancellu wrote:
> > On Tue, 28 Sep 2021, Luca Fancellu wrote:
> >> Introduce the uefi,cfg-load DT property of /chosen
> >> node for ARM whose presence decide whether to force
> >> the load of the UEFI Xen configuration file.
> >> 
> >> The logic is that if any multiboot,module is found in
> >> the DT, then the uefi,cfg-load property is used to see
> >> if the UEFI Xen configuration file is needed.
> >> 
> >> Modify a comment in efi_arch_use_config_file, removing
> >> the part that states "dom0 required" because it's not
> >> true anymore with this commit.
> >> 
> >> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > 
> > The patch looks good. Only one minor change: given that this is a Xen
> > parameter that we are introducing and not a parameter defined by UEFI
> > Forum, I think uefi,cfg-load should be called xen,uefi-cfg-load instead.
> > Because "xen," is our prefix, while "uefi," is not.
> > 
> > With that minor change:
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> Yes I will rename it.
> 
> > 
> > 
> > Note that the uefi,binary property is different because that property is
> > for xen,domain nodes, so we are already in a Xen specific namespace when
> > we are using it. Instead this property is for /chosen which is not a Xen
> > specific node.
> 
> Given that uefi,binary will be used also for multiboot,module directly under 
> /chosen
> for Dom0, do you think I should rename also that to xen,uefi-binary?

Yes, maybe to stay on the safe side, it would be better to also rename
uefi,binary to xen,uefi-binary.



> >> ---
> >> v3 changes:
> >> - add documentation to misc/arm/device-tree/booting.txt
> >> - Modified variable name and logic from skip_cfg_file to
> >> load_cfg_file
> >> - Add in the commit message that I'm modifying a comment.
> >> v2 changes:
> >> - Introduced uefi,cfg-load property
> >> - Add documentation about the property
> >> ---
> >> docs/misc/arm/device-tree/booting.txt |  8 ++++++++
> >> docs/misc/efi.pandoc                  |  2 ++
> >> xen/arch/arm/efi/efi-boot.h           | 28 ++++++++++++++++++++++-----
> >> 3 files changed, 33 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/docs/misc/arm/device-tree/booting.txt 
> >> b/docs/misc/arm/device-tree/booting.txt
> >> index 44cd9e1a9a..cf878b478e 100644
> >> --- a/docs/misc/arm/device-tree/booting.txt
> >> +++ b/docs/misc/arm/device-tree/booting.txt
> >> @@ -121,6 +121,14 @@ A Xen-aware bootloader would set xen,xen-bootargs for 
> >> Xen, xen,dom0-bootargs
> >> for Dom0 and bootargs for native Linux.
> >> 
> >> 
> >> +UEFI boot and DT
> >> +================
> >> +
> >> +When Xen is booted using UEFI, it doesn't read the configuration file if 
> >> any
> >> +multiboot module is specified. To force Xen to load the configuration 
> >> file, the
> >> +boolean property uefi,cfg-load must be declared in the /chosen node.
> >> +
> >> +
> >> Creating Multiple Domains directly from Xen
> >> ===========================================
> >> 
> >> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> >> index ac3cd58cae..e289c5e7ba 100644
> >> --- a/docs/misc/efi.pandoc
> >> +++ b/docs/misc/efi.pandoc
> >> @@ -14,6 +14,8 @@ loaded the modules and describes them in the device tree 
> >> provided to Xen.  If a
> >> bootloader provides a device tree containing modules then any configuration
> >> files are ignored, and the bootloader is responsible for populating all
> >> relevant device tree nodes.
> >> +The property "uefi,cfg-load" can be specified in the /chosen node to 
> >> force Xen
> >> +to load the configuration file even if multiboot modules are found.
> >> 
> >> Once built, `make install-xen` will place the resulting binary directly 
> >> into
> >> the EFI boot partition, provided `EFI_VENDOR` is set in the environment 
> >> (and
> >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> >> index cf9c37153f..4f1b01757d 100644
> >> --- a/xen/arch/arm/efi/efi-boot.h
> >> +++ b/xen/arch/arm/efi/efi-boot.h
> >> @@ -581,22 +581,40 @@ static void __init 
> >> efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
> >> 
> >> static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> >> {
> >> +    bool load_cfg_file = true;
> >>     /*
> >>      * For arm, we may get a device tree from GRUB (or other bootloader)
> >>      * that contains modules that have already been loaded into memory.  In
> >> -     * this case, we do not use a configuration file, and rely on the
> >> -     * bootloader to have loaded all required modules and appropriate
> >> -     * options.
> >> +     * this case, we search for the property uefi,cfg-load in the /chosen 
> >> node
> >> +     * to decide whether to skip the UEFI Xen configuration file or not.
> >>      */
> >> 
> >>     fdt = lookup_fdt_config_table(SystemTable);
> >>     dtbfile.ptr = fdt;
> >>     dtbfile.need_to_free = false; /* Config table memory can't be freed. */
> >> -    if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, 
> >> "multiboot,module") < 0 )
> >> +
> >> +    if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 )
> >> +    {
> >> +        /* Locate chosen node */
> >> +        int node = fdt_subnode_offset(fdt, 0, "chosen");
> >> +        const void *cfg_load_prop;
> >> +        int cfg_load_len;
> >> +
> >> +        if ( node > 0 )
> >> +        {
> >> +            /* Check if uefi,cfg-load property exists */
> >> +            cfg_load_prop = fdt_getprop(fdt, node, "uefi,cfg-load",
> >> +                                        &cfg_load_len);
> >> +            if ( !cfg_load_prop )
> >> +                load_cfg_file = false;
> >> +        }
> >> +    }
> >> +
> >> +    if ( !fdt || load_cfg_file )
> >>     {
> >>         /*
> >>          * We either have no FDT, or one without modules, so we must have a
> >> -         * Xen EFI configuration file to specify modules.  (dom0 required)
> >> +         * Xen EFI configuration file to specify modules.
> >>          */
> >>         return true;
> >>     }
> >> -- 
> >> 2.17.1
> >> 
> 
>