linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/4] Add reserved-memory
@ 2023-08-30 23:17 Simon Glass
  2023-08-30 23:17 ` [PATCH v5 2/4] Bring in some other reserved-memory files Simon Glass
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Simon Glass @ 2023-08-30 23:17 UTC (permalink / raw)
  To: devicetree
  Cc: Maximilian Brune, ron minnich, Tom Rini, Dhaval Sharma,
	U-Boot Mailing List, Mark Rutland, Yunhui Cui, linux-acpi,
	Ard Biesheuvel, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Rob Herring, Chiu Chasel, Simon Glass

Bring in this file from Linux v6.5

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v4)

Changes in v4:
- New patch

 .../reserved-memory/reserved-memory.yaml      | 181 ++++++++++++++++++
 1 file changed, 181 insertions(+)
 create mode 100644 dtschema/schemas/reserved-memory/reserved-memory.yaml

diff --git a/dtschema/schemas/reserved-memory/reserved-memory.yaml b/dtschema/schemas/reserved-memory/reserved-memory.yaml
new file mode 100644
index 0000000..c680e39
--- /dev/null
+++ b/dtschema/schemas/reserved-memory/reserved-memory.yaml
@@ -0,0 +1,181 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/reserved-memory.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: /reserved-memory Child Node Common
+
+maintainers:
+  - devicetree-spec@vger.kernel.org
+
+description: >
+  Reserved memory is specified as a node under the /reserved-memory node. The
+  operating system shall exclude reserved memory from normal usage one can
+  create child nodes describing particular reserved (excluded from normal use)
+  memory regions. Such memory regions are usually designed for the special
+  usage by various device drivers.
+
+  Each child of the reserved-memory node specifies one or more regions
+  of reserved memory. Each child node may either use a 'reg' property to
+  specify a specific range of reserved memory, or a 'size' property with
+  optional constraints to request a dynamically allocated block of
+  memory.
+
+  Following the generic-names recommended practice, node names should
+  reflect the purpose of the node (ie. "framebuffer" or "dma-pool").
+  Unit address (@<address>) should be appended to the name if the node
+  is a static allocation.
+
+properties:
+  reg: true
+
+  size:
+    oneOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - $ref: /schemas/types.yaml#/definitions/uint64
+    description: >
+      Length based on parent's \#size-cells. Size in bytes of memory to
+      reserve.
+
+  alignment:
+    oneOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - $ref: /schemas/types.yaml#/definitions/uint64
+    description: >
+      Length based on parent's \#size-cells. Address boundary for
+      alignment of allocation.
+
+  alloc-ranges:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: >
+      Address and Length pairs. Specifies regions of memory that are
+      acceptable to allocate from.
+
+  iommu-addresses:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: >
+      A list of phandle and specifier pairs that describe static IO virtual
+      address space mappings and carveouts associated with a given reserved
+      memory region. The phandle in the first cell refers to the device for
+      which the mapping or carveout is to be created.
+
+      The specifier consists of an address/size pair and denotes the IO
+      virtual address range of the region for the given device. The exact
+      format depends on the values of the "#address-cells" and "#size-cells"
+      properties of the device referenced via the phandle.
+
+      When used in combination with a "reg" property, an IOVA mapping is to
+      be established for this memory region. One example where this can be
+      useful is to create an identity mapping for physical memory that the
+      firmware has configured some hardware to access (such as a bootsplash
+      framebuffer).
+
+      If no "reg" property is specified, the "iommu-addresses" property
+      defines carveout regions in the IOVA space for the given device. This
+      can be useful if a certain memory region should not be mapped through
+      the IOMMU.
+
+  no-map:
+    type: boolean
+    description: >
+      Indicates the operating system must not create a virtual mapping
+      of the region as part of its standard mapping of system memory,
+      nor permit speculative access to it under any circumstances other
+      than under the control of the device driver using the region.
+
+  reusable:
+    type: boolean
+    description: >
+      The operating system can use the memory in this region with the
+      limitation that the device driver(s) owning the region need to be
+      able to reclaim it back. Typically that means that the operating
+      system can use that region to store volatile or cached data that
+      can be otherwise regenerated or migrated elsewhere.
+
+allOf:
+  - if:
+      required:
+        - no-map
+
+    then:
+      not:
+        required:
+          - reusable
+
+  - if:
+      required:
+        - reusable
+
+    then:
+      not:
+        required:
+          - no-map
+
+oneOf:
+  - oneOf:
+      - required:
+          - reg
+
+      - required:
+          - size
+
+  - oneOf:
+      # IOMMU reservations
+      - required:
+          - iommu-addresses
+
+      # IOMMU mappings
+      - required:
+          - reg
+          - iommu-addresses
+
+additionalProperties: true
+
+examples:
+  - |
+    / {
+      compatible = "foo";
+      model = "foo";
+
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        ranges;
+
+        adsp_resv: reservation-adsp {
+          /*
+           * Restrict IOVA mappings for ADSP buffers to the 512 MiB region
+           * from 0x40000000 - 0x5fffffff. Anything outside is reserved by
+           * the ADSP for I/O memory and private memory allocations.
+           */
+          iommu-addresses = <&adsp 0x0 0x00000000 0x00 0x40000000>,
+                            <&adsp 0x0 0x60000000 0xff 0xa0000000>;
+        };
+
+        fb: framebuffer@90000000 {
+          reg = <0x0 0x90000000 0x0 0x00800000>;
+          iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00800000>;
+        };
+      };
+
+      bus@0 {
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges = <0x0 0x0 0x0 0x40000000>;
+
+        adsp: adsp@2990000 {
+          reg = <0x2990000 0x2000>;
+          memory-region = <&adsp_resv>;
+        };
+
+        dc0: display@15200000 {
+          reg = <0x15200000 0x10000>;
+          memory-region = <&fb>;
+        };
+      };
+    };
+...
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 2/4] Bring in some other reserved-memory files
  2023-08-30 23:17 [PATCH v5 1/4] Add reserved-memory Simon Glass
@ 2023-08-30 23:17 ` Simon Glass
  2023-08-30 23:17 ` [PATCH v5 3/4] schemas: Add some common reserved-memory usages Simon Glass
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2023-08-30 23:17 UTC (permalink / raw)
  To: devicetree
  Cc: Maximilian Brune, ron minnich, Tom Rini, Dhaval Sharma,
	U-Boot Mailing List, Mark Rutland, Yunhui Cui, linux-acpi,
	Ard Biesheuvel, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Rob Herring, Chiu Chasel, Simon Glass

Add schema yaml files from v6.5 which are not vendor-specific, nor
Linux-specific.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v5:
- Trim back to just a subset of mostly generic schemas

Changes in v4:
- New patch

 .../schemas/reserved-memory/framebuffer.yaml  | 52 ++++++++++
 .../reserved-memory/memory-region.yaml        | 40 ++++++++
 .../reserved-memory/shared-dma-pool.yaml      | 97 +++++++++++++++++++
 3 files changed, 189 insertions(+)
 create mode 100644 dtschema/schemas/reserved-memory/framebuffer.yaml
 create mode 100644 dtschema/schemas/reserved-memory/memory-region.yaml
 create mode 100644 dtschema/schemas/reserved-memory/shared-dma-pool.yaml

diff --git a/dtschema/schemas/reserved-memory/framebuffer.yaml b/dtschema/schemas/reserved-memory/framebuffer.yaml
new file mode 100644
index 0000000..851ec24
--- /dev/null
+++ b/dtschema/schemas/reserved-memory/framebuffer.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/framebuffer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: /reserved-memory framebuffer node
+
+maintainers:
+  - devicetree-spec@vger.kernel.org
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: framebuffer
+    description: >
+      This indicates a region of memory meant to be used as a framebuffer for
+      a set of display devices. It can be used by an operating system to keep
+      the framebuffer from being overwritten and use it as the backing memory
+      for a display device (such as simple-framebuffer).
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    / {
+        compatible = "foo";
+        model = "foo";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        chosen {
+            framebuffer {
+                compatible = "simple-framebuffer";
+                memory-region = <&fb>;
+            };
+        };
+
+        reserved-memory {
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges;
+
+            fb: framebuffer@80000000 {
+                compatible = "framebuffer";
+                reg = <0x80000000 0x007e9000>;
+            };
+        };
+    };
+...
diff --git a/dtschema/schemas/reserved-memory/memory-region.yaml b/dtschema/schemas/reserved-memory/memory-region.yaml
new file mode 100644
index 0000000..592f180
--- /dev/null
+++ b/dtschema/schemas/reserved-memory/memory-region.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/memory-region.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Reserved Memory Region
+
+maintainers:
+  - devicetree-spec@vger.kernel.org
+
+description: |
+  Regions in the /reserved-memory node may be referenced by other device
+  nodes by adding a memory-region property to the device node.
+
+select: true
+
+properties:
+  memory-region:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: >
+      Phandle to a /reserved-memory child node assigned to the device.
+
+  memory-region-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: >
+      A list of names, one for each corresponding entry in the
+      memory-region property
+
+additionalProperties: true
+
+examples:
+  - |
+    fb0: video@12300000 {
+        /* ... */
+        reg = <0x12300000 0x1000>;
+        memory-region = <&display_reserved>;
+    };
+
+...
diff --git a/dtschema/schemas/reserved-memory/shared-dma-pool.yaml b/dtschema/schemas/reserved-memory/shared-dma-pool.yaml
new file mode 100644
index 0000000..457de09
--- /dev/null
+++ b/dtschema/schemas/reserved-memory/shared-dma-pool.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/shared-dma-pool.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: /reserved-memory DMA pool
+
+maintainers:
+  - devicetree-spec@vger.kernel.org
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    oneOf:
+      - const: shared-dma-pool
+        description: >
+          This indicates a region of memory meant to be used as a shared
+          pool of DMA buffers for a set of devices. It can be used by an
+          operating system to instantiate the necessary pool management
+          subsystem if necessary.
+
+      - const: restricted-dma-pool
+        description: >
+          This indicates a region of memory meant to be used as a pool
+          of restricted DMA buffers for a set of devices. The memory
+          region would be the only region accessible to those devices.
+          When using this, the no-map and reusable properties must not
+          be set, so the operating system can create a virtual mapping
+          that will be used for synchronization. The main purpose for
+          restricted DMA is to mitigate the lack of DMA access control
+          on systems without an IOMMU, which could result in the DMA
+          accessing the system memory at unexpected times and/or
+          unexpected addresses, possibly leading to data leakage or
+          corruption. The feature on its own provides a basic level of
+          protection against the DMA overwriting buffer contents at
+          unexpected times. However, to protect against general data
+          leakage and system memory corruption, the system needs to
+          provide way to lock down the memory access, e.g., MPU. Note
+          that since coherent allocation needs remapping, one must set
+          up another device coherent pool by shared-dma-pool and use
+          dma_alloc_from_dev_coherent instead for atomic coherent
+          allocation.
+
+  linux,cma-default:
+    type: boolean
+    description: >
+      If this property is present, then Linux will use the region for
+      the default pool of the contiguous memory allocator.
+
+  linux,dma-default:
+    type: boolean
+    description: >
+      If this property is present, then Linux will use the region for
+      the default pool of the consistent DMA allocator.
+
+if:
+  properties:
+    compatible:
+      contains:
+        const: restricted-dma-pool
+then:
+  properties:
+    no-map: false
+    reusable: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+      reserved-memory {
+          #address-cells = <1>;
+          #size-cells = <1>;
+          ranges;
+
+          /* global autoconfigured region for contiguous allocations */
+          linux,cma {
+              compatible = "shared-dma-pool";
+              reusable;
+              size = <0x4000000>;
+              alignment = <0x2000>;
+              linux,cma-default;
+          };
+
+          display_reserved: framebuffer@78000000 {
+              reg = <0x78000000 0x800000>;
+          };
+
+          restricted_dma_reserved: restricted-dma-pool@50000000 {
+              compatible = "restricted-dma-pool";
+              reg = <0x50000000 0x4000000>;
+          };
+      };
+
+...
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-08-30 23:17 [PATCH v5 1/4] Add reserved-memory Simon Glass
  2023-08-30 23:17 ` [PATCH v5 2/4] Bring in some other reserved-memory files Simon Glass
