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

[Chiu, Chasel]

> -----Original Message-----
> From: Rob Herring <robh at kernel.org>
> Sent: Wednesday, January 3, 2024 8:01 AM
> To: Ard Biesheuvel <ardb at kernel.org>
> Cc: Chiu, Chasel <chasel.chiu at intel.com>; Simon Glass <sjg at chromium.org>;
> devicetree at vger.kernel.org; Mark Rutland <mark.rutland at arm.com>; Tan, Lean
> Sheng <sheng.tan at 9elements.com>; lkml <linux-kernel at vger.kernel.org>;
> Dhaval Sharma <dhaval at rivosinc.com>; Brune, Maximilian
> <maximilian.brune at 9elements.com>; Yunhui Cui <cuiyunhui at bytedance.com>;
> Dong, Guo <guo.dong at intel.com>; Tom Rini <trini at konsulko.com>; ron minnich
> <rminnich at gmail.com>; Guo, Gua <gua.guo at intel.com>; linux-
> acpi at vger.kernel.org; U-Boot Mailing List <u-boot at lists.denx.de>
> Subject: Re: [PATCH v7 2/2] schemas: Add some common reserved-memory
> usages
> 
> On Fri, Dec 22, 2023 at 5:48 AM Ard Biesheuvel <ardb at kernel.org> wrote:
> >
> > On Thu, 21 Dec 2023 at 17:50, Chiu, Chasel <chasel.chiu at intel.com> wrote:
> > >
> > >
> > > Hi Ard,
> > >
> > > Please see my reply below inline and let me know your thoughts.
> > >
> > > Thanks,
> > > Chasel
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel <ardb at kernel.org>
> > > > Sent: Thursday, December 21, 2023 6:31 AM
> > > > To: Chiu, Chasel <chasel.chiu at intel.com>
> > > > Cc: Simon Glass <sjg at chromium.org>; devicetree at vger.kernel.org;
> > > > Mark Rutland <mark.rutland at arm.com>; Rob Herring
> > > > <robh at kernel.org>; Tan, Lean Sheng <sheng.tan at 9elements.com>; lkml
> > > > <linux-kernel at vger.kernel.org>; Dhaval Sharma
> > > > <dhaval at rivosinc.com>; Brune, Maximilian
> > > > <maximilian.brune at 9elements.com>; Yunhui Cui
> > > > <cuiyunhui at bytedance.com>; Dong, Guo <guo.dong at intel.com>; Tom
> > > > Rini <trini at konsulko.com>; ron minnich <rminnich at gmail.com>; Guo,
> > > > Gua <gua.guo at intel.com>; linux- acpi at vger.kernel.org; U-Boot
> > > > Mailing List <u-boot at lists.denx.de>
> > > > Subject: Re: [PATCH v7 2/2] schemas: Add some common
> > > > reserved-memory usages
> > > >
> > > > On Tue, 28 Nov 2023 at 21:31, Chiu, Chasel <chasel.chiu at intel.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ard Biesheuvel <ardb at kernel.org>
> > > > > > Sent: Tuesday, November 28, 2023 10:08 AM
> > > > > > To: Chiu, Chasel <chasel.chiu at intel.com>
> > > > > > Cc: Simon Glass <sjg at chromium.org>;
> > > > > > devicetree at vger.kernel.org; Mark Rutland
> > > > > > <mark.rutland at arm.com>; Rob Herring <robh at kernel.org>; Tan,
> > > > > > Lean Sheng <sheng.tan at 9elements.com>; lkml
> > > > > > <linux-kernel at vger.kernel.org>; Dhaval Sharma
> > > > > > <dhaval at rivosinc.com>; Brune, Maximilian
> > > > > > <maximilian.brune at 9elements.com>; Yunhui Cui
> > > > > > <cuiyunhui at bytedance.com>; Dong, Guo <guo.dong at intel.com>; Tom
> > > > > > Rini <trini at konsulko.com>; ron minnich <rminnich at gmail.com>;
> > > > > > Guo, Gua <gua.guo at intel.com>; linux- acpi at vger.kernel.org;
> > > > > > U-Boot Mailing List <u-boot at lists.denx.de>
> > > > > > Subject: Re: [PATCH v7 2/2] schemas: Add some common
> > > > > > reserved-memory usages
> > > > > >
> > > > > > You are referring to a 2000 line patch so it is not 100% clear where to
> look tbh.
> > > > > >
> > > > > >
> > > > > > On Tue, 21 Nov 2023 at 19:37, Chiu, Chasel <chasel.chiu at intel.com>
> wrote:
> > > > > > >
> > > > > > >
> > > > > > > In PR, UefiPayloadPkg/Library/FdtParserLib/FdtParserLib.c,
> > > > > > > line
> > > > > > > 268 is for
> > > > > > related example code.
> > > > > > >
> > > > > >
> > > > > > That refers to a 'memory-allocation' node, right? How does
> > > > > > that relate to the 'reserved-memory' node?
> > > > > >
> > > > > > And crucially, how does this clarify in which way
> > > > > > "runtime-code" and
> > > > > > "runtime- data" reservations are being used?
> > > > > >
> > > > > > Since the very beginning of this discussion, I have been
> > > > > > asking repeatedly for examples that describe the wider context
> > > > > > in which these
> > > > reservations are used.
> > > > > > The "runtime" into runtime-code and runtime-data means that
> > > > > > these regions have a special significance to the operating
> > > > > > system, not just to the next bootloader stage. So I want to
> > > > > > understand exactly why it is necessary to describe these
> > > > > > regions in a way where the operating system might be expected
> > > > > > to interpret this information and act
> > > > upon it.
> > > > > >
> > > > >
> > > > >
> > > > > I think runtime code and data today are mainly for supporting
> > > > > UEFI runtime
> > > > services - some BIOS functions for OS to utilize, OS may follow
> > > > below ACPI spec to treat them as reserved range:
> > > > > https://uefi.org/specs/ACPI/6.5/15_System_Address_Map_Interfaces
> > > > > .html# uefi-memory-types-and-mapping-to-acpi-address-range-types
> > > > >
> > > > > Like I mentioned earlier, that PR is still in early phase and
> > > > > has not reflected all
> > > > the required changes yet, but the idea is to build
> > > > gEfiMemoryTypeInformationGuid HOB from FDT reserved-memory nodes.
> > > > > UEFI generic Payload has DxeMain integrated, however Memory
> > > > > Types are
> > > > platform-specific, for example, some platforms may need bigger
> > > > runtime memory for their implementation, that's why we want such
> > > > FDT reserved-memory node to tell DxeMain.
> > > > >
> > > >
> > > > > The Payload flow will be like this:
> > > > >   Payload creates built-in default MemoryTypes table ->
> > > > >     FDT reserved-memory node to override if required (this also
> > > > > ensures the
> > > > same memory map cross boots so ACPI S4 works) ->
> > > > >       Build gEfiMemoryTypeInformationGuid HOB by "platfom specific"
> > > > MemoryTypes Table ->
> > > > >         DxeMain/GCD to consume this MemoryTypes table and setup
> > > > > memory
> > > > service ->
> > > > >           Install memory types table to UEFI system table.Configuration
> table...
> > > > >
> > > > > Note: if Payload built-in default MemoryTypes table works fine
> > > > > for the platform, then FDT reserved-memory node does not need to
> > > > > provide such
> > > > 'usage' compatible strings. (optional) This FDT node could allow
> > > > flexibility/compatibility without rebuilding Payload binary.
> > > > >
> > > > > Not sure if I answered all your questions, please highlight
> > > > > which area you need
> > > > more information.
> > > > >
> > > >
> > > > The gEfiMemoryTypeInformationGuid HOB typically carries platform
> > > > defaults, and the actual memory type information is kept in a
> > > > non-volatile EFI variable, which gets updated when the memory
> > > > usage changes. Is this different for UefiPayloadPkg?
> > > >
> > > > (For those among the cc'ees less versed in EFI/EDK2: when you get
> > > > the 'config changed -rebooting' message from the boot firmware, it
> > > > typically means that this memory type table has changed, and a
> > > > reboot is necessary.)
> > > >
> > > > So the platform init needs to read this variable, or get the
> > > > information in a different way. I assume it is the payload, not
> > > > the platform init that updates the variable when necessary. This
> > > > means the information flows from payload(n) to platform init(n+1),
> > > > where n is a monotonic index tracking consecutive boots of the system.
> > > >
> > > > Can you explain how the DT fits into this? How are the
> > > > runtime-code and runtime-data memory reservation nodes under
> > > > /reserved-memory used to implement this information exchange
> > > > between platform init and payload? And how do the HOB and the EFI
> variable fit into this picture?
> > >
> > >
> > > 1. With some offline discussion, we would move
> > > gEfiMemoryTypeInformationGuid usage to FDT->upl-custom node. This is
> > > because it is edk2 implementation choice and non-edk2 PlatformInit
> > > or Payload may not have such memory optimization implementation.
> > > (not a generic usage/requirement for PlatformInit and Payload)
> > >
> > > The edk2 example flow will be like below:
> > >
> > > PlatformInit to GetVariable of gEfiMemoryTypeInformationGuid and create
> Hob->
> > >   PlatformInit to initialize FDT->upl-custom node to report
> gEfiMemoryTypeInformationGuid HOB information ->
> > >     UefiPayload entry to re-create gEfiMemoryTypeInformationGuid HOB
> basing on FDT input (instead of the default MemoryType inside UefiPayload) ->
> > >       UefiPayload DxeMain/Gcd will consume gEfiMemoryTypeInformationGuid
> Hob for memory type information ->
> > >         UefiPayload to initialize UEFI environment (mainly DXE dispatcher) ->
> > >           (additional FV binary appended to common UefiPayload binary)
> PlatformPayload to provide VariableService which is platform specific ->
> > >             UefiPayload UefiBootManager will SetVariable if memory type change
> needed and request a warm reset ->
> > >               Back to PlatformInit ...
> > >
> >
> > OK so the upl-custom node can do whatever it needs to. I imagine these
> > will include the memory descriptor attribute field, and other parts
> > that may be missing from the /reserved-memory DT node specification?
> >
> > >
> > > 2. Now the proposed reserved-memory node usages will be for PlatformInit to
> provide data which may be used by Payload or OS. This is not edk2 specific and
> any PlatformInit/Payload could have same support.
> > > Note: all of below are optional and PlatformInit may choose to implement
> some of them or not.
> > >
> > >       - acpi
> > > If PlatformInit created some ACPI tables, this will report a memory region
> which contains all the tables to Payload and Payload may base on this to add
> some more tables if required.
> > >
> > >       - acpi-nvs
> > > If PlatformInit has created some ACPI tables which having ACPI NVS memory
> dependency, this will be that nvs region.
> > >
> >
> > These make sense.
> >
> > >       - boot-code
> > > When PlatformInit having some FW boot phase code that could be freed
> > > for OS to use when payload transferring control to UEFI OS
> > >
> > >       - boot-data
> > > When PlatformInit having some FW boot phase data that could be freed for OS
> to use when payload transferring control to UEFI OS.
> > >
> > >       - runtime-code
> > > PlatformInit may provide some services code that can be used for Payload to
> initialize UEFI Runtime Services for supporting UEFI OS.
> > >
> > >       - runtime-data
> > > PlatformInit may provide some services data that can be used for Payload to
> Initialize UEFI Runtime Services for supporting UEFI OS.
> > >
> 
> I'll say it again. "boot" and "runtime" on their own could mean about anything,
> but the usage here is clearly tied to UEFI (or the EDK2
> implementation) and its meaning of boot and runtime. So the naming needs to
> reflect that.
> 



How about renaming to below?
uefiboot-code
uefiboot-data
uefiruntime-code
uefiruntime-data



> 
> > A UEFI OS must consume this information from the UEFI memory map, not
> > from the /reserved-memory nodes. So these nodes must either not be
> > visible to the OS at all, or carry an annotation that the OS must
> > ignore them.
> 
> The kernel will process /reserved-memory for UEFI boot, so the expectation is
> anything in the EFI memory map is not present there. An annotation to ignore
> some nodes would require going back in time or accepting 2 sources of truth on
> existing OS.
> 


I thought UEFI boot will rely on UEFI GetMemoryMap() for knowing current memory map, and the DT memory/reserved-memory nodes should have been included/converted by GetMemoryMap() function.
How kernel handling DT nodes vs GetMemoryMap() in UEFI boot case?



> > Would it be possible to include a restriction in the DT schema that
> > these are only valid in the firmware boot phase?
> 
> The only way ATM is including a schema or not when running validation on a DT
> for a particular boot phase. Include the schema in the project that wants to use
> these nodes and don't include it in cases that don't use it. I don't see a reason
> why this needs to be in dtschema.
> 
> Rob