[RFC PATCH] env: Export environment config to OS devicetree

[Stefano Babic]

Hi Frieder,

On 03.08.23 14:51, Frieder Schrempf wrote:
> Hi Stefano,
> 
> On 03.08.23 10:14, Stefano Babic wrote:
>> Hi Frieder,
>>
>> On 01.08.23 16:46, Frieder Schrempf wrote:
>>> From: Frieder Schrempf <frieder.schrempf at kontron.de>
>>>
>>> There are plenty of cases where the OS wants to know where the
>>> persistent environment is stored in order to print or manipulate it.
>>>
>>> In most cases a userspace tool like libubootenv [1] is used to handle
>>> the environment. It uses hardcoded settings in a config file in the
>>> rootfs to determine the exact location of the environment.
>>>
>>
>> ...or the configuration file is generated, or it is located on a rw
>> (overlayfs) partition.
>>
>>> In order to make systems more flexible and let userspace tools
>>> detect the location of the environment currently in use, let's
>>> add an option to export the runtime configuration to the FDT passed
>>> to the OS.
>>>> This is especially useful when your device is able to use multiple
>>> locations for the environment and the rootfs containing the
>>> fw_env.config file should be kept universal.
>>
>> Ok
>>
>> One possible issue is there is a hard dependency between the properties
>> set in DT and the user space tool. The tool in user space is bound to
>> the list of properties, and changes / extensions for the user tool
>> requires changes in DT semantics (which properties, their path, etc).
>>
>> The same hard dependency is set by fw_env.config with its strong
>> hardcoded and position dependent syntax. This one of the reason I
>> introduced YAML support in libubootenv, so that the user tool is
>> independent and can add own properties.
>>
>> I am just asking if this use case requires a new interface, or it is
>> already available in some way. I mean, the configuration is YAML instead
>> of fw_env.config and contains multiple setup ("namespaces"), like:
>>
>> mmc:
>>    size : 0x100000
>>    lockfile : /var/lock/fw_printenv.lock
>>    devices:
>>      - path : /dev/mmcblk0boot0
>>        offset : 0x1E0000
>>        sectorsize : 0x100000
>>      - path : /dev/mmcblk0boot0
>>        offset : 0x1E0000
>>        sectorsize : 0x100000
>> spi:
>>    size : 0x100000
>>    lockfile : /var/lock/fw_printenv.lock
>>    devices:
>>      - path : /dev/mtd0
>>        offset : 0x1E0000
>>        sectorsize : 0x100000
>>      - path : /dev/mtd0
>>        offset : 0x1F0000
>>        sectorsize : 0x100000
>>
>> This configuration file can safely stored in your common rootfs and
>> covers all possible storage for environment. It does not cover "runtime"
>> changes, but well, I do not think this is a use case.
>>
>> An advantage here is that new options, useful for the User Space tool,
>> can be simply introduced for the tool without the need to synchronize
>> with DT spec and U-Boot.
>>
>> A mapping between location index and device path is not required. What
>> is still required is a selector, and this can be implemented in DT or as
>> kernel parameter.
>>
>> Does this already cover your use case or do we need the introduction of
>> this new interface ?
> 
> Thanks for the feedback! I've seen the YAML configuration feature in
> libubootenv but I missed the fact, that it already supports something
> like the namespace parameter for selecting different configurations.
> 
> Indeed this would solve one part of the issue. And yes, I think we could
> use the DT or a kernel parameter to pass a selector from U-Boot to
> userspace.

Exactly.

> 
> Do you think we could add something to libubootenv to automatically
> select the default namespace based on some DT property or kernel parameter?

Yes, we just need to add a way to set up a default "namespace". There is 
currently a default "uboot" namespace, and we just need a way to pass 
the information to the library. Maybe with an env ?

> 
> If yes, I think this would be a viable solution for me.

Fine !