@ 2023-08-30 23:17 ` Simon Glass
  2023-09-05 21:44   ` Ard Biesheuvel
  2023-08-30 23:17 ` [PATCH v5 4/4] memory: Add ECC properties Simon Glass
  2023-09-07 16:41 ` [PATCH v5 1/4] Add reserved-memory Rob Herring
  3 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2023-08-30 23:17 UTC (permalink / raw)
  To: devicetree
  Cc: Maximilian Brune, ron minnich, Tom Rini, Dhaval Sharma,
	U-Boot Mailing List, Mark Rutland, Yunhui Cui, linux-acpi,
	Ard Biesheuvel, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Rob Herring, Chiu Chasel, Simon Glass

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.

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.

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@chromium.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@chromium.org>
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  $nodename:
+    enum:
+      - acpi-reclaim
+      - acpi-nvs
+      - boot-code
+      - boot-data
+      - runtime-code
+      - runtime-data
+
+  reg:
+    description: region of memory that is reserved for the purpose indicated
+      by the node name.
+
+required:
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    reserved-memory {
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        boot-code@12340000 {
+            reg = <0x12340000 0x00800000>;
+        };
+
+        boot-data@43210000 {
+            reg = <0x43210000 0x00800000>;
+        };
+    };
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 4/4] memory: Add ECC properties
  2023-08-30 23:17 [PATCH v5 1/4] Add reserved-memory Simon Glass
  2023-08-30 23:17 ` [PATCH v5 2/4] Bring in some other reserved-memory files Simon Glass
  2023-08-30 23:17 ` [PATCH v5 3/4] schemas: Add some common reserved-memory usages Simon Glass
