Re: [PATCH v7 2/2] schemas: Add some common reserved-memory usages

[Simon Glass]

Hi Rob,

On Mon, 16 Oct 2023 at 10:50, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Fri, Oct 13, 2023 at 4:09 PM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> >
> > Hi Rob,
> >
> > On Fri, 13 Oct 2023 at 13:42, Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 6, 2023 at 7:03 PM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Fri, 6 Oct 2023 at 17:00, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 6 Oct 2023 at 20:17, Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > On Fri, 6 Oct 2023 at 11:33, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Mon, 2 Oct 2023 at 19:54, Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > Hi Rob,
> > > > > > > >
> > > > > > > > On Tue, 26 Sept 2023 at 13:42, Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > This aims to provide an small schema addition for the memory mapping
> > > > > > > > > needed to keep these two pieces working together well.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes in v7:
> > > > > > > > > - Rename acpi-reclaim to acpi
> > > > > > > > > - Drop individual mention of when memory can be reclaimed
> > > > > > > > > - Rewrite the item descriptions
> > > > > > > > > - Add back the UEFI text (with trepidation)
> > > > > > > >
> > > > > > > > I am again checking on this series. Can it be applied, please?
> > > > > > > >
> > > > > > >
> > > > > > > Apologies for the delay in response. I have been away.
> > > > > >
> > > > > > OK, I hope you had a nice trip.
> > > > > >
> > > > >
> > > > > Thanks, it was wonderful!
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Changes in v6:
> > > > > > > > > - Drop mention of UEFI
> > > > > > > > > - Use compatible strings instead of node names
> > > > > > > > >
> > > > > > > > > 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      | 71 +++++++++++++++++++
> > > > > > > > >  1 file changed, 71 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..f7fbdfd
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/dtschema/schemas/reserved-memory/common-reserved.yaml
> > > > > > > > > @@ -0,0 +1,71 @@
> > > > > > > > > +# 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 compatible string.
> > > > > > > > > +
> > > > > > > > > +  Clients may reuse this reserved memory if they understand what it is for,
> > > > > > > > > +  subject to the notes below.
> > > > > > > > > +
> > > > > > > > > +maintainers:
> > > > > > > > > +  - Simon Glass <sjg@xxxxxxxxxxxx>
> > > > > > > > > +
> > > > > > > > > +allOf:
> > > > > > > > > +  - $ref: reserved-memory.yaml
> > > > > > > > > +
> > > > > > > > > +properties:
> > > > > > > > > +  compatible:
> > > > > > > > > +    description: |
> > > > > > > > > +      This describes some common memory reservations, with the compatible
> > > > > > > > > +      string indicating what it is used for:
> > > > > > > > > +
> > > > > > > > > +         acpi: Advanced Configuration and Power Interface (ACPI) tables
> > > > > > > > > +         acpi-nvs: ACPI Non-Volatile-Sleeping Memory (NVS). This is reserved by
> > > > > > > > > +           the firmware for its use and is required to be saved and restored
> > > > > > > > > +           across an NVS sleep
> > > > > > > > > +         boot-code: Contains code used for booting which is not needed by the OS
> > > > > > > > > +         boot-code: Contains data used for booting which is not needed by the OS
> > > > > > > > > +         runtime-code: Contains code used for interacting with the system when
> > > > > > > > > +           running the OS
> > > > > > > > > +         runtime-data: Contains data used for interacting with the system when
> > > > > > > > > +           running the OS
> > > > > > > > > +
> > > > > > > > > +    enum:
> > > > > > > > > +      - acpi
> > > > > > > > > +      - acpi-nvs
> > > > > > > > > +      - boot-code
> > > > > > > > > +      - boot-data
> > > > > > > > > +      - runtime-code
> > > > > > > > > +      - runtime-data
> > > > > > > > > +
> > > > > > >
> > > > > > > As I mentioned a few times already, I don't think these compatibles
> > > > > > > should be introduced here.
> > > > > > >
> > > > > > > A reserved region has a specific purpose, and the compatible should be
> > > > > > > more descriptive than the enum above. If the consumer does not
> > > > > > > understand this purpose, it should simply treat the memory as reserved
> > > > > > > and not touch it. Alternatively, these regions can be referenced from
> > > > > > > other DT nodes using phandles if needed.
> > > > > >
> > > > > > We still need some description of what these regions are used for, so
> > > > > > that the payload can use the correct regions. I do not have any other
> > > > > > solution to this problem. We are in v7 at present. At least explain
> > > > > > where you want the compatible strings to be introduced.
> > > > > >
> > > > >
> > > > > My point is really that by themselves, these regions are not usable by
> > > > > either a payload or an OS that consumes this information. Unless there
> > > > > is some other information being provided (via DT I imagine) that
> > > > > describes how these things are supposed to be used, they are nothing
> > > > > more than memory reservations that should be honored, and providing
> > > > > this arbitrary set of labels is unnecessary.
> > > > >
> > > > > > What sort of extra detail are you looking for? Please be specific and
> > > > > > preferably add some suggestions so I can close this out ASAP.
> > > > > >
> > > > >
> > > > > A payload or OS can do nothing with a memory reservation called
> > > > > 'runtime-code' it it doesn't know what is inside.
> > >
> > > Agreed. The question is WHAT runtime-code? The compatible needs to answer that.
> > >
> > > For example, we have 'ramoops' as a compatible in reserved memory.
> > > That tells us *exactly* what's there. We know how to parse it. If we
> > > know ramoops is not supported, then we know we can toss it out and
> > > reclaim the memory.
> >
> > So if we said:
> >
> >    compatible = "runtime-code-efi"
> >
> > would that be OK? We seem to be in catch 22 here, because if I don't
> > mention EFI unhappy, but if I do, Ard is unhappy.
>
> Better yes, because then it is for something specific. However, AIUI,
> that's setup for the OS and defining that region is already defined by
> the EFI memory map. That's Ard's issue. If there's a need outside of
> the EFI to OS handoff,

