Message-ID: <CAMj1kXFjVPwnu226R8bHbo0i0LZ7jQE+vLPNQa6cvrCYqGD+YA@mail.gmail.com>
Date:   Thu, 7 Sep 2023 17:07:19 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Simon Glass <sjg@...omium.org>
Cc:     Rob Herring <robh@...nel.org>,
        Devicetree Discuss <devicetree@...r.kernel.org>,
        Maximilian Brune <maximilian.brune@...ements.com>,
        ron minnich <rminnich@...il.com>,
        Tom Rini <trini@...sulko.com>,
        Dhaval Sharma <dhaval@...osinc.com>,
        U-Boot Mailing List <u-boot@...ts.denx.de>,
        Mark Rutland <mark.rutland@....com>,
        Yunhui Cui <cuiyunhui@...edance.com>,
        linux-acpi@...r.kernel.org, Gua Guo <gua.guo@...el.com>,
        Lean Sheng Tan <sheng.tan@...ements.com>,
        Guo Dong <guo.dong@...el.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Chiu Chasel <chasel.chiu@...el.com>
Subject: Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages

On Thu, 7 Sept 2023 at 16:50, Simon Glass <sjg@...omium.org> wrote:
>
> Hi Ard,
>
> On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@...nel.org> wrote:
> >
> > On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@...omium.org> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@...nel.org> wrote:
> > > >
> > > > On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@...omium.org> wrote:
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@...nel.org> wrote:
> > > > >>
> > > > >> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@...omium.org> wrote:
> > > > >> >
> > > > >> > Hi Rob, Ard,
> > > > >> >
> > > > >> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@...nel.org> wrote:
> > > > >> > >
> > > > >> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@...nel.org> wrote:
> > > > >> > > >
> > > > >> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@...omium.org> wrote:
> > > > >> > > > >
> > > > >> > > > > The Devicetree specification skips over handling of a logical view of
> > > > >> > > > > the memory map, pointing users to the UEFI specification.
> > > > >> > > > >
> > > > >> > > > > It is common to split firmware into 'Platform Init', which does the
> > > > >> > > > > initial hardware setup and a "Payload" which selects the OS to be booted.
> > > > >> > > > > Thus an handover interface is required between these two pieces.
> > > > >> > > > >
> > > > >> > > > > Where UEFI boot-time services are not available, but UEFI firmware is
> > > > >> > > > > present on either side of this interface, information about memory usage
> > > > >> > > > > and attributes must be presented to the "Payload" in some form.
> > > > >> > > > >
> > > > >> > > >
> > > > >> > > > I don't think the UEFI references are needed or helpful here.
> > > > >> > > >
> > > > >> > > > > This aims to provide an small schema addition for this mapping.
> > > > >> > > > >
> > > > >> > > > > For now, no attempt is made to create an exhaustive binding, so there are
> > > > >> > > > > some example types listed. More can be added later.
> > > > >> > > > >
> > > > >> > > > > The compatible string is not included, since the node name is enough to
> > > > >> > > > > indicate the purpose of a node, as per the existing reserved-memory
> > > > >> > > > > schema.
> > > > >> > >
> > > > >> > > Node names reflect the 'class', but not what's specifically in the
> > > > >> > > node. So really, all reserved-memory nodes should have the same name,
> > > > >> > > but that ship already sailed for existing users. 'compatible' is the
> > > > >> > > right thing here. As to what the node name should be, well, we haven't
> > > > >> > > defined that. I think we just used 'memory' on some platforms.
> > > > >> >
> > > > >> > OK
> > > > >> >
> > > > >> > >
> > > > >> > > > > This binding does not include a binding for the memory 'attribute'
> > > > >> > > > > property, defined by EFI_BOOT_SERVICES.GetMemoryMap(). It may be useful
> > > > >> > > > > to have that as well, but perhaps not as a bit mask.
> > > > >> > > > >
> > > > >> > > > > Signed-off-by: Simon Glass <sjg@...omium.org>
> > > > >> > > > > ---
> > > > >> > > > >
> > > > >> > > > > Changes in v5:
> > > > >> > > > > - Drop the memory-map node (should have done that in v4)
> > > > >> > > > > - Tidy up schema a bit
> > > > >> > > > >
> > > > >> > > > > Changes in v4:
> > > > >> > > > > - Make use of the reserved-memory node instead of creating a new one
> > > > >> > > > >
> > > > >> > > > > Changes in v3:
> > > > >> > > > > - Reword commit message again
> > > > >> > > > > - cc a lot more people, from the FFI patch
> > > > >> > > > > - Split out the attributes into the /memory nodes
> > > > >> > > > >
> > > > >> > > > > Changes in v2:
> > > > >> > > > > - Reword commit message
> > > > >> > > > >
> > > > >> > > > >  .../reserved-memory/common-reserved.yaml      | 53 +++++++++++++++++++
> > > > >> > > > >  1 file changed, 53 insertions(+)
> > > > >> > > > >  create mode 100644 dtschema/schemas/reserved-memory/common-reserved.yaml
> > > > >> > > > >
> > > > >> > > > > diff --git a/dtschema/schemas/reserved-memory/common-reserved.yaml b/dtschema/schemas/reserved-memory/common-reserved.yaml
> > > > >> > > > > new file mode 100644
> > > > >> > > > > index 0000000..d1b466b
> > > > >> > > > > --- /dev/null
> > > > >> > > > > +++ b/dtschema/schemas/reserved-memory/common-reserved.yaml
> > > > >> > > > > @@ -0,0 +1,53 @@
> > > > >> > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > >> > > > > +%YAML 1.2
> > > > >> > > > > +---
> > > > >> > > > > +$id: http://devicetree.org/schemas/reserved-memory/common-reserved.yaml#
> > > > >> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > >> > > > > +
> > > > >> > > > > +title: Common memory reservations
> > > > >> > > > > +
> > > > >> > > > > +description: |
> > > > >> > > > > +  Specifies that the reserved memory region can be used for the purpose
> > > > >> > > > > +  indicated by its node name.
> > > > >> > > > > +
> > > > >> > > > > +  Clients may reuse this reserved memory if they understand what it is for.
> > > > >> > > > > +
> > > > >> > > > > +maintainers:
> > > > >> > > > > +  - Simon Glass <sjg@...omium.org>
> > > > >> > > > > +
> > > > >> > > > > +allOf:
> > > > >> > > > > +  - $ref: reserved-memory.yaml
> > > > >> > > > > +
> > > > >> > > > > +properties:
> > > > >> > > > > +  $nodename:
> > > > >> > > > > +    enum:
> > > > >> > > > > +      - acpi-reclaim
> > > > >> > > > > +      - acpi-nvs
> > > > >> > > > > +      - boot-code
> > > > >> > > > > +      - boot-data
> > > > >> > > > > +      - runtime-code
> > > > >> > > > > +      - runtime-data
> > > > >> > > > > +
> > > > >> > > >
> > > > >> > > > These types are used by firmware to describe the nature of certain
> > > > >> > > > memory regions to the OS. Boot code and data can be discarded, as well
> > > > >> > > > as ACPI reclaim after its contents have been consumed. Runtime code
> > > > >> > > > and data need to be mapped for runtime features to work.
> > > > >> > > >
> > > > >> > > > When one firmware phase communicates the purpose of a certain memory
> > > > >> > > > reservation to another, it is typically not limited to whether its
> > > > >> > > > needs to be preserved and when it needs to be mapped (and with which
> > > > >> > > > attributes). I'd expect a memory reservation appearing under this node
> > > > >> > > > to have a clearly defined purpose, and the subsequent phases need to
> > > > >> > > > be able to discover this information.
> > > > >> > > >
> > > > >> > > > For example, a communication buffer for secure<->non-secure
> > > > >> > > > communication or a page with spin tables used by PSCI. None of the
> > > > >> > > > proposed labels are appropriate for this, and I'd much rather have a
> > > > >> > > > compatible string or some other property that clarifies the nature in
> > > > >> > > > a more suitable way. Note that 'no-map' already exists to indicate
> > > > >> > > > that the CPU should not map this memory unless it does so for the
> > > > >> > > > specific purpose that the reservation was made for.
> > > > >> > >
> > > > >> > > I agree. I think compatible is the better approach. Some property like
> > > > >> > > 'discard' may not be sufficient information if the OS needs to consume
> > > > >> > > the region first and then discard it. Better to state exactly what's
> > > > >> > > there and then the OS can imply the rest.
> > > > >> >
> > > > >> > OK, so what sort of compatible strings?
> > > > >> >
> > > > >> > How about:
> > > > >> > "acpi-reclaim" - holds ACPI tables; memory can be reclaimed once the
> > > > >> > tables are read and no-longer needed
> > > > >>
> > > > >> ACPI reclaim is a policy, not a purpose. This memory could contain
> > > > >> many different things.
> > > > >>
> > > > >> > "boot-code" - holds boot code; memory can be reclaimed once the boot
> > > > >> > phase is complete
> > > > >> > "runtime-code" - holds runtime code; memory can be reclaimed only if
> > > > >> > this code will not be used from that point
> > > > >> >
> > > > >>
> > > > >> These are also policies. They can be inferred from the purpose.
> > > > >>
> > > > >> > etc. We can then have more specific compatibles, like:
> > > > >> >
> > > > >> > "psci-spin-table" - holds PSCI spin tables
> > > > >> >
> > > > >> > so you could do:
> > > > >> >
> > > > >> > compatible = "runtime-code", "psci-spin-table";
> > > > >> >
> > > > >>
> > > > >> I understand that this binding targets firmware<->firmware rather than
> > > > >> firmware<->OS, which makes it much more difficult to keep it both
> > > > >> generic and sufficiently descriptive.
> > > > >>
> > > > >> However, I still feel that all the overlap with UEFI memory types is
> > > > >> not what we want here. UEFI knows how to manage its own memory map,
> > > > >> what it needs to know is what memory is already in use and for which
> > > > >> exact purpose. Whether or not that implies that the memory can be
> > > > >> freed at some point or can be mapped or not should follow from that.
> > > > >
> > > > >
> > > > > Can you please make a suggestion? I am unsure what you are looking for.
> > > > >
> > > >
> > > > I'm happy to help flesh this out, but you still have not provided us
> > > > with an actual use case, so I can only draw from my own experience
> > > > putting together firmware for virtual and physical ARM machines.
> > >
> > > I did explain that this is needed when Tianocore is on both sides of
> > > the interface, since Platform Init places some things in memory and
> > > the Payload needs to preserve them there, and/or know where they are.
> > >
> > > I think the problem might be that you don't agree with that, but it
> > > seems to be a fact, so I am not sure how I can alter it.
> > >
> > > Please can you clearly explain which part of the use case you are missing.
> > >
> >
> > 'Tianocore on both sides of the interface' means that Tianocore runs
> > as the platform init code, and uses a bespoke DT based protocol to
> > launch another instance of Tianocore as the payload, right?
>
> Not another instance, no. Just the other half of Tianocore. The first
> half does platform init and the second half does the loading of the
> OS.
>