@ 2023-08-30 23:17 ` Simon Glass
  2023-09-07 16:58   ` Rob Herring
  2023-09-07 16:41 ` [PATCH v5 1/4] Add reserved-memory Rob Herring
  3 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2023-08-30 23:17 UTC (permalink / raw)
  To: devicetree
  Cc: Maximilian Brune, ron minnich, Tom Rini, Dhaval Sharma,
	U-Boot Mailing List, Mark Rutland, Yunhui Cui, linux-acpi,
	Ard Biesheuvel, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Rob Herring, Chiu Chasel, Simon Glass

Some memories provide ECC detection and/or correction. For software which
wants to check memory, it is helpful to see which regions provide this
feature.

Add this as a property of the /memory nodes, since it presumably follows
the hardware-level memory system.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v5:
- Redo to make this property specific to ECC
- Provide properties both for detection and correction

Changes in v3:
- Add new patch to update the /memory nodes

 dtschema/schemas/memory.yaml | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/dtschema/schemas/memory.yaml b/dtschema/schemas/memory.yaml
index 1d74410..944aa9f 100644
--- a/dtschema/schemas/memory.yaml
+++ b/dtschema/schemas/memory.yaml
@@ -34,7 +34,37 @@ patternProperties:
         description:
           For the purpose of identification, each NUMA node is associated with
           a unique token known as a node id.
+      ecc-detection:
+        $ref: /schemas/types.yaml#/definitions/string
+        enum:
+          - none
+          - single-bit
+          - multi-bit
+        description: |
+          If present, this inidcates the type of memory errors which can be
+          detected and reported by the Error-Correction Code (ECC) memory
+          subsystem:
 
+            none       - No error detection is possible
+            single-bit - Detects and reports single-bit ECC errors
+            multi-bit  - Detects and reports multiple-bit ECC errors
+
+          If not present, this is equivalent to 'none'.
+      ecc-correction:
+        $ref: /schemas/types.yaml#/definitions/string
+        enum:
+          - none
+          - single-bit
+          - multi-bit
+        description: |
+          If present, this inidcates the type of memory errors which can be
+          corrected by the Error-Correction Code (ECC) memory subsystem:
+
+            none       - No error correction is possible
+            single-bit - Corrects single-bit ECC errors
+            multi-bit  - Corrects multiple-bit ECC errors
+
+          If not present, this is equivalent to 'none'.
 
     required:
       - device_type
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-08-30 23:17 ` [PATCH v5 3/4] schemas: Add some common reserved-memory usages Simon Glass
@ 2023-09-05 21:44   ` Ard Biesheuvel
  2023-09-06 14:34     ` Rob Herring
  0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2023-09-05 21:44 UTC (permalink / raw)
  To: Simon Glass
  Cc: devicetree, Maximilian Brune, ron minnich, Tom Rini,
	Dhaval Sharma, U-Boot Mailing List, Mark Rutland, Yunhui Cui,
	linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml, Rob Herring,
	Chiu Chasel

