Re: [PATCH 00/31] passage: Define a standard for firmware data flow

["François Ozog" <francois.ozog@linaro.org>]

Hi Simon,

On Tue, 2 Nov 2021 at 15:59, Simon Glass <sjg@chromium.org> wrote:

> Hi François,
>
> On Mon, 1 Nov 2021 at 02:53, François Ozog <francois.ozog@linaro.org>
> wrote:
> >
> > Hi Simon,
> >
> > this seems a great endeavor. I'd like to better understand the scope of
> it.
> >
> > Is it to be used as part of what could become a U-Boot entry ABI scheme?
> By that I mean giving some fixed aspects
> > to U-Boot entry while letting boards to have flexibility (say for
> instance that the first 5 architecture ABI
> > parameter registers are reserved for U-Boot), and the Passage is about
> specifying either those reserved registers
> > or one of them?
>
> The goal is to provide a standard entry scheme for all firmware
> binaries. Whether it achieves that (or can with some mods) is up for
> discussion.
>
> If you say
a) define a U-Boot entry ABI and providing a firmware-to-firmware
information passing facility which would be part of all firmware ABIs (as
the projects decide to define their own ABI) it looks good.
but If you say
b) define a standard entry scheme (register map, processor state, MMU
state, SMMU state, GIC state...) that does not look realistic.
I think you mean a) but just want to be sure.

> Re the registers, do you think we need 5?
>
> >
> > Thinking entry ABI, here is what I observed on Arm:
> >
> > Linux has two entry ABIs:
> > - plain: x0 = dtb;
> >           command line = dtb:/chosen/bootargs; initrd =
> dtb:/chosen/linux,initrd-*
> > - EFI: x0=handle, x1=systemtable, x30=return address;
> >            dtb = EFI_UUID config table; initrd = efi:<loadfile2(INITRD
> vendor media UUID); command line = efi: image_protocol::load_options
> >
> > U-Boot (proper) has plenty of schemes:
> > - dtb is passed as either x0, x1, fixed memory area (Qemu which is bad
> in itself), or other registers
> > - additional information passing: board specific register scheme, SMC
> calls
> > - U-Boot for RPI boards implement a Linux shaped entry ABI to be
> launched by Videocore firmware
> >
> > Based on all the above, I would tend to think that RPI scheme is a good
> idea but also
> > shall not prevent additional schemes for the boards.
>
> I was not actually considering Linux since I believe/assume its entry
> scheme is fixed and not up for discussion.
>
> I also did not think about the EFI case. As I understand it we cannot
> touch it as it is used by UEFI today. Maybe it is even in the
> standard?
>
It is in the spec and we are making it evolve, or its understanding evolve
(jurisprudence) for instance on initrd standard handling.

>
> Really I am hoping we can start afresh...?
>
> >
> > What about a U-Boot Arm entry ABI like:
> > - plain: x0=dtb, x1=<Passage defined>, x2-x5 = <reserved>, other
> registers are per platform, SMC calls allowed too
>
> Hmm we don't actually need the dtb as it is available in the bloblist.
>
If you don't have x0=dtb, then you will not be able to use U-Boot on RPI4.
Unless you want to redo everything the RPI firmware is doing.

> But I added an offset to it as a convenience.
>
> > - EFI: x0=handle, x1=systemtable, x30=return address;  (when U-Boot is
> launched as an EFI app)
> >        dtb = EFI_UUID config table, + Passage = Passage UUID config table
>
> I don't understand the last line. Where is the passage info /
> bloblist? Do you mean it goes in the HOB list with a UUID? I suppose
> that is the most EFI-compatible way.
>
The Passage config table  could just contain the "head" of the
bloblist/Passage information.

>
> What do you think about the idea of using an offset into the bloblist
> for the dtb?

It is possible but as I said, failing to mimic Linux entry ABI would miss
the opportunity to just boot without changes on RPI4.

> Also, can we make the standard passage ABI a build-time
> option, so it is deterministic?
>
> Looks good. I would look into stating that for SystemReady we would advise
to use that option and make it standard for Trusted Substrate (Linaro
recipes that we upstreaming to make SystemReady compliance easy and
consistent across platforms).

> >
> > We could further leverage Passage to pass Operating Systems parameters
> that could be removed from device tree (migration of /chosen to Passage).
> Memory inventory would still be in DT but allocations for CMA or GPUs would
> be in Passage. This idea is to reach a point where  device tree is a
> "pristine" hardware description.
>
> I'm worried about this becoming a substitute for devicetree. Really my
> intent is to provide a way to pass simple info, whereas what you talk
> about there seems like something that should be DT, just that it might
> need suitable bindings.
>
> I see your point and I agree It should not be a substitute.
here is an expanded version of what I had in mind when I wrote those lines.
cma, initrd and other Linux kernel parameters can be conveyed either
through command line or DT.
When using the non UEFI Linux entry ABI, you need to use the DT to pass
those parameters.
When using the UEFI Linux entry ABI, you *can* (not must) use the command
line to pass all information, leaving the DT passed to the OS without any
/chosen.
When introducing Passage, I was wondering if we could pass command line to
Linux and, same as UEFI, leave the DT free from /chosen.
I am not sure it is a good goal though. I may be too pushing for a DT free
from parameters.

