* [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
[not found] <cover.1694425833.git.fan.wang@intel.com>
@ 2023-09-11 9:51 ` Wang Fan
2023-10-16 20:59 ` Michael D Kinney
[not found] ` <1783CF665DB6E42F.23877@groups.io>
1 sibling, 1 reply; 10+ messages in thread
From: Wang Fan @ 2023-09-11 9:51 UTC (permalink / raw)
To: devel; +Cc: Wang,Fan, Michael D Kinney, Liming Gao, Guomin Jiang, Dandan Bi
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
There are use cases which not all FVs need be migrated from TempRam to
permanent memory before TempRam tears down. This new guid is introduced
to avoid unnecessary FV migration to improve boot performance. Platform
can publish ToMigrateFvInfo hob with this guid to customize FV migration
info, and PeiCore will only migrate FVs indicated by this Hob info.
This is a backwards compatible change, PeiCore will check ToMigrateFvInfo
hob before migration. If ToMigrateFvInfo hobs exists, only migrate FVs
recorded by hobs. If ToMigrateFvInfo hobs not exists, migrate all FVs to
permanent memory.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Wang Fan <fan.wang@intel.com>
---
MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82 +++++++++++++------
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
MdeModulePkg/MdeModulePkg.dec | 3 +-
4 files changed, 79 insertions(+), 26 deletions(-)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index 5f32ebb560ae..e84849ec6db1 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -1184,7 +1184,11 @@ EvacuateTempRam (
PEI_CORE_FV_HANDLE PeiCoreFvHandle;
EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
+ EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
EDKII_MIGRATED_FV_INFO MigratedFvInfo;
+ EFI_PEI_HOB_POINTERS Hob;
+ BOOLEAN MigrateAllFvs;
+ UINT32 MigrationFlags;
ASSERT (Private->PeiMemoryInstalled);
@@ -1211,6 +1215,17 @@ EvacuateTempRam (
ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
+ //
+ // Check if platform defined hobs to indicate which FVs are expected to migrate or keep raw data.
+ // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by hobs.
+ // If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent memory.
+ //
+ MigrateAllFvs = TRUE;
+ Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
+ if (Hob.Raw != NULL) {
+ MigrateAllFvs = FALSE;
+ }
+
for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
FvHeader = Private->Fv[FvIndex].FvHeader;
ASSERT (FvHeader != NULL);
@@ -1224,6 +1239,25 @@ EvacuateTempRam (
)
)
{
+ if (MigrateAllFvs) {
+ MigrationFlags = 0;
+ } else {
+ MigrationFlags = FLAGS_SKIP_FV_MIGRATION | FLAGS_SKIP_FV_RAW_DATA_COPY;
+ Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
+ while (Hob.Raw != NULL) {
+ ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
+ if (ToMigrateFvInfo->FvOrgBase == (UINT32)(UINTN)FvHeader) {
+ MigrationFlags = ToMigrateFvInfo->MigrationFlags;
+ break;
+ }
+ Hob.Raw = GET_NEXT_HOB (Hob);
+ Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid, Hob.Raw);
+ }
+ }
+ if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
+ continue;
+ }
+
//
// Allocate page to save the rebased PEIMs, the PEIMs will get dispatched later.
//
@@ -1234,18 +1268,7 @@ EvacuateTempRam (
);
ASSERT_EFI_ERROR (Status);
MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvHeaderAddress;
-
- //
- // Allocate pool to save the raw PEIMs, which is used to keep consistent context across
- // multiple boot and PCR0 will keep the same no matter if the address of allocated page is changed.
- //
- Status = PeiServicesAllocatePages (
- EfiBootServicesCode,
- EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
- &FvHeaderAddress
- );
- ASSERT_EFI_ERROR (Status);
- RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvHeaderAddress;
+ CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader->FvLength);
DEBUG ((
DEBUG_VERBOSE,
@@ -1256,18 +1279,29 @@ EvacuateTempRam (
));
//
- // Copy the context to the rebased pages and raw pages, and create hob to save the
- // information. The MigratedFvInfo HOB will never be produced when
- // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD control the
- // feature.
+ // Copy the context to the raw pages, and create hob to save the information. The MigratedFvInfo
+ // HOB will never be produced when PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
+ // controls the feature.
//
- CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader->FvLength);
- CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader->FvLength);
- MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
- MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
- MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
- MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader->FvLength;
- BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo, sizeof (MigratedFvInfo));
+ if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) != FLAGS_SKIP_FV_RAW_DATA_COPY) {
+ //
+ // Allocate pool to save the raw PEIMs, which is used to keep consistent context across
+ // multiple boot and PCR0 will keep the same no matter if the address of allocated page is changed.
+ //
+ Status = PeiServicesAllocatePages (
+ EfiBootServicesCode,
+ EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
+ &FvHeaderAddress
+ );
+ ASSERT_EFI_ERROR (Status);
+ RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvHeaderAddress;
+ CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader->FvLength);
+ MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
+ MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
+ MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
+ MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader->FvLength;
+ BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo, sizeof (MigratedFvInfo));
+ }
//
// Migrate any children for this FV now
@@ -1330,8 +1364,6 @@ EvacuateTempRam (
}
}
- RemoveFvHobsInTemporaryMemory (Private);
-
return Status;
}
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 0cf357371a16..944b230b0e19 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -78,6 +78,7 @@
gEfiFirmwareFileSystem3Guid
gStatusCodeCallbackGuid
gEdkiiMigratedFvInfoGuid ## SOMETIMES_PRODUCES ## HOB
+ gEdkiiToMigrateFvInfoGuid ## SOMETIMES_CONSUMES ## HOB
[Ppis]
gEfiPeiStatusCodePpiGuid ## SOMETIMES_CONSUMES # PeiReportStatusService is not ready if this PPI doesn't exist
diff --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
index aca2332a0ec6..543cd9ba7ddd 100644
--- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
+++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
@@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
#define __EDKII_MIGRATED_FV_INFO_GUID_H__
+#define FLAGS_SKIP_FV_MIGRATION BIT0
+#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
+
+///
+/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published by platform to indicate
+/// one FV is expected to migrate to permarnant memory or not before TempRam tears down.
+///
+typedef struct {
+ UINT32 FvOrgBase; // original FV address
+ //
+ // Migration Flags:
+ // Bit0: Indicate to skip FV migration or not
+ // Bit1: Indicate to skip FV raw data copy or not
+ // Others: Reserved bits
+ //
+ UINT32 MigrationFlags;
+} EDKII_TO_MIGRATE_FV_INFO;
+
typedef struct {
UINT32 FvOrgBase; // original FV address
UINT32 FvNewBase; // new FV address
@@ -16,6 +34,7 @@ typedef struct {
UINT32 FvLength; // Fv Length
} EDKII_MIGRATED_FV_INFO;
+extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
extern EFI_GUID gEdkiiMigratedFvInfoGuid;
#endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index dd182c02fdf6..d6cbcc721a5e 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -416,7 +416,8 @@
gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93, 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
## Include/Guid/MigratedFvInfo.h
- gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
+ gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93, 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
+ gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
## Include/Guid/RngAlgorithm.h
gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108482): https://edk2.groups.io/g/devel/message/108482
Mute This Topic: https://groups.io/mt/101289546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
[not found] ` <1783CF665DB6E42F.23877@groups.io>
@ 2023-09-22 5:32 ` Wang Fan
0 siblings, 0 replies; 10+ messages in thread
From: Wang Fan @ 2023-09-22 5:32 UTC (permalink / raw)
To: devel@edk2.groups.io, Kinney, Michael D, Gao, Liming,
Jiang, Guomin, Bi, Dandan
Cc: Wang, Fan
Hi Mike, Liming, Guomin and Dandan
Could you help review this change?
Best Regards
Fan
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wang Fan
Sent: Monday, September 11, 2023 5:52 PM
To: devel@edk2.groups.io
Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
There are use cases which not all FVs need be migrated from TempRam to permanent memory before TempRam tears down. This new guid is introduced to avoid unnecessary FV migration to improve boot performance. Platform can publish ToMigrateFvInfo hob with this guid to customize FV migration info, and PeiCore will only migrate FVs indicated by this Hob info.
This is a backwards compatible change, PeiCore will check ToMigrateFvInfo hob before migration. If ToMigrateFvInfo hobs exists, only migrate FVs recorded by hobs. If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent memory.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Wang Fan <fan.wang@intel.com>
---
MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82 +++++++++++++------
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
MdeModulePkg/MdeModulePkg.dec | 3 +-
4 files changed, 79 insertions(+), 26 deletions(-)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index 5f32ebb560ae..e84849ec6db1 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -1184,7 +1184,11 @@ EvacuateTempRam (
PEI_CORE_FV_HANDLE PeiCoreFvHandle;
EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
+ EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
EDKII_MIGRATED_FV_INFO MigratedFvInfo;
+ EFI_PEI_HOB_POINTERS Hob;
+ BOOLEAN MigrateAllFvs;
+ UINT32 MigrationFlags;
ASSERT (Private->PeiMemoryInstalled);
@@ -1211,6 +1215,17 @@ EvacuateTempRam (
ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
+ //
+ // Check if platform defined hobs to indicate which FVs are expected to migrate or keep raw data.
+ // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by hobs.
+ // If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent memory.
+ //
+ MigrateAllFvs = TRUE;
+ Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
+ if (Hob.Raw != NULL) {
+ MigrateAllFvs = FALSE;
+ }
+
for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
FvHeader = Private->Fv[FvIndex].FvHeader;
ASSERT (FvHeader != NULL);
@@ -1224,6 +1239,25 @@ EvacuateTempRam (
)
)
{
+ if (MigrateAllFvs) {
+ MigrationFlags = 0;
+ } else {
+ MigrationFlags = FLAGS_SKIP_FV_MIGRATION | FLAGS_SKIP_FV_RAW_DATA_COPY;
+ Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
+ while (Hob.Raw != NULL) {
+ ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
+ if (ToMigrateFvInfo->FvOrgBase == (UINT32)(UINTN)FvHeader) {
+ MigrationFlags = ToMigrateFvInfo->MigrationFlags;
+ break;
+ }
+ Hob.Raw = GET_NEXT_HOB (Hob);
+ Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid, Hob.Raw);
+ }
+ }
+ if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
+ continue;
+ }
+
//
// Allocate page to save the rebased PEIMs, the PEIMs will get dispatched later.
//
@@ -1234,18 +1268,7 @@ EvacuateTempRam (
);
ASSERT_EFI_ERROR (Status);
MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvHeaderAddress;
-
- //
- // Allocate pool to save the raw PEIMs, which is used to keep consistent context across
- // multiple boot and PCR0 will keep the same no matter if the address of allocated page is changed.
- //
- Status = PeiServicesAllocatePages (
- EfiBootServicesCode,
- EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
- &FvHeaderAddress
- );
- ASSERT_EFI_ERROR (Status);
- RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvHeaderAddress;
+ CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader->FvLength);
DEBUG ((
DEBUG_VERBOSE,
@@ -1256,18 +1279,29 @@ EvacuateTempRam (
));
//
- // Copy the context to the rebased pages and raw pages, and create hob to save the
- // information. The MigratedFvInfo HOB will never be produced when
- // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD control the
- // feature.
+ // Copy the context to the raw pages, and create hob to save the information. The MigratedFvInfo
+ // HOB will never be produced when PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
+ // controls the feature.
//
- CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader->FvLength);
- CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader->FvLength);
- MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
- MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
- MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
- MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader->FvLength;
- BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo, sizeof (MigratedFvInfo));
+ if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) != FLAGS_SKIP_FV_RAW_DATA_COPY) {
+ //
+ // Allocate pool to save the raw PEIMs, which is used to keep consistent context across
+ // multiple boot and PCR0 will keep the same no matter if the address of allocated page is changed.
+ //
+ Status = PeiServicesAllocatePages (
+ EfiBootServicesCode,
+ EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
+ &FvHeaderAddress
+ );
+ ASSERT_EFI_ERROR (Status);
+ RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvHeaderAddress;
+ CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader->FvLength);
+ MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
+ MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
+ MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
+ MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader->FvLength;
+ BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo, sizeof (MigratedFvInfo));
+ }
//
// Migrate any children for this FV now @@ -1330,8 +1364,6 @@ EvacuateTempRam (
}
}
- RemoveFvHobsInTemporaryMemory (Private);
-
return Status;
}
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 0cf357371a16..944b230b0e19 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -78,6 +78,7 @@
gEfiFirmwareFileSystem3Guid
gStatusCodeCallbackGuid
gEdkiiMigratedFvInfoGuid ## SOMETIMES_PRODUCES ## HOB
+ gEdkiiToMigrateFvInfoGuid ## SOMETIMES_CONSUMES ## HOB
[Ppis]
gEfiPeiStatusCodePpiGuid ## SOMETIMES_CONSUMES # PeiReportStatusService is not ready if this PPI doesn't exist
diff --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
index aca2332a0ec6..543cd9ba7ddd 100644
--- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
+++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
@@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ #define __EDKII_MIGRATED_FV_INFO_GUID_H__
+#define FLAGS_SKIP_FV_MIGRATION BIT0
+#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
+
+///
+/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published by
+platform to indicate /// one FV is expected to migrate to permarnant memory or not before TempRam tears down.
+///
+typedef struct {
+ UINT32 FvOrgBase; // original FV address
+ //
+ // Migration Flags:
+ // Bit0: Indicate to skip FV migration or not
+ // Bit1: Indicate to skip FV raw data copy or not
+ // Others: Reserved bits
+ //
+ UINT32 MigrationFlags;
+} EDKII_TO_MIGRATE_FV_INFO;
+
typedef struct {
UINT32 FvOrgBase; // original FV address
UINT32 FvNewBase; // new FV address
@@ -16,6 +34,7 @@ typedef struct {
UINT32 FvLength; // Fv Length
} EDKII_MIGRATED_FV_INFO;
+extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
extern EFI_GUID gEdkiiMigratedFvInfoGuid;
#endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index dd182c02fdf6..d6cbcc721a5e 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -416,7 +416,8 @@
gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93, 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
## Include/Guid/MigratedFvInfo.h
- gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
+ gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93,
+ 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } } gEdkiiMigratedFvInfoGuid
+ = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17,
+ 0x98, 0x71 } }
## Include/Guid/RngAlgorithm.h
gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108978): https://edk2.groups.io/g/devel/message/108978
Mute This Topic: https://groups.io/mt/101289546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-09-11 9:51 ` [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information Wang Fan
@ 2023-10-16 20:59 ` Michael D Kinney
2023-10-27 8:23 ` Wang Fan
0 siblings, 1 reply; 10+ messages in thread
From: Michael D Kinney @ 2023-10-16 20:59 UTC (permalink / raw)
To: Wang, Fan, devel@edk2.groups.io
Cc: Gao, Liming, Jiang, Guomin, Bi, Dandan, Kinney, Michael D
Hi Fan,
The logic looks functional, but there are some names and descriptions
that could be added to help describe this feature and some code
changes to make the code easier to understand.
1) The GUID gEdkiiToMigrateFvInfoGuid is hard to understand what
information it conveys from the name of the GUID variable.
I know that the intent is that it is a GUID with data that
tells the PEI core which FVs in temporary RAM should be
migrated to permanent RAM. And that the use of this information
is only a performance optimization to not migrate FVs that
are not needed after the PEI Core migrates temp RAM to
permanent RAM. The name and description in comments do not
express all these details.
2) The structure name EDKII_TO_MIGRATE_FV_INFO has the same
issue as (1).
a) Should FvOrgBase be a UINT64 or support temp RAM above 4GB?
b) Since only temp RAM to perm RAM migrations are supported
the field name "FvOrgBase" should be "FvTemporaryRamBase".
3) The #define for FLAGS_SKIP_FV_MIGRATION should have a comment
block that describes the meaning of this bit. What is the
meaning when the bit is set and what is the meaning when the
bit is clear.
4) The #define for FLAGS_SKIP_FV_RAW_DATA_COPY should have a comment
block that describes the meaning of this bit. What is the
meaning when the bit is set and what is the meaning when the
bit is clear.
5) Why are there 2 bits? If an FV is skipped, then that means
do not copy. If an FV is not skipped, then that means do
copy.
6) I think the variable MigrateAllFvs can be removed, and the HOB
list check can be made for gEdkiiToMigrateFvInfoGuid can be made
on each FV found in temporary RAM. This looks like a performance
optimization that makes the logic harder to understand and it
is not clear that the performance optimization has any benefit.
7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed. Why?
8) The comment where memory is allocated for the migrated FV says
"allocate pool" when PeiServicesAllocatePages() is used. Please
update the comment.
Mike
> -----Original Message-----
> From: Wang, Fan <fan.wang@intel.com>
> Sent: Monday, September 11, 2023 2:52 AM
> To: devel@edk2.groups.io
> Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration
> Information
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
>
> There are use cases which not all FVs need be migrated from TempRam to
> permanent memory before TempRam tears down. This new guid is
> introduced
> to avoid unnecessary FV migration to improve boot performance.
> Platform
> can publish ToMigrateFvInfo hob with this guid to customize FV
> migration
> info, and PeiCore will only migrate FVs indicated by this Hob info.
>
> This is a backwards compatible change, PeiCore will check
> ToMigrateFvInfo
> hob before migration. If ToMigrateFvInfo hobs exists, only migrate FVs
> recorded by hobs. If ToMigrateFvInfo hobs not exists, migrate all FVs
> to
> permanent memory.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82 +++++++++++++-----
> -
> MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
> MdeModulePkg/MdeModulePkg.dec | 3 +-
> 4 files changed, 79 insertions(+), 26 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 5f32ebb560ae..e84849ec6db1 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1184,7 +1184,11 @@ EvacuateTempRam (
>
> PEI_CORE_FV_HANDLE PeiCoreFvHandle;
> EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> + EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
> EDKII_MIGRATED_FV_INFO MigratedFvInfo;
> + EFI_PEI_HOB_POINTERS Hob;
> + BOOLEAN MigrateAllFvs;
> + UINT32 MigrationFlags;
>
> ASSERT (Private->PeiMemoryInstalled);
>
> @@ -1211,6 +1215,17 @@ EvacuateTempRam (
>
> ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
>
> + //
> + // Check if platform defined hobs to indicate which FVs are
> expected to migrate or keep raw data.
> + // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by
> hobs.
> + // If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent
> memory.
> + //
> + MigrateAllFvs = TRUE;
> + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> + if (Hob.Raw != NULL) {
> + MigrateAllFvs = FALSE;
> + }
> +
> for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
> FvHeader = Private->Fv[FvIndex].FvHeader;
> ASSERT (FvHeader != NULL);
> @@ -1224,6 +1239,25 @@ EvacuateTempRam (
> )
> )
> {
> + if (MigrateAllFvs) {
> + MigrationFlags = 0;
> + } else {
> + MigrationFlags = FLAGS_SKIP_FV_MIGRATION |
> FLAGS_SKIP_FV_RAW_DATA_COPY;
> + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> + while (Hob.Raw != NULL) {
> + ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
> + if (ToMigrateFvInfo->FvOrgBase == (UINT32)(UINTN)FvHeader)
> {
> + MigrationFlags = ToMigrateFvInfo->MigrationFlags;
> + break;
> + }
> + Hob.Raw = GET_NEXT_HOB (Hob);
> + Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid,
> Hob.Raw);
> + }
> + }
> + if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
> + continue;
> + }
> +
> //
> // Allocate page to save the rebased PEIMs, the PEIMs will get
> dispatched later.
> //
> @@ -1234,18 +1268,7 @@ EvacuateTempRam (
> );
> ASSERT_EFI_ERROR (Status);
> MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> -
> - //
> - // Allocate pool to save the raw PEIMs, which is used to keep
> consistent context across
> - // multiple boot and PCR0 will keep the same no matter if the
> address of allocated page is changed.
> - //
> - Status = PeiServicesAllocatePages (
> - EfiBootServicesCode,
> - EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> - &FvHeaderAddress
> - );
> - ASSERT_EFI_ERROR (Status);
> - RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> + CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> >FvLength);
>
> DEBUG ((
> DEBUG_VERBOSE,
> @@ -1256,18 +1279,29 @@ EvacuateTempRam (
> ));
>
> //
> - // Copy the context to the rebased pages and raw pages, and
> create hob to save the
> - // information. The MigratedFvInfo HOB will never be produced
> when
> - // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the
> PCD control the
> - // feature.
> + // Copy the context to the raw pages, and create hob to save
> the information. The MigratedFvInfo
> + // HOB will never be produced when
> PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> + // controls the feature.
> //
> - CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> >FvLength);
> - CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader-
> >FvLength);
> - MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> - MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
> - MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> - MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader->FvLength;
> - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> sizeof (MigratedFvInfo));
> + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=
> FLAGS_SKIP_FV_RAW_DATA_COPY) {
> + //
> + // Allocate pool to save the raw PEIMs, which is used to keep
> consistent context across
> + // multiple boot and PCR0 will keep the same no matter if the
> address of allocated page is changed.
> + //
> + Status = PeiServicesAllocatePages (
> + EfiBootServicesCode,
> + EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> + &FvHeaderAddress
> + );
> + ASSERT_EFI_ERROR (Status);
> + RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> + CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader-
> >FvLength);
> + MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> + MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
> + MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> + MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader-
> >FvLength;
> + BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> sizeof (MigratedFvInfo));
> + }
>
> //
> // Migrate any children for this FV now
> @@ -1330,8 +1364,6 @@ EvacuateTempRam (
> }
> }
>
> - RemoveFvHobsInTemporaryMemory (Private);
> -
> return Status;
> }
>
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> b/MdeModulePkg/Core/Pei/PeiMain.inf
> index 0cf357371a16..944b230b0e19 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> @@ -78,6 +78,7 @@
> gEfiFirmwareFileSystem3Guid
> gStatusCodeCallbackGuid
> gEdkiiMigratedFvInfoGuid ## SOMETIMES_PRODUCES
> ## HOB
> + gEdkiiToMigrateFvInfoGuid ## SOMETIMES_CONSUMES
> ## HOB
>
> [Ppis]
> gEfiPeiStatusCodePpiGuid ## SOMETIMES_CONSUMES
> # PeiReportStatusService is not ready if this PPI doesn't exist
> diff --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> index aca2332a0ec6..543cd9ba7ddd 100644
> --- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> @@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
> #define __EDKII_MIGRATED_FV_INFO_GUID_H__
>
> +#define FLAGS_SKIP_FV_MIGRATION BIT0
> +#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
> +
> +///
> +/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published by
> platform to indicate
> +/// one FV is expected to migrate to permarnant memory or not before
> TempRam tears down.
> +///
> +typedef struct {
> + UINT32 FvOrgBase; // original FV address
> + //
> + // Migration Flags:
> + // Bit0: Indicate to skip FV migration or not
> + // Bit1: Indicate to skip FV raw data copy or not
> + // Others: Reserved bits
> + //
> + UINT32 MigrationFlags;
> +} EDKII_TO_MIGRATE_FV_INFO;
> +
> typedef struct {
> UINT32 FvOrgBase; // original FV address
> UINT32 FvNewBase; // new FV address
> @@ -16,6 +34,7 @@ typedef struct {
> UINT32 FvLength; // Fv Length
> } EDKII_MIGRATED_FV_INFO;
>
> +extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
> extern EFI_GUID gEdkiiMigratedFvInfoGuid;
>
> #endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index dd182c02fdf6..d6cbcc721a5e 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -416,7 +416,8 @@
> gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93,
> 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
>
> ## Include/Guid/MigratedFvInfo.h
> - gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> + gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93,
> 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
> + gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
>
> ## Include/Guid/RngAlgorithm.h
> gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> --
> 2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109655): https://edk2.groups.io/g/devel/message/109655
Mute This Topic: https://groups.io/mt/101289546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-10-16 20:59 ` Michael D Kinney
@ 2023-10-27 8:23 ` Wang Fan
2023-11-20 6:30 ` Wang Fan
2023-12-01 1:01 ` 回复: " gaoliming via groups.io
0 siblings, 2 replies; 10+ messages in thread
From: Wang Fan @ 2023-10-27 8:23 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Gao, Liming, Jiang, Guomin, Bi, Dandan
Hi Mike
Thank you for your feedback.
I have updated the patch to v3: https://edk2.groups.io/g/devel/message/110197
Pull Request: https://github.com/tianocore/edk2/pull/4970
Based on V2, this update includes changes:
- Add more descriptions for "gEdkiiToMigrateFvInfoGuid" PPI usages and background, but still keep the name.
- Update "FvOrgBase" to "FvTemporaryRamBase" in EDKII_TO_MIGRATE_FV_INFO.
- Remove flag "FLAGS_SKIP_FV_MIGRATION", since it's not needed.
- Add more descriptions to flag "FLAGS_SKIP_FV_RAW_DATA_COPY".
- Remove the variable MigrateAllFvs to simplify logic.
- Correct "allocate pool" to "allocate pages" to align with descriptions.
For other two questions you are mentioning:
1. For "Should FvOrgBase be a UINT64 or support temp RAM above 4GB?":
I think UINT32 should be enough, all pre-mem FVs are below 4G as far as I know, even we enabled X64 in pre-mem phase. This setting is also aligning with "FvOrgBase" defined in "EDKII_MIGRATED_FV_INFO".
2. For "The call to RemoveFvHobsInTemporaryMemory (Private) was removed":
Since this function will remove all FV hobs for physical addresses, it should be called only when all pre-mem FVs are migrated. In EvacuateTempRam(), when one FV is migrated, ConvertFvHob() will be called for this FV and all its' child FVs, this is enough to ensure already migrated FV hobs are all pointing to addresses on permanent address.
What do you think?
Best Regards
Fan
-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Tuesday, October 17, 2023 5:00 AM
To: Wang, Fan <fan.wang@intel.com>; devel@edk2.groups.io
Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
Hi Fan,
The logic looks functional, but there are some names and descriptions that could be added to help describe this feature and some code changes to make the code easier to understand.
1) The GUID gEdkiiToMigrateFvInfoGuid is hard to understand what
information it conveys from the name of the GUID variable.
I know that the intent is that it is a GUID with data that
tells the PEI core which FVs in temporary RAM should be
migrated to permanent RAM. And that the use of this information
is only a performance optimization to not migrate FVs that
are not needed after the PEI Core migrates temp RAM to
permanent RAM. The name and description in comments do not
express all these details.
2) The structure name EDKII_TO_MIGRATE_FV_INFO has the same
issue as (1).
a) Should FvOrgBase be a UINT64 or support temp RAM above 4GB?
b) Since only temp RAM to perm RAM migrations are supported
the field name "FvOrgBase" should be "FvTemporaryRamBase".
3) The #define for FLAGS_SKIP_FV_MIGRATION should have a comment
block that describes the meaning of this bit. What is the
meaning when the bit is set and what is the meaning when the
bit is clear.
4) The #define for FLAGS_SKIP_FV_RAW_DATA_COPY should have a comment
block that describes the meaning of this bit. What is the
meaning when the bit is set and what is the meaning when the
bit is clear.
5) Why are there 2 bits? If an FV is skipped, then that means
do not copy. If an FV is not skipped, then that means do
copy.
6) I think the variable MigrateAllFvs can be removed, and the HOB
list check can be made for gEdkiiToMigrateFvInfoGuid can be made
on each FV found in temporary RAM. This looks like a performance
optimization that makes the logic harder to understand and it
is not clear that the performance optimization has any benefit.
7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed. Why?
8) The comment where memory is allocated for the migrated FV says
"allocate pool" when PeiServicesAllocatePages() is used. Please
update the comment.
Mike
> -----Original Message-----
> From: Wang, Fan <fan.wang@intel.com>
> Sent: Monday, September 11, 2023 2:52 AM
> To: devel@edk2.groups.io
> Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration
> Information
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
>
> There are use cases which not all FVs need be migrated from TempRam to
> permanent memory before TempRam tears down. This new guid is
> introduced to avoid unnecessary FV migration to improve boot
> performance.
> Platform
> can publish ToMigrateFvInfo hob with this guid to customize FV
> migration info, and PeiCore will only migrate FVs indicated by this
> Hob info.
>
> This is a backwards compatible change, PeiCore will check
> ToMigrateFvInfo hob before migration. If ToMigrateFvInfo hobs exists,
> only migrate FVs recorded by hobs. If ToMigrateFvInfo hobs not exists,
> migrate all FVs to permanent memory.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82 +++++++++++++-----
> -
> MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
> MdeModulePkg/MdeModulePkg.dec | 3 +-
> 4 files changed, 79 insertions(+), 26 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 5f32ebb560ae..e84849ec6db1 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1184,7 +1184,11 @@ EvacuateTempRam (
>
> PEI_CORE_FV_HANDLE PeiCoreFvHandle;
> EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> + EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
> EDKII_MIGRATED_FV_INFO MigratedFvInfo;
> + EFI_PEI_HOB_POINTERS Hob;
> + BOOLEAN MigrateAllFvs;
> + UINT32 MigrationFlags;
>
> ASSERT (Private->PeiMemoryInstalled);
>
> @@ -1211,6 +1215,17 @@ EvacuateTempRam (
>
> ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
>
> + //
> + // Check if platform defined hobs to indicate which FVs are
> expected to migrate or keep raw data.
> + // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by
> hobs.
> + // If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent
> memory.
> + //
> + MigrateAllFvs = TRUE;
> + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> + if (Hob.Raw != NULL) {
> + MigrateAllFvs = FALSE;
> + }
> +
> for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
> FvHeader = Private->Fv[FvIndex].FvHeader;
> ASSERT (FvHeader != NULL);
> @@ -1224,6 +1239,25 @@ EvacuateTempRam (
> )
> )
> {
> + if (MigrateAllFvs) {
> + MigrationFlags = 0;
> + } else {
> + MigrationFlags = FLAGS_SKIP_FV_MIGRATION |
> FLAGS_SKIP_FV_RAW_DATA_COPY;
> + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> + while (Hob.Raw != NULL) {
> + ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
> + if (ToMigrateFvInfo->FvOrgBase == (UINT32)(UINTN)FvHeader)
> {
> + MigrationFlags = ToMigrateFvInfo->MigrationFlags;
> + break;
> + }
> + Hob.Raw = GET_NEXT_HOB (Hob);
> + Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid,
> Hob.Raw);
> + }
> + }
> + if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
> + continue;
> + }
> +
> //
> // Allocate page to save the rebased PEIMs, the PEIMs will get
> dispatched later.
> //
> @@ -1234,18 +1268,7 @@ EvacuateTempRam (
> );
> ASSERT_EFI_ERROR (Status);
> MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> -
> - //
> - // Allocate pool to save the raw PEIMs, which is used to keep
> consistent context across
> - // multiple boot and PCR0 will keep the same no matter if the
> address of allocated page is changed.
> - //
> - Status = PeiServicesAllocatePages (
> - EfiBootServicesCode,
> - EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> - &FvHeaderAddress
> - );
> - ASSERT_EFI_ERROR (Status);
> - RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> + CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> >FvLength);
>
> DEBUG ((
> DEBUG_VERBOSE,
> @@ -1256,18 +1279,29 @@ EvacuateTempRam (
> ));
>
> //
> - // Copy the context to the rebased pages and raw pages, and
> create hob to save the
> - // information. The MigratedFvInfo HOB will never be produced
> when
> - // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the
> PCD control the
> - // feature.
> + // Copy the context to the raw pages, and create hob to save
> the information. The MigratedFvInfo
> + // HOB will never be produced when
> PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> + // controls the feature.
> //
> - CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> >FvLength);
> - CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader-
> >FvLength);
> - MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> - MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
> - MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> - MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader->FvLength;
> - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> sizeof (MigratedFvInfo));
> + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=
> FLAGS_SKIP_FV_RAW_DATA_COPY) {
> + //
> + // Allocate pool to save the raw PEIMs, which is used to keep
> consistent context across
> + // multiple boot and PCR0 will keep the same no matter if the
> address of allocated page is changed.
> + //
> + Status = PeiServicesAllocatePages (
> + EfiBootServicesCode,
> + EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> + &FvHeaderAddress
> + );
> + ASSERT_EFI_ERROR (Status);
> + RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> + CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader-
> >FvLength);
> + MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> + MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
> + MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> + MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader-
> >FvLength;
> + BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> sizeof (MigratedFvInfo));
> + }
>
> //
> // Migrate any children for this FV now @@ -1330,8 +1364,6 @@
> EvacuateTempRam (
> }
> }
>
> - RemoveFvHobsInTemporaryMemory (Private);
> -
> return Status;
> }
>
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> b/MdeModulePkg/Core/Pei/PeiMain.inf
> index 0cf357371a16..944b230b0e19 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> @@ -78,6 +78,7 @@
> gEfiFirmwareFileSystem3Guid
> gStatusCodeCallbackGuid
> gEdkiiMigratedFvInfoGuid ## SOMETIMES_PRODUCES
> ## HOB
> + gEdkiiToMigrateFvInfoGuid ## SOMETIMES_CONSUMES
> ## HOB
>
> [Ppis]
> gEfiPeiStatusCodePpiGuid ## SOMETIMES_CONSUMES
> # PeiReportStatusService is not ready if this PPI doesn't exist diff
> --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> index aca2332a0ec6..543cd9ba7ddd 100644
> --- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> @@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #ifndef
> __EDKII_MIGRATED_FV_INFO_GUID_H__ #define
> __EDKII_MIGRATED_FV_INFO_GUID_H__
>
> +#define FLAGS_SKIP_FV_MIGRATION BIT0
> +#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
> +
> +///
> +/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published by
> platform to indicate
> +/// one FV is expected to migrate to permarnant memory or not before
> TempRam tears down.
> +///
> +typedef struct {
> + UINT32 FvOrgBase; // original FV address
> + //
> + // Migration Flags:
> + // Bit0: Indicate to skip FV migration or not
> + // Bit1: Indicate to skip FV raw data copy or not
> + // Others: Reserved bits
> + //
> + UINT32 MigrationFlags;
> +} EDKII_TO_MIGRATE_FV_INFO;
> +
> typedef struct {
> UINT32 FvOrgBase; // original FV address
> UINT32 FvNewBase; // new FV address
> @@ -16,6 +34,7 @@ typedef struct {
> UINT32 FvLength; // Fv Length
> } EDKII_MIGRATED_FV_INFO;
>
> +extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
> extern EFI_GUID gEdkiiMigratedFvInfoGuid;
>
> #endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ diff --git
> a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index
> dd182c02fdf6..d6cbcc721a5e 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -416,7 +416,8 @@
> gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93,
> 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
>
> ## Include/Guid/MigratedFvInfo.h
> - gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> + gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93,
> 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
> + gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
>
> ## Include/Guid/RngAlgorithm.h
> gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> --
> 2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110200): https://edk2.groups.io/g/devel/message/110200
Mute This Topic: https://groups.io/mt/101289546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-10-27 8:23 ` Wang Fan
@ 2023-11-20 6:30 ` Wang Fan
2023-12-01 1:01 ` 回复: " gaoliming via groups.io
1 sibling, 0 replies; 10+ messages in thread
From: Wang Fan @ 2023-11-20 6:30 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Liming, Kinney, Michael D
Hi Mike and Liming
Do you have time to take a look this update?
V3: https://edk2.groups.io/g/devel/message/110197
Pull Request: https://github.com/tianocore/edk2/pull/4970
Best Regards
Fan
-----Original Message-----
From: Wang, Fan
Sent: Friday, October 27, 2023 4:24 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin <Guomin.Jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
Hi Mike
Thank you for your feedback.
I have updated the patch to v3: https://edk2.groups.io/g/devel/message/110197
Pull Request: https://github.com/tianocore/edk2/pull/4970
Based on V2, this update includes changes:
- Add more descriptions for "gEdkiiToMigrateFvInfoGuid" PPI usages and background, but still keep the name.
- Update "FvOrgBase" to "FvTemporaryRamBase" in EDKII_TO_MIGRATE_FV_INFO.
- Remove flag "FLAGS_SKIP_FV_MIGRATION", since it's not needed.
- Add more descriptions to flag "FLAGS_SKIP_FV_RAW_DATA_COPY".
- Remove the variable MigrateAllFvs to simplify logic.
- Correct "allocate pool" to "allocate pages" to align with descriptions.
For other two questions you are mentioning:
1. For "Should FvOrgBase be a UINT64 or support temp RAM above 4GB?":
I think UINT32 should be enough, all pre-mem FVs are below 4G as far as I know, even we enabled X64 in pre-mem phase. This setting is also aligning with "FvOrgBase" defined in "EDKII_MIGRATED_FV_INFO".
2. For "The call to RemoveFvHobsInTemporaryMemory (Private) was removed":
Since this function will remove all FV hobs for physical addresses, it should be called only when all pre-mem FVs are migrated. In EvacuateTempRam(), when one FV is migrated, ConvertFvHob() will be called for this FV and all its' child FVs, this is enough to ensure already migrated FV hobs are all pointing to addresses on permanent address.
What do you think?
Best Regards
Fan
-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Tuesday, October 17, 2023 5:00 AM
To: Wang, Fan <fan.wang@intel.com>; devel@edk2.groups.io
Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
Hi Fan,
The logic looks functional, but there are some names and descriptions that could be added to help describe this feature and some code changes to make the code easier to understand.
1) The GUID gEdkiiToMigrateFvInfoGuid is hard to understand what
information it conveys from the name of the GUID variable.
I know that the intent is that it is a GUID with data that
tells the PEI core which FVs in temporary RAM should be
migrated to permanent RAM. And that the use of this information
is only a performance optimization to not migrate FVs that
are not needed after the PEI Core migrates temp RAM to
permanent RAM. The name and description in comments do not
express all these details.
2) The structure name EDKII_TO_MIGRATE_FV_INFO has the same
issue as (1).
a) Should FvOrgBase be a UINT64 or support temp RAM above 4GB?
b) Since only temp RAM to perm RAM migrations are supported
the field name "FvOrgBase" should be "FvTemporaryRamBase".
3) The #define for FLAGS_SKIP_FV_MIGRATION should have a comment
block that describes the meaning of this bit. What is the
meaning when the bit is set and what is the meaning when the
bit is clear.
4) The #define for FLAGS_SKIP_FV_RAW_DATA_COPY should have a comment
block that describes the meaning of this bit. What is the
meaning when the bit is set and what is the meaning when the
bit is clear.
5) Why are there 2 bits? If an FV is skipped, then that means
do not copy. If an FV is not skipped, then that means do
copy.
6) I think the variable MigrateAllFvs can be removed, and the HOB
list check can be made for gEdkiiToMigrateFvInfoGuid can be made
on each FV found in temporary RAM. This looks like a performance
optimization that makes the logic harder to understand and it
is not clear that the performance optimization has any benefit.
7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed. Why?
8) The comment where memory is allocated for the migrated FV says
"allocate pool" when PeiServicesAllocatePages() is used. Please
update the comment.
Mike
> -----Original Message-----
> From: Wang, Fan <fan.wang@intel.com>
> Sent: Monday, September 11, 2023 2:52 AM
> To: devel@edk2.groups.io
> Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration
> Information
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
>
> There are use cases which not all FVs need be migrated from TempRam to
> permanent memory before TempRam tears down. This new guid is
> introduced to avoid unnecessary FV migration to improve boot
> performance.
> Platform
> can publish ToMigrateFvInfo hob with this guid to customize FV
> migration info, and PeiCore will only migrate FVs indicated by this
> Hob info.
>
> This is a backwards compatible change, PeiCore will check
> ToMigrateFvInfo hob before migration. If ToMigrateFvInfo hobs exists,
> only migrate FVs recorded by hobs. If ToMigrateFvInfo hobs not exists,
> migrate all FVs to permanent memory.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82 +++++++++++++-----
> -
> MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
> MdeModulePkg/MdeModulePkg.dec | 3 +-
> 4 files changed, 79 insertions(+), 26 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 5f32ebb560ae..e84849ec6db1 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1184,7 +1184,11 @@ EvacuateTempRam (
>
> PEI_CORE_FV_HANDLE PeiCoreFvHandle;
> EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> + EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
> EDKII_MIGRATED_FV_INFO MigratedFvInfo;
> + EFI_PEI_HOB_POINTERS Hob;
> + BOOLEAN MigrateAllFvs;
> + UINT32 MigrationFlags;
>
> ASSERT (Private->PeiMemoryInstalled);
>
> @@ -1211,6 +1215,17 @@ EvacuateTempRam (
>
> ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
>
> + //
> + // Check if platform defined hobs to indicate which FVs are
> expected to migrate or keep raw data.
> + // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by
> hobs.
> + // If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent
> memory.
> + //
> + MigrateAllFvs = TRUE;
> + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> + if (Hob.Raw != NULL) {
> + MigrateAllFvs = FALSE;
> + }
> +
> for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
> FvHeader = Private->Fv[FvIndex].FvHeader;
> ASSERT (FvHeader != NULL);
> @@ -1224,6 +1239,25 @@ EvacuateTempRam (
> )
> )
> {
> + if (MigrateAllFvs) {
> + MigrationFlags = 0;
> + } else {
> + MigrationFlags = FLAGS_SKIP_FV_MIGRATION |
> FLAGS_SKIP_FV_RAW_DATA_COPY;
> + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> + while (Hob.Raw != NULL) {
> + ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
> + if (ToMigrateFvInfo->FvOrgBase == (UINT32)(UINTN)FvHeader)
> {
> + MigrationFlags = ToMigrateFvInfo->MigrationFlags;
> + break;
> + }
> + Hob.Raw = GET_NEXT_HOB (Hob);
> + Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid,
> Hob.Raw);
> + }
> + }
> + if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
> + continue;
> + }
> +
> //
> // Allocate page to save the rebased PEIMs, the PEIMs will get
> dispatched later.
> //
> @@ -1234,18 +1268,7 @@ EvacuateTempRam (
> );
> ASSERT_EFI_ERROR (Status);
> MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> -
> - //
> - // Allocate pool to save the raw PEIMs, which is used to keep
> consistent context across
> - // multiple boot and PCR0 will keep the same no matter if the
> address of allocated page is changed.
> - //
> - Status = PeiServicesAllocatePages (
> - EfiBootServicesCode,
> - EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> - &FvHeaderAddress
> - );
> - ASSERT_EFI_ERROR (Status);
> - RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> + CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> >FvLength);
>
> DEBUG ((
> DEBUG_VERBOSE,
> @@ -1256,18 +1279,29 @@ EvacuateTempRam (
> ));
>
> //
> - // Copy the context to the rebased pages and raw pages, and
> create hob to save the
> - // information. The MigratedFvInfo HOB will never be produced
> when
> - // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the
> PCD control the
> - // feature.
> + // Copy the context to the raw pages, and create hob to save
> the information. The MigratedFvInfo
> + // HOB will never be produced when
> PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> + // controls the feature.
> //
> - CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> >FvLength);
> - CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader-
> >FvLength);
> - MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> - MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
> - MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> - MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader->FvLength;
> - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> sizeof (MigratedFvInfo));
> + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=
> FLAGS_SKIP_FV_RAW_DATA_COPY) {
> + //
> + // Allocate pool to save the raw PEIMs, which is used to keep
> consistent context across
> + // multiple boot and PCR0 will keep the same no matter if the
> address of allocated page is changed.
> + //
> + Status = PeiServicesAllocatePages (
> + EfiBootServicesCode,
> + EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> + &FvHeaderAddress
> + );
> + ASSERT_EFI_ERROR (Status);
> + RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> + CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN)FvHeader-
> >FvLength);
> + MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> + MigratedFvInfo.FvNewBase = (UINT32)(UINTN)MigratedFvHeader;
> + MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> + MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader-
> >FvLength;
> + BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> sizeof (MigratedFvInfo));
> + }
>
> //
> // Migrate any children for this FV now @@ -1330,8 +1364,6 @@
> EvacuateTempRam (
> }
> }
>
> - RemoveFvHobsInTemporaryMemory (Private);
> -
> return Status;
> }
>
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> b/MdeModulePkg/Core/Pei/PeiMain.inf
> index 0cf357371a16..944b230b0e19 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> @@ -78,6 +78,7 @@
> gEfiFirmwareFileSystem3Guid
> gStatusCodeCallbackGuid
> gEdkiiMigratedFvInfoGuid ## SOMETIMES_PRODUCES
> ## HOB
> + gEdkiiToMigrateFvInfoGuid ## SOMETIMES_CONSUMES
> ## HOB
>
> [Ppis]
> gEfiPeiStatusCodePpiGuid ## SOMETIMES_CONSUMES
> # PeiReportStatusService is not ready if this PPI doesn't exist diff
> --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> index aca2332a0ec6..543cd9ba7ddd 100644
> --- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> @@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #ifndef
> __EDKII_MIGRATED_FV_INFO_GUID_H__ #define
> __EDKII_MIGRATED_FV_INFO_GUID_H__
>
> +#define FLAGS_SKIP_FV_MIGRATION BIT0
> +#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
> +
> +///
> +/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published by
> platform to indicate
> +/// one FV is expected to migrate to permarnant memory or not before
> TempRam tears down.
> +///
> +typedef struct {
> + UINT32 FvOrgBase; // original FV address
> + //
> + // Migration Flags:
> + // Bit0: Indicate to skip FV migration or not
> + // Bit1: Indicate to skip FV raw data copy or not
> + // Others: Reserved bits
> + //
> + UINT32 MigrationFlags;
> +} EDKII_TO_MIGRATE_FV_INFO;
> +
> typedef struct {
> UINT32 FvOrgBase; // original FV address
> UINT32 FvNewBase; // new FV address
> @@ -16,6 +34,7 @@ typedef struct {
> UINT32 FvLength; // Fv Length
> } EDKII_MIGRATED_FV_INFO;
>
> +extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
> extern EFI_GUID gEdkiiMigratedFvInfoGuid;
>
> #endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ diff --git
> a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index
> dd182c02fdf6..d6cbcc721a5e 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -416,7 +416,8 @@
> gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93,
> 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
>
> ## Include/Guid/MigratedFvInfo.h
> - gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> + gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93,
> 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
> + gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
>
> ## Include/Guid/RngAlgorithm.h
> gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> --
> 2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111458): https://edk2.groups.io/g/devel/message/111458
Mute This Topic: https://groups.io/mt/101289546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
* 回复: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-10-27 8:23 ` Wang Fan
2023-11-20 6:30 ` Wang Fan
@ 2023-12-01 1:01 ` gaoliming via groups.io
2023-12-01 3:05 ` Ni, Ray
1 sibling, 1 reply; 10+ messages in thread
From: gaoliming via groups.io @ 2023-12-01 1:01 UTC (permalink / raw)
To: devel, fan.wang, 'Kinney, Michael D'
Cc: 'Jiang, Guomin', 'Bi, Dandan'
Fan:
I add my comment below.
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Wang Fan
> 发送时间: 2023年10月27日 16:24
> 收件人: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> 抄送: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> 主题: Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized
> FV Migration Information
>
> Hi Mike
>
> Thank you for your feedback.
>
> I have updated the patch to v3:
> https://edk2.groups.io/g/devel/message/110197
>
> Pull Request: https://github.com/tianocore/edk2/pull/4970
>
> Based on V2, this update includes changes:
> - Add more descriptions for "gEdkiiToMigrateFvInfoGuid" PPI usages and
> background, but still keep the name.
> - Update "FvOrgBase" to "FvTemporaryRamBase" in
> EDKII_TO_MIGRATE_FV_INFO.
> - Remove flag "FLAGS_SKIP_FV_MIGRATION", since it's not needed.
> - Add more descriptions to flag "FLAGS_SKIP_FV_RAW_DATA_COPY".
[Liming] RAW_DATA_COPY is for measure boot to make sure PCR0 have the same
value on the different boot.
If RAW_DATA_COPY is not set, it will impact the measure boot. So, I don't
think we can skip raw data copy.
> - Remove the variable MigrateAllFvs to simplify logic.
> - Correct "allocate pool" to "allocate pages" to align with descriptions.
>
> For other two questions you are mentioning:
>
> 1. For "Should FvOrgBase be a UINT64 or support temp RAM above 4GB?":
> I think UINT32 should be enough, all pre-mem FVs are below 4G as far as I
> know, even we enabled X64 in pre-mem phase. This setting is also aligning
> with "FvOrgBase" defined in "EDKII_MIGRATED_FV_INFO".
> 2. For "The call to RemoveFvHobsInTemporaryMemory (Private) was
> removed":
> Since this function will remove all FV hobs for physical addresses, it
should be
> called only when all pre-mem FVs are migrated. In EvacuateTempRam(), when
> one FV is migrated, ConvertFvHob() will be called for this FV and all its'
child
> FVs, this is enough to ensure already migrated FV hobs are all pointing to
> addresses on permanent address.
>
[Liming] So, RemoveFvHobsInTemporaryMemory () does nothing now. Right?
If yes, it is safe to remove it.
Thanks
Liming
> What do you think?
>
> Best Regards
> Fan
>
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Tuesday, October 17, 2023 5:00 AM
> To: Wang, Fan <fan.wang@intel.com>; devel@edk2.groups.io
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV
> Migration Information
>
> Hi Fan,
>
> The logic looks functional, but there are some names and descriptions that
> could be added to help describe this feature and some code changes to make
> the code easier to understand.
>
> 1) The GUID gEdkiiToMigrateFvInfoGuid is hard to understand what
> information it conveys from the name of the GUID variable.
> I know that the intent is that it is a GUID with data that
> tells the PEI core which FVs in temporary RAM should be
> migrated to permanent RAM. And that the use of this information
> is only a performance optimization to not migrate FVs that
> are not needed after the PEI Core migrates temp RAM to
> permanent RAM. The name and description in comments do not
> express all these details.
>
> 2) The structure name EDKII_TO_MIGRATE_FV_INFO has the same
> issue as (1).
> a) Should FvOrgBase be a UINT64 or support temp RAM above 4GB?
> b) Since only temp RAM to perm RAM migrations are supported
> the field name "FvOrgBase" should be "FvTemporaryRamBase".
>
>
> 3) The #define for FLAGS_SKIP_FV_MIGRATION should have a comment
> block that describes the meaning of this bit. What is the
> meaning when the bit is set and what is the meaning when the
> bit is clear.
>
> 4) The #define for FLAGS_SKIP_FV_RAW_DATA_COPY should have a comment
> block that describes the meaning of this bit. What is the
> meaning when the bit is set and what is the meaning when the
> bit is clear.
>
> 5) Why are there 2 bits? If an FV is skipped, then that means
> do not copy. If an FV is not skipped, then that means do
> copy.
>
> 6) I think the variable MigrateAllFvs can be removed, and the HOB
> list check can be made for gEdkiiToMigrateFvInfoGuid can be made
> on each FV found in temporary RAM. This looks like a performance
> optimization that makes the logic harder to understand and it
> is not clear that the performance optimization has any benefit.
>
> 7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed.
> Why?
>
> 8) The comment where memory is allocated for the migrated FV says
> "allocate pool" when PeiServicesAllocatePages() is used. Please
> update the comment.
>
> Mike
>
>
>
> > -----Original Message-----
> > From: Wang, Fan <fan.wang@intel.com>
> > Sent: Monday, September 11, 2023 2:52 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> > Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> > <dandan.bi@intel.com>
> > Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration
> > Information
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
> >
> > There are use cases which not all FVs need be migrated from TempRam to
> > permanent memory before TempRam tears down. This new guid is
> > introduced to avoid unnecessary FV migration to improve boot
> > performance.
> > Platform
> > can publish ToMigrateFvInfo hob with this guid to customize FV
> > migration info, and PeiCore will only migrate FVs indicated by this
> > Hob info.
> >
> > This is a backwards compatible change, PeiCore will check
> > ToMigrateFvInfo hob before migration. If ToMigrateFvInfo hobs exists,
> > only migrate FVs recorded by hobs. If ToMigrateFvInfo hobs not exists,
> > migrate all FVs to permanent memory.
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Signed-off-by: Wang Fan <fan.wang@intel.com>
> > ---
> > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82
> +++++++++++++-----
> > -
> > MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> > MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
> > MdeModulePkg/MdeModulePkg.dec | 3 +-
> > 4 files changed, 79 insertions(+), 26 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > index 5f32ebb560ae..e84849ec6db1 100644
> > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > @@ -1184,7 +1184,11 @@ EvacuateTempRam (
> >
> > PEI_CORE_FV_HANDLE PeiCoreFvHandle;
> > EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> > + EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
> > EDKII_MIGRATED_FV_INFO MigratedFvInfo;
> > + EFI_PEI_HOB_POINTERS Hob;
> > + BOOLEAN MigrateAllFvs;
> > + UINT32 MigrationFlags;
> >
> > ASSERT (Private->PeiMemoryInstalled);
> >
> > @@ -1211,6 +1215,17 @@ EvacuateTempRam (
> >
> > ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
> >
> > + //
> > + // Check if platform defined hobs to indicate which FVs are
> > expected to migrate or keep raw data.
> > + // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by
> > hobs.
> > + // If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent
> > memory.
> > + //
> > + MigrateAllFvs = TRUE;
> > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > + if (Hob.Raw != NULL) {
> > + MigrateAllFvs = FALSE;
> > + }
> > +
> > for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
> > FvHeader = Private->Fv[FvIndex].FvHeader;
> > ASSERT (FvHeader != NULL);
> > @@ -1224,6 +1239,25 @@ EvacuateTempRam (
> > )
> > )
> > {
> > + if (MigrateAllFvs) {
> > + MigrationFlags = 0;
> > + } else {
> > + MigrationFlags = FLAGS_SKIP_FV_MIGRATION |
> > FLAGS_SKIP_FV_RAW_DATA_COPY;
> > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > + while (Hob.Raw != NULL) {
> > + ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
> > + if (ToMigrateFvInfo->FvOrgBase ==
> (UINT32)(UINTN)FvHeader)
> > {
> > + MigrationFlags = ToMigrateFvInfo->MigrationFlags;
> > + break;
> > + }
> > + Hob.Raw = GET_NEXT_HOB (Hob);
> > + Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid,
> > Hob.Raw);
> > + }
> > + }
> > + if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
> > + continue;
> > + }
> > +
> > //
> > // Allocate page to save the rebased PEIMs, the PEIMs will get
> > dispatched later.
> > //
> > @@ -1234,18 +1268,7 @@ EvacuateTempRam (
> > );
> > ASSERT_EFI_ERROR (Status);
> > MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > *)(UINTN)FvHeaderAddress;
> > -
> > - //
> > - // Allocate pool to save the raw PEIMs, which is used to keep
> > consistent context across
> > - // multiple boot and PCR0 will keep the same no matter if the
> > address of allocated page is changed.
> > - //
> > - Status = PeiServicesAllocatePages (
> > - EfiBootServicesCode,
> > - EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> > - &FvHeaderAddress
> > - );
> > - ASSERT_EFI_ERROR (Status);
> > - RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > *)(UINTN)FvHeaderAddress;
> > + CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > >FvLength);
> >
> > DEBUG ((
> > DEBUG_VERBOSE,
> > @@ -1256,18 +1279,29 @@ EvacuateTempRam (
> > ));
> >
> > //
> > - // Copy the context to the rebased pages and raw pages, and
> > create hob to save the
> > - // information. The MigratedFvInfo HOB will never be produced
> > when
> > - // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because
> the
> > PCD control the
> > - // feature.
> > + // Copy the context to the raw pages, and create hob to save
> > the information. The MigratedFvInfo
> > + // HOB will never be produced when
> > PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> > + // controls the feature.
> > //
> > - CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > >FvLength);
> > - CopyMem (RawDataFvHeader, MigratedFvHeader,
> (UINTN)FvHeader-
> > >FvLength);
> > - MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > - MigratedFvInfo.FvNewBase =
> (UINT32)(UINTN)MigratedFvHeader;
> > - MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> > - MigratedFvInfo.FvLength =
> (UINT32)(UINTN)FvHeader->FvLength;
> > - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> > sizeof (MigratedFvInfo));
> > + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=
> > FLAGS_SKIP_FV_RAW_DATA_COPY) {
> > + //
> > + // Allocate pool to save the raw PEIMs, which is used to keep
> > consistent context across
> > + // multiple boot and PCR0 will keep the same no matter if the
> > address of allocated page is changed.
> > + //
> > + Status = PeiServicesAllocatePages (
> > + EfiBootServicesCode,
> > + EFI_SIZE_TO_PAGES
> ((UINTN)FvHeader->FvLength),
> > + &FvHeaderAddress
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > + RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > *)(UINTN)FvHeaderAddress;
> > + CopyMem (RawDataFvHeader, MigratedFvHeader,
> (UINTN)FvHeader-
> > >FvLength);
> > + MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > + MigratedFvInfo.FvNewBase =
> (UINT32)(UINTN)MigratedFvHeader;
> > + MigratedFvInfo.FvDataBase =
> (UINT32)(UINTN)RawDataFvHeader;
> > + MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader-
> > >FvLength;
> > + BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid,
> &MigratedFvInfo,
> > sizeof (MigratedFvInfo));
> > + }
> >
> > //
> > // Migrate any children for this FV now @@ -1330,8 +1364,6 @@
> > EvacuateTempRam (
> > }
> > }
> >
> > - RemoveFvHobsInTemporaryMemory (Private);
> > -
> > return Status;
> > }
> >
> > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> > b/MdeModulePkg/Core/Pei/PeiMain.inf
> > index 0cf357371a16..944b230b0e19 100644
> > --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> > @@ -78,6 +78,7 @@
> > gEfiFirmwareFileSystem3Guid
> > gStatusCodeCallbackGuid
> > gEdkiiMigratedFvInfoGuid ##
> SOMETIMES_PRODUCES
> > ## HOB
> > + gEdkiiToMigrateFvInfoGuid ##
> SOMETIMES_CONSUMES
> > ## HOB
> >
> > [Ppis]
> > gEfiPeiStatusCodePpiGuid ##
> SOMETIMES_CONSUMES
> > # PeiReportStatusService is not ready if this PPI doesn't exist diff
> > --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > index aca2332a0ec6..543cd9ba7ddd 100644
> > --- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > @@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #ifndef
> > __EDKII_MIGRATED_FV_INFO_GUID_H__ #define
> > __EDKII_MIGRATED_FV_INFO_GUID_H__
> >
> > +#define FLAGS_SKIP_FV_MIGRATION BIT0
> > +#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
> > +
> > +///
> > +/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published by
> > platform to indicate
> > +/// one FV is expected to migrate to permarnant memory or not before
> > TempRam tears down.
> > +///
> > +typedef struct {
> > + UINT32 FvOrgBase; // original FV address
> > + //
> > + // Migration Flags:
> > + // Bit0: Indicate to skip FV migration or not
> > + // Bit1: Indicate to skip FV raw data copy or not
> > + // Others: Reserved bits
> > + //
> > + UINT32 MigrationFlags;
> > +} EDKII_TO_MIGRATE_FV_INFO;
> > +
> > typedef struct {
> > UINT32 FvOrgBase; // original FV address
> > UINT32 FvNewBase; // new FV address
> > @@ -16,6 +34,7 @@ typedef struct {
> > UINT32 FvLength; // Fv Length
> > } EDKII_MIGRATED_FV_INFO;
> >
> > +extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
> > extern EFI_GUID gEdkiiMigratedFvInfoGuid;
> >
> > #endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ diff --git
> > a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index
> > dd182c02fdf6..d6cbcc721a5e 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -416,7 +416,8 @@
> > gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93,
> > 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
> >
> > ## Include/Guid/MigratedFvInfo.h
> > - gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> > + gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93,
> > 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
> > + gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> >
> > ## Include/Guid/RngAlgorithm.h
> > gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> > 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> > --
> > 2.29.2.windows.2
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111955): https://edk2.groups.io/g/devel/message/111955
Mute This Topic: https://groups.io/mt/102906881/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-12-01 1:01 ` 回复: " gaoliming via groups.io
@ 2023-12-01 3:05 ` Ni, Ray
2023-12-04 10:31 ` Wang Fan
0 siblings, 1 reply; 10+ messages in thread
From: Ni, Ray @ 2023-12-01 3:05 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Liming, Wang, Fan, Kinney, Michael D
Cc: Jiang, Guomin, Bi, Dandan
When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, FV in flash (cached in code cache) is copied to physical memory and all C global pointers (PPIs, GUIDs) are fixed up.
But TCG needs to measure the original FV contents, so the original FV is saved into MigratedFvInfo HOB when the FV migration is done.
Now we are going to add a new ToMigrate HOB to tell something.
Initially without reading the patch, I thought the HOB is to tell which FV needs to be migrated.
But what the patch does is: migration is always done when PCD is true. The only thing that's skipped is not to save the original FV contents, then the MigratedFvInfo HOB is not produced as well.
More strangely, if a FV is listed in "ToMigrate" HOB with flag 0, it's the same as the "ToMigrate" HOB does not exist.
It's a bit confusing, in my opinion.
There are several approaches:
1. Do not add "ToMigrate" HOB. Instead, avoid saving the whole original FV content, only save the delta that TCG driver uses the new FV content and the delta to calculate the HASH. The save of delta should not impact the performance very much.
2. Abandon the PCD, and update today's logic to follow the new "ToMigrate" HOB to perform migration. A new flag tells whether the original FV content needs to be saved.
3. Keep the PCD. If "ToMigrate" HOB does not exist, follow PCD. If "ToMigrate" HOB exists, follow the HOB and ignores the PCD. HOB takes higher priority. It's still backward compatible and allows edk2 to gradually retire that PCD. (Having multiple interfaces controlling one behavior is always confusing.)
3.1 The flag name in the patch is "*SKIP*". I prefer we reverse the meaning of the flag, meaning when the flag is set, FV original content is saved. The flag name can be "FLAGS_BUILD_MIGRATED_FV_INFO" which directly maps to the behavior the flag controls. I agree "MigratedFvInfo" name is confusing but at least we avoid adding another confusing "SKIP_FV_RAW_DATA_COPY" flag.
Thanks,
Ray
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> via groups.io
> Sent: Friday, December 1, 2023 9:01 AM
> To: devel@edk2.groups.io; Wang, Fan <fan.wang@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Cc: Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: 回复: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support
> customized FV Migration Information
>
> Fan:
> I add my comment below.
>
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Wang Fan
> > 发送时间: 2023年10月27日 16:24
> > 收件人: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > 抄送: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> > 主题: Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support
> customized
> > FV Migration Information
> >
> > Hi Mike
> >
> > Thank you for your feedback.
> >
> > I have updated the patch to v3:
> > https://edk2.groups.io/g/devel/message/110197
> >
> > Pull Request: https://github.com/tianocore/edk2/pull/4970
> >
> > Based on V2, this update includes changes:
> > - Add more descriptions for "gEdkiiToMigrateFvInfoGuid" PPI usages and
> > background, but still keep the name.
> > - Update "FvOrgBase" to "FvTemporaryRamBase" in
> > EDKII_TO_MIGRATE_FV_INFO.
> > - Remove flag "FLAGS_SKIP_FV_MIGRATION", since it's not needed.
> > - Add more descriptions to flag "FLAGS_SKIP_FV_RAW_DATA_COPY".
> [Liming] RAW_DATA_COPY is for measure boot to make sure PCR0 have the
> same
> value on the different boot.
> If RAW_DATA_COPY is not set, it will impact the measure boot. So, I don't
> think we can skip raw data copy.
>
> > - Remove the variable MigrateAllFvs to simplify logic.
> > - Correct "allocate pool" to "allocate pages" to align with descriptions.
> >
> > For other two questions you are mentioning:
> >
> > 1. For "Should FvOrgBase be a UINT64 or support temp RAM above 4GB?":
> > I think UINT32 should be enough, all pre-mem FVs are below 4G as far as I
> > know, even we enabled X64 in pre-mem phase. This setting is also aligning
> > with "FvOrgBase" defined in "EDKII_MIGRATED_FV_INFO".
> > 2. For "The call to RemoveFvHobsInTemporaryMemory (Private) was
> > removed":
> > Since this function will remove all FV hobs for physical addresses, it
> should be
> > called only when all pre-mem FVs are migrated. In EvacuateTempRam(),
> when
> > one FV is migrated, ConvertFvHob() will be called for this FV and all its'
> child
> > FVs, this is enough to ensure already migrated FV hobs are all pointing to
> > addresses on permanent address.
> >
> [Liming] So, RemoveFvHobsInTemporaryMemory () does nothing now. Right?
> If yes, it is safe to remove it.
>
> Thanks
> Liming
> > What do you think?
> >
> > Best Regards
> > Fan
> >
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Tuesday, October 17, 2023 5:00 AM
> > To: Wang, Fan <fan.wang@intel.com>; devel@edk2.groups.io
> > Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV
> > Migration Information
> >
> > Hi Fan,
> >
> > The logic looks functional, but there are some names and descriptions that
> > could be added to help describe this feature and some code changes to make
> > the code easier to understand.
> >
> > 1) The GUID gEdkiiToMigrateFvInfoGuid is hard to understand what
> > information it conveys from the name of the GUID variable.
> > I know that the intent is that it is a GUID with data that
> > tells the PEI core which FVs in temporary RAM should be
> > migrated to permanent RAM. And that the use of this information
> > is only a performance optimization to not migrate FVs that
> > are not needed after the PEI Core migrates temp RAM to
> > permanent RAM. The name and description in comments do not
> > express all these details.
> >
> > 2) The structure name EDKII_TO_MIGRATE_FV_INFO has the same
> > issue as (1).
> > a) Should FvOrgBase be a UINT64 or support temp RAM above 4GB?
> > b) Since only temp RAM to perm RAM migrations are supported
> > the field name "FvOrgBase" should be "FvTemporaryRamBase".
> >
> >
> > 3) The #define for FLAGS_SKIP_FV_MIGRATION should have a comment
> > block that describes the meaning of this bit. What is the
> > meaning when the bit is set and what is the meaning when the
> > bit is clear.
> >
> > 4) The #define for FLAGS_SKIP_FV_RAW_DATA_COPY should have a
> comment
> > block that describes the meaning of this bit. What is the
> > meaning when the bit is set and what is the meaning when the
> > bit is clear.
> >
> > 5) Why are there 2 bits? If an FV is skipped, then that means
> > do not copy. If an FV is not skipped, then that means do
> > copy.
> >
> > 6) I think the variable MigrateAllFvs can be removed, and the HOB
> > list check can be made for gEdkiiToMigrateFvInfoGuid can be made
> > on each FV found in temporary RAM. This looks like a performance
> > optimization that makes the logic harder to understand and it
> > is not clear that the performance optimization has any benefit.
> >
> > 7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed.
> > Why?
> >
> > 8) The comment where memory is allocated for the migrated FV says
> > "allocate pool" when PeiServicesAllocatePages() is used. Please
> > update the comment.
> >
> > Mike
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Fan <fan.wang@intel.com>
> > > Sent: Monday, September 11, 2023 2:52 AM
> > > To: devel@edk2.groups.io
> > > Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> > > Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> > > <dandan.bi@intel.com>
> > > Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV
> Migration
> > > Information
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
> > >
> > > There are use cases which not all FVs need be migrated from TempRam to
> > > permanent memory before TempRam tears down. This new guid is
> > > introduced to avoid unnecessary FV migration to improve boot
> > > performance.
> > > Platform
> > > can publish ToMigrateFvInfo hob with this guid to customize FV
> > > migration info, and PeiCore will only migrate FVs indicated by this
> > > Hob info.
> > >
> > > This is a backwards compatible change, PeiCore will check
> > > ToMigrateFvInfo hob before migration. If ToMigrateFvInfo hobs exists,
> > > only migrate FVs recorded by hobs. If ToMigrateFvInfo hobs not exists,
> > > migrate all FVs to permanent memory.
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > Cc: Dandan Bi <dandan.bi@intel.com>
> > > Signed-off-by: Wang Fan <fan.wang@intel.com>
> > > ---
> > > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82
> > +++++++++++++-----
> > > -
> > > MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> > > MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
> > > MdeModulePkg/MdeModulePkg.dec | 3 +-
> > > 4 files changed, 79 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > index 5f32ebb560ae..e84849ec6db1 100644
> > > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > @@ -1184,7 +1184,11 @@ EvacuateTempRam (
> > >
> > > PEI_CORE_FV_HANDLE PeiCoreFvHandle;
> > > EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> > > + EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
> > > EDKII_MIGRATED_FV_INFO MigratedFvInfo;
> > > + EFI_PEI_HOB_POINTERS Hob;
> > > + BOOLEAN MigrateAllFvs;
> > > + UINT32 MigrationFlags;
> > >
> > > ASSERT (Private->PeiMemoryInstalled);
> > >
> > > @@ -1211,6 +1215,17 @@ EvacuateTempRam (
> > >
> > > ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
> > >
> > > + //
> > > + // Check if platform defined hobs to indicate which FVs are
> > > expected to migrate or keep raw data.
> > > + // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by
> > > hobs.
> > > + // If ToMigrateFvInfo hobs not exists, migrate all FVs to permanent
> > > memory.
> > > + //
> > > + MigrateAllFvs = TRUE;
> > > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > > + if (Hob.Raw != NULL) {
> > > + MigrateAllFvs = FALSE;
> > > + }
> > > +
> > > for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
> > > FvHeader = Private->Fv[FvIndex].FvHeader;
> > > ASSERT (FvHeader != NULL);
> > > @@ -1224,6 +1239,25 @@ EvacuateTempRam (
> > > )
> > > )
> > > {
> > > + if (MigrateAllFvs) {
> > > + MigrationFlags = 0;
> > > + } else {
> > > + MigrationFlags = FLAGS_SKIP_FV_MIGRATION |
> > > FLAGS_SKIP_FV_RAW_DATA_COPY;
> > > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > > + while (Hob.Raw != NULL) {
> > > + ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
> > > + if (ToMigrateFvInfo->FvOrgBase ==
> > (UINT32)(UINTN)FvHeader)
> > > {
> > > + MigrationFlags = ToMigrateFvInfo->MigrationFlags;
> > > + break;
> > > + }
> > > + Hob.Raw = GET_NEXT_HOB (Hob);
> > > + Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid,
> > > Hob.Raw);
> > > + }
> > > + }
> > > + if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
> > > + continue;
> > > + }
> > > +
> > > //
> > > // Allocate page to save the rebased PEIMs, the PEIMs will get
> > > dispatched later.
> > > //
> > > @@ -1234,18 +1268,7 @@ EvacuateTempRam (
> > > );
> > > ASSERT_EFI_ERROR (Status);
> > > MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > *)(UINTN)FvHeaderAddress;
> > > -
> > > - //
> > > - // Allocate pool to save the raw PEIMs, which is used to keep
> > > consistent context across
> > > - // multiple boot and PCR0 will keep the same no matter if the
> > > address of allocated page is changed.
> > > - //
> > > - Status = PeiServicesAllocatePages (
> > > - EfiBootServicesCode,
> > > - EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> > > - &FvHeaderAddress
> > > - );
> > > - ASSERT_EFI_ERROR (Status);
> > > - RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > *)(UINTN)FvHeaderAddress;
> > > + CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > > >FvLength);
> > >
> > > DEBUG ((
> > > DEBUG_VERBOSE,
> > > @@ -1256,18 +1279,29 @@ EvacuateTempRam (
> > > ));
> > >
> > > //
> > > - // Copy the context to the rebased pages and raw pages, and
> > > create hob to save the
> > > - // information. The MigratedFvInfo HOB will never be produced
> > > when
> > > - // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because
> > the
> > > PCD control the
> > > - // feature.
> > > + // Copy the context to the raw pages, and create hob to save
> > > the information. The MigratedFvInfo
> > > + // HOB will never be produced when
> > > PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> > > + // controls the feature.
> > > //
> > > - CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > > >FvLength);
> > > - CopyMem (RawDataFvHeader, MigratedFvHeader,
> > (UINTN)FvHeader-
> > > >FvLength);
> > > - MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > > - MigratedFvInfo.FvNewBase =
> > (UINT32)(UINTN)MigratedFvHeader;
> > > - MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> > > - MigratedFvInfo.FvLength =
> > (UINT32)(UINTN)FvHeader->FvLength;
> > > - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> > > sizeof (MigratedFvInfo));
> > > + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=
> > > FLAGS_SKIP_FV_RAW_DATA_COPY) {
> > > + //
> > > + // Allocate pool to save the raw PEIMs, which is used to keep
> > > consistent context across
> > > + // multiple boot and PCR0 will keep the same no matter if the
> > > address of allocated page is changed.
> > > + //
> > > + Status = PeiServicesAllocatePages (
> > > + EfiBootServicesCode,
> > > + EFI_SIZE_TO_PAGES
> > ((UINTN)FvHeader->FvLength),
> > > + &FvHeaderAddress
> > > + );
> > > + ASSERT_EFI_ERROR (Status);
> > > + RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > *)(UINTN)FvHeaderAddress;
> > > + CopyMem (RawDataFvHeader, MigratedFvHeader,
> > (UINTN)FvHeader-
> > > >FvLength);
> > > + MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > > + MigratedFvInfo.FvNewBase =
> > (UINT32)(UINTN)MigratedFvHeader;
> > > + MigratedFvInfo.FvDataBase =
> > (UINT32)(UINTN)RawDataFvHeader;
> > > + MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader-
> > > >FvLength;
> > > + BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid,
> > &MigratedFvInfo,
> > > sizeof (MigratedFvInfo));
> > > + }
> > >
> > > //
> > > // Migrate any children for this FV now @@ -1330,8 +1364,6 @@
> > > EvacuateTempRam (
> > > }
> > > }
> > >
> > > - RemoveFvHobsInTemporaryMemory (Private);
> > > -
> > > return Status;
> > > }
> > >
> > > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> > > b/MdeModulePkg/Core/Pei/PeiMain.inf
> > > index 0cf357371a16..944b230b0e19 100644
> > > --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> > > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> > > @@ -78,6 +78,7 @@
> > > gEfiFirmwareFileSystem3Guid
> > > gStatusCodeCallbackGuid
> > > gEdkiiMigratedFvInfoGuid ##
> > SOMETIMES_PRODUCES
> > > ## HOB
> > > + gEdkiiToMigrateFvInfoGuid ##
> > SOMETIMES_CONSUMES
> > > ## HOB
> > >
> > > [Ppis]
> > > gEfiPeiStatusCodePpiGuid ##
> > SOMETIMES_CONSUMES
> > > # PeiReportStatusService is not ready if this PPI doesn't exist diff
> > > --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > index aca2332a0ec6..543cd9ba7ddd 100644
> > > --- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > @@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #ifndef
> > > __EDKII_MIGRATED_FV_INFO_GUID_H__ #define
> > > __EDKII_MIGRATED_FV_INFO_GUID_H__
> > >
> > > +#define FLAGS_SKIP_FV_MIGRATION BIT0
> > > +#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
> > > +
> > > +///
> > > +/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published
> by
> > > platform to indicate
> > > +/// one FV is expected to migrate to permarnant memory or not before
> > > TempRam tears down.
> > > +///
> > > +typedef struct {
> > > + UINT32 FvOrgBase; // original FV address
> > > + //
> > > + // Migration Flags:
> > > + // Bit0: Indicate to skip FV migration or not
> > > + // Bit1: Indicate to skip FV raw data copy or not
> > > + // Others: Reserved bits
> > > + //
> > > + UINT32 MigrationFlags;
> > > +} EDKII_TO_MIGRATE_FV_INFO;
> > > +
> > > typedef struct {
> > > UINT32 FvOrgBase; // original FV address
> > > UINT32 FvNewBase; // new FV address
> > > @@ -16,6 +34,7 @@ typedef struct {
> > > UINT32 FvLength; // Fv Length
> > > } EDKII_MIGRATED_FV_INFO;
> > >
> > > +extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
> > > extern EFI_GUID gEdkiiMigratedFvInfoGuid;
> > >
> > > #endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ diff --git
> > > a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index
> > > dd182c02fdf6..d6cbcc721a5e 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -416,7 +416,8 @@
> > > gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93,
> > > 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
> > >
> > > ## Include/Guid/MigratedFvInfo.h
> > > - gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> > > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> > > + gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93,
> > > 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
> > > + gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2,
> > > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> > >
> > > ## Include/Guid/RngAlgorithm.h
> > > gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> > > 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> > > --
> > > 2.29.2.windows.2
> >
> >
> >
> >
> >
>
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111964): https://edk2.groups.io/g/devel/message/111964
Mute This Topic: https://groups.io/mt/102908572/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-12-01 3:05 ` Ni, Ray
@ 2023-12-04 10:31 ` Wang Fan
2023-12-04 11:46 ` 回复: " gaoliming via groups.io
2023-12-05 8:16 ` Ni, Ray
0 siblings, 2 replies; 10+ messages in thread
From: Wang Fan @ 2023-12-04 10:31 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Gao, Liming, Kinney, Michael D
Cc: Jiang, Guomin
Thank you Ray and Liming, for your comments.
Ray,
There's something to clarify on this HOB usage. In this patch, when this Migration feature PCD is set to TRUE, the PeiCore will only migrate FVs which are recorded by ToMigrateFvInfo hob (each FV should publish their own ToMigrateFvInfo hob in this flow), and check MigrationFlags to decide also copy raw data or not. If no ToMigrateFvInfo hobs published, it will go into the legacy flow to migrate all FVs for backward compatibility.
Agree on your suggestion on the flag name change to "FLAGS_BUILD_MIGRATED_FV_INFO", I will update in V4 patch. But I think a little different on your listed approaches: I prefer to refine the hob usage to include all ToMigrateFvInfo data in one hob (rather than each FV publish their own) and a field "MigrateAll" in this hob for those platforms who have no performance concern. In future, if we want to retire Migration feature PCD, we just need replace "check PCD TRUE or FALSE" with "check Hob exists or not". What do you think this way?
Liming,
For your question "RAW_DATA_COPY is for measure boot to make sure PCR0 have the same value on the different boot.", we have gEfiPeiFirmwareVolumeInfoMeasurementExcludedPpiGuid to skip FV measurement when boot guard already measured these FVs. So I'm thinking it's valid to assume not all FVs need copy raw data for measurement.
For your another question "If RemoveFvHobsInTemporaryMemory () does nothing now.", it's true from my investigation.
Best Regards
Fan
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, December 1, 2023 11:06 AM
To: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Wang, Fan <fan.wang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: RE: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, FV in flash (cached in code cache) is copied to physical memory and all C global pointers (PPIs, GUIDs) are fixed up.
But TCG needs to measure the original FV contents, so the original FV is saved into MigratedFvInfo HOB when the FV migration is done.
Now we are going to add a new ToMigrate HOB to tell something.
Initially without reading the patch, I thought the HOB is to tell which FV needs to be migrated.
But what the patch does is: migration is always done when PCD is true. The only thing that's skipped is not to save the original FV contents, then the MigratedFvInfo HOB is not produced as well.
More strangely, if a FV is listed in "ToMigrate" HOB with flag 0, it's the same as the "ToMigrate" HOB does not exist.
It's a bit confusing, in my opinion.
There are several approaches:
1. Do not add "ToMigrate" HOB. Instead, avoid saving the whole original FV content, only save the delta that TCG driver uses the new FV content and the delta to calculate the HASH. The save of delta should not impact the performance very much.
2. Abandon the PCD, and update today's logic to follow the new "ToMigrate" HOB to perform migration. A new flag tells whether the original FV content needs to be saved.
3. Keep the PCD. If "ToMigrate" HOB does not exist, follow PCD. If "ToMigrate" HOB exists, follow the HOB and ignores the PCD. HOB takes higher priority. It's still backward compatible and allows edk2 to gradually retire that PCD. (Having multiple interfaces controlling one behavior is always confusing.)
3.1 The flag name in the patch is "*SKIP*". I prefer we reverse the meaning of the flag, meaning when the flag is set, FV original content is saved. The flag name can be "FLAGS_BUILD_MIGRATED_FV_INFO" which directly maps to the behavior the flag controls. I agree "MigratedFvInfo" name is confusing but at least we avoid adding another confusing "SKIP_FV_RAW_DATA_COPY" flag.
Thanks,
Ray
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> gaoliming via groups.io
> Sent: Friday, December 1, 2023 9:01 AM
> To: devel@edk2.groups.io; Wang, Fan <fan.wang@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: 回复: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support
> customized FV Migration Information
>
> Fan:
> I add my comment below.
>
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Wang Fan
> > 发送时间: 2023年10月27日 16:24
> > 收件人: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > 抄送: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> > 主题: Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support
> customized
> > FV Migration Information
> >
> > Hi Mike
> >
> > Thank you for your feedback.
> >
> > I have updated the patch to v3:
> > https://edk2.groups.io/g/devel/message/110197
> >
> > Pull Request: https://github.com/tianocore/edk2/pull/4970
> >
> > Based on V2, this update includes changes:
> > - Add more descriptions for "gEdkiiToMigrateFvInfoGuid" PPI usages
> > and background, but still keep the name.
> > - Update "FvOrgBase" to "FvTemporaryRamBase" in
> > EDKII_TO_MIGRATE_FV_INFO.
> > - Remove flag "FLAGS_SKIP_FV_MIGRATION", since it's not needed.
> > - Add more descriptions to flag "FLAGS_SKIP_FV_RAW_DATA_COPY".
> [Liming] RAW_DATA_COPY is for measure boot to make sure PCR0 have the
> same value on the different boot.
> If RAW_DATA_COPY is not set, it will impact the measure boot. So, I
> don't think we can skip raw data copy.
>
> > - Remove the variable MigrateAllFvs to simplify logic.
> > - Correct "allocate pool" to "allocate pages" to align with descriptions.
> >
> > For other two questions you are mentioning:
> >
> > 1. For "Should FvOrgBase be a UINT64 or support temp RAM above 4GB?":
> > I think UINT32 should be enough, all pre-mem FVs are below 4G as far
> > as I know, even we enabled X64 in pre-mem phase. This setting is
> > also aligning with "FvOrgBase" defined in "EDKII_MIGRATED_FV_INFO".
> > 2. For "The call to RemoveFvHobsInTemporaryMemory (Private) was
> > removed":
> > Since this function will remove all FV hobs for physical addresses,
> > it
> should be
> > called only when all pre-mem FVs are migrated. In EvacuateTempRam(),
> when
> > one FV is migrated, ConvertFvHob() will be called for this FV and all its'
> child
> > FVs, this is enough to ensure already migrated FV hobs are all
> > pointing to addresses on permanent address.
> >
> [Liming] So, RemoveFvHobsInTemporaryMemory () does nothing now. Right?
> If yes, it is safe to remove it.
>
> Thanks
> Liming
> > What do you think?
> >
> > Best Regards
> > Fan
> >
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Tuesday, October 17, 2023 5:00 AM
> > To: Wang, Fan <fan.wang@intel.com>; devel@edk2.groups.io
> > Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV
> > Migration Information
> >
> > Hi Fan,
> >
> > The logic looks functional, but there are some names and
> > descriptions that could be added to help describe this feature and
> > some code changes to make the code easier to understand.
> >
> > 1) The GUID gEdkiiToMigrateFvInfoGuid is hard to understand what
> > information it conveys from the name of the GUID variable.
> > I know that the intent is that it is a GUID with data that
> > tells the PEI core which FVs in temporary RAM should be
> > migrated to permanent RAM. And that the use of this information
> > is only a performance optimization to not migrate FVs that
> > are not needed after the PEI Core migrates temp RAM to
> > permanent RAM. The name and description in comments do not
> > express all these details.
> >
> > 2) The structure name EDKII_TO_MIGRATE_FV_INFO has the same
> > issue as (1).
> > a) Should FvOrgBase be a UINT64 or support temp RAM above 4GB?
> > b) Since only temp RAM to perm RAM migrations are supported
> > the field name "FvOrgBase" should be "FvTemporaryRamBase".
> >
> >
> > 3) The #define for FLAGS_SKIP_FV_MIGRATION should have a comment
> > block that describes the meaning of this bit. What is the
> > meaning when the bit is set and what is the meaning when the
> > bit is clear.
> >
> > 4) The #define for FLAGS_SKIP_FV_RAW_DATA_COPY should have a
> comment
> > block that describes the meaning of this bit. What is the
> > meaning when the bit is set and what is the meaning when the
> > bit is clear.
> >
> > 5) Why are there 2 bits? If an FV is skipped, then that means
> > do not copy. If an FV is not skipped, then that means do
> > copy.
> >
> > 6) I think the variable MigrateAllFvs can be removed, and the HOB
> > list check can be made for gEdkiiToMigrateFvInfoGuid can be made
> > on each FV found in temporary RAM. This looks like a performance
> > optimization that makes the logic harder to understand and it
> > is not clear that the performance optimization has any benefit.
> >
> > 7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed.
> > Why?
> >
> > 8) The comment where memory is allocated for the migrated FV says
> > "allocate pool" when PeiServicesAllocatePages() is used. Please
> > update the comment.
> >
> > Mike
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Fan <fan.wang@intel.com>
> > > Sent: Monday, September 11, 2023 2:52 AM
> > > To: devel@edk2.groups.io
> > > Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Gao, Liming
> > > <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> > > Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV
> Migration
> > > Information
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
> > >
> > > There are use cases which not all FVs need be migrated from
> > > TempRam to permanent memory before TempRam tears down. This new
> > > guid is introduced to avoid unnecessary FV migration to improve
> > > boot performance.
> > > Platform
> > > can publish ToMigrateFvInfo hob with this guid to customize FV
> > > migration info, and PeiCore will only migrate FVs indicated by
> > > this Hob info.
> > >
> > > This is a backwards compatible change, PeiCore will check
> > > ToMigrateFvInfo hob before migration. If ToMigrateFvInfo hobs
> > > exists, only migrate FVs recorded by hobs. If ToMigrateFvInfo hobs
> > > not exists, migrate all FVs to permanent memory.
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > Cc: Dandan Bi <dandan.bi@intel.com>
> > > Signed-off-by: Wang Fan <fan.wang@intel.com>
> > > ---
> > > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82
> > +++++++++++++-----
> > > -
> > > MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> > > MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
> > > MdeModulePkg/MdeModulePkg.dec | 3 +-
> > > 4 files changed, 79 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > index 5f32ebb560ae..e84849ec6db1 100644
> > > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > @@ -1184,7 +1184,11 @@ EvacuateTempRam (
> > >
> > > PEI_CORE_FV_HANDLE PeiCoreFvHandle;
> > > EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> > > + EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
> > > EDKII_MIGRATED_FV_INFO MigratedFvInfo;
> > > + EFI_PEI_HOB_POINTERS Hob;
> > > + BOOLEAN MigrateAllFvs;
> > > + UINT32 MigrationFlags;
> > >
> > > ASSERT (Private->PeiMemoryInstalled);
> > >
> > > @@ -1211,6 +1215,17 @@ EvacuateTempRam (
> > >
> > > ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
> > >
> > > + //
> > > + // Check if platform defined hobs to indicate which FVs are
> > > expected to migrate or keep raw data.
> > > + // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by
> > > hobs.
> > > + // If ToMigrateFvInfo hobs not exists, migrate all FVs to
> > > + permanent
> > > memory.
> > > + //
> > > + MigrateAllFvs = TRUE;
> > > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > > + if (Hob.Raw != NULL) {
> > > + MigrateAllFvs = FALSE;
> > > + }
> > > +
> > > for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
> > > FvHeader = Private->Fv[FvIndex].FvHeader;
> > > ASSERT (FvHeader != NULL);
> > > @@ -1224,6 +1239,25 @@ EvacuateTempRam (
> > > )
> > > )
> > > {
> > > + if (MigrateAllFvs) {
> > > + MigrationFlags = 0;
> > > + } else {
> > > + MigrationFlags = FLAGS_SKIP_FV_MIGRATION |
> > > FLAGS_SKIP_FV_RAW_DATA_COPY;
> > > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > > + while (Hob.Raw != NULL) {
> > > + ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
> > > + if (ToMigrateFvInfo->FvOrgBase ==
> > (UINT32)(UINTN)FvHeader)
> > > {
> > > + MigrationFlags = ToMigrateFvInfo->MigrationFlags;
> > > + break;
> > > + }
> > > + Hob.Raw = GET_NEXT_HOB (Hob);
> > > + Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid,
> > > Hob.Raw);
> > > + }
> > > + }
> > > + if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
> > > + continue;
> > > + }
> > > +
> > > //
> > > // Allocate page to save the rebased PEIMs, the PEIMs will
> > > get dispatched later.
> > > //
> > > @@ -1234,18 +1268,7 @@ EvacuateTempRam (
> > > );
> > > ASSERT_EFI_ERROR (Status);
> > > MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > *)(UINTN)FvHeaderAddress;
> > > -
> > > - //
> > > - // Allocate pool to save the raw PEIMs, which is used to keep
> > > consistent context across
> > > - // multiple boot and PCR0 will keep the same no matter if the
> > > address of allocated page is changed.
> > > - //
> > > - Status = PeiServicesAllocatePages (
> > > - EfiBootServicesCode,
> > > - EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength),
> > > - &FvHeaderAddress
> > > - );
> > > - ASSERT_EFI_ERROR (Status);
> > > - RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > *)(UINTN)FvHeaderAddress;
> > > + CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > > >FvLength);
> > >
> > > DEBUG ((
> > > DEBUG_VERBOSE,
> > > @@ -1256,18 +1279,29 @@ EvacuateTempRam (
> > > ));
> > >
> > > //
> > > - // Copy the context to the rebased pages and raw pages, and
> > > create hob to save the
> > > - // information. The MigratedFvInfo HOB will never be produced
> > > when
> > > - // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because
> > the
> > > PCD control the
> > > - // feature.
> > > + // Copy the context to the raw pages, and create hob to
> > > + save
> > > the information. The MigratedFvInfo
> > > + // HOB will never be produced when
> > > PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> > > + // controls the feature.
> > > //
> > > - CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > > >FvLength);
> > > - CopyMem (RawDataFvHeader, MigratedFvHeader,
> > (UINTN)FvHeader-
> > > >FvLength);
> > > - MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > > - MigratedFvInfo.FvNewBase =
> > (UINT32)(UINTN)MigratedFvHeader;
> > > - MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
> > > - MigratedFvInfo.FvLength =
> > (UINT32)(UINTN)FvHeader->FvLength;
> > > - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo,
> > > sizeof (MigratedFvInfo));
> > > + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=
> > > FLAGS_SKIP_FV_RAW_DATA_COPY) {
> > > + //
> > > + // Allocate pool to save the raw PEIMs, which is used to
> > > + keep
> > > consistent context across
> > > + // multiple boot and PCR0 will keep the same no matter if
> > > + the
> > > address of allocated page is changed.
> > > + //
> > > + Status = PeiServicesAllocatePages (
> > > + EfiBootServicesCode,
> > > + EFI_SIZE_TO_PAGES
> > ((UINTN)FvHeader->FvLength),
> > > + &FvHeaderAddress
> > > + );
> > > + ASSERT_EFI_ERROR (Status);
> > > + RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > *)(UINTN)FvHeaderAddress;
> > > + CopyMem (RawDataFvHeader, MigratedFvHeader,
> > (UINTN)FvHeader-
> > > >FvLength);
> > > + MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > > + MigratedFvInfo.FvNewBase =
> > (UINT32)(UINTN)MigratedFvHeader;
> > > + MigratedFvInfo.FvDataBase =
> > (UINT32)(UINTN)RawDataFvHeader;
> > > + MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader-
> > > >FvLength;
> > > + BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid,
> > &MigratedFvInfo,
> > > sizeof (MigratedFvInfo));
> > > + }
> > >
> > > //
> > > // Migrate any children for this FV now @@ -1330,8 +1364,6
> > > @@ EvacuateTempRam (
> > > }
> > > }
> > >
> > > - RemoveFvHobsInTemporaryMemory (Private);
> > > -
> > > return Status;
> > > }
> > >
> > > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> > > b/MdeModulePkg/Core/Pei/PeiMain.inf
> > > index 0cf357371a16..944b230b0e19 100644
> > > --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> > > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> > > @@ -78,6 +78,7 @@
> > > gEfiFirmwareFileSystem3Guid
> > > gStatusCodeCallbackGuid
> > > gEdkiiMigratedFvInfoGuid ##
> > SOMETIMES_PRODUCES
> > > ## HOB
> > > + gEdkiiToMigrateFvInfoGuid ##
> > SOMETIMES_CONSUMES
> > > ## HOB
> > >
> > > [Ppis]
> > > gEfiPeiStatusCodePpiGuid ##
> > SOMETIMES_CONSUMES
> > > # PeiReportStatusService is not ready if this PPI doesn't exist
> > > diff --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > index aca2332a0ec6..543cd9ba7ddd 100644
> > > --- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > @@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ #define
> > > __EDKII_MIGRATED_FV_INFO_GUID_H__
> > >
> > > +#define FLAGS_SKIP_FV_MIGRATION BIT0
> > > +#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
> > > +
> > > +///
> > > +/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published
> by
> > > platform to indicate
> > > +/// one FV is expected to migrate to permarnant memory or not
> > > +before
> > > TempRam tears down.
> > > +///
> > > +typedef struct {
> > > + UINT32 FvOrgBase; // original FV address
> > > + //
> > > + // Migration Flags:
> > > + // Bit0: Indicate to skip FV migration or not
> > > + // Bit1: Indicate to skip FV raw data copy or not
> > > + // Others: Reserved bits
> > > + //
> > > + UINT32 MigrationFlags;
> > > +} EDKII_TO_MIGRATE_FV_INFO;
> > > +
> > > typedef struct {
> > > UINT32 FvOrgBase; // original FV address
> > > UINT32 FvNewBase; // new FV address
> > > @@ -16,6 +34,7 @@ typedef struct {
> > > UINT32 FvLength; // Fv Length
> > > } EDKII_MIGRATED_FV_INFO;
> > >
> > > +extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
> > > extern EFI_GUID gEdkiiMigratedFvInfoGuid;
> > >
> > > #endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ diff --git
> > > a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index
> > > dd182c02fdf6..d6cbcc721a5e 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -416,7 +416,8 @@
> > > gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, {
> > > 0x93, 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
> > >
> > > ## Include/Guid/MigratedFvInfo.h
> > > - gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, {
> > > 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> > > + gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, {
> > > + 0x93,
> > > 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
> > > + gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, {
> > > + 0xa2,
> > > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> > >
> > > ## Include/Guid/RngAlgorithm.h
> > > gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> > > 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> > > --
> > > 2.29.2.windows.2
> >
> >
> >
> >
> >
>
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112029): https://edk2.groups.io/g/devel/message/112029
Mute This Topic: https://groups.io/mt/102908572/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
* 回复: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-12-04 10:31 ` Wang Fan
@ 2023-12-04 11:46 ` gaoliming via groups.io
2023-12-05 8:16 ` Ni, Ray
1 sibling, 0 replies; 10+ messages in thread
From: gaoliming via groups.io @ 2023-12-04 11:46 UTC (permalink / raw)
To: devel, fan.wang, 'Ni, Ray', 'Kinney, Michael D'
Cc: 'Jiang, Guomin'
Fan:
I add my comment below.
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Wang Fan
> 发送时间: 2023年12月4日 18:31
> 收件人: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> 抄送: Jiang, Guomin <guomin.jiang@intel.com>
> 主题: Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized
> FV Migration Information
>
> Thank you Ray and Liming, for your comments.
>
> Ray,
>
> There's something to clarify on this HOB usage. In this patch, when this
> Migration feature PCD is set to TRUE, the PeiCore will only migrate FVs which
> are recorded by ToMigrateFvInfo hob (each FV should publish their own
> ToMigrateFvInfo hob in this flow), and check MigrationFlags to decide also
> copy raw data or not. If no ToMigrateFvInfo hobs published, it will go into the
> legacy flow to migrate all FVs for backward compatibility.
>
> Agree on your suggestion on the flag name change to
> "FLAGS_BUILD_MIGRATED_FV_INFO", I will update in V4 patch. But I think a
> little different on your listed approaches: I prefer to refine the hob usage to
> include all ToMigrateFvInfo data in one hob (rather than each FV publish their
> own) and a field "MigrateAll" in this hob for those platforms who have no
> performance concern. In future, if we want to retire Migration feature PCD,
> we just need replace "check PCD TRUE or FALSE" with "check Hob exists or
> not". What do you think this way?
>
> Liming,
>
> For your question "RAW_DATA_COPY is for measure boot to make sure PCR0
> have the same value on the different boot.", we have
> gEfiPeiFirmwareVolumeInfoMeasurementExcludedPpiGuid to skip FV
> measurement when boot guard already measured these FVs. So I'm thinking
> it's valid to assume not all FVs need copy raw data for measurement.
[Liming] If so, please highlight this usage in comments that gEfiPeiFirmwareVolumeInfoMeasurementExcludedPpiGuid
must be configured if FV RAW_DATA_COPY is not set.
Thanks
Liming
>
> For your another question "If RemoveFvHobsInTemporaryMemory () does
> nothing now.", it's true from my investigation.
>
> Best Regards
> Fan
>
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, December 1, 2023 11:06 AM
> To: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Wang,
> Fan <fan.wang@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: RE: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support
> customized FV Migration Information
>
> When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, FV in flash
> (cached in code cache) is copied to physical memory and all C global pointers
> (PPIs, GUIDs) are fixed up.
> But TCG needs to measure the original FV contents, so the original FV is saved
> into MigratedFvInfo HOB when the FV migration is done.
>
> Now we are going to add a new ToMigrate HOB to tell something.
>
> Initially without reading the patch, I thought the HOB is to tell which FV needs
> to be migrated.
> But what the patch does is: migration is always done when PCD is true. The
> only thing that's skipped is not to save the original FV contents, then the
> MigratedFvInfo HOB is not produced as well.
> More strangely, if a FV is listed in "ToMigrate" HOB with flag 0, it's the same as
> the "ToMigrate" HOB does not exist.
>
> It's a bit confusing, in my opinion.
>
> There are several approaches:
> 1. Do not add "ToMigrate" HOB. Instead, avoid saving the whole original FV
> content, only save the delta that TCG driver uses the new FV content and the
> delta to calculate the HASH. The save of delta should not impact the
> performance very much.
>
> 2. Abandon the PCD, and update today's logic to follow the new "ToMigrate"
> HOB to perform migration. A new flag tells whether the original FV content
> needs to be saved.
>
> 3. Keep the PCD. If "ToMigrate" HOB does not exist, follow PCD. If "ToMigrate"
> HOB exists, follow the HOB and ignores the PCD. HOB takes higher priority.
> It's still backward compatible and allows edk2 to gradually retire that PCD.
> (Having multiple interfaces controlling one behavior is always confusing.)
> 3.1 The flag name in the patch is "*SKIP*". I prefer we reverse the meaning of
> the flag, meaning when the flag is set, FV original content is saved. The flag
> name can be "FLAGS_BUILD_MIGRATED_FV_INFO" which directly maps to
> the behavior the flag controls. I agree "MigratedFvInfo" name is confusing but
> at least we avoid adding another confusing "SKIP_FV_RAW_DATA_COPY" flag.
>
>
>
> Thanks,
> Ray
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > gaoliming via groups.io
> > Sent: Friday, December 1, 2023 9:01 AM
> > To: devel@edk2.groups.io; Wang, Fan <fan.wang@intel.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Cc: Jiang, Guomin <guomin.jiang@intel.com>; Bi, Dandan
> > <dandan.bi@intel.com>
> > Subject: 回复: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support
> > customized FV Migration Information
> >
> > Fan:
> > I add my comment below.
> >
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Wang
> Fan
> > > 发送时间: 2023年10月27日 16:24
> > > 收件人: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > devel@edk2.groups.io
> > > 抄送: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> > > 主题: Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support
> > customized
> > > FV Migration Information
> > >
> > > Hi Mike
> > >
> > > Thank you for your feedback.
> > >
> > > I have updated the patch to v3:
> > > https://edk2.groups.io/g/devel/message/110197
> > >
> > > Pull Request: https://github.com/tianocore/edk2/pull/4970
> > >
> > > Based on V2, this update includes changes:
> > > - Add more descriptions for "gEdkiiToMigrateFvInfoGuid" PPI usages
> > > and background, but still keep the name.
> > > - Update "FvOrgBase" to "FvTemporaryRamBase" in
> > > EDKII_TO_MIGRATE_FV_INFO.
> > > - Remove flag "FLAGS_SKIP_FV_MIGRATION", since it's not needed.
> > > - Add more descriptions to flag "FLAGS_SKIP_FV_RAW_DATA_COPY".
> > [Liming] RAW_DATA_COPY is for measure boot to make sure PCR0 have the
> > same value on the different boot.
> > If RAW_DATA_COPY is not set, it will impact the measure boot. So, I
> > don't think we can skip raw data copy.
> >
> > > - Remove the variable MigrateAllFvs to simplify logic.
> > > - Correct "allocate pool" to "allocate pages" to align with descriptions.
> > >
> > > For other two questions you are mentioning:
> > >
> > > 1. For "Should FvOrgBase be a UINT64 or support temp RAM above
> 4GB?":
> > > I think UINT32 should be enough, all pre-mem FVs are below 4G as far
> > > as I know, even we enabled X64 in pre-mem phase. This setting is
> > > also aligning with "FvOrgBase" defined in "EDKII_MIGRATED_FV_INFO".
> > > 2. For "The call to RemoveFvHobsInTemporaryMemory (Private) was
> > > removed":
> > > Since this function will remove all FV hobs for physical addresses,
> > > it
> > should be
> > > called only when all pre-mem FVs are migrated. In EvacuateTempRam(),
> > when
> > > one FV is migrated, ConvertFvHob() will be called for this FV and all its'
> > child
> > > FVs, this is enough to ensure already migrated FV hobs are all
> > > pointing to addresses on permanent address.
> > >
> > [Liming] So, RemoveFvHobsInTemporaryMemory () does nothing now. Right?
> > If yes, it is safe to remove it.
> >
> > Thanks
> > Liming
> > > What do you think?
> > >
> > > Best Regards
> > > Fan
> > >
> > > -----Original Message-----
> > > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Sent: Tuesday, October 17, 2023 5:00 AM
> > > To: Wang, Fan <fan.wang@intel.com>; devel@edk2.groups.io
> > > Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kinney,
> > > Michael D <michael.d.kinney@intel.com>
> > > Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV
> > > Migration Information
> > >
> > > Hi Fan,
> > >
> > > The logic looks functional, but there are some names and
> > > descriptions that could be added to help describe this feature and
> > > some code changes to make the code easier to understand.
> > >
> > > 1) The GUID gEdkiiToMigrateFvInfoGuid is hard to understand what
> > > information it conveys from the name of the GUID variable.
> > > I know that the intent is that it is a GUID with data that
> > > tells the PEI core which FVs in temporary RAM should be
> > > migrated to permanent RAM. And that the use of this information
> > > is only a performance optimization to not migrate FVs that
> > > are not needed after the PEI Core migrates temp RAM to
> > > permanent RAM. The name and description in comments do not
> > > express all these details.
> > >
> > > 2) The structure name EDKII_TO_MIGRATE_FV_INFO has the same
> > > issue as (1).
> > > a) Should FvOrgBase be a UINT64 or support temp RAM above 4GB?
> > > b) Since only temp RAM to perm RAM migrations are supported
> > > the field name "FvOrgBase" should be "FvTemporaryRamBase".
> > >
> > >
> > > 3) The #define for FLAGS_SKIP_FV_MIGRATION should have a comment
> > > block that describes the meaning of this bit. What is the
> > > meaning when the bit is set and what is the meaning when the
> > > bit is clear.
> > >
> > > 4) The #define for FLAGS_SKIP_FV_RAW_DATA_COPY should have a
> > comment
> > > block that describes the meaning of this bit. What is the
> > > meaning when the bit is set and what is the meaning when the
> > > bit is clear.
> > >
> > > 5) Why are there 2 bits? If an FV is skipped, then that means
> > > do not copy. If an FV is not skipped, then that means do
> > > copy.
> > >
> > > 6) I think the variable MigrateAllFvs can be removed, and the HOB
> > > list check can be made for gEdkiiToMigrateFvInfoGuid can be made
> > > on each FV found in temporary RAM. This looks like a performance
> > > optimization that makes the logic harder to understand and it
> > > is not clear that the performance optimization has any benefit.
> > >
> > > 7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed.
> > > Why?
> > >
> > > 8) The comment where memory is allocated for the migrated FV says
> > > "allocate pool" when PeiServicesAllocatePages() is used. Please
> > > update the comment.
> > >
> > > Mike
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Fan <fan.wang@intel.com>
> > > > Sent: Monday, September 11, 2023 2:52 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Wang, Fan <fan.wang@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Gao, Liming
> > > > <gaoliming@byosoft.com.cn>; Jiang, Guomin
> > > > <guomin.jiang@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> > > > Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV
> > Migration
> > > > Information
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4533
> > > >
> > > > There are use cases which not all FVs need be migrated from
> > > > TempRam to permanent memory before TempRam tears down. This
> new
> > > > guid is introduced to avoid unnecessary FV migration to improve
> > > > boot performance.
> > > > Platform
> > > > can publish ToMigrateFvInfo hob with this guid to customize FV
> > > > migration info, and PeiCore will only migrate FVs indicated by
> > > > this Hob info.
> > > >
> > > > This is a backwards compatible change, PeiCore will check
> > > > ToMigrateFvInfo hob before migration. If ToMigrateFvInfo hobs
> > > > exists, only migrate FVs recorded by hobs. If ToMigrateFvInfo hobs
> > > > not exists, migrate all FVs to permanent memory.
> > > >
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > > Cc: Dandan Bi <dandan.bi@intel.com>
> > > > Signed-off-by: Wang Fan <fan.wang@intel.com>
> > > > ---
> > > > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 82
> > > +++++++++++++-----
> > > > -
> > > > MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> > > > MdeModulePkg/Include/Guid/MigratedFvInfo.h | 19 +++++
> > > > MdeModulePkg/MdeModulePkg.dec | 3 +-
> > > > 4 files changed, 79 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > > index 5f32ebb560ae..e84849ec6db1 100644
> > > > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > > > @@ -1184,7 +1184,11 @@ EvacuateTempRam (
> > > >
> > > > PEI_CORE_FV_HANDLE PeiCoreFvHandle;
> > > > EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> > > > + EDKII_TO_MIGRATE_FV_INFO *ToMigrateFvInfo;
> > > > EDKII_MIGRATED_FV_INFO MigratedFvInfo;
> > > > + EFI_PEI_HOB_POINTERS Hob;
> > > > + BOOLEAN MigrateAllFvs;
> > > > + UINT32 MigrationFlags;
> > > >
> > > > ASSERT (Private->PeiMemoryInstalled);
> > > >
> > > > @@ -1211,6 +1215,17 @@ EvacuateTempRam (
> > > >
> > > > ConvertPeiCorePpiPointers (Private, &PeiCoreFvHandle);
> > > >
> > > > + //
> > > > + // Check if platform defined hobs to indicate which FVs are
> > > > expected to migrate or keep raw data.
> > > > + // If ToMigrateFvInfo hobs exists, only migrate FVs recorded by
> > > > hobs.
> > > > + // If ToMigrateFvInfo hobs not exists, migrate all FVs to
> > > > + permanent
> > > > memory.
> > > > + //
> > > > + MigrateAllFvs = TRUE;
> > > > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > > > + if (Hob.Raw != NULL) {
> > > > + MigrateAllFvs = FALSE;
> > > > + }
> > > > +
> > > > for (FvIndex = 0; FvIndex < Private->FvCount; FvIndex++) {
> > > > FvHeader = Private->Fv[FvIndex].FvHeader;
> > > > ASSERT (FvHeader != NULL);
> > > > @@ -1224,6 +1239,25 @@ EvacuateTempRam (
> > > > )
> > > > )
> > > > {
> > > > + if (MigrateAllFvs) {
> > > > + MigrationFlags = 0;
> > > > + } else {
> > > > + MigrationFlags = FLAGS_SKIP_FV_MIGRATION |
> > > > FLAGS_SKIP_FV_RAW_DATA_COPY;
> > > > + Hob.Raw = GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid);
> > > > + while (Hob.Raw != NULL) {
> > > > + ToMigrateFvInfo = GET_GUID_HOB_DATA (Hob);
> > > > + if (ToMigrateFvInfo->FvOrgBase ==
> > > (UINT32)(UINTN)FvHeader)
> > > > {
> > > > + MigrationFlags = ToMigrateFvInfo->MigrationFlags;
> > > > + break;
> > > > + }
> > > > + Hob.Raw = GET_NEXT_HOB (Hob);
> > > > + Hob.Raw = GetNextGuidHob (&gEdkiiToMigrateFvInfoGuid,
> > > > Hob.Raw);
> > > > + }
> > > > + }
> > > > + if (MigrationFlags & FLAGS_SKIP_FV_MIGRATION) {
> > > > + continue;
> > > > + }
> > > > +
> > > > //
> > > > // Allocate page to save the rebased PEIMs, the PEIMs will
> > > > get dispatched later.
> > > > //
> > > > @@ -1234,18 +1268,7 @@ EvacuateTempRam (
> > > > );
> > > > ASSERT_EFI_ERROR (Status);
> > > > MigratedFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > > *)(UINTN)FvHeaderAddress;
> > > > -
> > > > - //
> > > > - // Allocate pool to save the raw PEIMs, which is used to keep
> > > > consistent context across
> > > > - // multiple boot and PCR0 will keep the same no matter if the
> > > > address of allocated page is changed.
> > > > - //
> > > > - Status = PeiServicesAllocatePages (
> > > > - EfiBootServicesCode,
> > > > - EFI_SIZE_TO_PAGES
> ((UINTN)FvHeader->FvLength),
> > > > - &FvHeaderAddress
> > > > - );
> > > > - ASSERT_EFI_ERROR (Status);
> > > > - RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > > *)(UINTN)FvHeaderAddress;
> > > > + CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > > > >FvLength);
> > > >
> > > > DEBUG ((
> > > > DEBUG_VERBOSE,
> > > > @@ -1256,18 +1279,29 @@ EvacuateTempRam (
> > > > ));
> > > >
> > > > //
> > > > - // Copy the context to the rebased pages and raw pages, and
> > > > create hob to save the
> > > > - // information. The MigratedFvInfo HOB will never be produced
> > > > when
> > > > - // PcdMigrateTemporaryRamFirmwareVolumes is FALSE,
> because
> > > the
> > > > PCD control the
> > > > - // feature.
> > > > + // Copy the context to the raw pages, and create hob to
> > > > + save
> > > > the information. The MigratedFvInfo
> > > > + // HOB will never be produced when
> > > > PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> > > > + // controls the feature.
> > > > //
> > > > - CopyMem (MigratedFvHeader, FvHeader, (UINTN)FvHeader-
> > > > >FvLength);
> > > > - CopyMem (RawDataFvHeader, MigratedFvHeader,
> > > (UINTN)FvHeader-
> > > > >FvLength);
> > > > - MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > > > - MigratedFvInfo.FvNewBase =
> > > (UINT32)(UINTN)MigratedFvHeader;
> > > > - MigratedFvInfo.FvDataBase =
> (UINT32)(UINTN)RawDataFvHeader;
> > > > - MigratedFvInfo.FvLength =
> > > (UINT32)(UINTN)FvHeader->FvLength;
> > > > - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid,
> &MigratedFvInfo,
> > > > sizeof (MigratedFvInfo));
> > > > + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=
> > > > FLAGS_SKIP_FV_RAW_DATA_COPY) {
> > > > + //
> > > > + // Allocate pool to save the raw PEIMs, which is used to
> > > > + keep
> > > > consistent context across
> > > > + // multiple boot and PCR0 will keep the same no matter if
> > > > + the
> > > > address of allocated page is changed.
> > > > + //
> > > > + Status = PeiServicesAllocatePages (
> > > > + EfiBootServicesCode,
> > > > + EFI_SIZE_TO_PAGES
> > > ((UINTN)FvHeader->FvLength),
> > > > + &FvHeaderAddress
> > > > + );
> > > > + ASSERT_EFI_ERROR (Status);
> > > > + RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> > > > *)(UINTN)FvHeaderAddress;
> > > > + CopyMem (RawDataFvHeader, MigratedFvHeader,
> > > (UINTN)FvHeader-
> > > > >FvLength);
> > > > + MigratedFvInfo.FvOrgBase = (UINT32)(UINTN)FvHeader;
> > > > + MigratedFvInfo.FvNewBase =
> > > (UINT32)(UINTN)MigratedFvHeader;
> > > > + MigratedFvInfo.FvDataBase =
> > > (UINT32)(UINTN)RawDataFvHeader;
> > > > + MigratedFvInfo.FvLength = (UINT32)(UINTN)FvHeader-
> > > > >FvLength;
> > > > + BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid,
> > > &MigratedFvInfo,
> > > > sizeof (MigratedFvInfo));
> > > > + }
> > > >
> > > > //
> > > > // Migrate any children for this FV now @@ -1330,8 +1364,6
> > > > @@ EvacuateTempRam (
> > > > }
> > > > }
> > > >
> > > > - RemoveFvHobsInTemporaryMemory (Private);
> > > > -
> > > > return Status;
> > > > }
> > > >
> > > > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> > > > b/MdeModulePkg/Core/Pei/PeiMain.inf
> > > > index 0cf357371a16..944b230b0e19 100644
> > > > --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> > > > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> > > > @@ -78,6 +78,7 @@
> > > > gEfiFirmwareFileSystem3Guid
> > > > gStatusCodeCallbackGuid
> > > > gEdkiiMigratedFvInfoGuid ##
> > > SOMETIMES_PRODUCES
> > > > ## HOB
> > > > + gEdkiiToMigrateFvInfoGuid ##
> > > SOMETIMES_CONSUMES
> > > > ## HOB
> > > >
> > > > [Ppis]
> > > > gEfiPeiStatusCodePpiGuid ##
> > > SOMETIMES_CONSUMES
> > > > # PeiReportStatusService is not ready if this PPI doesn't exist
> > > > diff --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > > b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > > index aca2332a0ec6..543cd9ba7ddd 100644
> > > > --- a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > > +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> > > > @@ -9,6 +9,24 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ #define
> > > > __EDKII_MIGRATED_FV_INFO_GUID_H__
> > > >
> > > > +#define FLAGS_SKIP_FV_MIGRATION BIT0
> > > > +#define FLAGS_SKIP_FV_RAW_DATA_COPY BIT1
> > > > +
> > > > +///
> > > > +/// EDKII_TO_MIGRATE_FV_INFO Hob information should be published
> > by
> > > > platform to indicate
> > > > +/// one FV is expected to migrate to permarnant memory or not
> > > > +before
> > > > TempRam tears down.
> > > > +///
> > > > +typedef struct {
> > > > + UINT32 FvOrgBase; // original FV address
> > > > + //
> > > > + // Migration Flags:
> > > > + // Bit0: Indicate to skip FV migration or not
> > > > + // Bit1: Indicate to skip FV raw data copy or not
> > > > + // Others: Reserved bits
> > > > + //
> > > > + UINT32 MigrationFlags;
> > > > +} EDKII_TO_MIGRATE_FV_INFO;
> > > > +
> > > > typedef struct {
> > > > UINT32 FvOrgBase; // original FV address
> > > > UINT32 FvNewBase; // new FV address
> > > > @@ -16,6 +34,7 @@ typedef struct {
> > > > UINT32 FvLength; // Fv Length
> > > > } EDKII_MIGRATED_FV_INFO;
> > > >
> > > > +extern EFI_GUID gEdkiiToMigrateFvInfoGuid;
> > > > extern EFI_GUID gEdkiiMigratedFvInfoGuid;
> > > >
> > > > #endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__ diff --git
> > > > a/MdeModulePkg/MdeModulePkg.dec
> > > b/MdeModulePkg/MdeModulePkg.dec index
> > > > dd182c02fdf6..d6cbcc721a5e 100644
> > > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > > @@ -416,7 +416,8 @@
> > > > gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, {
> > > > 0x93, 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
> > > >
> > > > ## Include/Guid/MigratedFvInfo.h
> > > > - gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, {
> > > > 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> > > > + gEdkiiToMigrateFvInfoGuid = { 0xb4b140a5, 0x72f6, 0x4c21, {
> > > > + 0x93,
> > > > 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } }
> > > > + gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, {
> > > > + 0xa2,
> > > > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> > > >
> > > > ## Include/Guid/RngAlgorithm.h
> > > > gEdkiiRngAlgorithmUnSafe = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> > > > 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> > > > --
> > > > 2.29.2.windows.2
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> >
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112037): https://edk2.groups.io/g/devel/message/112037
Mute This Topic: https://groups.io/mt/102968594/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information
2023-12-04 10:31 ` Wang Fan
2023-12-04 11:46 ` 回复: " gaoliming via groups.io
@ 2023-12-05 8:16 ` Ni, Ray
1 sibling, 0 replies; 10+ messages in thread
From: Ni, Ray @ 2023-12-05 8:16 UTC (permalink / raw)
To: Wang, Fan, devel@edk2.groups.io, Gao, Liming, Kinney, Michael D
Cc: Jiang, Guomin
> I prefer to refine the hob usage to
> clude all ToMigrateFvInfo data in one hob (rather than each FV publish their
> own) and a field "MigrateAll" in this hob for those platforms who have no
> performance concern. In future, if we want to retire Migration feature PCD, we
> just need replace "check PCD TRUE or FALSE" with "check Hob exists or not".
> What do you think this way?
I am ok with your approach.
Each FV entry can tell two requests:
* Migrate FV or not
* Preserve RAW FV or not
As long as the global flag can provide the two information, I am ok😊
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112069): https://edk2.groups.io/g/devel/message/112069
Mute This Topic: https://groups.io/mt/102908572/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-05 8:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1694425833.git.fan.wang@intel.com>
2023-09-11 9:51 ` [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information Wang Fan
2023-10-16 20:59 ` Michael D Kinney
2023-10-27 8:23 ` Wang Fan
2023-11-20 6:30 ` Wang Fan
2023-12-01 1:01 ` 回复: " gaoliming via groups.io
2023-12-01 3:05 ` Ni, Ray
2023-12-04 10:31 ` Wang Fan
2023-12-04 11:46 ` 回复: " gaoliming via groups.io
2023-12-05 8:16 ` Ni, Ray
[not found] ` <1783CF665DB6E42F.23877@groups.io>
2023-09-22 5:32 ` Wang Fan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox