From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0C0C67489; Fri, 13 Oct 2023 20:42:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="a9MZmd4W" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8CB5BC433CB; Fri, 13 Oct 2023 20:42:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697229734; bh=QedSm7IKkC09g8K3qwvy20LU//M50NwLS1IxJ9WwyJI=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=a9MZmd4WNOULw62DDrmkoCcloPckNv8puimcdTStZjSnlgKOeJLg7w214syisdfw7 +EDhLsRxSBmYSDWYJ0853Fqtm3q1bx12EecKRKn0ltWrvWBeZKCVI6YeUK2cq7aTi/ eemXY0FqN+FFSEclNZhAVBYPGQ4yOaQ811nYuPY6zexB7sEZPqDr4jJAq+e2Bu/3tn IHmhTE9wC3Goa6HtdgSkw2PAqshQ0RLwvE4fXR5JKO7y0X7xvKnhdtyS1rHHo6kVZa LEWgHxSxNXcw0CEdtsOwwj9EWOEiWlFZNS9/5+spyCkXqXtuzksCuJe4OTcXEXmYxX cJhnG6Rr5GxKA== Received: by mail-lf1-f47.google.com with SMTP id 2adb3069b0e04-504a7f9204eso3178609e87.3; Fri, 13 Oct 2023 13:42:14 -0700 (PDT) X-Gm-Message-State: AOJu0YwfUL75dYvpTYIWlxc9zjekb+q4jQbrM+TpwLX8jHQtCqhpBPSZ L6gLv30GW0vxZY7hsM2q0aQQx6Mog9XQfjAMPQ== X-Google-Smtp-Source: AGHT+IFkOrZNk8wWFwlBJcf/yE0XhIdqYph8oMA9Z+tuzUogoVeyAPSThh8ZpntpX2Q8yFyz3XaeAQLdLNs3D7Bw9PU= X-Received: by 2002:ac2:5f55:0:b0:502:ffff:feff with SMTP id 21-20020ac25f55000000b00502fffffeffmr20811060lfz.58.1697229732643; Fri, 13 Oct 2023 13:42:12 -0700 (PDT) Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20230926194242.2732127-1-sjg@chromium.org> <20230926194242.2732127-2-sjg@chromium.org> In-Reply-To: From: Rob Herring Date: Fri, 13 Oct 2023 15:42:00 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v7 2/2] schemas: Add some common reserved-memory usages To: Simon Glass Cc: Ard Biesheuvel , devicetree@vger.kernel.org, Mark Rutland , Lean Sheng Tan , lkml , Dhaval Sharma , Maximilian Brune , Yunhui Cui , Guo Dong , Tom Rini , ron minnich , Gua Guo , Chiu Chasel , linux-acpi@vger.kernel.org, U-Boot Mailing List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Oct 6, 2023 at 7:03=E2=80=AFPM Simon Glass wrote= : > > Hi Ard, > > On Fri, 6 Oct 2023 at 17:00, Ard Biesheuvel wrote: > > > > On Fri, 6 Oct 2023 at 20:17, Simon Glass wrote: > > > > > > Hi Ard, > > > > > > On Fri, 6 Oct 2023 at 11:33, Ard Biesheuvel wrote: > > > > > > > > On Mon, 2 Oct 2023 at 19:54, Simon Glass wrote: > > > > > > > > > > Hi Rob, > > > > > > > > > > On Tue, 26 Sept 2023 at 13:42, Simon Glass wro= te: > > > > > > > > > > > > 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 firmw= are is > > > > > > present on either side of this interface, information about mem= ory usage > > > > > > and attributes must be presented to the "Payload" in some form. > > > > > > > > > > > > This aims to provide an small schema addition for the memory ma= pping > > > > > > needed to keep these two pieces working together well. > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > --- > > > > > > > > > > > > Changes in v7: > > > > > > - Rename acpi-reclaim to acpi > > > > > > - Drop individual mention of when memory can be reclaimed > > > > > > - Rewrite the item descriptions > > > > > > - Add back the UEFI text (with trepidation) > > > > > > > > > > I am again checking on this series. Can it be applied, please? > > > > > > > > > > > > > Apologies for the delay in response. I have been away. > > > > > > OK, I hope you had a nice trip. > > > > > > > Thanks, it was wonderful! > > > > > > > > > > > > > > > > > > > > > > > Changes in v6: > > > > > > - Drop mention of UEFI > > > > > > - Use compatible strings instead of node names > > > > > > > > > > > > Changes in v5: > > > > > > - Drop the memory-map node (should have done that in v4) > > > > > > - Tidy up schema a bit > > > > > > > > > > > > Changes in v4: > > > > > > - Make use of the reserved-memory node instead of creating a ne= w one > > > > > > > > > > > > Changes in v3: > > > > > > - Reword commit message again > > > > > > - cc a lot more people, from the FFI patch > > > > > > - Split out the attributes into the /memory nodes > > > > > > > > > > > > Changes in v2: > > > > > > - Reword commit message > > > > > > > > > > > > .../reserved-memory/common-reserved.yaml | 71 +++++++++++= ++++++++ > > > > > > 1 file changed, 71 insertions(+) > > > > > > create mode 100644 dtschema/schemas/reserved-memory/common-res= erved.yaml > > > > > > > > > > > > diff --git a/dtschema/schemas/reserved-memory/common-reserved.y= aml b/dtschema/schemas/reserved-memory/common-reserved.yaml > > > > > > new file mode 100644 > > > > > > index 0000000..f7fbdfd > > > > > > --- /dev/null > > > > > > +++ b/dtschema/schemas/reserved-memory/common-reserved.yaml > > > > > > @@ -0,0 +1,71 @@ > > > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > > > > +%YAML 1.2 > > > > > > +--- > > > > > > +$id: http://devicetree.org/schemas/reserved-memory/common-rese= rved.yaml# > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > + > > > > > > +title: Common memory reservations > > > > > > + > > > > > > +description: | > > > > > > + Specifies that the reserved memory region can be used for th= e purpose > > > > > > + indicated by its compatible string. > > > > > > + > > > > > > + Clients may reuse this reserved memory if they understand wh= at it is for, > > > > > > + subject to the notes below. > > > > > > + > > > > > > +maintainers: > > > > > > + - Simon Glass > > > > > > + > > > > > > +allOf: > > > > > > + - $ref: reserved-memory.yaml > > > > > > + > > > > > > +properties: > > > > > > + compatible: > > > > > > + description: | > > > > > > + This describes some common memory reservations, with the= compatible > > > > > > + string indicating what it is used for: > > > > > > + > > > > > > + acpi: Advanced Configuration and Power Interface (ACP= I) tables > > > > > > + acpi-nvs: ACPI Non-Volatile-Sleeping Memory (NVS). Th= is is reserved by > > > > > > + the firmware for its use and is required to be save= d and restored > > > > > > + across an NVS sleep > > > > > > + boot-code: Contains code used for booting which is no= t needed by the OS > > > > > > + boot-code: Contains data used for booting which is no= t needed by the OS > > > > > > + runtime-code: Contains code used for interacting with= the system when > > > > > > + running the OS > > > > > > + runtime-data: Contains data used for interacting with= the system when > > > > > > + running the OS > > > > > > + > > > > > > + enum: > > > > > > + - acpi > > > > > > + - acpi-nvs > > > > > > + - boot-code > > > > > > + - boot-data > > > > > > + - runtime-code > > > > > > + - runtime-data > > > > > > + > > > > > > > > As I mentioned a few times already, I don't think these compatibles > > > > should be introduced here. > > > > > > > > A reserved region has a specific purpose, and the compatible should= be > > > > more descriptive than the enum above. If the consumer does not > > > > understand this purpose, it should simply treat the memory as reser= ved > > > > and not touch it. Alternatively, these regions can be referenced fr= om > > > > other DT nodes using phandles if needed. > > > > > > We still need some description of what these regions are used for, so > > > that the payload can use the correct regions. I do not have any other > > > solution to this problem. We are in v7 at present. At least explain > > > where you want the compatible strings to be introduced. > > > > > > > My point is really that by themselves, these regions are not usable by > > either a payload or an OS that consumes this information. Unless there > > is some other information being provided (via DT I imagine) that > > describes how these things are supposed to be used, they are nothing > > more than memory reservations that should be honored, and providing > > this arbitrary set of labels is unnecessary. > > > > > What sort of extra detail are you looking for? Please be specific and > > > preferably add some suggestions so I can close this out ASAP. > > > > > > > A payload or OS can do nothing with a memory reservation called > > 'runtime-code' it it doesn't know what is inside. Agreed. The question is WHAT runtime-code? The compatible needs to answer t= hat. For example, we have 'ramoops' as a compatible in reserved memory. That tells us *exactly* what's there. We know how to parse it. If we know ramoops is not supported, then we know we can toss it out and reclaim the memory. > > So there is another > > DT node somewhere that describes this, and that can simply point to > > this region (via a phandle) if it needs to describe the > > correspondence. This is more idiomatic for DT afaik (but I am not the > > expert). I don't see why we need that indirection. > > But more importantly, it avoids overloading some vague > > labels with behavior (e.g., executable permissions for code regions) > > that should only be displayed for regions with a particular use, > > rather than for a ill defined class of reservations the purpose of > > which is not clear. > > > > What I am trying to avoid is the OS ending up being forced to consume > > this information in parallel to the EFI memory map, and having to > > reconcile them. I'd be much happier if this gets contributed to a spec > > that only covers firmware-to-firmware, and is prevented from leaking > > into the OS facing interface. > > I don't know about "another DT node". We don't have one at present. > > There is already a note in the DT spec about this: > > > 3.5.4 /reserved-memory and UEFI > > > When booting via [UEFI], static /reserved-memory regions must also be l= isted in the system memory map obtained > > via the GetMemoryMap() UEFI boot time service as defined in [UEFI] =C2= =A7 7.2. The reserved memory regions need to be > > included in the UEFI memory map to protect against allocations by UEFI = applications. > > > > Reserved regions with the no-map property must be listed in the memory = map with type EfiReservedMemoryType. All > > other reserved regions must be listed with type EfiBootServicesData. > > > > Dynamic reserved memory regions must not be listed in the [UEFI] memory= map because they are allocated by the OS > > after exiting firmware boot services. > > I don't fully understand what all that means, but does it cover your conc= ern? This section is purely about using UEFI mechanisms to load and boot the OS. If we're not talking about this stage of booting, then none of this applies. Rob