[PATCH v6 1/2] schemas: Add some common reserved-memory usages

[Simon Glass]

Hi Rob,

On Thu, 21 Sept 2023 at 10:45, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Rob,
>
> On Thu, 21 Sept 2023 at 09:20, Rob Herring <robh at kernel.org> wrote:
> >
> > On Thu, Sep 21, 2023 at 9:38 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Thu, 21 Sept 2023 at 08:25, Rob Herring <robh at kernel.org> wrote:
> > > >
> > > > On Thu, Sep 7, 2023 at 4:40 PM Simon Glass <sjg at 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.
> > > > >
> > > > > 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 at chromium.org>
> > > > > ---
> > > > >
> > > > > 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..4889f59
> > > > > --- /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 at chromium.org>
> > > > > +
> > > > > +allOf:
> > > > > +  - $ref: reserved-memory.yaml
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    description: |
> > > > > +      This describes some common memory reservations:
> > > > > +
> > > > > +         acpi-reclaim: Contains ACPI tables; memory may be reclaimed when the
> > > > > +           tables are no-longer needed
> > > >
> > > > I think you are mixing 2 things with the name here. What the memory
> > > > contains and what to do with it. You don't need the latter. The
> > > > consumer of the region will know what to do with it if anything based
> > > > on knowing what is in the region. For example, The DTB passed to the
> > > > OS is typically in a reserved region (probably still /mem-reserve/
> > > > though). The DTB may remain there forever or the OS could copy it
> > > > somewhere else and free the reserved region. The Linux kernel does
> > > > both depending on the arch. (Of course there is no "dtb" compatible
> > > > because we have to pass the location of the dtb to even find the
> > > > reserved regions in the first place.)
> > > >
> > > > So the question here is whether just "acpi" (or "acpi-tables"?) would
> > > > be explicit enough?
> > >
> > > Yes acpi-tables would be enough.
> > >
> > > >
> > > > > +         acpi-nvs: Contains ACPI Non-volatile-storage data; memory may be
> > > > > +           reclaimed when the tables are no-longer needed
> > > >
> > > > No need to say anything about reclaiming.
> > >
> > > OK...so what about all that discussion about being able to reclaim the
> > > memory if you know what it is for? Where should that be written? Or is
> > > it somewhere else already?
> >
> > Reclaiming is a policy of the consumer of the data. It belongs in the
> > documentation of the consumer if you are going to document it.
> >
> > The global policy is you can't use reserved regions and you can't
> > reclaim them if you don't know what they are. If you want to add
> > something to that effect in reserved-memory.yaml or the spec, that's
> > fine, but that's not a per compatible thing.
>
> OK I will do that.
>
> >
> > > > I know some ACPIisms (e.g. DSDT), but I don't know what NVS or
> > > > "Non-volatile-storage data" is in an ACPI context.
> > > >
> > > > > +         boot-code: Contains code used for booting; memory may be reclaimed by
> > > > > +           the OS when it is running
> > > > > +         boot-code: Contains data used for booting; memory may be reclaimed by
> > > >
> > > > boot-data?
> > >
> > > Yes
> > >
> > > >
> > > > > +           the OS when it is running
> > > >
> > > > I thought these were for stages before we get to OS?
> > >
> > > Yes...but of course these will be passed on to the OS in some form.
> > > See above re reclaiming.
> > >
> > > >
> > > > > +         runtime-code: Contains code used for interacting with the system when
> > > > > +           running; memory may be reclaimed if this code is not called
> > > > > +         runtime-data: Contains data used for interacting with the system when
> > > > > +           running; memory may be reclaimed if the runtime code is not used
> > > >
> > > > "boot" vs. "runtime" seem too vague. However, if these mean EFI boot
> > > > time services and runtime services, then I understand exactly what
> > > > they are. In that case dropping 'uefi,' was a mistake. But EFI has its
> > > > own way to define these regions, right?
> > >
> > > I really don't want to do another round of that circle. I was asked to
> > > drop mention of EFI which I did. If these are too vague, what should I
> > > do instead? Perhaps:
> > >
> > > boot-code / data
> > > os-code / data
> >
> > The kernel is boot code (and os code and runtime code). Can I use
>
> What do you mean by that? The kernel is an OS. It might have an EFI
> stub or other stuff, but it is not a boot loader. We have to have some
> generally accepted terms for what is the OS and what is the firmware.
>
> > these for the kernel image? Certainly not. But they are too vague to
> > understand when to use them and when not to. Are they specific to EDK2
> > implementation? Then perhaps they need an 'edk2' prefix.
> >
> > Either these are related to EFI boot time services and runtime
> > services or they aren't. Which is it? If they are related, then EFI
> > should certainly be mentioned. Review comments are wrong all the time
> > when the reasoning is missing or misunderstood. Please back up and
> > explain why these are needed. Maybe it's buried in prior threads
> > somewhere, but I'm not going back to re-read all that. This patch has
> > to stand on its own.
>
> For my email client it is buried in this thread, see the ~20 messages
> above, or [1]. I took all the EFI stuff out of here because it was
> going nowhere.
>
> The text I removed in this version is:
>
> >>>
> 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 I tried to explain it in terms of Tianocore being split into two
> parts, one of which does Platform Init (e.g. silicon setup) and the
> other which boots the OS. But that got lost is discussion about DXEs,
> etc. There is no other communication mechanism between these two
> pieces, nor does either piece know (at runtime or build time) whether
> the other is there, or whether it is something else (e.g. coreboot or
> U-Boot). This allows supporting Platform Init / Payload combinations
> like coreboot->Tianocore, Tianocore->Tianocore. coreboot->U-Boot,
> etc., i.e. universal compatibility.
>
> IMhO this binding doesn't need to be about EFI. The concept of code
> which is used during boot and code which is used by the OS doesn't
> seem specific to EFI. After all, ARM has PSCI code which presumably
> lives somewhere.
>
> So do you really want it to mention EFI?
>
> I have volunteered to take this on on behalf of the EDK2 people, who
> are copied on this thread and hoping / relying on something being
> resolved.

Do you have any thoughts on the above? I'd really like to understand
what needs to be done here. We have a meeting tomorrow on the handoff
format and I don't have a lot to show for my efforts of the last 5
weeks.

Regards,
SImon

> [1] https://lore.kernel.org/lkml/CAMj1kXHGpCt8qkd6XYQF8mMdivQkTnEWjv6NzsFK=+N72LAn=Q@mail.gmail.com/