Re: [PATCH v2] schemas: Add a schema for memory map

[Simon Glass]

Hi Rob,

On Tue, 22 Aug 2023 at 12:53, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Mon, Aug 21, 2023 at 2:48 PM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> >
> > The Devicespec specification skips over handling of a logical view of
> > the memory map, pointing users to the UEFI specification.
>
> It's more that the DT spec defines what is not used with UEFI. If UEFI
> covers more than the DT Spec defined, then we should look at that.
>
> I would look some into (IBM) PowerPC for any prior art in this area.
> Unfortunately, not publicly documented other than any users.

OK, but I'm not sure what you are looking for here. The DT (as
currently specified) is an incomplete description of memory, for
EFI-type firmware. I recall the ePAPR thing, but not much else. Any
pointers?

>
> > 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 initial schema for this mapping.
> >
> > Note that this is separate from the existing /memory and /reserved-memory
> > nodes, since it is mostly concerned with what the memory is used for. It
> > may cover only a small fraction of available memory, although it could be
> > used to signal which area of memory has ECC.
> >
> > For now, no attempt is made to create an exhaustive binding, so there are
> > some example types lists. This can be completed once this has passed
> > initial review.
>
> I don't have much interest in picking this up unless there's some
> wider agreement. From the previously referenced discussion[1], it
> didn't seem like there was. But none of those folk are Cc'ed here.

Yes, Ron pointed me to that...although it isn't quite the same thing.
That is implementing a way to pass SMBIOS and ACPI tables through to
Linux, right? But it does mention memory types, so I'll send a new
version with those people on cc, in case they don't look at linux-acpi
much.

But note, this is for *firmware* (on both sides of the interface).
Whether it is useful for Linux is another question. But in any case,
we should avoid having things in the DT which Linux cannot validate /
parse.

>
> > ---
> >
> > Changes in v2:
> > - Reword commit message
> >
> >  dtschema/schemas/memory-map.yaml | 51 ++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644 dtschema/schemas/memory-map.yaml
> >
> > diff --git a/dtschema/schemas/memory-map.yaml b/dtschema/schemas/memory-map.yaml
> > new file mode 100644
> > index 0000000..97e531e
> > --- /dev/null
> > +++ b/dtschema/schemas/memory-map.yaml
> > @@ -0,0 +1,51 @@
> > +# SPDX-License-Identifier: BSD-2-Clause
> > +# Copyright 2023 Google LLC
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/memory-map.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: /memory-map nodes
> > +description: |
> > +  Common properties always required in /memory-map nodes. These nodes are
> > +  intended to resolve the nonchalant clause 3.4.1 ("/memory node and UEFI")
> > +  in the Devicetree Specification.
> > +
> > +maintainers:
> > +  - Simon Glass <sjg@xxxxxxxxxxxx>
> > +
> > +properties:
> > +  $nodename:
> > +    const: '/'
>
> This goes in the root node?

I suppose I'm just confused about how the schema is described. I think
it is better to have a /memory-map node with subnodes of that for each
region.

>
> > +  usage:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: |
> > +      Describes the usage of the memory region, e.g.:
> > +
> > +        "acpi-reclaim", "acpi-nvs", "bootcode", "bootdata", "bootdata",
> > +        "runtime-code", "runtime-data"
>
> Can't these be covered by reserved-memory? The client is free to
> reclaim any regions if it knows what they are.

I don't see that in the schema, but given what you say, it is
definitely an option.

If the reserved-memory node hiding somewhere?

>
> > +  attr:
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +    description: |
> > +      Attributes possessed by this memory region:
> > +
> > +        "single-bit-ecc" - supports single-bit ECC
> > +        "multi-bit-ecc" - supports multiple-bit ECC
> > +        "no-ecc" - non-ECC memory
>
> Isn't this pretty much a property of a memory region as a whole. IOW,
> couldn't it just go into /memory node(s)?

Yes I think so. I wasn't sure if adding it there would break things,
but it seems not.

>
> > +
> > +patternProperties:
> > +  "^([a-z][a-z0-9\\-]+@[0-9a-f]+)?$":
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        minItems: 1
> > +        maxItems: 1024
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: true
> > +
> > +...
> > --
> > 2.42.0.rc1.204.g551eb34607-goog
>
> [1] https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@xxxxxxxxxxxxx/

I collected email addresses from that thread as best I could, and will
cc them on v3.

Regards,
Simon