Message-ID: <CAMj1kXEHpRjk_YKOm4czCnnpjqgahj2jV8MMfGLx7b1RdnBnVw@mail.gmail.com>
Date:   Wed, 23 Aug 2023 16:23:38 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Simon Glass <sjg@...omium.org>, devicetree@...r.kernel.org,
        Rob Herring <robh@...nel.org>,
        Chiu Chasel <chasel.chiu@...el.com>,
        U-Boot Mailing List <u-boot@...ts.denx.de>,
        Gua Guo <gua.guo@...el.com>, linux-acpi@...r.kernel.org,
        lkml <linux-kernel@...r.kernel.org>,
        Yunhui Cui <cuiyunhui@...edance.com>,
        ron minnich <rminnich@...il.com>,
        Tom Rini <trini@...sulko.com>,
        Lean Sheng Tan <sheng.tan@...ements.com>
Subject: Re: [PATCH v3 1/2] schemas: Add a schema for memory map

On Wed, 23 Aug 2023 at 10:59, Mark Rutland <mark.rutland@....com> wrote:
>
> On Tue, Aug 22, 2023 at 02:34:42PM -0600, Simon Glass 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.

Not quite.

This seems to be intended for consumption by Linux booting in ACPI
mode, but not via UEFI, right? Some proposed changes to support that
were rejected on the basis that ACPI on non-x86 is strictly tied to
ACPI boot, not only because the ACPI root table (rsdp) can only be
discovered via UEFI, but also because the ACPI subsystem in Linux
cross-references any memory mappings created from AML code against the
UEFI memory map.

Unfortunately, having a vague, non-exhaustive approximation of the
UEFI memory map is unlikely to be sufficient here. The existing logic
assumes that addresses not covered by the UEFI memory map are MMIO,
which means that either a) the UEFI memory map needs to describe all
RAM exhaustively or b) the existing logic needs to be modified to take
memblock or other information sources into account as well in order to
reason about whether a certain address requires device or normal
memory attributes.

Note that the ACPI spec only lists EFI as a valid way to obtain the
root table pointer on architectures other than x86. If other ways are
needed, they should be contributed to the spec, rather than being
added to Linux as an ad hoc workaround for bootloaders that have
trouble implementing the spec as is.

>
> Today Linux does that by passing:
>
>   /chosen/linux,uefi-mmap-start
>   /chosen/linux,uefi-mmap-size
>   /chosen/linux,uefi-mmap-desc-size
>   /chosen/linux,uefi-mmap-desc-ver
>
> ... or /chosen/xen,* variants of those.
>
> Can't we document / genericise that?
>

Given the ACPI angle, promoting this to external ABI would introduce a
DT dependency to ACPI boot. So we'll at least have to be very clear
about which takes precedence, or maybe disregard everything except the
/chosen node when doing ACPI boot?

This also argues for not creating an ordinary binding for this (i.e.,
describing it as part of the platform topology), but putting it under
/chosen as a Linux-only boot tweak.

> Pointing to that rather than re-encoding it in DT means that it stays in-sync
> with the EFI spec and we won't back ourselves into a corner where we cannot
> encode something due to a structural difference. I don't think it's a good idea
> to try to re-encode it, or we're just setting ourselves up for futher pain.
>

What I would prefer is to formalize pseudo-EFI boot and define the
bare required minimum (system table + memory map + config tables) in
an arch-agnostic manner. That way, the only thing that needs to be
passed via DT/boot_params/etc is the (pseudo-)EFI system table
address, and everything else (SMBIOS, ACPI as well as the EFI memory
map and even the initrd) can be passed via config tables as usual, all
of which is already supported in (mostly) generic kernel code.

This means that booting via the EFI stub would not be required, nor is
implementing boot services or runtime services or any of the other
things that people appear to hate with such a passion.

I'd actually prefer not to use those /chosen DT nodes, but pass the
memory map via a config table. This, however, requires* that the
runtime services memory regions are mapped 1:1 so it is not something
we can do for all currently supported systems. But for pseudo-EFI
boot, stipulating 1:1 runtime mappings is fine (or no runtime mappings
at all if the firmware does not expose runtime services)

* SetVirtualAddressMap() relocates the config table array, and without
a memory map, it is not possible to convert those address back to
physical. SetVirtualAddressMap() is evil and needs to die, but this is
off-topic for the thread at hand.

> >
> > This aims to provide an initial schema for this mapping.
> >
> > Note that this is separate from the existing /memory and /reserved-memory
> > nodes, since it is mostly concerned with what the memory is used for. It
> > may cover only a small fraction of available memory.
> >
> > For now, no attempt is made to create an exhaustive binding, so there are
> > some example types listed. This can be completed once this has passed
> > initial review.
> >
> > 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@...omium.org>
> > ---
> >
> > 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
> >
> >  dtschema/schemas/memory-map.yaml | 61 ++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 dtschema/schemas/memory-map.yaml
> >
> > diff --git a/dtschema/schemas/memory-map.yaml b/dtschema/schemas/memory-map.yaml
> > new file mode 100644
> > index 0000000..4b06583
> > --- /dev/null
> > +++ b/dtschema/schemas/memory-map.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: BSD-2-Clause
> > +# Copyright 2023 Google LLC
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/memory-map.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: /memory-map nodes
> > +description: |
> > +  Common properties always required in /memory-map nodes. These nodes are
> > +  intended to resolve the nonchalant clause 3.4.1 ("/memory node and UEFI")
> > +  in the Devicetree Specification.
> > +
> > +maintainers:
> > +  - Simon Glass <sjg@...omium.org>
> > +
> > +properties:
> > +  $nodename:
> > +    const: 'memory-map'
> > +
> > +patternProperties:
> > +  "^([a-z][a-z0-9\\-]+@[0-9a-f]+)?$":
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        minItems: 1
> > +        maxItems: 1024
> > +
> > +      usage:
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +        description: |
> > +          Describes the usage of the memory region, e.g.:
> > +
> > +            "acpi-reclaim", "acpi-nvs", "bootcode", "bootdata", "bootdata",
> > +            "runtime-code", "runtime-data".
> > +
> > +            See enum EFI_MEMORY_TYPE in "Unified Extensible Firmware Interface
> > +            (UEFI) Specification" for all the types. For now there are not
> > +            listed here.
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    memory-map {
> > +        acpi@...00 {
> > +            reg = <0xf0000 0x4000>;
> > +            usage = "acpi-reclaim";
> > +        };
> > +
> > +        runtime@...00000 {
> > +            reg = <0x12300000 0x28000>;
> > +            usage = "runtime-code";
> > +        };
> > +    };
> > +...
> > --
> > 2.42.0.rc1.204.g551eb34607-goog
> >

Powered by blists - more mailing lists