Hi,
On Thu, 3 Aug 2023 at 07:21, Stefano Babic <sba...@denx.de> wrote:
>
> 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.schre...@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 <sba...@denx.de>
> >>> Cc: Michael Walle <mich...@walle.cc>
> >>> Signed-off-by: Frieder Schrempf <frieder.schre...@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
It seems to me that U-Boot controls where the env areas are and which
ones are used. How can we have a file in the root fs in that case? If
we update the firmware, who updates the file?
Re the DT binding, we should send that upstream. I think it would be
better if we can indicate which device holds each env, e.g. with a
phandle from that device, or a phandle the other way? I suspect Rob
Herring will have some ideas there.
Regards,
Simon
