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

[Chiu, Chasel]

> -----Original Message-----
> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Sent: Thursday, January 4, 2024 12:43 AM
> To: Chiu, Chasel <chasel.chiu@xxxxxxxxx>
> Cc: Simon Glass <sjg@xxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; Mark Rutland
> <mark.rutland@xxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Tan, Lean Sheng
> <sheng.tan@xxxxxxxxxxxxx>; lkml <linux-kernel@xxxxxxxxxxxxxxx>; Dhaval
> Sharma <dhaval@xxxxxxxxxxxx>; Brune, Maximilian
> <maximilian.brune@xxxxxxxxxxxxx>; Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>;
> Dong, Guo <guo.dong@xxxxxxxxx>; Tom Rini <trini@xxxxxxxxxxxx>; ron minnich
> <rminnich@xxxxxxxxx>; Guo, Gua <gua.guo@xxxxxxxxx>; linux-
> acpi@xxxxxxxxxxxxxxx; U-Boot Mailing List <u-boot@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v7 2/2] schemas: Add some common reserved-memory
> usages
> 
> On Thu, 4 Jan 2024 at 01:25, Chiu, Chasel <chasel.chiu@xxxxxxxxx> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > Sent: Wednesday, January 3, 2024 7:22 AM
> > > To: Chiu, Chasel <chasel.chiu@xxxxxxxxx>
> > > Cc: Simon Glass <sjg@xxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; Mark
> > > Rutland <mark.rutland@xxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Tan,
> > > Lean Sheng <sheng.tan@xxxxxxxxxxxxx>; lkml
> > > <linux-kernel@xxxxxxxxxxxxxxx>; Dhaval Sharma <dhaval@xxxxxxxxxxxx>;
> > > Brune, Maximilian <maximilian.brune@xxxxxxxxxxxxx>; Yunhui Cui
> > > <cuiyunhui@xxxxxxxxxxxxx>; Dong, Guo <guo.dong@xxxxxxxxx>; Tom Rini
> > > <trini@xxxxxxxxxxxx>; ron minnich <rminnich@xxxxxxxxx>; Guo, Gua
> > > <gua.guo@xxxxxxxxx>; linux- acpi@xxxxxxxxxxxxxxx; U-Boot Mailing
> > > List <u-boot@xxxxxxxxxxxxx>
> > > Subject: Re: [PATCH v7 2/2] schemas: Add some common reserved-memory
> > > usages
> > >
> > > On Fri, 22 Dec 2023 at 20:52, Chiu, Chasel <chasel.chiu@xxxxxxxxx> wrote:
> > > >
> > > >
> > > > Please see my reply below inline.
> > > >
> > > > Thanks,
> > > > Chasel
> > > >
> > > ...
> > > > > > > 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?
> > > >
> > > >
> > > > Yes, if needed by edk2 specific implementation, not generic
> > > > enough, we may
> > > consider to use upl-custom node to pass those data.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > 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.
> > > > > >
> > > > >
> > > > > 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.
> > > > >
> > > > > Would it be possible to include a restriction in the DT schema
> > > > > that these are only valid in the firmware boot phase?
> > > >
> > > >
> > > > https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#ef
> > > > i-bo ot-services-exitbootservices Per UEFI specification, UEFI OS
> > > > will always call UEFI GetMemoryMap function to retrieve memory
> > > > map, so FDT
> > > node present or not does not matter to UEFI OS. We probably could
> > > have annotation in UPL specification to emphasize this.
> > > > I'm not familiar with Linux FDT boot, but if non-UEFI OS does not
> > > > call UEFI
> > > GetMemoryMap() and does not know what is runtime-code/data, boot-
> > > code/data, it might just treat such reserved-memory nodes as
> > > 'regular' reserved memory nodes, and that's still ok because
> > > non-UEFI OS will not call to any runtime service or re-purpose boot-code/data
> memory regions.
> > > >
> > >
> > > You are saying the same thing but in a different way. A UEFI OS must
> > > only rely on GetMemoryMap(), and not on the /reserved-memory node to
> > > obtain this information. But this requirement needs to be stated
> > > somewhere: the UEFI spec does not reason about other sources of EFI
> > > memory information at all, and this DT schema does not mention any of this
> either.
> > >
> > > > Would you provide a real OS case which will be impacted by this
> > > > reserved-
> > > memory schema so we can discuss basing on real case?
> > > >
> > >
> > > Funny, that is what I have been trying to get from you :-)
> > >
> > > The problem I am anticipating here is that the information in
> > > /reserved-memory may be out of sync with the EFI memory map. It
> > > needs to be made clear that the EFI memory map is the only source of
> > > truth when the OS is involved, and this /reserved-memory mechanism
> > > should only be used by other firmware stages. But the schema does
> > > not mention this at all. The schema also does not mention that the
> > > information in /reserved-memory is not actually sufficient to
> > > reconstruct the EFI memory map that the firmware payload expects
> > > (which is why the upl- custom-node exists too)
> >
> >
> >
> > Does below solve your concerns if we mention those in schema
> > description? (please feel free to add more if you have) . boot-code/boot-data
> and runtime-code/runtime-data usages are following UEFI specification
> >   . before ExitBootServices:
> https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#memory-
> type-usage-before-exitbootservices
> >   . after ExitBootServices:
> > https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#memory
> > -type-usage-after-exitbootservices
> > . These usages do not intend to construct full UEFI memory map, it is only for
> PlatformInit to pass pre-installed tables or services to Payload for supporting UEFI
> OS boot.
> > . These usages are optional
> > . Typically UEFI OS boot will always call GetMemoryMap() to retrieve
> > memory map following UEFI spec, no matter DT nodes present or not
> > (https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-b
> > oot-services-exitbootservices) . Typically Non-UEFI OS boot will treat
> > those  boot* or runtime* reserved-memory as 'regular' reserved memory if
> present.
> >
> 
> This already helps quite a lot, thanks.
> 
> But why should a non-UEFI OS be required to keep boot* or runtime* regions
> reserved? The firmware stage that boots the OS knows whether it is performing
> an UEFI boot or a non-UEFI boot, and it should only present the information that
> goes along with that. The OS should never have to worry about reconciling two
> sources of truth.
> 
> And to Rob's point about boot / runtime being ill-defined: I would argue that
> 'runtime' quite clearly implies 'under the OS', and so UEFI
> runtime* reservations are assumed to always be relevant to UEFI OSes.
> 
> I think there is a fundamental difference of opinion here, where the position of
> the firmware developers is that the DT should be the same across all boot stages,
> while my position reasoning from the OS side is that the OS should be able to
> observe only the abstractions that are part of the contract between firmware and
> OS.

I agree that boot* and runtime* can be utilized by non-UEFI OS too, we are just reusing existing definitions from UEFI spec.
  . boot-code/boot-data: firmware stage code/data that can be freed after firmware stage ending so OS will have more usable memory.
  . runtime-code/runtime-data: firmware stage code/data that are intended to be utilized by OS stage.
Non-UEFI OS still can implement/support boot* or runtime* memory if they want, and the runtime service can be 'non-UEFI' runtime service too as long as OS/FW aligning each other.
Or non-UEFI OS can simply treat them as "usable memory" if they do not call to any runtime services from those memory regions. (in this case runtime* memory can be repurposed just like boot* memory)
That will be OS choices and we may add some example OS handling to schema description too.

While we are working on UPL specific DT, we got agreement that 2 separate DT are unnecessary, we better align/merge with existing OS DT and OS could utilize those additional UPL DT information too if they want.
This also simplifies/unifies PlatformInit as same DT could support different OS loader Payloads.