Message-ID: <CAPnjgZ1fVJE=ar_rB_So+vjkOZ_pDjaO5wwPn3pMKe=n3MmBeg@mail.gmail.com>
Date:   Thu, 7 Sep 2023 09:56:37 -0600
From:   Simon Glass <sjg@...omium.org>
To:     Ard Biesheuvel <ardb@...nel.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

Hi Ard,

On Thu, 7 Sept 2023 at 09:07, Ard Biesheuvel <ardb@...nel.org> wrote:
>
> 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.

Right, but that is my use case.

>
> 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.

By prior art do you mean code, or an existing binding? In either case,
can you please point me to it? Is this a generic binding used on x86
as well, or just for ARM?

My goal here is to augment the binding.

>
> 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

Yes I can add more detail, if that is all that is needed. But we seem
to still not be aligned on the goal.


> - 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.

Well if you can find another way to do this in the DT, that is fine.

Regards,
Simon

Powered by blists - more mailing lists