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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 30 Sep 2021, Bertrand Marquis wrote:
> Hi Luca,
> 
> > On 30 Sep 2021, at 15:28, Luca Fancellu <Luca.Fancellu@xxxxxxx> 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>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

I committed this patch


> > ---
> > 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.
> > 
> > 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 )
> > +    {
> > +        /* 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;
> >     }
> > -- 
> > 2.17.1
> > 
>