On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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.
>
> 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@chromium.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@chromium.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.


> +  reg:
> +    description: region of memory that is reserved for the purpose indicated
> +      by the node name.
> +
> +required:
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        boot-code@12340000 {
> +            reg = <0x12340000 0x00800000>;
> +        };
> +
> +        boot-data@43210000 {
> +            reg = <0x43210000 0x00800000>;
> +        };
> +    };
> --
> 2.42.0.rc2.253.gd59a3bf2b4-goog
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-05 21:44   ` Ard Biesheuvel
@ 2023-09-06 14:34     ` Rob Herring
  2023-09-06 14:53       ` Simon Glass
  0 siblings, 1 reply; 19+ messages in thread
From: Rob Herring @ 2023-09-06 14:34 UTC (permalink / raw)
  To: Ard Biesheuvel, Simon Glass
  Cc: devicetree, Maximilian Brune, ron minnich, Tom Rini,
	Dhaval Sharma, U-Boot Mailing List, Mark Rutland, Yunhui Cui,
	linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml, Chiu Chasel

On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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.

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

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-06 14:34     ` Rob Herring
@ 2023-09-06 14:53       ` Simon Glass
  2023-09-06 16:08         ` Ard Biesheuvel
  0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2023-09-06 14:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: Ard Biesheuvel, devicetree, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

Hi Rob, Ard,

On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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
"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

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

Regards,
Simon

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-06 14:53       ` Simon Glass
@ 2023-09-06 16:08         ` Ard Biesheuvel
       [not found]           ` <CAPnjgZ1oGF0Ni3RhK4fv6mJk40YjqyFVJxt6FfS9AW2rkcs9iA@mail.gmail.com>
  0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2023-09-06 16:08 UTC (permalink / raw)
  To: Simon Glass
  Cc: Rob Herring, devicetree, Maximilian Brune, ron minnich, Tom Rini,
	Dhaval Sharma, U-Boot Mailing List, Mark Rutland, Yunhui Cui,
	linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml, Chiu Chasel

On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rob, Ard,
>
> On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
       [not found]           ` <CAPnjgZ1oGF0Ni3RhK4fv6mJk40YjqyFVJxt6FfS9AW2rkcs9iA@mail.gmail.com>
@ 2023-09-07 13:31             ` Ard Biesheuvel
  2023-09-07 13:56               ` Simon Glass
  0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2023-09-07 13:31 UTC (permalink / raw)
  To: Simon Glass
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ard,
>
> On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
>> >
>> > Hi Rob, Ard,
>> >
>> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
>> > >
>> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>> > > >
>> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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.

All the ACPI and UEFI lingo needs to be dropped. This is relevant only
to the OS, and only if the prior stage exposes UEFI interfaces, in
which case it does not need this binding.

So on one side, there is the requirement for each memory reservation
to be described with sufficient detail so that a subsequent boot stage
(firmware or OS) can use it for its intended purpose, provided that
this boot stage is aware of its purpose (i.e., it has a driver that
matches on the compatible string in question, and actually maps/uses
the memory)

On the other side, we need to describe how a memory reservation should
be treated if the boot stage doesn't know its purpose, has no interest
in using it or has consumed the contents and has no longer a need for
the region. We already have no-map to describe that the memory should
never be mapped (and this may be disregarded by an actual driver for
the region). I imagine we might add 'discardable' as a boolean DT
property, meaning that stage N may use the memory whichever way it
wants if it is not going to use it for its intended purpose, provided
that it deletes the node from the DT before passing it on to stage
N+1.

One thing that needs to be clarified is how this binding interacts
with /memory nodes. I assume that currently, /reserved-memory is
independent, i.e., it could describe mappable memory that is not
covered by /memory at all. If this is the case, we have to decide
whether or not discardable regions can be treated in the same way, or
whether we should require that discardable regions are covered by
/memory.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 13:31             ` Ard Biesheuvel
@ 2023-09-07 13:56               ` Simon Glass
  2023-09-07 14:12                 ` Ard Biesheuvel
  2023-09-07 15:43                 ` Rob Herring
  0 siblings, 2 replies; 19+ messages in thread
From: Simon Glass @ 2023-09-07 13:56 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

Hi Ard,

On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ard,
> >
> > On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
> >> >
> >> > Hi Rob, Ard,
> >> >
> >> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
> >> > >
> >> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >> > > >
> >> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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.

>
> All the ACPI and UEFI lingo needs to be dropped. This is relevant only
> to the OS, and only if the prior stage exposes UEFI interfaces, in
> which case it does not need this binding.

OK I can drop those from the commit message.

>
> So on one side, there is the requirement for each memory reservation
> to be described with sufficient detail so that a subsequent boot stage
> (firmware or OS) can use it for its intended purpose, provided that
> this boot stage is aware of its purpose (i.e., it has a driver that
> matches on the compatible string in question, and actually maps/uses
> the memory)
>
> On the other side, we need to describe how a memory reservation should
> be treated if the boot stage doesn't know its purpose, has no interest
> in using it or has consumed the contents and has no longer a need for
> the region. We already have no-map to describe that the memory should
> never be mapped (and this may be disregarded by an actual driver for
> the region). I imagine we might add 'discardable' as a boolean DT
> property, meaning that stage N may use the memory whichever way it
> wants if it is not going to use it for its intended purpose, provided
> that it deletes the node from the DT before passing it on to stage
> N+1.