> As you know I have more expansive views about what should be in DT.
>
I think both of us are huge supporters of DT format and self describing
capabilities.
I am inclined to put rules into what fits into what lands in the DT that is
passed to the OS.
I am a fan of having DT used more in ad-hoc files.

> >
> > Cheers
> >
> > PS: as Ilias mentions, this patch set contains bug fixes, non
> immediately related additional functions (DM stats). It would be great to
> carve those out to fast path them and keep this one with the very core of
> your idea.
>
> The DM stats is used in 'passage: Report the devicetree source'. I
> know it is sideways but I think it is better to make the output line
> more useful than just reporting the devicetree source.
>
> I believe the DM stats has merits in its own. You could upstream this
independently and then Passage would be yet another "customer" of the
feature.

> The first patch is indeed unrelated. I will pick it up so we can drop
> it for the next rev.
>
> Regards,
> Simon
>
>
> >
> > On Mon, 1 Nov 2021 at 02:17, Simon Glass <sjg@chromium.org> wrote:
> >>
> >>
> >> This series adds a standard way of passing information between different
> >> firmware phases. This already exists in U-Boot at a very basic level, in
> >> the form of a bloblist containing an spl_handoff structure, but the
> intent
> >> here is to define something useful across projects.
> >>
> >> The need for this is growing as firmware fragments into multiple
> binaries
> >> each with its own purpose. Without any run-time connection, we must rely
> >> on build-time settings which are brittle and painful to keep in sync.
> >>
> >> This feature is named 'standard passage' since the name is more unique
> >> than many others that could be chosen, it is a passage in the sense that
> >> information is flowing from one place to another and it is standard,
> >> because that is what we want to create.
> >>
> >> The implementation is simply a pointer to a bloblist in a register, with
> >> an extra register to point to a devicetree, for more complex data, if
> one
> >> is present in the bloblist. This should cover all cases (small memory
> >> footprint as well as complex data flow) and be easy enough to implement
> on
> >> all architectures.
> >>
> >> The core bloblist code is relicensed to BSD-3-Clause in case it is
> useful
> >> in non-GPL projects but there is no requirement to use the same code.
> >>
> >> This series includes tweaks to the bloblist implementation in U-Boot to
> >> make it more suitable for the task, including:
> >>
> >>    - Allocate tags explicitly in the enum
> >>    - Put the magic number first
> >>    - Define a process for adding tags
> >>
> >> The emphasis is on enabling open communcation between binaries, not
> >> enabling passage of secret, undocumented data, although this is possible
> >> in a private environment.
> >>
> >> This series is built on the OF_BOARD series It is available at
> >> u-boot-dm/pass-working or:
> >>
> >>
> https://source.denx.de/u-boot/custodians/u-boot-dm/-/commit/073b5c156f222c69a98b8ebcaa563d1ff10eb217
> >>
> >>
> >> Simon Glass (31):
> >>   Makefile: Correct TPL rule for OF_REAL
> >>   kconfig: Add support for conditional values
> >>   dm: core: Allow getting some basic stats
> >>   stddef: Avoid warning with clang with offsetof()
> >>   fdt: Drop SPL_BUILD macro
> >>   bloblist: Put the magic number first
> >>   bloblist: Rename the SPL tag
> >>   bloblist: Drop unused tags
> >>   bloblist: Use explicit numbering for the tags
> >>   bloblist: Support allocating the bloblist
> >>   bloblist: Use LOG_CATEGORY to simply logging
> >>   bloblist: Use 'phase' consistently for bloblists
> >>   bloblist: Refactor Kconfig to support alloc or fixed
> >>   arm: qemu: Add an SPL build
> >>   bloblist: Add functions to obtain base address and size
> >>   passage: Support an incoming passage
> >>   passage: Support a control devicetree
> >>   passage: arm: Accept a passage from the previous phase
> >>   passage: spl: Support adding the dtb to the passage bloblist
> >>   passage: spl: Support passing the passage to U-Boot
> >>   passage: Record where the devicetree came from
> >>   passage: Report the devicetree source
> >>   passage: Add a qemu test for ARM
> >>   bloblist: doc: Bring in the API documentation
> >>   bloblist: Relicense to allow BSD-3-Clause
> >>   sandbox: Add a way of checking structs for standard passage
> >>   passage: Add documentation
> >>   passage: Add docs for spl_handoff
> >>   x86: Move Intel GNVS file into the common include directory
> >>   passage: Add checks for pre-existing blobs
> >>   WIP: RFC: Add a gitlab test
> >>
> >>  .gitlab-ci.yml                                |   6 +
> >>  MAINTAINERS                                   |  10 +
> >>  Makefile                                      |   2 +-
> >>  arch/arm/cpu/armv7/start.S                    |   7 +-
> >>  arch/arm/dts/qemu-arm-u-boot.dtsi             |  22 ++
> >>  arch/arm/lib/crt0.S                           |   4 +
> >>  arch/arm/mach-qemu/Kconfig                    |   9 +
> >>  arch/sandbox/cpu/spl.c                        |   2 +-
> >>  arch/x86/cpu/apollolake/acpi.c                |   2 +-
> >>  arch/x86/cpu/broadwell/cpu_from_spl.c         |   4 +-
> >>  arch/x86/cpu/intel_common/acpi.c              |   2 +-
> >>  .../include/asm/arch-apollolake/global_nvs.h  |   2 +-
> >>  arch/x86/lib/spl.c                            |   2 +-
> >>  arch/x86/lib/tpl.c                            |   2 +-
> >>  board/emulation/qemu-arm/Kconfig              |  23 +-
> >>  board/emulation/qemu-arm/MAINTAINERS          |   1 +
> >>  board/emulation/qemu-arm/Makefile             |   1 +
> >>  board/emulation/qemu-arm/spl.c                |  27 ++
> >>  board/google/chromebook_coral/coral.c         |   2 +-
> >>  board/sandbox/Makefile                        |   3 +-
> >>  board/sandbox/stdpass_check.c                 | 107 ++++++
> >>  cmd/bdinfo.c                                  |   2 +
> >>  common/Kconfig                                | 161 ++++++++-
> >>  common/bloblist.c                             | 124 +++++--
> >>  common/board_f.c                              |  48 ++-
> >>  common/board_r.c                              |  18 +
> >>  common/spl/spl.c                              |  74 +++-
> >>  configs/qemu_arm_spl_defconfig                |  78 +++++
> >>  doc/board/emulation/qemu-arm.rst              |  38 +++
> >>  doc/develop/bloblist.rst                      |  28 +-
> >>  doc/develop/index.rst                         |   1 +
> >>  doc/develop/std_passage.rst                   | 319 ++++++++++++++++++
> >>  drivers/core/device.c                         |  11 +
> >>  drivers/core/root.c                           |   7 +
> >>  drivers/core/uclass.c                         |  13 +
> >>  drivers/serial/serial-uclass.c                |   3 +-
> >>  dts/Kconfig                                   |  12 +
> >>  include/asm-generic/global_data.h             |  35 ++
> >>  include/bloblist.h                            | 175 +++++++---
> >>  include/dm/device.h                           |  11 +-
> >>  include/dm/root.h                             |   8 +
> >>  include/dm/uclass-internal.h                  |   7 +
> >>  include/fdtdec.h                              |  40 ++-
> >>  include/handoff.h                             |   8 +-
> >>  .../x86/include/asm => include}/intel_gnvs.h  |   0
> >>  include/linux/kconfig.h                       |  18 +
> >>  include/linux/stddef.h                        |   8 +-
> >>  include/spl.h                                 |  15 +
> >>  include/stdpass/README                        |   4 +
> >>  include/stdpass/tpm2_eventlog.h               |  42 +++
> >>  include/stdpass/vboot_ctx.h                   | 267 +++++++++++++++
> >>  lib/asm-offsets.c                             |   5 +
> >>  lib/fdtdec.c                                  |  65 +++-
> >>  scripts/config_whitelist.txt                  |   1 +
> >>  test/bloblist.c                               |  21 +-
> >>  test/dm/core.c                                |  41 +++
> >>  test/py/tests/test_passage.py                 |  11 +
> >>  57 files changed, 1798 insertions(+), 161 deletions(-)
> >>  create mode 100644 arch/arm/dts/qemu-arm-u-boot.dtsi
> >>  create mode 100644 board/emulation/qemu-arm/spl.c
> >>  create mode 100644 board/sandbox/stdpass_check.c
> >>  create mode 100644 configs/qemu_arm_spl_defconfig
> >>  create mode 100644 doc/develop/std_passage.rst
> >>  rename {arch/x86/include/asm => include}/intel_gnvs.h (100%)
> >>  create mode 100644 include/stdpass/README
> >>  create mode 100644 include/stdpass/tpm2_eventlog.h
> >>  create mode 100644 include/stdpass/vboot_ctx.h
> >>  create mode 100644 test/py/tests/test_passage.py
> >>
> >> --
> >> 2.33.1.1089.g2158813163f-goog
> >>
> >
> >
> > --
> > François-Frédéric Ozog | Director Business Development
> > T: +33.67221.6485
> > francois.ozog@linaro.org | Skype: ffozog
> >
>


-- 
François-Frédéric Ozog | *Director Business Development*
T: +33.67221.6485
francois.ozog@linaro.org | Skype: ffozog