Hi Frieder,
On Mon, 7 Aug 2023 at 06:19, Frieder Schrempf
<frieder.schre...@kontron.de> wrote:
>
> Hi Simon, hi Stefano,
>
> On 04.08.23 15:39, Stefano Babic wrote:
> > Hi Simon,
> >
> > On 04.08.23 05:42, Simon Glass wrote:
> >> 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?
> >
> > I think it is adifferent topic. This is raises a compatibility issue
> > between User Space and bootloader, and the update concept must of course
> > take care of this, that means, if U-Boot is updated, the SW (rootfs)
> > must be updated as well if not compatible anymore. This already happens
> > any time there is compatibility issue between U-Boot and User Space
> > (switch from legacy image to fitImage for example, etc.).
> >
> > Some sort of automatic detection in user space can also be possible,
> >
> >>
> >> 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.
> >
> > My point is that, apart the added dependencies between U-Boot and User
> > Space, we still need a sort of intermediate layer in User Space to map
> > between such as device index / phandle, and the device node. A location
> > = <X> does not help a lot when we access from User Space, and such sort
> > of mapping can be simply avoided by configuring tools in USer Space with
> > the names provided by the kernel, that can change from board to board
> > (for example depends on the order MTD devices are detected).
>
> A rather sophisticated solution could maybe work without needing a
> separate mapping between U-Boot env device and kernel device.
>
> Given that U-Boot is able to match the env device with a node in the
> devicetree passed to the kernel it could store a phandle to that node
> (e.g. /chosen/u-boot,env = <&mmc1>).
Yes.
>
> For specifying a partition/offset we would need a representation of the
> partition in the devicetree. This would work for MTD devices. I'm not
> sure about eMMC with hardware partitions (boot0, boot1, etc.). If they
> could be represented in DT this could work, too. No idea what about
> other potential env devices.
>
> I also see that there is already some binding for MTD devices to specify
> the U-Boot env partition in [1].
>
> Anyway this is way beyond what I can do at the moment and our primary
> goal to create a root FS that supports multiple env devices and is able
> to determine the one that is currently in use, can be achieved with
> minor extensions of U-Boot and libubootenv as proposed above by Stefano.
> So I think I will try to go down that road for now.
OK.
>
> Best regards
> Frieder
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml
Regards,
Simon
