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.kin...@intel.com>; > devel@edk2.groups.io > 抄送: Gao, Liming <gaolim...@byosoft.com.cn>; Jiang, Guomin > <guomin.ji...@intel.com>; Bi, Dandan <dandan...@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.kin...@intel.com> > Sent: Tuesday, October 17, 2023 5:00 AM > To: Wang, Fan <fan.w...@intel.com>; devel@edk2.groups.io > Cc: Gao, Liming <gaolim...@byosoft.com.cn>; Jiang, Guomin > <guomin.ji...@intel.com>; Bi, Dandan <dandan...@intel.com>; Kinney, > Michael D <michael.d.kin...@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.w...@intel.com> > > Sent: Monday, September 11, 2023 2:52 AM > > To: devel@edk2.groups.io > > Cc: Wang, Fan <fan.w...@intel.com>; Kinney, Michael D > > <michael.d.kin...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > > Jiang, Guomin <guomin.ji...@intel.com>; Bi, Dandan > > <dandan...@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.kin...@intel.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Guomin Jiang <guomin.ji...@intel.com> > > Cc: Dandan Bi <dandan...@intel.com> > > Signed-off-by: Wang Fan <fan.w...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
