From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id BC96FAC02E5 for ; Fri, 1 Dec 2023 01:01:25 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=UXFrYqfT8iaAzj4AunvB5sV+P6lc7jleLCN2hG0cq4Q=; c=relaxed/simple; d=groups.io; h=From:To:Cc:References:In-Reply-To:Subject:Date:Message-ID:MIME-Version:Thread-Index:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding:Content-Language; s=20140610; t=1701392484; v=1; b=TkwF2DuhMX1KEnxXozYuL4oZaOTWKS04ut7c/0Uz+fmhxyEq3p7kV0O4yh4jQ6rCETUxevvu v6zgAh6l3r7o75rY6MgNWVK8JCXZLyJLolue2mKmItKpHw8AfTMTI16w9QXEERRDKlkbqdv1d94 znqF/m3VWKBER2JJAtYHnRCk= X-Received: by 127.0.0.2 with SMTP id 8MRaYY7687511xWFA0HBeV0g; Thu, 30 Nov 2023 17:01:24 -0800 X-Received: from cxsh.intel-email.com (cxsh.intel-email.com [121.46.250.151]) by mx.groups.io with SMTP id smtpd.web10.12354.1701392482037400515 for ; Thu, 30 Nov 2023 17:01:22 -0800 X-Received: from cxsh.intel-email.com (localhost [127.0.0.1]) by cxsh.intel-email.com (Postfix) with ESMTP id EDD07DDA802 for ; Fri, 1 Dec 2023 09:01:18 +0800 (CST) X-Received: from localhost (localhost [127.0.0.1]) by cxsh.intel-email.com (Postfix) with ESMTP id E91A2DDA800 for ; Fri, 1 Dec 2023 09:01:18 +0800 (CST) X-Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by cxsh.intel-email.com (Postfix) with SMTP id 07503DDA7F4 for ; Fri, 1 Dec 2023 09:01:15 +0800 (CST) X-Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP(SSL) for ; Fri, 01 Dec 2023 09:01:11 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming via groups.io" To: , , "'Kinney, Michael D'" Cc: "'Jiang, Guomin'" , "'Bi, Dandan'" References: <76cde14616a559c6939f8814d0e79afa7f1a7fcf.1694425833.git.fan.wang@intel.com> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIFYyIDEvMV0gTWRlTW9kdWxlUGtnOiBTdXBwb3J0IGN1c3RvbWl6ZWQgRlYgTWlncmF0aW9uIEluZm9ybWF0aW9u?= Date: Fri, 1 Dec 2023 09:01:13 +0800 Message-ID: <061c01da23f1$e1b27370$a5175a50$@byosoft.com.cn> MIME-Version: 1.0 Thread-Index: AQJTGx1UM9cUL7hG4ST5y74+5oRg/wGZLb9TAtoqMVcA97tyO692U8Cw Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,gaoliming@byosoft.com.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Z8qdzRCZz6yuVNDRW2Ii1WcUx7686176AA= Content-Type: text/plain; charset="gb2312" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=TkwF2Duh; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Fan: I add my comment below.=20 > -----=D3=CA=BC=FE=D4=AD=BC=FE----- > =B7=A2=BC=FE=C8=CB: devel@edk2.groups.io =B4=FA=B1= =ED Wang Fan > =B7=A2=CB=CD=CA=B1=BC=E4: 2023=C4=EA10=D4=C227=C8=D5 16:24 > =CA=D5=BC=FE=C8=CB: Kinney, Michael D ; > devel@edk2.groups.io > =B3=AD=CB=CD: Gao, Liming ; Jiang, Guomin > ; Bi, Dandan > =D6=F7=CC=E2: Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support custo= mized > FV Migration Information >=20 > Hi Mike >=20 > Thank you for your feedback. >=20 > I have updated the patch to v3: > https://edk2.groups.io/g/devel/message/110197 >=20 > Pull Request: https://github.com/tianocore/edk2/pull/4970 >=20 > 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.=20 If RAW_DATA_COPY is not set, it will impact the measure boot. So, I don't think we can skip raw data copy.=20 > - Remove the variable MigrateAllFvs to simplify logic. > - Correct "allocate pool" to "allocate pages" to align with descriptions. >=20 > For other two questions you are mentioning: >=20 > 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 t= o > addresses on permanent address. >=20 [Liming] So, RemoveFvHobsInTemporaryMemory () does nothing now. Right?=20 If yes, it is safe to remove it.=20 Thanks Liming > What do you think? >=20 > Best Regards > Fan >=20 > -----Original Message----- > From: Kinney, Michael D > Sent: Tuesday, October 17, 2023 5:00 AM > To: Wang, Fan ; devel@edk2.groups.io > Cc: Gao, Liming ; Jiang, Guomin > ; Bi, Dandan ; Kinney, > Michael D > Subject: RE: [PATCH V2 1/1] MdeModulePkg: Support customized FV > Migration Information >=20 > Hi Fan, >=20 > The logic looks functional, but there are some names and descriptions tha= t > could be added to help describe this feature and some code changes to mak= e > the code easier to understand. >=20 > 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. >=20 > 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". >=20 >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 7) The call to RemoveFvHobsInTemporaryMemory (Private) was removed. > Why? >=20 > 8) The comment where memory is allocated for the migrated FV says > "allocate pool" when PeiServicesAllocatePages() is used. Please > update the comment. >=20 > Mike >=20 >=20 >=20 > > -----Original Message----- > > From: Wang, Fan > > Sent: Monday, September 11, 2023 2:52 AM > > To: devel@edk2.groups.io > > Cc: Wang, Fan ; Kinney, Michael D > > ; Gao, Liming ; > > Jiang, Guomin ; Bi, Dandan > > > > Subject: [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration > > Information > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4533 > > > > 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 > > Cc: Liming Gao > > Cc: Guomin Jiang > > Cc: Dandan Bi > > Signed-off-by: Wang Fan > > --- > > 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 =3D TRUE; > > + Hob.Raw =3D GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid); > > + if (Hob.Raw !=3D NULL) { > > + MigrateAllFvs =3D FALSE; > > + } > > + > > for (FvIndex =3D 0; FvIndex < Private->FvCount; FvIndex++) { > > FvHeader =3D Private->Fv[FvIndex].FvHeader; > > ASSERT (FvHeader !=3D NULL); > > @@ -1224,6 +1239,25 @@ EvacuateTempRam ( > > ) > > ) > > { > > + if (MigrateAllFvs) { > > + MigrationFlags =3D 0; > > + } else { > > + MigrationFlags =3D FLAGS_SKIP_FV_MIGRATION | > > FLAGS_SKIP_FV_RAW_DATA_COPY; > > + Hob.Raw =3D GetFirstGuidHob (&gEdkiiToMigrateFvInfoGuid); > > + while (Hob.Raw !=3D NULL) { > > + ToMigrateFvInfo =3D GET_GUID_HOB_DATA (Hob); > > + if (ToMigrateFvInfo->FvOrgBase =3D=3D > (UINT32)(UINTN)FvHeader) > > { > > + MigrationFlags =3D ToMigrateFvInfo->MigrationFlags; > > + break; > > + } > > + Hob.Raw =3D GET_NEXT_HOB (Hob); > > + Hob.Raw =3D 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 =3D (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 =3D PeiServicesAllocatePages ( > > - EfiBootServicesCode, > > - EFI_SIZE_TO_PAGES ((UINTN)FvHeader->FvLength), > > - &FvHeaderAddress > > - ); > > - ASSERT_EFI_ERROR (Status); > > - RawDataFvHeader =3D (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 =3D (UINT32)(UINTN)FvHeader; > > - MigratedFvInfo.FvNewBase =3D > (UINT32)(UINTN)MigratedFvHeader; > > - MigratedFvInfo.FvDataBase =3D (UINT32)(UINTN)RawDataFvHeader; > > - MigratedFvInfo.FvLength =3D > (UINT32)(UINTN)FvHeader->FvLength; > > - BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo, > > sizeof (MigratedFvInfo)); > > + if ((MigrationFlags & FLAGS_SKIP_FV_RAW_DATA_COPY) !=3D > > 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 =3D PeiServicesAllocatePages ( > > + EfiBootServicesCode, > > + EFI_SIZE_TO_PAGES > ((UINTN)FvHeader->FvLength), > > + &FvHeaderAddress > > + ); > > + ASSERT_EFI_ERROR (Status); > > + RawDataFvHeader =3D (EFI_FIRMWARE_VOLUME_HEADER > > *)(UINTN)FvHeaderAddress; > > + CopyMem (RawDataFvHeader, MigratedFvHeader, > (UINTN)FvHeader- > > >FvLength); > > + MigratedFvInfo.FvOrgBase =3D (UINT32)(UINTN)FvHeader; > > + MigratedFvInfo.FvNewBase =3D > (UINT32)(UINTN)MigratedFvHeader; > > + MigratedFvInfo.FvDataBase =3D > (UINT32)(UINTN)RawDataFvHeader; > > + MigratedFvInfo.FvLength =3D (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 =3D { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93= , > > 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } } > > > > ## Include/Guid/MigratedFvInfo.h > > - gEdkiiMigratedFvInfoGuid =3D { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, > > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } } > > + gEdkiiToMigrateFvInfoGuid =3D { 0xb4b140a5, 0x72f6, 0x4c21, { 0x93, > > 0xe4, 0xac, 0xc4, 0xec, 0xcb, 0x23, 0x23 } } > > + gEdkiiMigratedFvInfoGuid =3D { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, > > 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } } > > > > ## Include/Guid/RngAlgorithm.h > > gEdkiiRngAlgorithmUnSafe =3D { 0x869f728c, 0x409d, 0x4ab4, {0xac, > > 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }} > > -- > > 2.29.2.windows.2 >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-