OK. For now I think that everything is discardable, so long as the
Payload knows the purpose and that it not needed. That is what Rob
seemed to be saying. If we add 'discardable', does that mean that
things default to non-discardable? Would that not be a change of
behaviour for existing users?

>
> One thing that needs to be clarified is how this binding interacts
> with /memory nodes. I assume that currently, /reserved-memory is
> independent, i.e., it could describe mappable memory that is not
> covered by /memory at all. If this is the case, we have to decide
> whether or not discardable regions can be treated in the same way, or
> whether we should require that discardable regions are covered by
> /memory.

I would expect all memory to be described in /memory nodes. What is
the use case for omitting it? Are you thinking of SRAM, etc?

Regards,
Simon

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 13:56               ` Simon Glass
@ 2023-09-07 14:12                 ` Ard Biesheuvel
  2023-09-07 14:50                   ` Simon Glass
  2023-09-07 15:43                 ` Rob Herring
  1 sibling, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2023-09-07 14:12 UTC (permalink / raw)
  To: Simon Glass
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ard,
>
> On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>
> > >> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
> > >> >
> > >> > Hi Rob, Ard,
> > >> >
> > >> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
> > >> > >
> > >> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >> > > >
> > >> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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?

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.

...
> >
> > So on one side, there is the requirement for each memory reservation
> > to be described with sufficient detail so that a subsequent boot stage
> > (firmware or OS) can use it for its intended purpose, provided that
> > this boot stage is aware of its purpose (i.e., it has a driver that
> > matches on the compatible string in question, and actually maps/uses
> > the memory)
> >
> > On the other side, we need to describe how a memory reservation should
> > be treated if the boot stage doesn't know its purpose, has no interest
> > in using it or has consumed the contents and has no longer a need for
> > the region. We already have no-map to describe that the memory should
> > never be mapped (and this may be disregarded by an actual driver for
> > the region). I imagine we might add 'discardable' as a boolean DT
> > property, meaning that stage N may use the memory whichever way it
> > wants if it is not going to use it for its intended purpose, provided
> > that it deletes the node from the DT before passing it on to stage
> > N+1.
>
> OK. For now I think that everything is discardable, so long as the
> Payload knows the purpose and that it not needed. That is what Rob
> seemed to be saying. If we add 'discardable', does that mean that
> things default to non-discardable? Would that not be a change of
> behaviour for existing users?
>

Excellent question. I always assumed that /reserved-memory nodes with
a defined base address would be honored regardless.

> >
> > One thing that needs to be clarified is how this binding interacts
> > with /memory nodes. I assume that currently, /reserved-memory is
> > independent, i.e., it could describe mappable memory that is not
> > covered by /memory at all. If this is the case, we have to decide
> > whether or not discardable regions can be treated in the same way, or
> > whether we should require that discardable regions are covered by
> > /memory.
>
> I would expect all memory to be described in /memory nodes. What is
> the use case for omitting it? Are you thinking of SRAM, etc?
>

Indeed.

For example, the SynQuacer platform has SRAM that it uses for
platform-specific purposes (early stack and heap, passing information
between secure and non-secure at boot), but it is not wired into the
hardware cache coherency protocol, so it only tolerates non-shareable
mappings. Describing this as discardable would be accurate (given that
it is only used early in the boot), but that doesn't mean it can be
used as general purpose memory by the OS.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 14:12                 ` Ard Biesheuvel
@ 2023-09-07 14:50                   ` Simon Glass
  2023-09-07 15:07                     ` Ard Biesheuvel
  0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2023-09-07 14:50 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

Hi Ard,

On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ard,
> >
> > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >>
> > > >> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
> > > >> >
> > > >> > Hi Rob, Ard,
> > > >> >
> > > >> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
> > > >> > >
> > > >> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >> > > >
> > > >> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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.

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

>
> ...
> > >
> > > So on one side, there is the requirement for each memory reservation
> > > to be described with sufficient detail so that a subsequent boot stage
> > > (firmware or OS) can use it for its intended purpose, provided that
> > > this boot stage is aware of its purpose (i.e., it has a driver that
> > > matches on the compatible string in question, and actually maps/uses
> > > the memory)
> > >
> > > On the other side, we need to describe how a memory reservation should
> > > be treated if the boot stage doesn't know its purpose, has no interest
> > > in using it or has consumed the contents and has no longer a need for
> > > the region. We already have no-map to describe that the memory should
> > > never be mapped (and this may be disregarded by an actual driver for
> > > the region). I imagine we might add 'discardable' as a boolean DT
> > > property, meaning that stage N may use the memory whichever way it
> > > wants if it is not going to use it for its intended purpose, provided
> > > that it deletes the node from the DT before passing it on to stage
> > > N+1.
> >
> > OK. For now I think that everything is discardable, so long as the
> > Payload knows the purpose and that it not needed. That is what Rob
> > seemed to be saying. If we add 'discardable', does that mean that
> > things default to non-discardable? Would that not be a change of
> > behaviour for existing users?
> >
>
> Excellent question. I always assumed that /reserved-memory nodes with
> a defined base address would be honored regardless.

OK, anyway I think we can park that for now so as not to add more
loose ends here.

>
> > >
> > > One thing that needs to be clarified is how this binding interacts
> > > with /memory nodes. I assume that currently, /reserved-memory is
> > > independent, i.e., it could describe mappable memory that is not
> > > covered by /memory at all. If this is the case, we have to decide
> > > whether or not discardable regions can be treated in the same way, or
> > > whether we should require that discardable regions are covered by
> > > /memory.
> >
> > I would expect all memory to be described in /memory nodes. What is
> > the use case for omitting it? Are you thinking of SRAM, etc?
> >
>
> Indeed.
>
> For example, the SynQuacer platform has SRAM that it uses for
> platform-specific purposes (early stack and heap, passing information
> between secure and non-secure at boot), but it is not wired into the
> hardware cache coherency protocol, so it only tolerates non-shareable
> mappings. Describing this as discardable would be accurate (given that
> it is only used early in the boot), but that doesn't mean it can be
> used as general purpose memory by the OS.

OK.

Regards,
Simon

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 14:50                   ` Simon Glass
@ 2023-09-07 15:07                     ` Ard Biesheuvel
  2023-09-07 15:56                       ` Simon Glass
  0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2023-09-07 15:07 UTC (permalink / raw)
  To: Simon Glass
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