> 
>>
>>>
>>> Currently the general information like location/type, size, offset
>>> and redundancy are exported. Userspace tools might already be able
>>> to guess the correct device to access based on this information.
>>>
>>> For further device-specific information two additional properties
>>> 'id1' and 'id2' are used. The current implementation uses them for
>>> MMC and SPI FLASH like this:
>>>
>>> | Type       | id1                        | id2                        |
>>> | ---------- | -------------------------- | -------------------------- |
>>> | MMC        | MMC device index in U-Boot | MMC hwpart index in U-Boot |
>>> | SPI FLASH  | SPI bus index in U-Boot    | SPI CS index in U-Boot     |
>>>
>>> Further extensions for other environment devices are possible.
>>>
>>> We add the necessary information to the '/chosen' node. The following
>>> shows two examples:
>>>
>>> Redundant environment in MMC:
>>>
>>>     /chosen {
>>>       u-boot,env-config {
>>>         location = <5>;              # according to 'enum env_location'
>>>         offset = <0x1E0000>;
>>>         size = <0x100000>;
>>>         sect_size = <0x100000>;
>>>         id1 = <1>;                   # MMC device index
>>>         id2 = <0>;                   # MMC HW partition index
>>>       };
>>>       u-boot,env-redund-config {
>>>         offset = <0x1F0000>;
>>>       };
>>>     };
>>>
>>> Redundant environment in SPI NOR:
>>>
>>>     /chosen {
>>>       u-boot,env-config {
>>>         location = <10>;             # according to 'enum env_location'
>>>         offset = <0x1E0000>;
>>>         size = <0x100000>;
>>>         sect_size = <0x100000>;
>>>         id1 = <0>;                   # SPI bus
>>>         id2 = <0>;                   # SPI CS
>>>       };
>>>       u-boot,env-redund-config {
>>>         offset = <0x1F0000>;
>>>       };
>>>     };
>>>
>>> See [2] for an example parser implementation for libubootenv.
>>>
>>> [1]
>>> https://github.com/sbabic/libubootenv
>>> [2]
>>> https://github.com/fschrempf/libubootenv/commit/726a7ac9b1b1020354ee8fe750f4cad3df01f5e7
>>>
>>> Cc: Stefano Babic <sbabic at denx.de>
>>> Cc: Michael Walle <michael at walle.cc>
>>> Signed-off-by: Frieder Schrempf <frieder.schrempf at kontron.de>
>>> ---
>>>    boot/Kconfig     |  9 ++++++++
>>>    boot/image-fdt.c |  8 ++++++++
>>>    env/env.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>    include/env.h    |  4 ++++
>>>    4 files changed, 74 insertions(+)
>>>
>>> diff --git a/boot/Kconfig b/boot/Kconfig
>>> index e8fb03b801..16a94f9b35 100644
>>> --- a/boot/Kconfig
>>> +++ b/boot/Kconfig
>>> @@ -702,6 +702,15 @@ config OF_BOARD_SETUP
>>>          board-specific information in the device tree for use by the OS.
>>>          The device tree is then passed to the OS.
>>>    +config OF_EXPORT_ENV_CONFIG
>>> +    bool "Export environment config to device tree before boot"
>>> +    depends on OF_LIBFDT
>>> +    help
>>> +      This causes U-Boot to call fdt_environment_config() before
>>> booting into
>>> +      the operating system. This function exports the currently in use
>>> +      environemnt configuration to the "chosen" node of the fdt. This
>>> allows
>>> +      the OS to determine where the environment is located.
>>> +
>>>    config OF_SYSTEM_SETUP
>>>        bool "Set up system-specific details in device tree before boot"
>>>        depends on OF_LIBFDT
>>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>>> index f10200f647..c02c8f33ef 100644
>>> --- a/boot/image-fdt.c
>>> +++ b/boot/image-fdt.c
>>> @@ -643,6 +643,14 @@ int image_setup_libfdt(struct bootm_headers
>>> *images, void *blob,
>>>        /* Append PStore configuration */
>>>        fdt_fixup_pstore(blob);
>>>    #endif
>>> +    if (IS_ENABLED(CONFIG_OF_EXPORT_ENV_CONFIG)) {
>>> +        fdt_ret = fdt_environment_config(blob);
>>> +        if (fdt_ret) {
>>> +            printf("ERROR: environment config fdt fixup failed: %s\n",
>>> +                   fdt_strerror(fdt_ret));
>>> +            goto err;
>>> +        }
>>> +    }
>>>        if (IS_ENABLED(CONFIG_OF_BOARD_SETUP)) {
>>>            const char *skip_board_fixup;
>>>    diff --git a/env/env.c b/env/env.c
>>> index 2aa52c98f8..a640977205 100644
>>> --- a/env/env.c
>>> +++ b/env/env.c
>>> @@ -8,6 +8,7 @@
>>>    #include <env.h>
>>>    #include <env_internal.h>
>>>    #include <log.h>
>>> +#include <mmc.h>
>>>    #include <asm/global_data.h>
>>>    #include <linux/bitops.h>
>>>    #include <linux/bug.h>
>>> @@ -156,6 +157,58 @@ __weak enum env_location env_get_location(enum
>>> env_operation op, int prio)
>>>        return arch_env_get_location(op, prio);
>>>    }
>>>    +#ifdef CONFIG_OF_EXPORT_ENV_CONFIG
>>> +int fdt_environment_config(void *blob)
>>> +{
>>> +    struct mmc *mmc;
>>> +    u32 location;
>>> +    int chosen_offs, offs;
>>> +    int ret;
>>> +
>>> +    if (CONFIG_IS_ENABLED(ENV_IS_NOWHERE))
>>> +        return 0;
>>> +
>>> +    chosen_offs = fdt_path_offset(blob, "/chosen");
>>> +    if (chosen_offs < 0) {
>>> +        printf("Could not find chosen node.\n");
>>> +        return chosen_offs;
>>> +    }
>>> +
>>> +    offs = fdt_add_subnode(blob, chosen_offs, "u-boot,env-config");
>>> +    if (offs < 0) {
>>> +        printf("Could not create env-config node.\n");
>>> +        return offs;
>>> +    }
>>> +
>>> +    location = env_get_location(0, 0);
>>> +    fdt_setprop_u32(blob, offs, "location", location);
>>> +    fdt_setprop_u32(blob, offs, "offset", CONFIG_ENV_OFFSET);
>>> +    fdt_setprop_u32(blob, offs, "size", CONFIG_ENV_SIZE);
>>> +    fdt_setprop_u32(blob, offs, "sect_size", CONFIG_ENV_SECT_SIZE);
>>> +
>>
>> My major concern is regarding the dependency that is introduced. These
>> properties are then compiled into U-Boot. If a new property will be
>> required in future, this could required an update for the bootloader in
>> field. Even if yes, bootloader's updates are done in field, they are
>> often risky and not power-cut safe.
>>
>> If the whole configuration, new properties, etc, remains as much as
>> possible in User Space, to get the new feature we need an update of
>> rootfs (in your example), that is (mostly) safe (and then it is not, the
>> update procedure should be revisited). A single rootfs for multiple
>> hardware using different locations and maybe different options (if
>> needed or if they will be introduced) is possible, the yaml
>> configuration will contain all of them and some detection (in user
>> space) can do the trick to select the right one.
>>
>> What do you think ?
> 
> Yeah, I get your point. My idea was that the bootloader exports
> "everything" it knows about the environment to the DT. But indeed, there
> is some (minimal?) risk that changes to the exported data are needed
> which would require the bootloader to be updated.

Right - the idea with libubootenv is that it is hardware-independent, 
and setup is done just via configuration files.

> 
> Using the YAML config with multiple namespaces sounds like a good
> alternative. But detecting the right namespace to use can only be done
> based on information passed by the bootloader.

Agree.

> So if we could build a
> default namespace selection by DT property or kernel parameter into
> libubootenv that would be great.

Of course, we can !

Best regards,
Stefano

> 
> Thanks
> Frieder

-- 
=====================================================================
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich,   Office: Kirchenstr.5, 82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================