Received-SPF: pass (google.com: domain of linux-kernel+bounces-15723-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20230926194242.2732127-1-sjg@chromium.org> <20230926194242.2732127-2-sjg@chromium.org>
 <BN9PR11MB5483FF3039913334C7EA83E1E6AEA@BN9PR11MB5483.namprd11.prod.outlook.com>
 <CAMj1kXFG92NpL7T7YocOup0xLKyopt3MnSCp0RL8cLzozzJz7A@mail.gmail.com>
 <BN9PR11MB548303B09536EB1577472029E6B3A@BN9PR11MB5483.namprd11.prod.outlook.com>
 <CAPnjgZ36t8g7E=0MSJyaV8-QKv9RVYe47Jd5E=NU-mFM4LWBQA@mail.gmail.com>
 <CAMj1kXHAEeK7x2f13k_JV3Xcw61nNLasyvXQf+mKwKekQ48EpQ@mail.gmail.com>
 <BN9PR11MB548334E0DA6495C438FBFDE1E6BBA@BN9PR11MB5483.namprd11.prod.outlook.com>
 <BN9PR11MB548314DDE8D4C9503103D51CE6BBA@BN9PR11MB5483.namprd11.prod.outlook.com>
 <CAMj1kXHbM+ArLgNZgnmiok4gOfv6QLYxzyB9OCwfhEkJ2xGK_g@mail.gmail.com>
 <BN9PR11MB5483C2FBCD07DE61DCCDB523E6BCA@BN9PR11MB5483.namprd11.prod.outlook.com>
 <CAMj1kXHmu=ykgBMRiFqG4_ra3FJtHa=GASoMUJswdMFa9v4Xgw@mail.gmail.com>
 <BN9PR11MB54837EEB391CC2A8FA6C0BF5E695A@BN9PR11MB5483.namprd11.prod.outlook.com>
 <CAMj1kXEQL9n1Adedow5KEyZ5gdFQY3Fn+Fz8vSK3mHib_vDFig@mail.gmail.com>
In-Reply-To: <CAMj1kXEQL9n1Adedow5KEyZ5gdFQY3Fn+Fz8vSK3mHib_vDFig@mail.gmail.com>
From: Rob Herring <robh@kernel.org>
Date: Wed, 3 Jan 2024 09:00:37 -0700
Message-ID: <CAL_JsqKkC1H0Hi2KdF1CH3959Use9ZaruE7uh4ncAyG4C6AqYQ@mail.gmail.com>
Subject: Re: [PATCH v7 2/2] schemas: Add some common reserved-memory usages
To: Ard Biesheuvel <ardb@kernel.org>
Cc: "Chiu, Chasel" <chasel.chiu@intel.com>, Simon Glass <sjg@chromium.org>, 
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	"Tan, Lean Sheng" <sheng.tan@9elements.com>, lkml <linux-kernel@vger.kernel.org>, 
	Dhaval Sharma <dhaval@rivosinc.com>, "Brune, Maximilian" <maximilian.brune@9elements.com>, 
	Yunhui Cui <cuiyunhui@bytedance.com>, "Dong, Guo" <guo.dong@intel.com>, 
	Tom Rini <trini@konsulko.com>, ron minnich <rminnich@gmail.com>, "Guo, Gua" <gua.guo@intel.com>, 
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>, U-Boot Mailing List <u-boot@lists.denx.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 22, 2023 at 5:48=E2=80=AFAM Ard Biesheuvel <ardb@kernel.org> wr=
ote:
>
> On Thu, 21 Dec 2023 at 17:50, Chiu, Chasel <chasel.chiu@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@kernel.org>
> > > Sent: Thursday, December 21, 2023 6:31 AM
> > > To: Chiu, Chasel <chasel.chiu@intel.com>
> > > Cc: Simon Glass <sjg@chromium.org>; devicetree@vger.kernel.org; Mark =
Rutland
> > > <mark.rutland@arm.com>; Rob Herring <robh@kernel.org>; Tan, Lean Shen=
g
> > > <sheng.tan@9elements.com>; lkml <linux-kernel@vger.kernel.org>; Dhava=
l
> > > Sharma <dhaval@rivosinc.com>; Brune, Maximilian
> > > <maximilian.brune@9elements.com>; Yunhui Cui <cuiyunhui@bytedance.com=
>;
> > > Dong, Guo <guo.dong@intel.com>; Tom Rini <trini@konsulko.com>; ron mi=
nnich
> > > <rminnich@gmail.com>; Guo, Gua <gua.guo@intel.com>; linux-
> > > acpi@vger.kernel.org; U-Boot Mailing List <u-boot@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@intel.com> wr=
ote:
> > > >
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > Sent: Tuesday, November 28, 2023 10:08 AM
> > > > > To: Chiu, Chasel <chasel.chiu@intel.com>
> > > > > Cc: Simon Glass <sjg@chromium.org>; devicetree@vger.kernel.org; M=
ark
> > > > > Rutland <mark.rutland@arm.com>; Rob Herring <robh@kernel.org>; Ta=
n,
> > > > > Lean Sheng <sheng.tan@9elements.com>; lkml
> > > > > <linux-kernel@vger.kernel.org>; Dhaval Sharma <dhaval@rivosinc.co=
m>;
> > > > > Brune, Maximilian <maximilian.brune@9elements.com>; Yunhui Cui
> > > > > <cuiyunhui@bytedance.com>; Dong, Guo <guo.dong@intel.com>; Tom Ri=
ni
> > > > > <trini@konsulko.com>; ron minnich <rminnich@gmail.com>; Guo, Gua
> > > > > <gua.guo@intel.com>; linux- acpi@vger.kernel.org; U-Boot Mailing
> > > > > List <u-boot@lists.denx.de>
> > > > > Subject: Re: [PATCH v7 2/2] schemas: Add some common reserved-mem=
ory
> > > > > usages
> > > > >
> > > > > You are referring to a 2000 line patch so it is not 100% clear wh=
ere to look tbh.
> > > > >
> > > > >
> > > > > On Tue, 21 Nov 2023 at 19:37, Chiu, Chasel <chasel.chiu@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 exactl=
y
> > > > > 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.ht=
ml#
> > > > uefi-memory-types-and-mapping-to-acpi-address-range-types
> > > >
> > > > Like I mentioned earlier, that PR is still in early phase and has n=
ot 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 a=
re
> > > platform-specific, for example, some platforms may need bigger runtim=
e 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 ens=
ures 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 mem=
ory
> > > service ->
> > > >           Install memory types table to UEFI system table.Configura=
tion table...
> > > >
> > > > Note: if Payload built-in default MemoryTypes table works fine for =
the
> > > > platform, then FDT reserved-memory node does not need to provide su=
ch
> > > '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 a=
rea you need
> > > more information.
> > > >
> > >
> > > The gEfiMemoryTypeInformationGuid HOB typically carries platform defa=
ults, and
> > > the actual memory type information is kept in a non-volatile EFI vari=
able, 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 mean=
s that this
> > > memory type table has changed, and a reboot is necessary.)
> > >
> > > So the platform init needs to read this variable, or get the informat=
ion 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 payloa=
d(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 a=
nd
> > > 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 gEfiMemoryTypeInformatio=
nGuid usage to FDT->upl-custom node. This is because it is edk2 implementat=
ion choice and non-edk2 PlatformInit or Payload may not have such memory op=
timization implementation. (not a generic usage/requirement for PlatformIni=
t 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 gEfiMemoryT=
ypeInformationGuid HOB information ->
> >     UefiPayload entry to re-create gEfiMemoryTypeInformationGuid HOB ba=
sing on FDT input (instead of the default MemoryType inside UefiPayload) ->
> >       UefiPayload DxeMain/Gcd will consume gEfiMemoryTypeInformationGui=
d Hob for memory type information ->
> >         UefiPayload to initialize UEFI environment (mainly DXE dispatch=
er) ->
> >           (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 PlatformIni=
t to provide data which may be used by Payload or OS. This is not edk2 spec=
ific and any PlatformInit/Payload could have same support.
> > Note: all of below are optional and PlatformInit may choose to implemen=
t some of them or not.
> >
> >       - acpi
> > If PlatformInit created some ACPI tables, this will report a memory reg=
ion which contains all the tables to Payload and Payload may base on this t=
o add some more tables if required.
> >
> >       - acpi-nvs
> > If PlatformInit has created some ACPI tables which having ACPI NVS memo=
ry dependency, this will be that nvs region.
> >
>
> These make sense.
>
> >       - boot-code
> > When PlatformInit having some FW boot phase code that could be freed fo=
r OS to use when payload transferring control to UEFI OS
> >
> >       - boot-data
> > When PlatformInit having some FW boot phase data that could be freed fo=
r OS to use when payload transferring control to UEFI OS.
> >
> >       - runtime-code
> > PlatformInit may provide some services code that can be used for Payloa=
d to initialize UEFI Runtime Services for supporting UEFI OS.
> >
> >       - runtime-data
> > PlatformInit may provide some services data that can be used for Payloa=
d 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.


> 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.

> 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