On Thu, 7 Sept 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ard,
>
> On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >>
> > > > >> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
> > > > >> >
> > > > >> > Hi Rob, Ard,
> > > > >> >
> > > > >> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
> > > > >> > >
> > > > >> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >> > > >
> > > > >> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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.

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.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 13:56               ` Simon Glass
  2023-09-07 14:12                 ` Ard Biesheuvel
@ 2023-09-07 15:43                 ` Rob Herring
  1 sibling, 0 replies; 19+ messages in thread
From: Rob Herring @ 2023-09-07 15:43 UTC (permalink / raw)
  To: Simon Glass
  Cc: Ard Biesheuvel, Devicetree Discuss, Maximilian Brune,
	ron minnich, Tom Rini, Dhaval Sharma, U-Boot Mailing List,
	Mark Rutland, Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan,
	Guo Dong, lkml, Chiu Chasel

On Thu, Sep 7, 2023 at 8:56 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ard,
>
> On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>
> > >> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
> > >> >
> > >> > Hi Rob, Ard,
> > >> >
> > >> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
> > >> > >
> > >> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >> > > >
> > >> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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.
>
> >
> > All the ACPI and UEFI lingo needs to be dropped. This is relevant only
> > to the OS, and only if the prior stage exposes UEFI interfaces, in
> > which case it does not need this binding.
>
> OK I can drop those from the commit message.
>
> >
> > So on one side, there is the requirement for each memory reservation
> > to be described with sufficient detail so that a subsequent boot stage
> > (firmware or OS) can use it for its intended purpose, provided that
> > this boot stage is aware of its purpose (i.e., it has a driver that
> > matches on the compatible string in question, and actually maps/uses
> > the memory)
> >
> > On the other side, we need to describe how a memory reservation should
> > be treated if the boot stage doesn't know its purpose, has no interest
> > in using it or has consumed the contents and has no longer a need for
> > the region. We already have no-map to describe that the memory should
> > never be mapped (and this may be disregarded by an actual driver for
> > the region). I imagine we might add 'discardable' as a boolean DT
> > property, meaning that stage N may use the memory whichever way it
> > wants if it is not going to use it for its intended purpose, provided
> > that it deletes the node from the DT before passing it on to stage
> > N+1.
>
> OK. For now I think that everything is discardable, so long as the
> Payload knows the purpose and that it not needed. That is what Rob
> seemed to be saying. If we add 'discardable', does that mean that
> things default to non-discardable? Would that not be a change of
> behaviour for existing users?

I believe I said "discardable" should be implied from the compatible,
not be a discreet property.

It is possible that some region is passed thru to multiple stages and
is only discardable by the last stage (or next to last or...). So
discardable depends on the consumer. There has to be some logic of "I
know what this foo-bar region is for and know it isn't needed". If the
consumer doesn't know what the region is, then it must default to
leaving it reserved.


> > One thing that needs to be clarified is how this binding interacts
> > with /memory nodes. I assume that currently, /reserved-memory is
> > independent, i.e., it could describe mappable memory that is not
> > covered by /memory at all. If this is the case, we have to decide
> > whether or not discardable regions can be treated in the same way, or
> > whether we should require that discardable regions are covered by
> > /memory.
>
> I would expect all memory to be described in /memory nodes. What is
> the use case for omitting it?

That is the expectation though nothing prevents a platform from
omitting areas. Most likely only the beginning or end of memory.
System partitioning usecases likely just describe each partition's
portion of memory. I don't recall ever seeing a bunch of memory
regions rather than 1-2 with reserved-memory nodes.

Also, I don't think we prevent a reserved region outside the memory
region(s) though I imagine (at least Linux) just ignores them.

> Are you thinking of SRAM, etc?

We have mmio-sram binding for that which has its own way to carve up
the regions.

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 15:07                     ` Ard Biesheuvel
@ 2023-09-07 15:56                       ` Simon Glass
  2023-09-07 16:19                         ` Ard Biesheuvel
  0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2023-09-07 15:56 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

Hi Ard,

