Re: [PATCH 1/2] xen/efi: Restrict check for DT boot modules on EFI boot

[Luca Fancellu <luca.fancellu@arm.com>]

> On 16 Sep 2021, at 01:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> Adding Jan for an opinion on the EFI common code changes. See below.
> 
> 
> On Wed, 15 Sep 2021, Luca Fancellu wrote:
>> When Xen is started as EFI application, it is checking
>> the presence of multiboot,module in the DT, if any is
>> found, the configuration file is skipped.
>> Restrict this check to just any multiboot,module that
>> is a direct child of the /chosen node.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> xen/arch/arm/efi/efi-boot.h | 30 ++++++++++++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index cf9c37153f..5ff626c6a0 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -581,6 +581,8 @@ 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)
>> {
>> +    int node;
>> +    bool dom0_module_found = 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
>> @@ -592,11 +594,35 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>>     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 )
>> +
>> +    /* Locate chosen node */
>> +    node = fdt_subnode_offset(fdt, 0, "chosen");
>> +
>> +    /* Cycle through every node inside chosen having multiboot,module */
>> +    do {
>> +        int depth = 0;
>> +        node = fdt_node_offset_by_compatible(fdt, node, "multiboot,module");
>> +        /*
>> +         * If the multiboot,module just found is placed at depth less than 3,
>> +         * it means that it is here: /chosen/<module> so it is a module to
>> +         * start dom0. (root is counted as 0)
>> +         */
>> +        if ( node > 0 )
>> +        {
>> +            depth = fdt_node_depth(fdt, node);
>> +            if ( (depth >= 0) && (depth < 3) )
>> +            {
>> +                dom0_module_found = true;
>> +                break;
>> +            }
>> +        }
>> +    } while(node > 0);
> 
> It should be possible to enable the uefi,binary bootflow for Dom0 and
> the Dom0 ramdisk too. It would be nice as we could have a 100% UEFI
> boot, not dependent on U-Boot, both Dom0 and Dom0less, without the
> xen.cfg file. It doesn't have to be done now by this series, but it
> should be possible from a device tree bindings perspective.
> 
> With that in mind, is this check accurate? This patch is saying that if
> Dom0 is not present in the device tree, then load xen.cfg. But what if
> it is a true dom0less configuration? Then we would have no dom0, only
> dom0less VMs, and we might still not want to load xen.cfg. True dom0less
> is another one of those configurations that don't have to be enabled now
> by this series but they should be possible from a device tree bindings
> perspective.
> 
> 
> I tried to think of ways to improve this check, for instance searching
> for a module that doesn't have "uefi,binary" but has the regular "reg"
> property. If there is one, then we could skip loading xen.cfg. But that
> doesn't work if we have a UEFI-only true dom0less configuration.
> 
> So I am thinking that we have no choice but introducing a new property
> to tell us whether we should or should not load xen.cfg when
> multiboot,modules are present.
> 
> Taking inspiration from HyperLaunch, it could be a new node:
> 
> chosen {
>    cfg {
>        compatible = "xen,uefi-config", "multiboot,module";
>        uefi,binary = "xen.cfg";
>    };
> };
> 
> In efi_arch_use_config_file we would check if there are any nodes
> compatible with "multiboot,module". If there are none, it returns true.
> 
> If there are any, and one of them is also compatible "xen,uefi-config",
> then efi_arch_use_config_file returns true and also the specified
> configuration filename.
> 
> If there are nodes compatible to "multiboot,module" but none of them is
> compatible to "module,uefi-config", then efi_arch_use_config_file
> returns false. We use the device tree only.
> 
> I think that would be clearer and more consistent from a specification
> perspective, but it requires one change in common code:
> efi_arch_use_config_file would not just return bool but it would also
> return a filename if found (it could be a pointer parameter to the
> function).
> 
> 
> Otherwise, we could add a simple property like the following, without a
> specific value and without a filename:
> 
> chosen {
>    xen,uefi-config;
> };
> 
> The presence of xen,uefi-config could mean that Xen should go through
> the usual guessing game to figure out the right cfg file to load. This
> would not require any common code changes because
> efi_arch_use_config_file could simply return bool as it does today.
> 
> My preference is the xen,uefi-config compatible node, but I think the
> property would also work.
> 
> 
> Jan, do you have an opinion on whether efi_arch_use_config_file has to
> stay as it is today, or would you be open to the possibility of making
> efi_arch_use_config_file return a filename too?

Hi Stefano,

True dom0less is a configuration that this serie enables: if there is no dom0 kernel
specified in the xen.cfg then only the domUs will be loaded and started.

The following cases are valid:
1) start only dom0 [dom0 modules in xen.cfg or embedded in Xen image]
2) start only domUs, true dom0less [no dom0 modules in xen.cfg and inside Xen image, domUs on DT]
3) start dom0 and domUs [(dom0 modules in xen.cfg or inside Xen image) and domUs on DT]

I don’t understand why we want to add new properties to avoid/not avoid to read the xen.cfg, am I missing
something?

Cheers,
Luca