Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB
On 1/24/24 18:46, Kinney, Michael D wrote: > Hi Laszlo, > > Yes. I can add more details in the commit message. > > The impact is for ACPI S4. There are many reasons why the set of > HOBs passed into the DXE Core may change from boot to boot or that > allocations in the early DXE init phase should change where the > memory type information bins are allocated. > > ACPI S4 does do a power cycle and it is possible to do FW updates > or FW setup config changes between an S4 save and S4 resume operation. > > It is even possible for one OS to do S4 save. Reboot the system > to boot a completely different OS. Reboot again and do an S4 resume > of the original OS. > > If a platform chooses to enable this feature, the number of scenarios > that an S4 resume can fail is reduced. Awesome info, I couldn't have imagined. (I never use, or develop for, ACPI S4.) Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114461): https://edk2.groups.io/g/devel/message/114461 Mute This Topic: https://groups.io/mt/103918464/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB
Hi Laszlo, Yes. I can add more details in the commit message. The impact is for ACPI S4. There are many reasons why the set of HOBs passed into the DXE Core may change from boot to boot or that allocations in the early DXE init phase should change where the memory type information bins are allocated. ACPI S4 does do a power cycle and it is possible to do FW updates or FW setup config changes between an S4 save and S4 resume operation. It is even possible for one OS to do S4 save. Reboot the system to boot a completely different OS. Reboot again and do an S4 resume of the original OS. If a platform chooses to enable this feature, the number of scenarios that an S4 resume can fail is reduced. Mike > -Original Message- > From: Laszlo Ersek > Sent: Wednesday, January 24, 2024 6:21 AM > To: devel@edk2.groups.io; Kinney, Michael D > Cc: Liming Gao ; Li, Aaron > ; Liu, Yun Y ; Andrew Fish > > Subject: Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set > MemoryTypeInfo bin range from HOB > > On 1/23/24 21:24, Michael D Kinney wrote: > > Provide an optional method for PEI to declare a specific address > > range to use for the Memory Type Information bins. The current > > algorithm uses heuristics that tends to place the Memory Type > > Information bins in the same location, but memory configuration > > changes across boots or algorithm changes across a firmware > > updates could potentially change the Memory Type Information bin > > location. > > (5) Why is such a rearrangement of the bins -- which is likely visible > in the UEFI memory map, too -- a problem? > > Can we include a hint on that in the commit message? > > I understand why it would be a problem across ACPI S4, but a memory > config change (DIMM addition/removal?), or a firmware update, seems to > require a full S5 power cycle. > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114346): https://edk2.groups.io/g/devel/message/114346 Mute This Topic: https://groups.io/mt/103918464/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB
Hi Laszlo, Responses below. Mike > -Original Message- > From: Laszlo Ersek > Sent: Wednesday, January 24, 2024 6:17 AM > To: devel@edk2.groups.io; Kinney, Michael D > Cc: Liming Gao ; Li, Aaron > ; Liu, Yun Y ; Andrew Fish > > Subject: Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set > MemoryTypeInfo bin range from HOB > > On 1/23/24 21:24, Michael D Kinney wrote: > > Provide an optional method for PEI to declare a specific address > > range to use for the Memory Type Information bins. The current > > algorithm uses heuristics that tends to place the Memory Type > > Information bins in the same location, but memory configuration > > changes across boots or algorithm changes across a firmware > > updates could potentially change the Memory Type Information bin > > location. > > > > If the HOB List contains a Resource Descriptor HOB that > > describes tested system memory and has an Owner GUID of > > gEfiMemoryTypeInformationGuid, then use the address range > > described by the Resource Descriptor HOB as the preferred > > location of the Memory Type Information bins. If this HOB is > > not detected, then the current behavior is preserved. > > > > The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid > > is ignored for the following conditions: > > * The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid > > is smaller than the Memory Type Information bins. > > * The HOB list contains more than one Resource Descriptor HOB > > with an owner GUID of gEfiMemoryTypeInformationGuid. > > * The Resource Descriptor HOB with an Owner GUID of > > gEfiMemoryTypeInformationGuid is the same Resource Descriptor > > HOB that that describes the PHIT memory range. > > I feel that, while this GUID usage makes sense, it overloads the already > existing meanings of gEfiMemoryTypeInformationGuid. > > We already use this GUID for two things: > > (see "MdeModulePkg/Include/Guid/MemoryTypeInformation.h"): > > - UEFI variable name space GUID for the "MemoryTypeInformation" variable > > - HOB of type EFI_HOB_TYPE_GUID_EXTENSION, controlling the page counts > in the various memory type bins. > > Now this new use would introduce a HOB of type > EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, where the Owner field is > gEfiMemoryTypeInformationGuid, and the range described by the > PhysicalStart and ResourceLength fields would accommodate the actual > bins. On one hand this new HOB is certainly related to the prior two > uses, but on the other hand, we'd now have to HOBs that used the "same > GUID" for different purposes. What distinguishes them is > EFI_HOB_TYPE_GUID_EXTENSION vs. EFI_HOB_TYPE_RESOURCE_DESCRIPTOR -- > which I find a bit too obscure. > > (1) So I'd either suggest generating a brand new GUID, *or* extending > the > comments in "MdeModulePkg/Include/Guid/MemoryTypeInformation.h" with the > new usage: I will extend the description of the GUID in MemoryTypeInformation.h > > > /** @file > > This file defines: > > * Memory Type Information GUID for HOB and Variable. > > * Memory Type Information Variable Name. > > * Memory Type Information GUID HOB data structure. > > > > The memory type information HOB and variable can > > be used to store the information for each memory type in Variable or > HOB. > > > > Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > Back to the patch: > > On 1/23/24 21:24, Michael D Kinney wrote: > > Update the DxeMain initialization order to initialize GCD > > services before any runtime allocations are performed. This > > is required to prevent runtime data fragmentation when the > > UEFI System Table and UEFI Runtime Service Table is allocated. > > (2) Should this be a separate patch? (First patch in the series?) This > seems > to effect behavior even if the new HOB is not present. I did consider doing this as a separate patch because the change in the order of init in DxeMain can be done independent of this new optional feature. I will break it out to make that clear and simpler to review. The new feature does depend on this new order so it has to be the first patch. > > > > > Cc: Liming Gao > > Cc: Aaron Li > > Cc: Liu Yun > > Cc: Andrew Fish > > Signed-off-by: Michael D Kinney > > --- > > MdeModulePkg/Core/Dxe/DxeMain.h | 6 ++ > > MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 23 +++--- > > MdeModulePkg/Core/Dxe/Gcd/Gcd.c |
Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB
On 1/23/24 21:24, Michael D Kinney wrote: > Provide an optional method for PEI to declare a specific address > range to use for the Memory Type Information bins. The current > algorithm uses heuristics that tends to place the Memory Type > Information bins in the same location, but memory configuration > changes across boots or algorithm changes across a firmware > updates could potentially change the Memory Type Information bin > location. (5) Why is such a rearrangement of the bins -- which is likely visible in the UEFI memory map, too -- a problem? Can we include a hint on that in the commit message? I understand why it would be a problem across ACPI S4, but a memory config change (DIMM addition/removal?), or a firmware update, seems to require a full S5 power cycle. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114301): https://edk2.groups.io/g/devel/message/114301 Mute This Topic: https://groups.io/mt/103918464/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB
On 1/23/24 21:24, Michael D Kinney wrote: > Provide an optional method for PEI to declare a specific address > range to use for the Memory Type Information bins. The current > algorithm uses heuristics that tends to place the Memory Type > Information bins in the same location, but memory configuration > changes across boots or algorithm changes across a firmware > updates could potentially change the Memory Type Information bin > location. > > If the HOB List contains a Resource Descriptor HOB that > describes tested system memory and has an Owner GUID of > gEfiMemoryTypeInformationGuid, then use the address range > described by the Resource Descriptor HOB as the preferred > location of the Memory Type Information bins. If this HOB is > not detected, then the current behavior is preserved. > > The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid > is ignored for the following conditions: > * The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid > is smaller than the Memory Type Information bins. > * The HOB list contains more than one Resource Descriptor HOB > with an owner GUID of gEfiMemoryTypeInformationGuid. > * The Resource Descriptor HOB with an Owner GUID of > gEfiMemoryTypeInformationGuid is the same Resource Descriptor > HOB that that describes the PHIT memory range. I feel that, while this GUID usage makes sense, it overloads the already existing meanings of gEfiMemoryTypeInformationGuid. We already use this GUID for two things: (see "MdeModulePkg/Include/Guid/MemoryTypeInformation.h"): - UEFI variable name space GUID for the "MemoryTypeInformation" variable - HOB of type EFI_HOB_TYPE_GUID_EXTENSION, controlling the page counts in the various memory type bins. Now this new use would introduce a HOB of type EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, where the Owner field is gEfiMemoryTypeInformationGuid, and the range described by the PhysicalStart and ResourceLength fields would accommodate the actual bins. On one hand this new HOB is certainly related to the prior two uses, but on the other hand, we'd now have to HOBs that used the "same GUID" for different purposes. What distinguishes them is EFI_HOB_TYPE_GUID_EXTENSION vs. EFI_HOB_TYPE_RESOURCE_DESCRIPTOR -- which I find a bit too obscure. (1) So I'd either suggest generating a brand new GUID, *or* extending the comments in "MdeModulePkg/Include/Guid/MemoryTypeInformation.h" with the new usage: > /** @file > This file defines: > * Memory Type Information GUID for HOB and Variable. > * Memory Type Information Variable Name. > * Memory Type Information GUID HOB data structure. > > The memory type information HOB and variable can > be used to store the information for each memory type in Variable or HOB. > > Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ Back to the patch: On 1/23/24 21:24, Michael D Kinney wrote: > Update the DxeMain initialization order to initialize GCD > services before any runtime allocations are performed. This > is required to prevent runtime data fragmentation when the > UEFI System Table and UEFI Runtime Service Table is allocated. (2) Should this be a separate patch? (First patch in the series?) This seems to effect behavior even if the new HOB is not present. > > Cc: Liming Gao > Cc: Aaron Li > Cc: Liu Yun > Cc: Andrew Fish > Signed-off-by: Michael D Kinney > --- > MdeModulePkg/Core/Dxe/DxeMain.h | 6 ++ > MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 23 +++--- > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 72 - > MdeModulePkg/Core/Dxe/Mem/Page.c| 101 > 4 files changed, 190 insertions(+), 12 deletions(-) > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h > index 86a7be2f5188..53e26703f8c7 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain.h > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h > @@ -277,6 +277,12 @@ CoreInitializePool ( >VOID >); > > +VOID > +CoreSetMemoryTypeInformationRange ( > + IN EFI_PHYSICAL_ADDRESS Start, > + IN UINT64Length > + ); > + > /** >Called to initialize the memory map and add descriptors to >the current descriptor list. > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > index 0e0f9769b99d..17d510a287e5 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > @@ -276,6 +276,18 @@ DxeMain ( > >MemoryProfileInit (HobStart); > > + // > + // Start the Image Services. > + // > + Status = CoreInitializeImageServices (HobStart); > + ASSERT_EFI_ERROR (Status); > + > + // > + // Initialize the Global Coherency Domain Services > + // > + Status = CoreInitializeGcdServices (, MemoryBaseAddress, > MemoryLength); > + ASSERT_EFI_ERROR (Status); > + >// >// Allocate the EFI System Table and EFI Runtime Service Table from > EfiRuntimeServicesData >// Use
[edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB
Provide an optional method for PEI to declare a specific address
range to use for the Memory Type Information bins. The current
algorithm uses heuristics that tends to place the Memory Type
Information bins in the same location, but memory configuration
changes across boots or algorithm changes across a firmware
updates could potentially change the Memory Type Information bin
location.
If the HOB List contains a Resource Descriptor HOB that
describes tested system memory and has an Owner GUID of
gEfiMemoryTypeInformationGuid, then use the address range
described by the Resource Descriptor HOB as the preferred
location of the Memory Type Information bins. If this HOB is
not detected, then the current behavior is preserved.
The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
is ignored for the following conditions:
* The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
is smaller than the Memory Type Information bins.
* The HOB list contains more than one Resource Descriptor HOB
with an owner GUID of gEfiMemoryTypeInformationGuid.
* The Resource Descriptor HOB with an Owner GUID of
gEfiMemoryTypeInformationGuid is the same Resource Descriptor
HOB that that describes the PHIT memory range.
Update the DxeMain initialization order to initialize GCD
services before any runtime allocations are performed. This
is required to prevent runtime data fragmentation when the
UEFI System Table and UEFI Runtime Service Table is allocated.
Cc: Liming Gao
Cc: Aaron Li
Cc: Liu Yun
Cc: Andrew Fish
Signed-off-by: Michael D Kinney
---
MdeModulePkg/Core/Dxe/DxeMain.h | 6 ++
MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 23 +++---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 72 -
MdeModulePkg/Core/Dxe/Mem/Page.c| 101
4 files changed, 190 insertions(+), 12 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 86a7be2f5188..53e26703f8c7 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -277,6 +277,12 @@ CoreInitializePool (
VOID
);
+VOID
+CoreSetMemoryTypeInformationRange (
+ IN EFI_PHYSICAL_ADDRESS Start,
+ IN UINT64Length
+ );
+
/**
Called to initialize the memory map and add descriptors to
the current descriptor list.
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 0e0f9769b99d..17d510a287e5 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -276,6 +276,18 @@ DxeMain (
MemoryProfileInit (HobStart);
+ //
+ // Start the Image Services.
+ //
+ Status = CoreInitializeImageServices (HobStart);
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Initialize the Global Coherency Domain Services
+ //
+ Status = CoreInitializeGcdServices (, MemoryBaseAddress,
MemoryLength);
+ ASSERT_EFI_ERROR (Status);
+
//
// Allocate the EFI System Table and EFI Runtime Service Table from
EfiRuntimeServicesData
// Use the templates to initialize the contents of the EFI System Table and
EFI Runtime Services Table
@@ -289,16 +301,9 @@ DxeMain (
gDxeCoreST->RuntimeServices = gDxeCoreRT;
//
- // Start the Image Services.
+ // Update DXE Core Loaded Image Protocol with allocated UEFI System Table
//
- Status = CoreInitializeImageServices (HobStart);
- ASSERT_EFI_ERROR (Status);
-
- //
- // Initialize the Global Coherency Domain Services
- //
- Status = CoreInitializeGcdServices (, MemoryBaseAddress,
MemoryLength);
- ASSERT_EFI_ERROR (Status);
+ gDxeCoreLoadedImage->SystemTable = gDxeCoreST;
//
// Call constructor for all libraries
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 792cd2e0af23..c450d1bf2531 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -2238,6 +2238,8 @@ CoreInitializeMemoryServices (
EFI_HOB_HANDOFF_INFO_TABLE *PhitHob;
EFI_HOB_RESOURCE_DESCRIPTOR *ResourceHob;
EFI_HOB_RESOURCE_DESCRIPTOR *PhitResourceHob;
+ EFI_HOB_RESOURCE_DESCRIPTOR *MemoryTypeInformationResourceHob;
+ UINTNCount;
EFI_PHYSICAL_ADDRESS BaseAddress;
UINT64 Length;
UINT64 Attributes;
@@ -2289,12 +2291,47 @@ CoreInitializeMemoryServices (
//
// See if a Memory Type Information HOB is available
//
- GuidHob = GetFirstGuidHob ();
+ MemoryTypeInformationResourceHob = NULL;
+ GuidHob = GetFirstGuidHob
();
if (GuidHob != NULL) {
EfiMemoryTypeInformation = GET_GUID_HOB_DATA (GuidHob);
DataSize = GET_GUID_HOB_DATA_SIZE (GuidHob);
if ((EfiMemoryTypeInformation != NULL) && (DataSize > 0) && (DataSize <=
(EfiMaxMemoryType + 1) * sizeof (EFI_MEMORY_TYPE_INFORMATION))) {
CopyMem (, EfiMemoryTypeInformation, DataSize);
+
+ //
+ // Look for Resource Descriptor HOB with a