That doesn't make any sense to me.

> >
> > Tianocore/EDK2 already implements methods to reinvoke itself if needed
> > (e.g., during a firmware update), and does so by launching a new DXE
> > core. So the boot sequence looks like
> >
> > SEC -> PEI -> DXE -> BDS -> app that invokes UpdateCapsule() -> DXE ->
> > firmware update
> >
> > So please elaborate on how this Tianocore on both sides of the
> > interface is put together when it uses this DT based handover. We
> > really need a better understanding of this in order to design a DT
> > binding that meets its needs.
>
> Are you familiar with building Tianocore as a coreboot payload, for
> example? That shows Tianocore running as just the Payload, with
> coreboot doing the platform init. So the use case I am talking about
> is similar to that.
>

Yes I am familiar with that, and it is a completely different thing.

As i explained before, there is already prior art for this in
Tianocore, i.e., launching a Tianocore build based on a DT description
of the platform, including /memory and /reserved-memory nodes.

I argued that Tianocore never consumes memory reservations with UEFI
semantics, given that it supplants whatever UEFI functionality the
previous stage may have provided. But it shouldn't step on the code
and data regions used by the previous stage if it is still running in
the background (e.g., OS at EL1 and PSCI at EL2 on ARM)

So this brings me back to the things I proposed in my previous reply:
- memory reservations should be described in detail so the consumer
knows what to do with it
- memory reservations should have attributes that describe how the
memory may be used if not for the described purpose

I still don't see a reason for things like runtime-code and
runtime-data etc based on the above. If stage N describes the memory
it occupies itself as system memory, it should reserve it as well if
it needs to be preserved after stage N+1 has taken over, so perhaps it
should be described as a discardable memory reservation but I don't
think it necessarily needs a type in that case.

Powered by blists - more mailing lists