lkml.org 
[lkml]   [2023]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v7 2/2] schemas: Add some common reserved-memory usages
    Hi Rob,

    On Mon, 16 Oct 2023 at 15:54, Simon Glass <sjg@chromium.org> wrote:
    >
    > Hi Rob,
    >
    > On Mon, 16 Oct 2023 at 10:50, Rob Herring <robh@kernel.org> wrote:
    > >
    > > On Fri, Oct 13, 2023 at 4:09 PM Simon Glass <sjg@chromium.org> wrote:
    > > >
    > > > Hi Rob,
    > > >
    > > > On Fri, 13 Oct 2023 at 13:42, Rob Herring <robh@kernel.org> wrote:
    > > > >
    > > > > On Fri, Oct 6, 2023 at 7:03 PM Simon Glass <sjg@chromium.org> wrote:
    > > > > >
    > > > > > Hi Ard,
    > > > > >
    > > > > > On Fri, 6 Oct 2023 at 17:00, Ard Biesheuvel <ardb@kernel.org> wrote:
    > > > > > >
    > > > > > > On Fri, 6 Oct 2023 at 20:17, Simon Glass <sjg@chromium.org> wrote:
    > > > > > > >
    > > > > > > > Hi Ard,
    > > > > > > >
    > > > > > > > On Fri, 6 Oct 2023 at 11:33, Ard Biesheuvel <ardb@kernel.org> wrote:
    > > > > > > > >
    > > > > > > > > On Mon, 2 Oct 2023 at 19:54, Simon Glass <sjg@chromium.org> wrote:
    > > > > > > > > >
    > > > > > > > > > Hi Rob,
    > > > > > > > > >
    > > > > > > > > > On Tue, 26 Sept 2023 at 13:42, Simon Glass <sjg@chromium.org> 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@chromium.org>
    > > > > > > > > > > ---
    > > > > > > > > > >
    > > > > > > > > > > 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@chromium.org>
    > > > > > > > > > > +
    > > > > > > > > > > +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?

    Can this be applied, please? If there are further comments, please let me know.

    Regards,
    Simon

    \
     
     \ /
      Last update: 2023-10-31 16:57    [W:3.435 / U:0.284 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site