On Thu, 7 Sept 2023 at 09:07, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 7 Sept 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ard,
> >
> > On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >
> > > > > On Wed, 6 Sept 2023 at 18:50, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > On Wed, Sep 6, 2023, 10:09 Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >>
> > > > > >> On Wed, 6 Sept 2023 at 16:54, Simon Glass <sjg@chromium.org> wrote:
> > > > > >> >
> > > > > >> > Hi Rob, Ard,
> > > > > >> >
> > > > > >> > On Wed, 6 Sept 2023 at 08:34, Rob Herring <robh@kernel.org> wrote:
> > > > > >> > >
> > > > > >> > > On Tue, Sep 5, 2023 at 4:44 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >> > > >
> > > > > >> > > > On Thu, 31 Aug 2023 at 01:18, Simon Glass <sjg@chromium.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@chromium.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@chromium.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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 15:56                       ` Simon Glass
@ 2023-09-07 16:19                         ` Ard Biesheuvel
  2023-09-07 21:39                           ` Simon Glass
  0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2023-09-07 16:19 UTC (permalink / raw)
  To: Simon Glass
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

On Thu, 7 Sept 2023 at 17:57, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ard,
>
> On Thu, 7 Sept 2023 at 09:07, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 7 Sept 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >
...
> > > > > >
> > > > > > 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.
>

OK. You alluded to Tianocore <-> Tianocore being your use case, which
is why I kept asking for clarification, as using a DT with this
binding seems unusual at the very least.

So coreboot does the platform init, and then hands over to Tianocore.

I take it we are not talking about x86 here, so there are no Intel FSP
blobs that may have dependencies on Tianocore/EDK2 pieces, right? So
there are no UEFI semantics in the memory descriptions that coreboot
provides to Tianocore.

So coreboot provides information to TIanocore about
- the platform topology (DT as usual)
- DRAM memory banks
- memory reservations
- secure firmware services perhaps?
- anything else?


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

No I mean code.

There is

https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Drivers/FdtClientDxe

which encapsulates the DT received from the previous boot stage, and
exposes it as a DXE protocol.

There are other drivers that depend on this protocol, e.g., to
discover additional memory nodes, virtio-mmio drivers and PCI host
bridges.

https://github.com/tianocore/edk2/tree/master/OvmfPkg/Fdt

The bindings used are the ones documented in the Linux kernel tree -
no ad-hoc bindings are being used as far as I know.

But the point I was making before re prior art was really about using
existing bindings rather than inventing new ones. Since we are now
talking about augmenting /reserved-memory, I think we're already on
the same page in this regard (with the caveat that the EDK2 code does
not actually honour /reserved-memory at this point, but this is
because none of the platforms it is being used on today uses that
node)

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

I do think we're converging, actually - it is just taking me some time
to get a clear mental picture of how this will be used.

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

It will all be under /reserved-memory, as far as I understand. We just
need to get to the right level of abstraction.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 1/4] Add reserved-memory
  2023-08-30 23:17 [PATCH v5 1/4] Add reserved-memory Simon Glass
                   ` (2 preceding siblings ...)
  2023-08-30 23:17 ` [PATCH v5 4/4] memory: Add ECC properties Simon Glass
@ 2023-09-07 16:41 ` Rob Herring
  3 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2023-09-07 16:41 UTC (permalink / raw)
  To: Simon Glass
  Cc: devicetree, Maximilian Brune, ron minnich, Tom Rini,
	Dhaval Sharma, U-Boot Mailing List, Mark Rutland, Yunhui Cui,
	linux-acpi, Ard Biesheuvel, Gua Guo, Lean Sheng Tan, Guo Dong,
	lkml, Chiu Chasel

On Wed, Aug 30, 2023 at 6:18 PM Simon Glass <sjg@chromium.org> wrote:
>
> Bring in this file from Linux v6.5
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v4)
>
> Changes in v4:
> - New patch
>
>  .../reserved-memory/reserved-memory.yaml      | 181 ++++++++++++++++++
>  1 file changed, 181 insertions(+)
>  create mode 100644 dtschema/schemas/reserved-memory/reserved-memory.yaml

I've applied this one and patch 2.

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 4/4] memory: Add ECC properties
  2023-08-30 23:17 ` [PATCH v5 4/4] memory: Add ECC properties Simon Glass
@ 2023-09-07 16:58   ` Rob Herring
  0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2023-09-07 16:58 UTC (permalink / raw)
  To: Simon Glass
  Cc: devicetree, Maximilian Brune, ron minnich, Tom Rini,
	Dhaval Sharma, U-Boot Mailing List, Mark Rutland, Yunhui Cui,
	linux-acpi, Ard Biesheuvel, Gua Guo, Lean Sheng Tan, Guo Dong,
	lkml, Chiu Chasel

On Wed, Aug 30, 2023 at 6:18 PM Simon Glass <sjg@chromium.org> wrote:
>
> Some memories provide ECC detection and/or correction. For software which
> wants to check memory, it is helpful to see which regions provide this
> feature.
>
> Add this as a property of the /memory nodes, since it presumably follows
> the hardware-level memory system.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v5:
> - Redo to make this property specific to ECC
> - Provide properties both for detection and correction
>
> Changes in v3:
> - Add new patch to update the /memory nodes
>
>  dtschema/schemas/memory.yaml | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/dtschema/schemas/memory.yaml b/dtschema/schemas/memory.yaml
> index 1d74410..944aa9f 100644
> --- a/dtschema/schemas/memory.yaml
> +++ b/dtschema/schemas/memory.yaml
> @@ -34,7 +34,37 @@ patternProperties:
>          description:
>            For the purpose of identification, each NUMA node is associated with
>            a unique token known as a node id.
> +      ecc-detection:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        enum:
> +          - none
> +          - single-bit
> +          - multi-bit
> +        description: |
> +          If present, this inidcates the type of memory errors which can be

typo

> +          detected and reported by the Error-Correction Code (ECC) memory
> +          subsystem:
>
> +            none       - No error detection is possible
> +            single-bit - Detects and reports single-bit ECC errors
> +            multi-bit  - Detects and reports multiple-bit ECC errors

