[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Luca Fancellu <luca.fancellu@xxxxxxx>
  • Date: Thu, 23 Sep 2021 11:42:34 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=moOsx1sdIhrnMVxdu08lygWSOT5+XJcJk6731bVt/Is=; b=g4svbwQs8HUZQ9eDWovYOQhbKRFFrjs54zuKDZSv/M+XzjUgvRoYuSwXjgCqZ1Gy8X28NZbz41S0Dcij5EYoQZo4TCX1tPY/3vSYxD94GJfdOdQ+5kIZ0mksbx0t5LOXHhN23RqRuYhXMN0GHlPLrqJv8aZwC+cJaj5uUA+Ul8iZTPHZFlPnvMIrsmHmrQSfPFK4rX9vewkv8fb9kz+9QORFks5YFizp8w66eSaGs8ifPrvlT5lN14LAydsOOatgGFqSM6G5ddWq8n1r98lfZW0mBzMRIRHpGOfkNpcWx000IAAnzn2QoSooUlc+na+RYob0FiKDJ56L1NQYL6FRSw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Wo0PioaQrF27kWDzUpGDy3GkN3nXnN8VL5gCzfQpFr3excwjXJ3/yApVyPE0MTNKDkM6ZJNmIislekP7OpvLkPLP0OJIxKhLtJNNQrpkEN1/xPEEROVcLF/rAsTasHA2kzZ75tOZhxbnMZexn2TS9yIcBvPvP6LpD7QpQXb1Zu7RwxD3sY8AHkFW1hz5zuWkDoyllq0RYnawGG5+1i7l4AdgS3gdmF4jLnuzuVP+5GhMtaCqYjgnAPz/x4fdqHevjIQbs2gI+fiuednmrYnZh5ECGBBjH/eJqBjY6o4DyFohh85rZvRDywu+7uoF/dom8D+LlTrDUyaOIsSPCBKbRQ==
  • Authentication-results-original: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=arm.com;
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Bertrand Marquis <bertrand.marquis@xxxxxxx>, wei.chen@xxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 23 Sep 2021 10:43:11 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=arm.com;


> On 22 Sep 2021, at 22:19, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 22 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.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> The patch looks OK, just a couple of minor comments below.
> 
> 
>> ---
>> v2 changes:
>> - Introduced uefi,cfg-load property
>> - Add documentation about the property
>> ---
>> docs/misc/efi.pandoc        |  2 ++
>> xen/arch/arm/efi/efi-boot.h | 28 +++++++++++++++++++++++-----
>> 2 files changed, 25 insertions(+), 5 deletions(-)
>> 
>> 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.
> 

Hi Stefano,

> I think that, in addition to this, we also need to add the property to
> docs/misc/arm/device-tree/booting.txt where our "official" device tree
> bindings are maintained. I would add it below "Command lines" and before
> "Creating Multiple Domains directly from Xen" maybe as a new chapter.
> It could be called "Other Options" but other ideas could be valid too.
> 
> You can say that uefi,cfg-load is a boolean.

Sure, I will add in v3, what about this:

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.

> 
>> 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..8ceeba4ad1 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 skip_cfg_file = 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
>> -     * 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 )
>> +                skip_cfg_file = true;
>> +        }
>> +    }
>> +
>> +    if ( !fdt || !skip_cfg_file )
> 
> Just a suggestion, but rather than the double negative, wouldn't it be
> simpler to define it as
> 
>    bool load_cfg_file = true;
> 
> ?

Sure I will modify it.

> 
> 
>>     {
>>         /*
>>          * 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.
>>          */
> 
> Also mention in the commit message that you are taking the opportunity
> to update this comment do remove "dom0 required".

Yes I will add it in the commit message

Cheers,
Luca

> 
> 
>>         return true;
>>     }
>> -- 
>> 2.17.1




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.