回复: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information

["gaoliming via groups.io" <gaoliming=byosoft.com.cn@groups.io>]

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]
-=-=-=-=-=-=-=-=-=-=-=-