I don't think 'multi' is specific enough. Perhaps this should be an
int instead with how many bits. (And '-bits' is a standard unit suffix
so a type isn't needed)

> +
> +          If not present, this is equivalent to 'none'.

Can be expressed as schema:

default: none

Though if that's the default why have it as a value? (It's fine though)

> +      ecc-correction:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        enum:
> +          - none
> +          - single-bit
> +          - multi-bit
> +        description: |
> +          If present, this inidcates the type of memory errors which can be

typo

> +          corrected by the Error-Correction Code (ECC) memory subsystem:
> +
> +            none       - No error correction is possible
> +            single-bit - Corrects single-bit ECC errors
> +            multi-bit  - Corrects multiple-bit ECC errors
> +
> +          If not present, this is equivalent to 'none'.

One issue is with 2 properties nonsensical combinations are allowed.
Not really any way to handle that in the schema though.

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages
  2023-09-07 16:19                         ` Ard Biesheuvel
@ 2023-09-07 21:39                           ` Simon Glass
  0 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2023-09-07 21:39 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Rob Herring, Devicetree Discuss, Maximilian Brune, ron minnich,
	Tom Rini, Dhaval Sharma, U-Boot Mailing List, Mark Rutland,
	Yunhui Cui, linux-acpi, Gua Guo, Lean Sheng Tan, Guo Dong, lkml,
	Chiu Chasel

Hi Ard,

On Thu, 7 Sept 2023 at 10:19, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 7 Sept 2023 at 17:57, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ard,
> >
> > On Thu, 7 Sept 2023 at 09:07, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Thu, 7 Sept 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >
> > > > > On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > > >
> ...
> > > > > > >
> > > > > > > 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.
> >
>
> OK. You alluded to Tianocore <-> Tianocore being your use case, which
> is why I kept asking for clarification, as using a DT with this
> binding seems unusual at the very least.

Nevertheless, that is the goal.

>
> So coreboot does the platform init, and then hands over to Tianocore.
>
> I take it we are not talking about x86 here, so there are no Intel FSP
> blobs that may have dependencies on Tianocore/EDK2 pieces, right? So
> there are no UEFI semantics in the memory descriptions that coreboot
> provides to Tianocore.
>
> So coreboot provides information to TIanocore about
> - the platform topology (DT as usual)
> - DRAM memory banks
> - memory reservations
> - secure firmware services perhaps?
> - anything else?

Please don't widen the discussion as we are having enough trouble as
it is. Let's focus on the memory reservations.

>
>
> > >
> > > 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.
> >
>
> No I mean code.
>
> There is
>
> https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Drivers/FdtClientDxe
>
> which encapsulates the DT received from the previous boot stage, and
> exposes it as a DXE protocol.
>
> There are other drivers that depend on this protocol, e.g., to
> discover additional memory nodes, virtio-mmio drivers and PCI host
> bridges.
>
> https://github.com/tianocore/edk2/tree/master/OvmfPkg/Fdt

That looks like Tianocore internals rather than a binding, so far as I
can tell. I do need a binding.

>
> The bindings used are the ones documented in the Linux kernel tree -
> no ad-hoc bindings are being used as far as I know.
>
> But the point I was making before re prior art was really about using
> existing bindings rather than inventing new ones. Since we are now
> talking about augmenting /reserved-memory, I think we're already on
> the same page in this regard (with the caveat that the EDK2 code does
> not actually honour /reserved-memory at this point, but this is
> because none of the platforms it is being used on today uses that
> node)

OK I'll try that patch again with compatible strings instead of node names[1]

>
> > >
> > > 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.
> >
> >
>
> I do think we're converging, actually - it is just taking me some time
> to get a clear mental picture of how this will be used.
>
> > > - 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.
> >
>
> It will all be under /reserved-memory, as far as I understand. We just
> need to get to the right level of abstraction.

OK I'll try again.

Regards,
Simon

[1] I was led down the node-name path by this text in the DT spec
"Following the generic-names recommended practice, node names should
reflect the purpose of the node (ie. “framebuffer” or “dma-pool”).
Unit address (@<address>) should be appended to the name if the node
is a static allocation."

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-09-07 21:39 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-30 23:17 [PATCH v5 1/4] Add reserved-memory Simon Glass
2023-08-30 23:17 ` [PATCH v5 2/4] Bring in some other reserved-memory files Simon Glass
2023-08-30 23:17 ` [PATCH v5 3/4] schemas: Add some common reserved-memory usages Simon Glass
2023-09-05 21:44   ` Ard Biesheuvel
2023-09-06 14:34     ` Rob Herring
2023-09-06 14:53       ` Simon Glass
2023-09-06 16:08         ` Ard Biesheuvel
     [not found]           ` <CAPnjgZ1oGF0Ni3RhK4fv6mJk40YjqyFVJxt6FfS9AW2rkcs9iA@mail.gmail.com>
2023-09-07 13:31             ` Ard Biesheuvel
2023-09-07 13:56               ` Simon Glass
2023-09-07 14:12                 ` Ard Biesheuvel
2023-09-07 14:50                   ` Simon Glass
2023-09-07 15:07                     ` Ard Biesheuvel
2023-09-07 15:56                       ` Simon Glass
2023-09-07 16:19                         ` Ard Biesheuvel
2023-09-07 21:39                           ` Simon Glass
2023-09-07 15:43                 ` Rob Herring
2023-08-30 23:17 ` [PATCH v5 4/4] memory: Add ECC properties Simon Glass
2023-09-07 16:58   ` Rob Herring
2023-09-07 16:41 ` [PATCH v5 1/4] Add reserved-memory Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).