Yes, we also need check PcdDxeIplSwitchToLongMode for AcpiS3SaveDxe in IntelFrameworkModulePkg.
It is good idea to add clarification for PcdDxeIplSwitchToLongMode. Reviewed-by: [email protected] Thank you Yao Jiewen From: Zeng, Star Sent: Tuesday, February 23, 2016 2:25 PM To: Yao, Jiewen; Ard Biesheuvel Cc: Ni, Ruiyu; Tian, Feng; [email protected]; [email protected]; [email protected]; Gao, Liming; Laszlo Ersek; Fan, Jeff Subject: Re: [edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB if needed On 2016/2/23 12:31, Yao, Jiewen wrote: > I discussed with Module owner. Below is summary for MdeModulePkg: > > 1) PCD Name: We suggest to use PcdDxeIplSwitchToLongMode as indicator that > PEI is 32 bit and DXE is 64 bit for X86 platform. It might be not clear > stated in PCD description, but it is the intent when this is introduced. We > suggest to define this to be FALSE for ARM, so that our core module can use > it. (Confirmed with Liming Gao) > > 2) AcpiTableDxe: As Star Zeng mentioned, we need make sure FACS is allocate > below 4G when PcdDxeIplSwitchToLongMode is set, because it is used by > S3Resume. > > 3) UefiBootManagerLib: As Ruiyu Ni mentioned, we need make allocate > L"PerfDataMemAddr" below 4G for when PcdDxeIplSwitchToLongMode is set, > because it is used by S3Resume. > > > 4) BootGraphicsResourceTableDxe: There is no need to add 4G limitation, we > can remove that directly. (Confirmed with Chao Zhang) > > 5) FirmwarePerformanceDataTableDxe: We can check PcdDxeIplSwitchToLongMode to > allocate Below4G memory for mAcpiS3PerformanceTable which is used in S3. And > we can remove 4G limitation for mAcpiBootPerformanceTable directly. > (Confirmed with Chao Zhang) > > 6) PiDxeS3BootScriptLib: We can check PcdDxeIplSwitchToLongMode for > S3TableBase and NewS3TableBase which is used in S3. (Confirmed with Star Zeng) > > 7) BootScriptExecutorDxe: We can check PcdDxeIplSwitchToLongMode for > FfsBuffer and BootScriptExecutorBuffer which is used in S3. (Confirmed with > Star Zeng) > > 8) CapsuleRuntimeDxe: Already check PcdDxeIplSwitchToLongMode. No update > needed. (Confirmed with Star Zeng) Should AcpiS3Save be updated? Personally, the PCD name is a little strange for this purpose. If this PCD will be preferred as the control finally. I would suggest to add comments to the PCD declaration and ASSERT in the code to ensure the PCD value is TRUE only for PEI is 32 bit and DXE is 64 bit. diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c index 6488880..348e084 100644 --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c @@ -43,6 +43,11 @@ HandOffToDxeCore ( EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi; // + // It should be FALSE for both PEI and DXE are 64-bit. + // + ASSERT (PcdGetBool (PcdDxeIplSwitchToLongMode) == FALSE); + + // // Get Vector Hand-off Info PPI and build Guided HOB // Status = PeiServicesLocatePpi ( diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index af7bcab..4a73f7b 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -712,8 +712,10 @@ ## Indicates if DxeIpl should switch to long mode to enter DXE phase. # It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore # is built in firmware. + # And it should be FALSE for both PEI and DXE are 64-bit. # TRUE - DxeIpl will load a 64-bit DxeCore and switch to long mode to hand over to DxeCore. - # FALSE - DxeIpl will load a 32-bit DxeCore and perform stack switch to hand over to DxeCore. + # FALSE - DxeIpl will load a 32-bit DxeCore and perform stack switch to hand over to DxeCore, + # or both PEI and DXE are 64-bit. # @Prompt DxeIpl switch to long mode. gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b Thanks, Star > > BTW: We are trying not use IntelFrameworkModulePkg. If there is replacement > in MdeModulePkg, we suggest use the one in MdeModulePkg, for example, BDS or > ACPI. > > Thank you > Yao Jiewen > > > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Monday, February 22, 2016 8:43 PM > To: Yao, Jiewen > Cc: Ni, Ruiyu; Laszlo Ersek; > [email protected]<mailto:[email protected]>; Tian, Feng; Zeng, > Star; [email protected]<mailto:[email protected]>; > [email protected]<mailto:[email protected]>; Fan, Jeff; > Gao, Liming > Subject: Re: [edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe: > only allocate below 4 GB if needed > > On 22 February 2016 at 13:40, Yao, Jiewen wrote: >> I did a search on current MdeModulePkg. I found there are more >> modules allocating Below4G memory. >> >> Besides BDS, we have BootGraphicsResourceTableDxe, >> BootScriptExecutorDxe, FirmwarePerformanceDataTableDxe, >> PiDxeS3BootScriptLib, CapsuleRuntimeDxe. >> >> >> >> BootGraphicsResourceTableDxe - I am not clear. >> >> CapsuleRuntimeDxe - Below4G is designed for PEI capsule. Or we do not >> need such capability. >> >> BootScriptExecutorDxe, FirmwarePerformanceDataTableDxe, >> PiDxeS3BootScriptLib >> - data need to be access in PEI phase. >> >> >> >> I think we might need more clean up for 32bit PEI. >> > > Agreed. > >> I am still thinking if we can reuse old Pcd - we can clarify in PCD >> description - it is only for 32bit PEI and 64bit DXE. >> >> Or a new PCD, and name to Pcd32BitPei? Please allow me collect more >> feedback on that. >> > > OK > >> In order to separate 32bit PEI issue from ACPI issue, I think we can >> check in ACPI patch at first, if you want. >> > > Should I wait for the maintainers of MdeModulePkg to give their Reviewed-by ? > _______________________________________________ > edk2-devel mailing list > [email protected]<mailto:[email protected]> > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
