Message-ID: <BN9PR11MB548370E8181B1BB3C6FB9A20E667A@BN9PR11MB5483.namprd11.prod.outlook.com>
Date: Thu, 4 Jan 2024 00:25:31 +0000
From: "Chiu, Chasel" <chasel.chiu@...el.com>
To: Ard Biesheuvel <ardb@...nel.org>
CC: Simon Glass <sjg@...omium.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, Mark Rutland <mark.rutland@....com>, "Rob
 Herring" <robh@...nel.org>, "Tan, Lean Sheng" <sheng.tan@...ements.com>, lkml
	<linux-kernel@...r.kernel.org>, Dhaval Sharma <dhaval@...osinc.com>, "Brune,
 Maximilian" <maximilian.brune@...ements.com>, Yunhui Cui
	<cuiyunhui@...edance.com>, "Dong, Guo" <guo.dong@...el.com>, Tom Rini
	<trini@...sulko.com>, ron minnich <rminnich@...il.com>, "Guo, Gua"
	<gua.guo@...el.com>, "linux-acpi@...r.kernel.org"
	<linux-acpi@...r.kernel.org>, U-Boot Mailing List <u-boot@...ts.denx.de>
Subject: RE: [PATCH v7 2/2] schemas: Add some common reserved-memory usages



> -----Original Message-----
> From: Ard Biesheuvel <ardb@...nel.org>
> Sent: Wednesday, January 3, 2024 7:22 AM
> To: Chiu, Chasel <chasel.chiu@...el.com>
> Cc: Simon Glass <sjg@...omium.org>; devicetree@...r.kernel.org; Mark Rutland
> <mark.rutland@....com>; Rob Herring <robh@...nel.org>; Tan, Lean Sheng
> <sheng.tan@...ements.com>; lkml <linux-kernel@...r.kernel.org>; Dhaval
> Sharma <dhaval@...osinc.com>; Brune, Maximilian
> <maximilian.brune@...ements.com>; Yunhui Cui <cuiyunhui@...edance.com>;
> Dong, Guo <guo.dong@...el.com>; Tom Rini <trini@...sulko.com>; ron minnich
> <rminnich@...il.com>; Guo, Gua <gua.guo@...el.com>; linux-
> acpi@...r.kernel.org; U-Boot Mailing List <u-boot@...ts.denx.de>
> 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@...el.com> 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#efi-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-boot-services-exitbootservices)
. Typically Non-UEFI OS boot will treat those  boot* or runtime* reserved-memory as 'regular' reserved memory if present.

Powered by blists - more mailing lists