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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 6 Oct 2021, at 19:32, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.
> 

Hi Julien,

> I think this wants to be clarified. Lets imagine both the Device-Tree and the 
> cfg provides a kernel. Which one will get used?

Yes this is will lead to a device tree where there will be two multiboot module 
for the dom0 kernel, I guess the one really loaded
will be the first multiboot kernel node processed and appended to the boot 
modules.

I see this is a possible issue and this is handled in the third patch of the 
serie.


> 
> 
>>    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?

Thanks for spotting that, I will push a patch to fix it, something like this 
should be ok:

+    if ( fdt && (fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 
0) )

Sorry I didn’t spotted earlier.

Cheers,
Luca

> 
>> +    {
>> +        /* 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