There is a need. Here is part of the commit message again. If there is
something else you need to know, please ask.

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

> then you need to define what that usecase looks
> like. Describe the problem rather than present your solution.
>
> If this is all specific to EDK2 then it should say that rather than
> 'efi'. I imagine Ard would be happier with something tied to EDK2 than
> *all* UEFI. Though maybe the problem could be any implementation? IDK.
> Maybe it's TF-A that needs to define where the EFI runtime services
> region is and that needs to be passed all the way thru to the EFI
> implementation? So again, define the problem.

It is not specific to EDK2. Imagine this boot sequence:

- Platform Init (U-Boot) starts up
- U-Boot uses its platform knowledge to sets some ACPI tables and put
various things in memory
- U-Boot sets up some runtime code and data for the OS
- U-Boot jumps to the Tianocore payload **
- Payload (Tianocore) wants to know where the ACPI tables are, for example
- Tianocore needs to provide boot services to the OS, so needs to know
the memory map, etc.

** At this point we want to use DT to pass the required information.

Of course, Platform Init could be coreboot or Tianocore or some
strange private binary. Payload could be U-Boot or something else.
That is the point of this effort, to build interoperability.

>
> > What about the boottime code....you want to know which project it is from?
>
> I think it is the same.
>
> >
> > +      - acpi
> > +      - acpi-nvs
> >
> > Those two should be enough info, right?
>
> I think so. NVS is not a term I've heard in relation to ACPI, but that
> may just be my limited ACPI knowledge.

Perhaps it is only an Intel thing. It stands for Non-Volatile-Sleeping
Memory and it has various platform settings in a binary format that is
normally SoC-specific.

>
> > +      - boot-code
> > +      - boot-data
> >
> > For these, they don't pertain to the OS, so perhaps they are OK?
>
> Hard to tell that just from the name... 'boot' could be any component
> involved in booting including the OS.

 suggested that 'boot' should mean booting the OS. If the OS does lots
of fixup stuff at the start of it, I don't know what that is called.

So if boot-code is no good, what do you suggest?

Alternatively I could remove these for now, if it will help make progress.

>
> > In
> > any case, using a generic term like this makes some sense to me. We
> > can always add a new compatible like "efi-boottime-services" later. It
> > may be that the boottime services would be handled by the payload, so
> > not needed in all cases.
>
> Why later? You have a specific use in mind and I imagine Ard has
> thoughts on that.

Because we don't need it right away, and just want to make some progress.

Perhaps the problem here is that Linux has tied itself up in knots
with its EFI stuff and DT fixups and what-not. But this is not that.
It is a simple handoff between two pieces of firmware, Platform Init
and Payload. It has nothing to do with the OS. With Tianocore they are
typically combined, but with this usage they are split, and we can
swap out one project for another on either side of the DT interface.

I do have views on the 'EFI' opt-out with reserved-memory, if you are
interested, but that relates to the OS. If you are wanting to place
some constraints on /reserved-memory and /memory we could do that
e.g. that the DT and the map returned by EFI boot services must be
consistent. But as it is, the nodes are ignored by the OS when booting
with EFI, aren't they?

Regards,
Simon